[Intel-gfx] [PATCH v2] drm/i915: use semaphores for the display plane

[Daniel Vetter]

On Wed, Apr 11, 2012 at 12:53:15PM +0100, Chris Wilson wrote:
> On Thu,  5 Apr 2012 14:47:36 -0700, Ben Widawsky <ben at bwidawsk.net> wrote:
> > In theory this will have performance and power improvements. Performance
> > because we don't need to stall when the scanout BO is busy, and power
> > because we don't have to stall when the BO is busy (and the ring can
> > even go to sleep if the HW supports it).
> > 
> > v2:
> > squash 2 patches into 1 (me)
> > un-inline the enable_semaphores function (Daniel)
> > remove comment about SNB hangs from i915_gem_object_sync (Chris)
> > rename intel_enable_semaphores to i915_semaphore_is_enabled (me)
> > removed page flip comment; "no why" (Chris)
> > 
> > To address other comments from Daniel (irc):
> > update the comment to say 'vt-d is crap, don't enable semaphores'
> >   - I think you misinterpreted Chris' comment, it already exists.
> > checking out whether we can pageflip on the render ring on ivb (didn't
> > work on early silicon)
> >   - We don't want to enable workarounds for early silicon unless we have
> >     to.
> >   - I can't find any references in the docs about this.
> > optionally use it if the fb is already busy on the render ring
> >   - This should be how the code already worked, unless I am
> >     misunderstanding your meaning.
> > 
> > CC: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> 
> > +int
> > +i915_gem_object_sync(struct drm_i915_gem_object *obj,
> > +		     struct intel_ring_buffer *to)
> > +{
> > +	struct intel_ring_buffer *from = obj->ring;
> > +	u32 seqno;
> > +	int ret, idx;
> > +
> > +	if (from == NULL || to == from)
> > +		return 0;
> > +
> > +	if (!i915_semaphore_is_enabled(obj->base.dev))
> Bug^ :(

To elaborate, for to == NULL we need to do a synchronous wait_rendering,
too. This happens for set_base and modeset. Furthermore I've noticed two
other things while reading this function that imo deserve each another
patch:
- we update from->sync_seqno before to->sync_to successfully emits the
  sync. That should happen after sync_to (and obviously only if that
  succeeds).
- the seqno - 1 semantics of sync_to is annoying me. Imo that kind of
  low-level stuff should be handled by the sync_to implementation.

Unfortunately neither the bug noticed by Chris nor the sync_seqno thing
can easily be exercised with i-g-t :(

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48