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

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, 11 Apr 2012 08:46:40 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> 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?

if (to == NULL || !i915_sempahores)
  return i915_gem_object_wait_rendering(obj);
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre