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

I had another look at this, and I think you're aiming for something other
than what this patch is trying to do.

The thing is that we are holding struct_mutex while waiting for pending
flips here, so we just need to get out asap to allow the reset work do
its thing.

Completing pending page flips when a reset occurs is separate issue.
This code never did any of that, and I don't see why it should. We
would need to complete them anyway regardless of whether anyone is
currently waiting for them.

Perhaps we can just call intel_finish_page_flip() from the reset
code and call it a day. I suppose doing that could end up unpinning
the current scanout buffer in case the display registers were never
written with the new values. One solution for that would be to always
do a set_base() after a reset. That would make sure we end up showing
the latest buffer at least, although the contents could be total
garbage.

-- 
Ville Syrjälä
Intel OTC