Re: [PATCH 02/14] drm/arm/hdlcd: Convert to Linux IRQ interfaces

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 9792 bytes --]

Hi Sam

Am 28.07.21 um 15:31 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Tue, Jul 27, 2021 at 08:27:09PM +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.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/arm/hdlcd_drv.c | 174 ++++++++++++++++++--------------
>>   drivers/gpu/drm/arm/hdlcd_drv.h |   1 +
>>   2 files changed, 97 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
>> index 81ae92390736..b9998fe3982f 100644
>> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
>> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
>> @@ -29,7 +29,6 @@
>>   #include <drm/drm_fb_helper.h>
>>   #include <drm/drm_gem_cma_helper.h>
>>   #include <drm/drm_gem_framebuffer_helper.h>
>> -#include <drm/drm_irq.h>
>>   #include <drm/drm_modeset_helper.h>
>>   #include <drm/drm_of.h>
>>   #include <drm/drm_probe_helper.h>
>> @@ -38,6 +37,94 @@
>>   #include "hdlcd_drv.h"
>>   #include "hdlcd_regs.h"
>>   
>> +static irqreturn_t hdlcd_irq(int irq, void *arg)
>> +{
>> +	struct drm_device *drm = arg;
>> +	struct hdlcd_drm_private *hdlcd = drm->dev_private;
>> +	unsigned long irq_status;
>> +
>> +	irq_status = hdlcd_read(hdlcd, HDLCD_REG_INT_STATUS);
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> +	if (irq_status & HDLCD_INTERRUPT_UNDERRUN)
>> +		atomic_inc(&hdlcd->buffer_underrun_count);
>> +
>> +	if (irq_status & HDLCD_INTERRUPT_DMA_END)
>> +		atomic_inc(&hdlcd->dma_end_count);
>> +
>> +	if (irq_status & HDLCD_INTERRUPT_BUS_ERROR)
>> +		atomic_inc(&hdlcd->bus_error_count);
>> +
>> +	if (irq_status & HDLCD_INTERRUPT_VSYNC)
>> +		atomic_inc(&hdlcd->vsync_count);
>> +
>> +#endif
>> +	if (irq_status & HDLCD_INTERRUPT_VSYNC)
>> +		drm_crtc_handle_vblank(&hdlcd->crtc);
>> +
>> +	/* acknowledge interrupt(s) */
>> +	hdlcd_write(hdlcd, HDLCD_REG_INT_CLEAR, irq_status);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static void hdlcd_irq_preinstall(struct drm_device *drm)
>> +{
>> +	struct hdlcd_drm_private *hdlcd = drm->dev_private;
>> +	/* Ensure interrupts are disabled */
>> +	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, 0);
>> +	hdlcd_write(hdlcd, HDLCD_REG_INT_CLEAR, ~0);
>> +}
>> +
>> +static void hdlcd_irq_postinstall(struct drm_device *drm)
>> +{
>> +#ifdef CONFIG_DEBUG_FS
>> +	struct hdlcd_drm_private *hdlcd = drm->dev_private;
>> +	unsigned long irq_mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
>> +
>> +	/* enable debug interrupts */
>> +	irq_mask |= HDLCD_DEBUG_INT_MASK;
>> +
>> +	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, irq_mask);
>> +#endif
>> +}
>> +
>> +static int hdlcd_irq_install(struct drm_device *dev, int irq)
> It is inconsistent that the drm_device * is named "dev", as similar
> functions in this patch uses the name "drm".
> 
>> +{
>> +	int ret;
>> +
>> +	if (irq == IRQ_NOTCONNECTED)
>> +		return -ENOTCONN;
> The code above is almost redundandt as request_irq has the same check.
> The only benefit of this check is that we avoid calling
> hdlcd_irq_preinstall().

I'd expect that there's a reason that the code is in _preinstall(). So 
testing for IRQF_NOTCONNECTED in request_irq() might be too late. I'd 
really like to here from driver maintainers that this can be changed.

I agree that the logic is duplicated and the whole thing could be 
simpler. But I rather stick with the original logic as used in 
drm_irq_install(). That function get's it totally wrong BTW.

> 
> And IRQ_NOTCONNECTED is only set for PCI devices which this is not.

Is that guaranteed?

Best regards
Thomas

> So I would thing the if () should be dropped here. ??
> 
> With the inputs considered/addressed:
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> 
> 
>> +
>> +	hdlcd_irq_preinstall(dev);
>> +
>> +	ret = request_irq(irq, hdlcd_irq, 0, dev->driver->name, dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	hdlcd_irq_postinstall(dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static void hdlcd_irq_uninstall(struct drm_device *drm)
>> +{
>> +	struct hdlcd_drm_private *hdlcd = drm->dev_private;
>> +	/* disable all the interrupts that we might have enabled */
>> +	unsigned long irq_mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> +	/* disable debug interrupts */
>> +	irq_mask &= ~HDLCD_DEBUG_INT_MASK;
>> +#endif
>> +
>> +	/* disable vsync interrupts */
>> +	irq_mask &= ~HDLCD_INTERRUPT_VSYNC;
>> +	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, irq_mask);
>> +
>> +	free_irq(hdlcd->irq, drm);
>> +}
>> +
>>   static int hdlcd_load(struct drm_device *drm, unsigned long flags)
>>   {
>>   	struct hdlcd_drm_private *hdlcd = drm->dev_private;
>> @@ -90,7 +177,12 @@ static int hdlcd_load(struct drm_device *drm, unsigned long flags)
>>   		goto setup_fail;
>>   	}
>>   
>> -	ret = drm_irq_install(drm, platform_get_irq(pdev, 0));
>> +	ret = platform_get_irq(pdev, 0);
>> +	if (ret < 0)
>> +		goto irq_fail;
>> +	hdlcd->irq = ret;
>> +
>> +	ret = hdlcd_irq_install(drm, hdlcd->irq);
>>   	if (ret < 0) {
>>   		DRM_ERROR("failed to install IRQ handler\n");
>>   		goto irq_fail;
>> @@ -122,76 +214,6 @@ static void hdlcd_setup_mode_config(struct drm_device *drm)
>>   	drm->mode_config.funcs = &hdlcd_mode_config_funcs;
>>   }
>>   
>> -static irqreturn_t hdlcd_irq(int irq, void *arg)
>> -{
>> -	struct drm_device *drm = arg;
>> -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
>> -	unsigned long irq_status;
>> -
>> -	irq_status = hdlcd_read(hdlcd, HDLCD_REG_INT_STATUS);
>> -
>> -#ifdef CONFIG_DEBUG_FS
>> -	if (irq_status & HDLCD_INTERRUPT_UNDERRUN)
>> -		atomic_inc(&hdlcd->buffer_underrun_count);
>> -
>> -	if (irq_status & HDLCD_INTERRUPT_DMA_END)
>> -		atomic_inc(&hdlcd->dma_end_count);
>> -
>> -	if (irq_status & HDLCD_INTERRUPT_BUS_ERROR)
>> -		atomic_inc(&hdlcd->bus_error_count);
>> -
>> -	if (irq_status & HDLCD_INTERRUPT_VSYNC)
>> -		atomic_inc(&hdlcd->vsync_count);
>> -
>> -#endif
>> -	if (irq_status & HDLCD_INTERRUPT_VSYNC)
>> -		drm_crtc_handle_vblank(&hdlcd->crtc);
>> -
>> -	/* acknowledge interrupt(s) */
>> -	hdlcd_write(hdlcd, HDLCD_REG_INT_CLEAR, irq_status);
>> -
>> -	return IRQ_HANDLED;
>> -}
>> -
>> -static void hdlcd_irq_preinstall(struct drm_device *drm)
>> -{
>> -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
>> -	/* Ensure interrupts are disabled */
>> -	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, 0);
>> -	hdlcd_write(hdlcd, HDLCD_REG_INT_CLEAR, ~0);
>> -}
>> -
>> -static int hdlcd_irq_postinstall(struct drm_device *drm)
>> -{
>> -#ifdef CONFIG_DEBUG_FS
>> -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
>> -	unsigned long irq_mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
>> -
>> -	/* enable debug interrupts */
>> -	irq_mask |= HDLCD_DEBUG_INT_MASK;
>> -
>> -	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, irq_mask);
>> -#endif
>> -	return 0;
>> -}
>> -
>> -static void hdlcd_irq_uninstall(struct drm_device *drm)
>> -{
>> -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
>> -	/* disable all the interrupts that we might have enabled */
>> -	unsigned long irq_mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
>> -
>> -#ifdef CONFIG_DEBUG_FS
>> -	/* disable debug interrupts */
>> -	irq_mask &= ~HDLCD_DEBUG_INT_MASK;
>> -#endif
>> -
>> -	/* disable vsync interrupts */
>> -	irq_mask &= ~HDLCD_INTERRUPT_VSYNC;
>> -
>> -	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, irq_mask);
>> -}
>> -
>>   #ifdef CONFIG_DEBUG_FS
>>   static int hdlcd_show_underrun_count(struct seq_file *m, void *arg)
>>   {
>> @@ -236,10 +258,6 @@ DEFINE_DRM_GEM_CMA_FOPS(fops);
>>   
>>   static const struct drm_driver hdlcd_driver = {
>>   	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
>> -	.irq_handler = hdlcd_irq,
>> -	.irq_preinstall = hdlcd_irq_preinstall,
>> -	.irq_postinstall = hdlcd_irq_postinstall,
>> -	.irq_uninstall = hdlcd_irq_uninstall,
>>   	DRM_GEM_CMA_DRIVER_OPS,
>>   #ifdef CONFIG_DEBUG_FS
>>   	.debugfs_init = hdlcd_debugfs_init,
>> @@ -316,7 +334,7 @@ static int hdlcd_drm_bind(struct device *dev)
>>   err_unload:
>>   	of_node_put(hdlcd->crtc.port);
>>   	hdlcd->crtc.port = NULL;
>> -	drm_irq_uninstall(drm);
>> +	hdlcd_irq_uninstall(drm);
>>   	of_reserved_mem_device_release(drm->dev);
>>   err_free:
>>   	drm_mode_config_cleanup(drm);
>> @@ -338,7 +356,7 @@ static void hdlcd_drm_unbind(struct device *dev)
>>   	hdlcd->crtc.port = NULL;
>>   	pm_runtime_get_sync(dev);
>>   	drm_atomic_helper_shutdown(drm);
>> -	drm_irq_uninstall(drm);
>> +	hdlcd_irq_uninstall(drm);
>>   	pm_runtime_put(dev);
>>   	if (pm_runtime_enabled(dev))
>>   		pm_runtime_disable(dev);
>> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.h b/drivers/gpu/drm/arm/hdlcd_drv.h
>> index fd438d177b64..909c39c28487 100644
>> --- a/drivers/gpu/drm/arm/hdlcd_drv.h
>> +++ b/drivers/gpu/drm/arm/hdlcd_drv.h
>> @@ -11,6 +11,7 @@ struct hdlcd_drm_private {
>>   	struct clk			*clk;
>>   	struct drm_crtc			crtc;
>>   	struct drm_plane		*plane;
>> +	unsigned int			irq;
>>   #ifdef CONFIG_DEBUG_FS
>>   	atomic_t buffer_underrun_count;
>>   	atomic_t bus_error_count;
>> -- 
>> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]