Re: [PATCH v2] drm/i915: use semaphores for the display plane

[Ben Widawsky <ben@bwidawsk.net>]

On Wed, 11 Apr 2012 14:06:42 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> 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@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@chris-wilson.co.uk>
> > > Signed-off-by: Ben Widawsky <ben@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:

if (from == NULL && !obj->active) should suffice?

This sounds like a pretty bad hack when it at some point in modesetting
code... who would have thought?

> - 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).

Probably a remnant of when ring_begin couldn't fail. This deserves to be
fixed, I will do this today if y'all haven't done it already.

> - 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.

If it ain't broke, don't fix it? I do agree with you, but I'll venture
to guess that I am less annoyed by it.

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