Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing

Chris Wilson
2014-06-17T07:55:20Z

On Tue, Jun 17, 2014 at 09:48:39AM +0200, Daniel Vetter wrote:
> On Tue, Jun 17, 2014 at 07:57:22AM +0100, Chris Wilson wrote:
> > On Mon, Jun 16, 2014 at 07:51:34PM +0200, Daniel Vetter wrote:
> > > +/**
> > > + * intel_fb_flip_prepare - flip frontbuffer
> > > + * @obj: GEM object to flush
> > > + *
> > > + * This function gets called after scheduling a flip on @obj. The actual
> > > + * frontbuffer flushing will be delayed until completion is signalled with
> > > + * intel_frontbuffer_flip_complete. If an invalidate happens in between this
> > > + * flush will be cancelled.
> > > + *
> > > + * Note that the frontbuffer tracking bits in obj->frontbuffer_bits must have
> > > + * been updated already.
> > > + */
> > > +void intel_fb_flip_prepare(struct drm_i915_gem_object *obj)
> > > +{
> > > + struct drm_device *dev = obj->base.dev;
> > > + struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > + WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> > > + WARN_ON(!obj->frontbuffer_bits);
> > > +
> > > + mutex_lock(&dev_priv->fb_tracking.lock);
> > > + dev_priv->fb_tracking.flip_bits
> > > + |= obj->frontbuffer_bits;
> > > + mutex_unlock(&dev_priv->fb_tracking.lock);
> > > +}
> > > +
> > > +/**
> > > + * intel_frontbuffer_flip_complete - complete frontbuffer flip
> > > + * @dev: DRM device
> > > + * @frontbuffer_bits: frontbuffer plane tracking bits
> > > + *
> > > + * This function gets called after the flip has been latched and will complete
> > > + * on the next vblank. It will execute the fush if it hasn't been cancalled yet.
> > > + *
> > > + * Can be called without any locks held.
> > > + */
> > > +void intel_frontbuffer_flip_complete(struct drm_device *dev,
> > > + unsigned frontbuffer_bits)
> > > +{
> > > + struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > + mutex_lock(&dev_priv->fb_tracking.lock);
> > > + /* Mask any cancelled flips. */
> > > + frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
> > > + dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
> > > + mutex_unlock(&dev_priv->fb_tracking.lock);
> > > +
> > > + intel_frontbuffer_flush(dev, frontbuffer_bits);
> > > +}
> >
> > Let's be consistent and call this intel_fb_flip_complete. I am happy
> > with the slight confusion between intel_fb and
> > i915_frontbuffer_tracking, since they are related and it is easy enough
> > to make the conceptual leap that only the fb that are on the scanout
> > (ala frontbuffers) need to be tracked - but we only every need to track
> > fbs...
>
> You need to pair an fb_flip_prepare with a flip_complete, otherwise
> there's no flush. It needs to be two-step to make sure we don't override
> any intermediate rendering (e.g. if someone flips and then right away goes
> ahead with a frontbuffer rendering batch). A direct flush otoh is
> different since it's not async, so we don't need any special protection
> for that window.

I was only critising the function name :) Treating the mmio updates as
single step flips comes later...
-Chris

--
Chris Wilson, Intel Open Source Technology Centre