Re: [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()

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

On Fri, Nov 02, 2012 at 03:25:59PM +0000, Chris Wilson wrote:
> On Thu,  1 Nov 2012 20:06:00 +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.
> > 
> > Now even the pending_flip counter is gone, so we should just
> > use intel_crtc_wait_for_pending_flips(), but since we're already holding
> > struct_mutex when we would call that function, we need another version
> > of it, that itself doesn't lock struct_mutex.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   51 +++++++++++++++++++++------------
> >  1 files changed, 32 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1a38267..7bf4749 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2228,6 +2228,37 @@ 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 (atomic_read(&dev_priv->mm.wedged))
> > +		return false;
> > +
> > +	spin_lock_irqsave(&dev->event_lock, flags);
> > +	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> > +	spin_unlock_irqrestore(&dev->event_lock, flags);
> > +
> > +	return pending;
> > +}
> > +
> > +static void intel_crtc_wait_for_pending_flips_locked(struct drm_crtc *crtc)
> > +{
> 
> Can we rearrange this such that the waiting logic is inside _locked()
> and then intel_crtc_wiat_for_pending_flips() becomes a wrapper that
> acquires the struct_mutex and then calls _locked()? Just to keep the
> code simpler at the expense of the pathological case.

Yeah that looks doable. It does mean we'll be holding struct_mutex
around the wait_event() always. As I was already doing that for the
panning case, doing the same in the crtc_disable() case shouldn't
be any worse.

But now I started to wonder a bit about the performance implications
of keeping struct_mutex locked for that long...

-- 
Ville Syrjälä
Intel OTC