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

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH 14/15] drm/i915: Track frontbuffer invalidation/flushing
Date: Tue, 17 Jun 2014 09:48:39 +0200	[thread overview]
Message-ID: <20140617074839.GY5821@phenom.ffwll.local> (raw)
In-Reply-To: <20140617065722.GH17744@nuc-i3427.alporthouse.com>

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.

Note that all flushing means: "Please upload the next full frame still to
the screen", i.e. one implied vblank wait is always there. Even for the
above synchronous flush.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch