On Wed, Nov 28, 2012 at 09:51:18PM +0100, Daniel Vetter wrote:
> On Tue, Nov 27, 2012 at 08:34:56PM +0200, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrj?l? <ville.syrjala at linux.intel.com>
> >
> > intel_pipe_set_base() never actually waited for any pending page flips
> > on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
> > the current front buffer. But the pending flips were actually tracked
> > in the BO of the previous front buffer, so the call to intel_finish_fb()
> > never did anything useful.
> >
> > intel_crtc_wait_for_pending_flips() is the current _working_ way to wait
> > for pending page flips. So use it in intel_pipe_set_base() too. Some
> > refactoring was necessary to avoid locking struct_mutex twice.
> >
> > v2: Shuffle the code around so that intel_crtc_wait_for_pending_flips()
> > just wraps intel_crtc_wait_for_pending_flips_locked().
> >
> > v3: Kill leftover wait_event() in intel_finish_fb()
> >
> > Signed-off-by: Ville Syrj?l? <ville.syrjala at linux.intel.com>
> > Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 80 +++++++++++++++++----------------
> > 1 files changed, 41 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 8c2d810..ea710af 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2238,10 +2238,6 @@ intel_finish_fb(struct drm_framebuffer *old_fb)
> > bool was_interruptible = dev_priv->mm.interruptible;
> > int ret;
> >
> > - wait_event(dev_priv->pending_flip_queue,
> > - i915_reset_in_progress(&dev_priv->gpu_error) ||
> > - atomic_read(&obj->pending_flip) == 0);
> > -
> > /* Big Hammer, we also need to ensure that any pending
> > * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> > * current scanout is retired before unpinning the old
> > @@ -2284,6 +2280,46 @@ static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y)
> > }
> > }
> >
> > +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> > +{
> > + struct drm_device *dev = crtc->dev;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + unsigned long flags;
> > + bool pending;
> > +
> > + if (i915_reset_in_progress(&dev_priv->gpu_error))
> > + return false;
>
> This check is not enough, since a full gpu reset might have happened while
> we didn't look. Which means that we'll still be waiting forever. To really
> close all gaps and races I think we need to full multi-state transistions
> like it's already implemented in __wait_seqno (minus the first kick, since
> no one is holding the dev->struct_mutex while waiting for a pageflip to
> complete). So
> - we need a reset_counter here like in wait_seqno
> - need to reset the unpin_work state (and fire off any pending drm events
> while at it) to unblock kernel waiters and userspace
>
> Also: igt test highly preferred ;-) Best would be to convert the hangman
> test over to the subtest infrastructure (maybe easier when first ported
> python and using argpars, but I don't mind bash's getopt support) and then
> adding a new subtestcase which exercises flips vs. hangs.
I'm going to plead innocent on this one. I just copied code around, but
you wrote it ;)
--
Ville Syrj?l?
Intel OTC
