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

[Ben Widawsky]

On Wed, 11 Apr 2012 16:52:33 +0100
Chris Wilson <chris at chris-wilson.co.uk> wrote:

> On Wed, 11 Apr 2012 08:46:40 -0700, Ben Widawsky <ben at bwidawsk.net> wrote:
> > On Wed, 11 Apr 2012 14:06:42 +0200
> > Daniel Vetter <daniel at 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 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:
> > 
> > if (from == NULL && !obj->active) should suffice?
> 
> if (to == NULL || !i915_sempahores)
>   return i915_gem_object_wait_rendering(obj);
> -Chris
> 

Ha, I misread it... I thought from->ring could be NULL, and we needed to
wait for rendering. So wait, in this case, to and from are both NULL?
Doesn't that mean both objects are already setup for use by display? A
bad assumption, but I think it would technically work as the code is,
today.