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
