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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer