| Message ID | 20220311170601.50995-2-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/7] drm: mxsfb: Simplify LCDIF clock handling | expand |
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut: > The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation > from the LCDIF block already and this is called in mxsfb_load() before > request_irq(), so explicitly disabling IRQ using custom function like > mxsfb_irq_disable() is not needed, remove it. > Have you checked that the drm_vblank_off in probe actually results in a call to mxsfb_crtc_disable_vblank? From my reading of the core code, this should be a no-op without a previous drm_vblank_on, so it would not result in the desired masking before the IRQ is requested. Regards, Lucas > The request_irq() call > would return -ENOTCONN if IRQ is IRQ_NOTCONNECTED already, so remove > the unnecessary check. Finally, remove both mxsfb_irq_install() and > mxsfb_irq_uninstall() as well, since they are no longer useful. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Alexander Stein <alexander.stein@ew.tq-group.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Peng Fan <peng.fan@nxp.com> > Cc: Robby Cai <robby.cai@nxp.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Stefan Agner <stefan@agner.ch> > --- > V2: No change > --- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 38 +++++++------------------------ > 1 file changed, 8 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index c790aeff0a6f0..94cafff7f68d5 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -157,33 +157,6 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data) > return IRQ_HANDLED; > } > > -static void mxsfb_irq_disable(struct drm_device *drm) > -{ > - struct mxsfb_drm_private *mxsfb = drm->dev_private; > - > - /* Disable and clear VBLANK IRQ */ > - writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR); > - writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); > -} > - > -static int mxsfb_irq_install(struct drm_device *dev, int irq) > -{ > - if (irq == IRQ_NOTCONNECTED) > - return -ENOTCONN; > - > - mxsfb_irq_disable(dev); > - > - return request_irq(irq, mxsfb_irq_handler, 0, dev->driver->name, dev); > -} > - > -static void mxsfb_irq_uninstall(struct drm_device *dev) > -{ > - struct mxsfb_drm_private *mxsfb = dev->dev_private; > - > - mxsfb_irq_disable(dev); > - free_irq(mxsfb->irq, dev); > -} > - > static int mxsfb_load(struct drm_device *drm, > const struct mxsfb_devdata *devdata) > { > @@ -259,7 +232,8 @@ static int mxsfb_load(struct drm_device *drm, > return ret; > mxsfb->irq = ret; > > - ret = mxsfb_irq_install(drm, mxsfb->irq); > + ret = request_irq(mxsfb->irq, mxsfb_irq_handler, 0, > + drm->driver->name, drm); > if (ret < 0) { > dev_err(drm->dev, "Failed to install IRQ handler\n"); > return ret; > @@ -276,16 +250,20 @@ static int mxsfb_load(struct drm_device *drm, > > static void mxsfb_unload(struct drm_device *drm) > { > + struct mxsfb_drm_private *mxsfb = drm->dev_private; > + > pm_runtime_get_sync(drm->dev); > > + drm_crtc_vblank_off(&mxsfb->crtc); > + > drm_kms_helper_poll_fini(drm); > drm_mode_config_cleanup(drm); > > - mxsfb_irq_uninstall(drm); > - > pm_runtime_put_sync(drm->dev); > pm_runtime_disable(drm->dev); > > + free_irq(mxsfb->irq, drm->dev); > + > drm->dev_private = NULL; > } >
On 4/6/22 21:41, Lucas Stach wrote: > Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut: >> The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation >> from the LCDIF block already and this is called in mxsfb_load() before >> request_irq(), so explicitly disabling IRQ using custom function like >> mxsfb_irq_disable() is not needed, remove it. >> > > Have you checked that the drm_vblank_off in probe actually results in a > call to mxsfb_crtc_disable_vblank? From my reading of the core code, > this should be a no-op without a previous drm_vblank_on, so it would > not result in the desired masking before the IRQ is requested. I must've missed the vblank->enabled check, but then, I am also not getting any interrupts, so presumably they are already disabled after the IP is reset.
Am Donnerstag, dem 07.04.2022 um 00:03 +0200 schrieb Marek Vasut: > On 4/6/22 21:41, Lucas Stach wrote: > > Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut: > > > The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation > > > from the LCDIF block already and this is called in mxsfb_load() before > > > request_irq(), so explicitly disabling IRQ using custom function like > > > mxsfb_irq_disable() is not needed, remove it. > > > > > > > Have you checked that the drm_vblank_off in probe actually results in a > > call to mxsfb_crtc_disable_vblank? From my reading of the core code, > > this should be a no-op without a previous drm_vblank_on, so it would > > not result in the desired masking before the IRQ is requested. > > I must've missed the vblank->enabled check, but then, I am also not > getting any interrupts, so presumably they are already disabled after > the IP is reset. Yep, it should be the default for every peripheral to not send any unsolicited interrupts. But then I don't see explicit reset of the IP in the driver probe. So maybe this is a workaround for situation where something running before Linux has already enabled the display controller and for whatever reason also enabled the interrupt requests? Either we should have a proper reset of the block in the probe path, or this interrupt masking must be kept in one form or the other. My vote would be on just masking the IRQs and dropping the useless drm_vblank_off. Regards, Lucas
On 4/7/22 10:01, Lucas Stach wrote: > Am Donnerstag, dem 07.04.2022 um 00:03 +0200 schrieb Marek Vasut: >> On 4/6/22 21:41, Lucas Stach wrote: >>> Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut: >>>> The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation >>>> from the LCDIF block already and this is called in mxsfb_load() before >>>> request_irq(), so explicitly disabling IRQ using custom function like >>>> mxsfb_irq_disable() is not needed, remove it. >>>> >>> >>> Have you checked that the drm_vblank_off in probe actually results in a >>> call to mxsfb_crtc_disable_vblank? From my reading of the core code, >>> this should be a no-op without a previous drm_vblank_on, so it would >>> not result in the desired masking before the IRQ is requested. >> >> I must've missed the vblank->enabled check, but then, I am also not >> getting any interrupts, so presumably they are already disabled after >> the IP is reset. > > Yep, it should be the default for every peripheral to not send any > unsolicited interrupts. But then I don't see explicit reset of the IP > in the driver probe. So maybe this is a workaround for situation where > something running before Linux has already enabled the display > controller and for whatever reason also enabled the interrupt > requests? > > Either we should have a proper reset of the block in the probe path, or > this interrupt masking must be kept in one form or the other. My vote > would be on just masking the IRQs and dropping the useless > drm_vblank_off. I can just discard this patch, OK.
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index c790aeff0a6f0..94cafff7f68d5 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c @@ -157,33 +157,6 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data) return IRQ_HANDLED; } -static void mxsfb_irq_disable(struct drm_device *drm) -{ - struct mxsfb_drm_private *mxsfb = drm->dev_private; - - /* Disable and clear VBLANK IRQ */ - writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR); - writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); -} - -static int mxsfb_irq_install(struct drm_device *dev, int irq) -{ - if (irq == IRQ_NOTCONNECTED) - return -ENOTCONN; - - mxsfb_irq_disable(dev); - - return request_irq(irq, mxsfb_irq_handler, 0, dev->driver->name, dev); -} - -static void mxsfb_irq_uninstall(struct drm_device *dev) -{ - struct mxsfb_drm_private *mxsfb = dev->dev_private; - - mxsfb_irq_disable(dev); - free_irq(mxsfb->irq, dev); -} - static int mxsfb_load(struct drm_device *drm, const struct mxsfb_devdata *devdata) { @@ -259,7 +232,8 @@ static int mxsfb_load(struct drm_device *drm, return ret; mxsfb->irq = ret; - ret = mxsfb_irq_install(drm, mxsfb->irq); + ret = request_irq(mxsfb->irq, mxsfb_irq_handler, 0, + drm->driver->name, drm); if (ret < 0) { dev_err(drm->dev, "Failed to install IRQ handler\n"); return ret; @@ -276,16 +250,20 @@ static int mxsfb_load(struct drm_device *drm, static void mxsfb_unload(struct drm_device *drm) { + struct mxsfb_drm_private *mxsfb = drm->dev_private; + pm_runtime_get_sync(drm->dev); + drm_crtc_vblank_off(&mxsfb->crtc); + drm_kms_helper_poll_fini(drm); drm_mode_config_cleanup(drm); - mxsfb_irq_uninstall(drm); - pm_runtime_put_sync(drm->dev); pm_runtime_disable(drm->dev); + free_irq(mxsfb->irq, drm->dev); + drm->dev_private = NULL; }
The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation from the LCDIF block already and this is called in mxsfb_load() before request_irq(), so explicitly disabling IRQ using custom function like mxsfb_irq_disable() is not needed, remove it. The request_irq() call would return -ENOTCONN if IRQ is IRQ_NOTCONNECTED already, so remove the unnecessary check. Finally, remove both mxsfb_irq_install() and mxsfb_irq_uninstall() as well, since they are no longer useful. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Alexander Stein <alexander.stein@ew.tq-group.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Peng Fan <peng.fan@nxp.com> Cc: Robby Cai <robby.cai@nxp.com> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Stefan Agner <stefan@agner.ch> --- V2: No change --- drivers/gpu/drm/mxsfb/mxsfb_drv.c | 38 +++++++------------------------ 1 file changed, 8 insertions(+), 30 deletions(-)