[PATCH 02/14] drm/arm/hdlcd: Convert to Linux IRQ interfaces
Thomas Zimmermann
tzimmermann at suse.de
Wed Jul 28 18:15:04 UTC 2021
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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20210728/09c95c18/attachment-0001.sig>
More information about the amd-gfx
mailing list