On Apr 27, 2011, at 10:58 AM, dri-devel-request@lists.freedesktop.org wrote:
Message: 5 Date: Wed, 27 Apr 2011 10:38:14 +0200 From: Michel D?nzer michel@daenzer.net Subject: Re: [PATCH 2/3] drm: Warn if vblank state has become inconsistent. To: christopher.halse.rogers@canonical.com Cc: dri-devel@lists.freedesktop.org Message-ID: 1303893494.5633.129.camel@thor.local Content-Type: text/plain; charset="UTF-8"
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
After emitting all the waiting vblank events no-one should hold a vblank reference. Emit a warning if this is not the case.
Signed-off-by: Christopher James Halse Rogers
christopher.halse.rogers@canonical.com
drivers/gpu/drm/drm_irq.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a1f12cb..72407fa 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) e->event.sequence); }
- WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0); spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
} EXPORT_SYMBOL(drm_vblank_off);
Reviewed-by: Michel D?nzer michel@daenzer.net
Any pending kms pageflip will also hold a reference on the vblank of a crtc, so having the refcount non-zero there is not really a sign of inconsistency, so i'm not sure if a warning is appropriate there.
-mario
********************************************************************* Mario Kleiner Max Planck Institute for Biological Cybernetics Spemannstr. 38 72076 Tuebingen Germany
e-mail: mario.kleiner@tuebingen.mpg.de office: +49 (0)7071/601-1623 fax: +49 (0)7071/601-616 www: http://www.kyb.tuebingen.mpg.de/~kleinerm ********************************************************************* "For a successful technology, reality must take precedence over public relations, for Nature cannot be fooled." (Richard Feynman)
On Wed, 2011-04-27 at 17:50 +0200, Mario Kleiner wrote:
On Apr 27, 2011, at 10:58 AM, dri-devel-request@lists.freedesktop.org wrote:
Message: 5 Date: Wed, 27 Apr 2011 10:38:14 +0200 From: Michel D?nzer michel@daenzer.net Subject: Re: [PATCH 2/3] drm: Warn if vblank state has become inconsistent. To: christopher.halse.rogers@canonical.com Cc: dri-devel@lists.freedesktop.org Message-ID: 1303893494.5633.129.camel@thor.local Content-Type: text/plain; charset="UTF-8"
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
After emitting all the waiting vblank events no-one should hold a vblank reference. Emit a warning if this is not the case.
Signed-off-by: Christopher James Halse Rogers
christopher.halse.rogers@canonical.com
drivers/gpu/drm/drm_irq.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a1f12cb..72407fa 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) e->event.sequence); }
- WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0); spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
} EXPORT_SYMBOL(drm_vblank_off);
Reviewed-by: Michel D?nzer michel@daenzer.net
Any pending kms pageflip will also hold a reference on the vblank of a crtc, so having the refcount non-zero there is not really a sign of inconsistency, so i'm not sure if a warning is appropriate there.
But at this point the vblank interrupts have been disabled, or at least the driver's disable function has been called. Will that not mean that any pending pageflips will wait indefinitely for a vblank interrupt that's not going to come - ie: exactly the state we're warning against?
On Apr 28, 2011, at 9:47 AM, Christopher James Halse Rogers wrote:
On Wed, 2011-04-27 at 17:50 +0200, Mario Kleiner wrote:
On Apr 27, 2011, at 10:58 AM, dri-devel-request@lists.freedesktop.org wrote:
Message: 5 Date: Wed, 27 Apr 2011 10:38:14 +0200 From: Michel D?nzer michel@daenzer.net Subject: Re: [PATCH 2/3] drm: Warn if vblank state has become inconsistent. To: christopher.halse.rogers@canonical.com Cc: dri-devel@lists.freedesktop.org Message-ID: 1303893494.5633.129.camel@thor.local Content-Type: text/plain; charset="UTF-8"
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
After emitting all the waiting vblank events no-one should hold a vblank reference. Emit a warning if this is not the case.
Signed-off-by: Christopher James Halse Rogers
christopher.halse.rogers@canonical.com
drivers/gpu/drm/drm_irq.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a1f12cb..72407fa 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) e->event.sequence); }
- WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0); spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
} EXPORT_SYMBOL(drm_vblank_off);
Reviewed-by: Michel D?nzer michel@daenzer.net
Any pending kms pageflip will also hold a reference on the vblank of a crtc, so having the refcount non-zero there is not really a sign of inconsistency, so i'm not sure if a warning is appropriate there.
But at this point the vblank interrupts have been disabled, or at least the driver's disable function has been called. Will that not mean that any pending pageflips will wait indefinitely for a vblank interrupt that's not going to come - ie: exactly the state we're warning against?
Ok, if that is the state we're warning against, then the warning makes sense. But it would make more sense to somehow handle this more gracefully. The pageflip completion path does depend on vblank irq's to unpin buffers, release fences and notify user apps of swap completion. Hanging there due to disabled vblank irq sounds bad.
Otoh the only caller of drm_vblank_off() i can find in 2.6.38 is the crtc disable code in intel_display.c and those routines protect against pending pageflips - they wait for all pageflips to finish before calling drm_vblank_off().
If this warning is just a precaution against possible future bugs then i happily rest my case. -mario
dri-devel@lists.freedesktop.org
-
Christopher James Halse Rogers -
Mario Kleiner