[PATCH] drm/irq: Don't disable vblank interrupts when already disabled

[PATCH] drm/irq: Don't disable vblank interrupts when already disabled

[Laurent Pinchart]

The .enable_vblank() operation is only called when vblank interrupts are
disabled, but no similar check exists when disabling vblank interrupts.
This leads to .disable_vblank() being called with vblank interrupts
already disabled and the device possibly runtime suspended. As the
operation is called with a spinlock held drivers can't runtime resume
the device there and thus must avoid touching device registers in that
case, requiring vblank refcounting.

As the DRM core tracks whether vblank interrupts are enabled just skip
the .disable_vblank() call when the interrupts are already disabled.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_irq.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

I've tested this patch in my omapdrm development branch available at

	git://linuxtv.org/pinchartl/fbdev.git omapdrm/next

It fixes a crash caused by accessing the hardware registers from the
.disable_vblank() operation with the device runtime suspended.

I'll post the omapdrm patches after completing conversion to the atomic
updates API.

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 4d79dad..cd384b0 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -185,8 +185,15 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 		return;
 	}
 
-	dev->driver->disable_vblank(dev, crtc);
-	vblank->enabled = false;
+	/*
+	 * Only disable vblank interrupts if they're enabled. This avoids
+	 * calling the ->disable_vblank() operation in atomic context with the
+	 * hardware potentially runtime suspended.
+	 */
+	if (vblank->enabled) {
+		dev->driver->disable_vblank(dev, crtc);
+		vblank->enabled = false;
+	}
 
 	/* No further vblank irq's will be processed after
 	 * this point. Get current hardware vblank count and
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/irq: Don't disable vblank interrupts when already disabled

[Michel Dänzer]

On 29.01.2015 08:09, Laurent Pinchart wrote:
> The .enable_vblank() operation is only called when vblank interrupts are
> disabled, but no similar check exists when disabling vblank interrupts.
> This leads to .disable_vblank() being called with vblank interrupts
> already disabled and the device possibly runtime suspended. As the
> operation is called with a spinlock held drivers can't runtime resume
> the device there and thus must avoid touching device registers in that
> case, requiring vblank refcounting.
> 
> As the DRM core tracks whether vblank interrupts are enabled just skip
> the .disable_vblank() call when the interrupts are already disabled.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/irq: Don't disable vblank interrupts when already disabled

[Daniel Vetter]

On Thu, Jan 29, 2015 at 12:15:03PM +0900, Michel Dänzer wrote:
> On 29.01.2015 08:09, Laurent Pinchart wrote:
> > The .enable_vblank() operation is only called when vblank interrupts are
> > disabled, but no similar check exists when disabling vblank interrupts.
> > This leads to .disable_vblank() being called with vblank interrupts
> > already disabled and the device possibly runtime suspended. As the
> > operation is called with a spinlock held drivers can't runtime resume
> > the device there and thus must avoid touching device registers in that
> > case, requiring vblank refcounting.
> > 
> > As the DRM core tracks whether vblank interrupts are enabled just skip
> > the .disable_vblank() call when the interrupts are already disabled.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>

Yeah makes sense, pulled into my drm misc pile.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel