Re: [PATCH v3 2/4] drm/i915: Wait for pending flips in intel_pipe_set_base()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
> > Reviewed-by: Chris Wilson <chris@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