| Message ID | 1400508575-8041-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 19 May 2014 16:09:35 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > We can apperently miss them, but breaking the entire driver hampers > testing. So bail out after one minute, our customerary "this is a lost > cause" timeout. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=78383 > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 0f8f9bcb3012..6eca24d8b282 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3284,8 +3284,9 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) > > WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue)); > > - wait_event(dev_priv->pending_flip_queue, > - !intel_crtc_has_pending_flip(crtc)); > + WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue, > + !intel_crtc_has_pending_flip(crtc), > + 60*HZ) == 0); > > mutex_lock(&dev->struct_mutex); > intel_finish_fb(crtc->primary->fb); Updating our page flip ioctl man page (hah!) with the timeout info would be good, in case people like Mario queue flips for after lunch. :)
On Mon, May 19, 2014 at 08:06:06AM -0700, Jesse Barnes wrote: > On Mon, 19 May 2014 16:09:35 +0200 > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > We can apperently miss them, but breaking the entire driver hampers > > testing. So bail out after one minute, our customerary "this is a lost > > cause" timeout. > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=78383 > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/intel_display.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 0f8f9bcb3012..6eca24d8b282 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -3284,8 +3284,9 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) > > > > WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue)); > > > > - wait_event(dev_priv->pending_flip_queue, > > - !intel_crtc_has_pending_flip(crtc)); > > + WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue, > > + !intel_crtc_has_pending_flip(crtc), > > + 60*HZ) == 0); > > > > mutex_lock(&dev->struct_mutex); > > intel_finish_fb(crtc->primary->fb); > > Updating our page flip ioctl man page (hah!) with the timeout info > would be good, in case people like Mario queue flips for after lunch. :) We don't do that in the kernel though, we only ever queue flips for the next vblank after rendering completed. Completed rendering we can detect (and have 60s timeouts in other places where the hangcheck isn't guaranteed to be around already), the additional vblank is negligible imo. Of course if we add support for flip queues in the kernel we might need to cancel outstanding flips properly when we kill the crtc, like we already do for vblank events. So imo no need to document anything. -Daniel
On Mon, 19 May 2014 17:18:40 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, May 19, 2014 at 08:06:06AM -0700, Jesse Barnes wrote: > > On Mon, 19 May 2014 16:09:35 +0200 > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > We can apperently miss them, but breaking the entire driver hampers > > > testing. So bail out after one minute, our customerary "this is a lost > > > cause" timeout. > > > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=78383 > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 0f8f9bcb3012..6eca24d8b282 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -3284,8 +3284,9 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) > > > > > > WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue)); > > > > > > - wait_event(dev_priv->pending_flip_queue, > > > - !intel_crtc_has_pending_flip(crtc)); > > > + WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue, > > > + !intel_crtc_has_pending_flip(crtc), > > > + 60*HZ) == 0); > > > > > > mutex_lock(&dev->struct_mutex); > > > intel_finish_fb(crtc->primary->fb); > > > > Updating our page flip ioctl man page (hah!) with the timeout info > > would be good, in case people like Mario queue flips for after lunch. :) > > We don't do that in the kernel though, we only ever queue flips for the > next vblank after rendering completed. Completed rendering we can detect > (and have 60s timeouts in other places where the hangcheck isn't > guaranteed to be around already), the additional vblank is negligible imo. > > Of course if we add support for flip queues in the kernel we might need to > cancel outstanding flips properly when we kill the crtc, like we already > do for vblank events. > > So imo no need to document anything. Ah right so we'd only be affected here by ridiculous refresh rates... so yeah should be fine. Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On Mon, May 19, 2014 at 08:35:27AM -0700, Jesse Barnes wrote: > On Mon, 19 May 2014 17:18:40 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Mon, May 19, 2014 at 08:06:06AM -0700, Jesse Barnes wrote: > > > On Mon, 19 May 2014 16:09:35 +0200 > > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > > > We can apperently miss them, but breaking the entire driver hampers > > > > testing. So bail out after one minute, our customerary "this is a lost > > > > cause" timeout. > > > > > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=78383 > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index 0f8f9bcb3012..6eca24d8b282 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -3284,8 +3284,9 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) > > > > > > > > WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue)); > > > > > > > > - wait_event(dev_priv->pending_flip_queue, > > > > - !intel_crtc_has_pending_flip(crtc)); > > > > + WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue, > > > > + !intel_crtc_has_pending_flip(crtc), > > > > + 60*HZ) == 0); > > > > > > > > mutex_lock(&dev->struct_mutex); > > > > intel_finish_fb(crtc->primary->fb); > > > > > > Updating our page flip ioctl man page (hah!) with the timeout info > > > would be good, in case people like Mario queue flips for after lunch. :) > > > > We don't do that in the kernel though, we only ever queue flips for the > > next vblank after rendering completed. Completed rendering we can detect > > (and have 60s timeouts in other places where the hangcheck isn't > > guaranteed to be around already), the additional vblank is negligible imo. > > > > Of course if we add support for flip queues in the kernel we might need to > > cancel outstanding flips properly when we kill the crtc, like we already > > do for vblank events. > > > > So imo no need to document anything. > > Ah right so we'd only be affected here by ridiculous refresh rates... > so yeah should be fine. If we are that concerned we could factor those into the timeout, say hangcheck + 10 * frame_interval. > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> Anyhow, I'd been meaning to do this myself, just kept wondering if perhaps hangcheck was a better place to drive it from. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Mon, May 19, 2014 at 04:41:31PM +0100, Chris Wilson wrote: > On Mon, May 19, 2014 at 08:35:27AM -0700, Jesse Barnes wrote: > > On Mon, 19 May 2014 17:18:40 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Mon, May 19, 2014 at 08:06:06AM -0700, Jesse Barnes wrote: > > > > On Mon, 19 May 2014 16:09:35 +0200 > > > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > > > > > We can apperently miss them, but breaking the entire driver hampers > > > > > testing. So bail out after one minute, our customerary "this is a lost > > > > > cause" timeout. > > > > > > > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=78383 > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_display.c | 5 +++-- > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > > index 0f8f9bcb3012..6eca24d8b282 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -3284,8 +3284,9 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) > > > > > > > > > > WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue)); > > > > > > > > > > - wait_event(dev_priv->pending_flip_queue, > > > > > - !intel_crtc_has_pending_flip(crtc)); > > > > > + WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue, > > > > > + !intel_crtc_has_pending_flip(crtc), > > > > > + 60*HZ) == 0); > > > > > > > > > > mutex_lock(&dev->struct_mutex); > > > > > intel_finish_fb(crtc->primary->fb); > > > > > > > > Updating our page flip ioctl man page (hah!) with the timeout info > > > > would be good, in case people like Mario queue flips for after lunch. :) > > > > > > We don't do that in the kernel though, we only ever queue flips for the > > > next vblank after rendering completed. Completed rendering we can detect > > > (and have 60s timeouts in other places where the hangcheck isn't > > > guaranteed to be around already), the additional vblank is negligible imo. > > > > > > Of course if we add support for flip queues in the kernel we might need to > > > cancel outstanding flips properly when we kill the crtc, like we already > > > do for vblank events. > > > > > > So imo no need to document anything. > > > > Ah right so we'd only be affected here by ridiculous refresh rates... > > so yeah should be fine. > > If we are that concerned we could factor those into the timeout, > say hangcheck + 10 * frame_interval. > > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > Anyhow, I'd been meaning to do this myself, just kept wondering if > perhaps hangcheck was a better place to drive it from. We still lack the add_request for flips ... And imo there's too many fragile parts in our pageflip sequence to go wrong, so a simple timeout at the very end seems best. Patch pulled in. -Daniel > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0f8f9bcb3012..6eca24d8b282 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3284,8 +3284,9 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue)); - wait_event(dev_priv->pending_flip_queue, - !intel_crtc_has_pending_flip(crtc)); + WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue, + !intel_crtc_has_pending_flip(crtc), + 60*HZ) == 0); mutex_lock(&dev->struct_mutex); intel_finish_fb(crtc->primary->fb);
We can apperently miss them, but breaking the entire driver hampers testing. So bail out after one minute, our customerary "this is a lost cause" timeout. References: https://bugs.freedesktop.org/show_bug.cgi?id=78383 Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)