| Message ID | 20231101-tidss-probe-v1-9-45149e0f9415@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tidss: Probe related fixes and cleanups | expand |
Hi Tomi, Thank you for the patch. On Wed, Nov 01, 2023 at 11:17:46AM +0200, Tomi Valkeinen wrote: > The IRQ setup code is overly complex. All we really need to do is > initialize the related fields in struct tidss_device, and request the > IRQ. > > We can drop all the HW accesses, as they are pointless: the driver will > set the IRQs correctly when it needs any of the IRQs, and at probe time > we have done a reset, so we know that all the IRQs are masked by default > in the hardware. Even for K2G ? > Thus we can combine the tidss_irq_preinstall() and > tidss_irq_postinstall() into the tidss_irq_install() function, drop the > HW accesses, and drop the use of spinlock, as this is done at init time > and there can be no races. > > We can also drop the HW access from the tidss_irq_uninstall(), as the > driver will anyway disable and suspend the hardware at remove time. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/gpu/drm/tidss/tidss_drv.c | 2 ++ > drivers/gpu/drm/tidss/tidss_irq.c | 54 ++++++--------------------------------- > 2 files changed, 10 insertions(+), 46 deletions(-) > > diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c > index 64914331715a..37693f30d98b 100644 > --- a/drivers/gpu/drm/tidss/tidss_drv.c > +++ b/drivers/gpu/drm/tidss/tidss_drv.c > @@ -138,6 +138,8 @@ static int tidss_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, tidss); > > + spin_lock_init(&tidss->wait_lock); > + > ret = dispc_init(tidss); > if (ret) { > dev_err(dev, "failed to initialize dispc: %d\n", ret); > diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c > index 0c681c7600bc..604334ef526a 100644 > --- a/drivers/gpu/drm/tidss/tidss_irq.c > +++ b/drivers/gpu/drm/tidss/tidss_irq.c > @@ -93,33 +93,21 @@ void tidss_irq_resume(struct tidss_device *tidss) > spin_unlock_irqrestore(&tidss->wait_lock, flags); > } > > -static void tidss_irq_preinstall(struct drm_device *ddev) > -{ > - struct tidss_device *tidss = to_tidss(ddev); > - > - spin_lock_init(&tidss->wait_lock); > - > - tidss_runtime_get(tidss); > - > - dispc_set_irqenable(tidss->dispc, 0); > - dispc_read_and_clear_irqstatus(tidss->dispc); > - > - tidss_runtime_put(tidss); > -} > - > -static void tidss_irq_postinstall(struct drm_device *ddev) > +int tidss_irq_install(struct drm_device *ddev, unsigned int irq) > { > struct tidss_device *tidss = to_tidss(ddev); > - unsigned long flags; > - unsigned int i; > + int ret; > > - tidss_runtime_get(tidss); > + if (irq == IRQ_NOTCONNECTED) > + return -ENOTCONN; > > - spin_lock_irqsave(&tidss->wait_lock, flags); > + ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev); > + if (ret) > + return ret; > > tidss->irq_mask = DSS_IRQ_DEVICE_OCP_ERR; > > - for (i = 0; i < tidss->num_crtcs; ++i) { > + for (unsigned int i = 0; i < tidss->num_crtcs; ++i) { > struct tidss_crtc *tcrtc = to_tidss_crtc(tidss->crtcs[i]); > > tidss->irq_mask |= DSS_IRQ_VP_SYNC_LOST(tcrtc->hw_videoport); > @@ -127,28 +115,6 @@ static void tidss_irq_postinstall(struct drm_device *ddev) > tidss->irq_mask |= DSS_IRQ_VP_FRAME_DONE(tcrtc->hw_videoport); > } > > - tidss_irq_update(tidss); > - > - spin_unlock_irqrestore(&tidss->wait_lock, flags); > - > - tidss_runtime_put(tidss); > -} > - > -int tidss_irq_install(struct drm_device *ddev, unsigned int irq) > -{ > - int ret; > - > - if (irq == IRQ_NOTCONNECTED) > - return -ENOTCONN; > - > - tidss_irq_preinstall(ddev); > - > - ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev); > - if (ret) > - return ret; > - > - tidss_irq_postinstall(ddev); > - > return 0; > } > > @@ -156,9 +122,5 @@ void tidss_irq_uninstall(struct drm_device *ddev) > { > struct tidss_device *tidss = to_tidss(ddev); > > - tidss_runtime_get(tidss); > - dispc_set_irqenable(tidss->dispc, 0); > - tidss_runtime_put(tidss); > - > free_irq(tidss->irq, ddev); > } >
On 01/11/2023 16:52, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Wed, Nov 01, 2023 at 11:17:46AM +0200, Tomi Valkeinen wrote: >> The IRQ setup code is overly complex. All we really need to do is >> initialize the related fields in struct tidss_device, and request the >> IRQ. >> >> We can drop all the HW accesses, as they are pointless: the driver will >> set the IRQs correctly when it needs any of the IRQs, and at probe time >> we have done a reset, so we know that all the IRQs are masked by default >> in the hardware. > > Even for K2G ? Good point. I'll add a simple manual reset for k2g, masking the IRQs and disabling the VPs. Tomi
diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c index 64914331715a..37693f30d98b 100644 --- a/drivers/gpu/drm/tidss/tidss_drv.c +++ b/drivers/gpu/drm/tidss/tidss_drv.c @@ -138,6 +138,8 @@ static int tidss_probe(struct platform_device *pdev) platform_set_drvdata(pdev, tidss); + spin_lock_init(&tidss->wait_lock); + ret = dispc_init(tidss); if (ret) { dev_err(dev, "failed to initialize dispc: %d\n", ret); diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c index 0c681c7600bc..604334ef526a 100644 --- a/drivers/gpu/drm/tidss/tidss_irq.c +++ b/drivers/gpu/drm/tidss/tidss_irq.c @@ -93,33 +93,21 @@ void tidss_irq_resume(struct tidss_device *tidss) spin_unlock_irqrestore(&tidss->wait_lock, flags); } -static void tidss_irq_preinstall(struct drm_device *ddev) -{ - struct tidss_device *tidss = to_tidss(ddev); - - spin_lock_init(&tidss->wait_lock); - - tidss_runtime_get(tidss); - - dispc_set_irqenable(tidss->dispc, 0); - dispc_read_and_clear_irqstatus(tidss->dispc); - - tidss_runtime_put(tidss); -} - -static void tidss_irq_postinstall(struct drm_device *ddev) +int tidss_irq_install(struct drm_device *ddev, unsigned int irq) { struct tidss_device *tidss = to_tidss(ddev); - unsigned long flags; - unsigned int i; + int ret; - tidss_runtime_get(tidss); + if (irq == IRQ_NOTCONNECTED) + return -ENOTCONN; - spin_lock_irqsave(&tidss->wait_lock, flags); + ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev); + if (ret) + return ret; tidss->irq_mask = DSS_IRQ_DEVICE_OCP_ERR; - for (i = 0; i < tidss->num_crtcs; ++i) { + for (unsigned int i = 0; i < tidss->num_crtcs; ++i) { struct tidss_crtc *tcrtc = to_tidss_crtc(tidss->crtcs[i]); tidss->irq_mask |= DSS_IRQ_VP_SYNC_LOST(tcrtc->hw_videoport); @@ -127,28 +115,6 @@ static void tidss_irq_postinstall(struct drm_device *ddev) tidss->irq_mask |= DSS_IRQ_VP_FRAME_DONE(tcrtc->hw_videoport); } - tidss_irq_update(tidss); - - spin_unlock_irqrestore(&tidss->wait_lock, flags); - - tidss_runtime_put(tidss); -} - -int tidss_irq_install(struct drm_device *ddev, unsigned int irq) -{ - int ret; - - if (irq == IRQ_NOTCONNECTED) - return -ENOTCONN; - - tidss_irq_preinstall(ddev); - - ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev); - if (ret) - return ret; - - tidss_irq_postinstall(ddev); - return 0; } @@ -156,9 +122,5 @@ void tidss_irq_uninstall(struct drm_device *ddev) { struct tidss_device *tidss = to_tidss(ddev); - tidss_runtime_get(tidss); - dispc_set_irqenable(tidss->dispc, 0); - tidss_runtime_put(tidss); - free_irq(tidss->irq, ddev); }
The IRQ setup code is overly complex. All we really need to do is initialize the related fields in struct tidss_device, and request the IRQ. We can drop all the HW accesses, as they are pointless: the driver will set the IRQs correctly when it needs any of the IRQs, and at probe time we have done a reset, so we know that all the IRQs are masked by default in the hardware. Thus we can combine the tidss_irq_preinstall() and tidss_irq_postinstall() into the tidss_irq_install() function, drop the HW accesses, and drop the use of spinlock, as this is done at init time and there can be no races. We can also drop the HW access from the tidss_irq_uninstall(), as the driver will anyway disable and suspend the hardware at remove time. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/gpu/drm/tidss/tidss_drv.c | 2 ++ drivers/gpu/drm/tidss/tidss_irq.c | 54 ++++++--------------------------------- 2 files changed, 10 insertions(+), 46 deletions(-)