diff mbox series

[v2,11/14] drm/tilcdc: Convert to Linux IRQ interfaces

Message ID 20210803090704.32152-12-tzimmermann@suse.de (mailing list archive)
State New, archived
Headers show
Series drm: Make DRM's IRQ helpers legacy | expand

Commit Message

Thomas Zimmermann Aug. 3, 2021, 9:07 a.m. UTC 
Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's
IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers
don't benefit from using it.

DRM IRQ callbacks are now being called directly or inlined.

Calls to platform_get_irq() can fail with a negative errno code.
Abort initialization in this case. The DRM IRQ midlayer does not
handle this case correctly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 51 ++++++++++++++++++++++-------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h |  3 ++
 2 files changed, 43 insertions(+), 11 deletions(-)

Comments

Sam Ravnborg Aug. 3, 2021, 3 p.m. UTC | #1 
Hi Thomas,

On Tue, Aug 03, 2021 at 11:07:01AM +0200, Thomas Zimmermann wrote:
> Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's
> IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers
> don't benefit from using it.
> 
> DRM IRQ callbacks are now being called directly or inlined.
> 
> Calls to platform_get_irq() can fail with a negative errno code.
> Abort initialization in this case. The DRM IRQ midlayer does not
> handle this case correctly.

I cannot see why the irq_enabled flag is needed here, and the changelog
do not help me.
What do I miss?

	Sam


> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 51 ++++++++++++++++++++++-------
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h |  3 ++
>  2 files changed, 43 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index f1d3a9f919fd..6b03f89a98d4 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -20,7 +20,6 @@
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> -#include <drm/drm_irq.h>
>  #include <drm/drm_mm.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_vblank.h>
> @@ -124,6 +123,39 @@ static int cpufreq_transition(struct notifier_block *nb,
>  }
>  #endif
>  
> +static irqreturn_t tilcdc_irq(int irq, void *arg)
> +{
> +	struct drm_device *dev = arg;
> +	struct tilcdc_drm_private *priv = dev->dev_private;
> +
> +	return tilcdc_crtc_irq(priv->crtc);
> +}
> +
> +static int tilcdc_irq_install(struct drm_device *dev, unsigned int irq)
> +{
> +	struct tilcdc_drm_private *priv = dev->dev_private;
> +	int ret;
> +
> +	ret = request_irq(irq, tilcdc_irq, 0, dev->driver->name, dev);
> +	if (ret)
> +		return ret;
> +
> +	priv->irq_enabled = false;
> +
> +	return 0;
> +}
> +
> +static void tilcdc_irq_uninstall(struct drm_device *dev)
> +{
> +	struct tilcdc_drm_private *priv = dev->dev_private;
> +
> +	if (!priv->irq_enabled)
> +		return;
> +
> +	free_irq(priv->irq, dev);
> +	priv->irq_enabled = false;
> +}
> +
>  /*
>   * DRM operations:
>   */
> @@ -145,7 +177,7 @@ static void tilcdc_fini(struct drm_device *dev)
>  		drm_dev_unregister(dev);
>  
>  	drm_kms_helper_poll_fini(dev);
> -	drm_irq_uninstall(dev);
> +	tilcdc_irq_uninstall(dev);
>  	drm_mode_config_cleanup(dev);
>  
>  	if (priv->clk)
> @@ -336,7 +368,12 @@ static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
>  		goto init_failed;
>  	}
>  
> -	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0)
> +		goto init_failed;
> +	priv->irq = ret;
> +
> +	ret = tilcdc_irq_install(ddev, priv->irq);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to install IRQ handler\n");
>  		goto init_failed;
> @@ -360,13 +397,6 @@ static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
>  	return ret;
>  }
>  
> -static irqreturn_t tilcdc_irq(int irq, void *arg)
> -{
> -	struct drm_device *dev = arg;
> -	struct tilcdc_drm_private *priv = dev->dev_private;
> -	return tilcdc_crtc_irq(priv->crtc);
> -}
> -
>  #if defined(CONFIG_DEBUG_FS)
>  static const struct {
>  	const char *name;
> @@ -454,7 +484,6 @@ DEFINE_DRM_GEM_CMA_FOPS(fops);
>  
>  static const struct drm_driver tilcdc_driver = {
>  	.driver_features    = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> -	.irq_handler        = tilcdc_irq,
>  	DRM_GEM_CMA_DRIVER_OPS,
>  #ifdef CONFIG_DEBUG_FS
>  	.debugfs_init       = tilcdc_debugfs_init,
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> index d29806ca8817..b818448c83f6 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> @@ -46,6 +46,8 @@ struct tilcdc_drm_private {
>  	struct clk *clk;         /* functional clock */
>  	int rev;                 /* IP revision */
>  
> +	unsigned int irq;
> +
>  	/* don't attempt resolutions w/ higher W * H * Hz: */
>  	uint32_t max_bandwidth;
>  	/*
> @@ -82,6 +84,7 @@ struct tilcdc_drm_private {
>  
>  	bool is_registered;
>  	bool is_componentized;
> +	bool irq_enabled;
>  };
>  
>  /* Sub-module for display.  Since we don't know at compile time what panels
> -- 
> 2.32.0
Thomas Zimmermann Aug. 4, 2021, 6:30 p.m. UTC | #2 
Hi

Am 03.08.21 um 17:00 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Tue, Aug 03, 2021 at 11:07:01AM +0200, Thomas Zimmermann wrote:
>> Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's
>> IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers
>> don't benefit from using it.
>>
>> DRM IRQ callbacks are now being called directly or inlined.
>>
>> Calls to platform_get_irq() can fail with a negative errno code.
>> Abort initialization in this case. The DRM IRQ midlayer does not
>> handle this case correctly.
> 
> I cannot see why the irq_enabled flag is needed here, and the changelog
> do not help me.
> What do I miss?

That's a good point. Usually, only the DRM IRQ helpers use irq_enabled 
in struct drm_device. It'll become legacy and we can ignore it for the 
driver conversion.

The exception is tilcdc, which also uses irq_enabled to make its error 
rollback work correctly. So I duplicated the state in the local private 
structure.

I'll add this explanation to the commit message.

Best regards
Thomas

> 
> 	Sam
> 
> 
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/tilcdc/tilcdc_drv.c | 51 ++++++++++++++++++++++-------
>>   drivers/gpu/drm/tilcdc/tilcdc_drv.h |  3 ++
>>   2 files changed, 43 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> index f1d3a9f919fd..6b03f89a98d4 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> @@ -20,7 +20,6 @@
>>   #include <drm/drm_fourcc.h>
>>   #include <drm/drm_gem_cma_helper.h>
>>   #include <drm/drm_gem_framebuffer_helper.h>
>> -#include <drm/drm_irq.h>
>>   #include <drm/drm_mm.h>
>>   #include <drm/drm_probe_helper.h>
>>   #include <drm/drm_vblank.h>
>> @@ -124,6 +123,39 @@ static int cpufreq_transition(struct notifier_block *nb,
>>   }
>>   #endif
>>   
>> +static irqreturn_t tilcdc_irq(int irq, void *arg)
>> +{
>> +	struct drm_device *dev = arg;
>> +	struct tilcdc_drm_private *priv = dev->dev_private;
>> +
>> +	return tilcdc_crtc_irq(priv->crtc);
>> +}
>> +
>> +static int tilcdc_irq_install(struct drm_device *dev, unsigned int irq)
>> +{
>> +	struct tilcdc_drm_private *priv = dev->dev_private;
>> +	int ret;
>> +
>> +	ret = request_irq(irq, tilcdc_irq, 0, dev->driver->name, dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	priv->irq_enabled = false;
>> +
>> +	return 0;
>> +}
>> +
>> +static void tilcdc_irq_uninstall(struct drm_device *dev)
>> +{
>> +	struct tilcdc_drm_private *priv = dev->dev_private;
>> +
>> +	if (!priv->irq_enabled)
>> +		return;
>> +
>> +	free_irq(priv->irq, dev);
>> +	priv->irq_enabled = false;
>> +}
>> +
>>   /*
>>    * DRM operations:
>>    */
>> @@ -145,7 +177,7 @@ static void tilcdc_fini(struct drm_device *dev)
>>   		drm_dev_unregister(dev);
>>   
>>   	drm_kms_helper_poll_fini(dev);
>> -	drm_irq_uninstall(dev);
>> +	tilcdc_irq_uninstall(dev);
>>   	drm_mode_config_cleanup(dev);
>>   
>>   	if (priv->clk)
>> @@ -336,7 +368,12 @@ static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
>>   		goto init_failed;
>>   	}
>>   
>> -	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
>> +	ret = platform_get_irq(pdev, 0);
>> +	if (ret < 0)
>> +		goto init_failed;
>> +	priv->irq = ret;
>> +
>> +	ret = tilcdc_irq_install(ddev, priv->irq);
>>   	if (ret < 0) {
>>   		dev_err(dev, "failed to install IRQ handler\n");
>>   		goto init_failed;
>> @@ -360,13 +397,6 @@ static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
>>   	return ret;
>>   }
>>   
>> -static irqreturn_t tilcdc_irq(int irq, void *arg)
>> -{
>> -	struct drm_device *dev = arg;
>> -	struct tilcdc_drm_private *priv = dev->dev_private;
>> -	return tilcdc_crtc_irq(priv->crtc);
>> -}
>> -
>>   #if defined(CONFIG_DEBUG_FS)
>>   static const struct {
>>   	const char *name;
>> @@ -454,7 +484,6 @@ DEFINE_DRM_GEM_CMA_FOPS(fops);
>>   
>>   static const struct drm_driver tilcdc_driver = {
>>   	.driver_features    = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
>> -	.irq_handler        = tilcdc_irq,
>>   	DRM_GEM_CMA_DRIVER_OPS,
>>   #ifdef CONFIG_DEBUG_FS
>>   	.debugfs_init       = tilcdc_debugfs_init,
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
>> index d29806ca8817..b818448c83f6 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
>> @@ -46,6 +46,8 @@ struct tilcdc_drm_private {
>>   	struct clk *clk;         /* functional clock */
>>   	int rev;                 /* IP revision */
>>   
>> +	unsigned int irq;
>> +
>>   	/* don't attempt resolutions w/ higher W * H * Hz: */
>>   	uint32_t max_bandwidth;
>>   	/*
>> @@ -82,6 +84,7 @@ struct tilcdc_drm_private {
>>   
>>   	bool is_registered;
>>   	bool is_componentized;
>> +	bool irq_enabled;
>>   };
>>   
>>   /* Sub-module for display.  Since we don't know at compile time what panels
>> -- 
>> 2.32.0
Sam Ravnborg Aug. 4, 2021, 6:33 p.m. UTC | #3 
Hi Thomas,
On Wed, Aug 04, 2021 at 08:30:41PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 03.08.21 um 17:00 schrieb Sam Ravnborg:
> > Hi Thomas,
> > 
> > On Tue, Aug 03, 2021 at 11:07:01AM +0200, Thomas Zimmermann wrote:
> > > Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's
> > > IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers
> > > don't benefit from using it.
> > > 
> > > DRM IRQ callbacks are now being called directly or inlined.
> > > 
> > > Calls to platform_get_irq() can fail with a negative errno code.
> > > Abort initialization in this case. The DRM IRQ midlayer does not
> > > handle this case correctly.
> > 
> > I cannot see why the irq_enabled flag is needed here, and the changelog
> > do not help me.
> > What do I miss?
> 
> That's a good point. Usually, only the DRM IRQ helpers use irq_enabled in
> struct drm_device. It'll become legacy and we can ignore it for the driver
> conversion.
> 
> The exception is tilcdc, which also uses irq_enabled to make its error
> rollback work correctly. So I duplicated the state in the local private
> structure.
> 
> I'll add this explanation to the commit message.
With this added:
Acked-by: Sam Ravnborg <sam@ravnborg.org>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index f1d3a9f919fd..6b03f89a98d4 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -20,7 +20,6 @@ 
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
-#include <drm/drm_irq.h>
 #include <drm/drm_mm.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
@@ -124,6 +123,39 @@  static int cpufreq_transition(struct notifier_block *nb,
 }
 #endif
 
+static irqreturn_t tilcdc_irq(int irq, void *arg)
+{
+	struct drm_device *dev = arg;
+	struct tilcdc_drm_private *priv = dev->dev_private;
+
+	return tilcdc_crtc_irq(priv->crtc);
+}
+
+static int tilcdc_irq_install(struct drm_device *dev, unsigned int irq)
+{
+	struct tilcdc_drm_private *priv = dev->dev_private;
+	int ret;
+
+	ret = request_irq(irq, tilcdc_irq, 0, dev->driver->name, dev);
+	if (ret)
+		return ret;
+
+	priv->irq_enabled = false;
+
+	return 0;
+}
+
+static void tilcdc_irq_uninstall(struct drm_device *dev)
+{
+	struct tilcdc_drm_private *priv = dev->dev_private;
+
+	if (!priv->irq_enabled)
+		return;
+
+	free_irq(priv->irq, dev);
+	priv->irq_enabled = false;
+}
+
 /*
  * DRM operations:
  */
@@ -145,7 +177,7 @@  static void tilcdc_fini(struct drm_device *dev)
 		drm_dev_unregister(dev);
 
 	drm_kms_helper_poll_fini(dev);
-	drm_irq_uninstall(dev);
+	tilcdc_irq_uninstall(dev);
 	drm_mode_config_cleanup(dev);
 
 	if (priv->clk)
@@ -336,7 +368,12 @@  static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
 		goto init_failed;
 	}
 
-	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0)
+		goto init_failed;
+	priv->irq = ret;
+
+	ret = tilcdc_irq_install(ddev, priv->irq);
 	if (ret < 0) {
 		dev_err(dev, "failed to install IRQ handler\n");
 		goto init_failed;
@@ -360,13 +397,6 @@  static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
 	return ret;
 }
 
-static irqreturn_t tilcdc_irq(int irq, void *arg)
-{
-	struct drm_device *dev = arg;
-	struct tilcdc_drm_private *priv = dev->dev_private;
-	return tilcdc_crtc_irq(priv->crtc);
-}
-
 #if defined(CONFIG_DEBUG_FS)
 static const struct {
 	const char *name;
@@ -454,7 +484,6 @@  DEFINE_DRM_GEM_CMA_FOPS(fops);
 
 static const struct drm_driver tilcdc_driver = {
 	.driver_features    = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
-	.irq_handler        = tilcdc_irq,
 	DRM_GEM_CMA_DRIVER_OPS,
 #ifdef CONFIG_DEBUG_FS
 	.debugfs_init       = tilcdc_debugfs_init,
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index d29806ca8817..b818448c83f6 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -46,6 +46,8 @@  struct tilcdc_drm_private {
 	struct clk *clk;         /* functional clock */
 	int rev;                 /* IP revision */
 
+	unsigned int irq;
+
 	/* don't attempt resolutions w/ higher W * H * Hz: */
 	uint32_t max_bandwidth;
 	/*
@@ -82,6 +84,7 @@  struct tilcdc_drm_private {
 
 	bool is_registered;
 	bool is_componentized;
+	bool irq_enabled;
 };
 
 /* Sub-module for display.  Since we don't know at compile time what panels