diff mbox series

[v2,2/7] drm: mxsfb: Simplify LCDIF IRQ handling

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

Commit Message

Marek Vasut March 11, 2022, 5:05 p.m. UTC
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(-)

Comments

Lucas Stach April 6, 2022, 7:41 p.m. UTC | #1
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;
>  }
>
Marek Vasut April 6, 2022, 10:03 p.m. UTC | #2
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.
Lucas Stach April 7, 2022, 8:01 a.m. UTC | #3
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
Marek Vasut April 17, 2022, 1:04 a.m. UTC | #4
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 mbox series

Patch

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;
 }