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 >> --- >> 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 >> #include >> #include >> -#include >> #include >> #include >> #include >> @@ -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 > > >> + >> + 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