On Thu, Feb 09, 2017 at 10:00:35AM +0200, Joonas Lahtinen wrote:
> On ke, 2017-02-08 at 18:04 +0000, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >
> > This removes the usage of intel_ring_emit in favour of
> > directly writing to the ring buffer.
> >
> > intel_ring_emit was preventing the compiler for optimising
> > fetch and increment of the current ring buffer pointer and
> > therefore generating very verbose code for every write.
> >
> > It had no useful purpose since all ringbuffer operations
> > are started and ended with intel_ring_begin and
> > intel_ring_advance respectively, with no bail out in the
> > middle possible, so it is fine to increment the tail in
> > intel_ring_begin and let the code manage the pointer
> > itself.
> >
> > Useless instruction removal amounts to approximately
> > two and half kilobytes of saved text on my build.
> >
> > Not sure if this has any measurable performance
> > implications but executing a ton of useless instructions
> > on fast paths cannot be good.
> >
> > Patch is not fully polished, but it compiles and runs
> > on Gen9 at least.
> >
> > v2:
> > * Change return from intel_ring_begin to error pointer by
> > popular demand.
> > * Move tail increment to intel_ring_advance to enable some
> > error checking.
> >
> > v3:
> > * Move tail advance back into intel_ring_begin.
> > * Rebase and tidy.
> >
> > v4:
> > * Complete rebase after a few months since v3.
> >
> > v5:
> > * Remove unecessary cast and fix !debug compile. (Chris Wilson)
> >
> > v6:
> > * Make intel_ring_offset take request as well.
> > * Fix recording of request postfix plus a sprinkle of asserts.
> > (Chris Wilson)
> >
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>
> <SNIP>
>
> > @@ -617,99 +616,92 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> > if (INTEL_GEN(dev_priv) >= 7)
> > len += 2 + (num_rings ? 4*num_rings + 6 : 0);
> >
> > - ret = intel_ring_begin(req, len);
> > - if (ret)
> > - return ret;
> > + out = intel_ring_begin(req, len);
> > + if (IS_ERR(out))
> > + return PTR_ERR(out);
> >
> > /* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
> > if (INTEL_GEN(dev_priv) >= 7) {
> > - intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> > + *out++ = MI_ARB_ON_OFF | MI_ARB_DISABLE;
>
> I expressed my concern in the previous iteration of this series months
> ago, and here goes again; Lets try to keep the writes easily greppable.
>
> So intel_ring_emit (or better name) could remain as a wrapper
>
> #define (something something)_emit(x, y) *(x)++ = (y)
My concern with intel_ring_emit() remaining is that we are no longer
operating on the ring. The pointer to use for emitting is retrieved from
the request, so I think pointer = i915_gem_request_emit(rq, num_dwords)
is what we want in the near future.
I suppose if that was
ring = i915_gem_request_emit(rq, num_dwords);
intel_ring_emit(ring, blah)
intel_ring_advance(rq, ring); /* this still needs polish */
It'll just about do, problem being that intel_ring_foo() is not
operating on an struct intel_ring. :|
s/intel_ring_emit/ring_emit/ ?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
