Re: [PATCH] drm/i915: Flush the pending flips on the CRTC before modification

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

On Fri, Sep 28, 2012 at 11:22:45AM +0100, Chris Wilson wrote:
> On Fri, 28 Sep 2012 13:05:51 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Sep 27, 2012 at 09:25:58PM +0100, Chris Wilson wrote:
> > > This was meant to be the purpose of the
> > > intel_crtc_wait_for_pending_flips() function which is called whilst
> > > preparing the CRTC for a modeset or before disabling. However, as Ville
> > > Syrjala pointed out, we set the pending flip notification on the old
> > > framebuffer that is no longer attached to the CRTC by the time we come
> > > to flush the pending operations. Instead, we can simply wait on the
> > > pending unpin work to be finished on this CRTC, knowning that the
> > > hardware has therefore finished modifying the registers, before proceeding
> > > with our direct access.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++++--
> > >  1 file changed, 22 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index a262326..39df185 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2896,15 +2896,36 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc)
> > >  	udelay(100);
> > >  }
> > >  
> > > +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);
> > 
> > The locking looks pointless here.
> 
> It does rather. Being pedagogical we should probably leave a mb of some
> sort in there...
> 
>   pending = to_intel_crtc(crtc)->unpin_work != NULL;
>   smp_rmb();
> 
> with the existing spin_lock providing the necessary barriers before the
> wake_up();

IIRC wake_up()/wait_event() already have the necessary barriers. And
based on a quick glance Documentation/memory-barriers.txt seems to
agree with me.

-- 
Ville Syrjälä
Intel OTC