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.ursu...@intel.com> > > > > 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.ursu...@intel.com> > > Cc: Chris Wilson <ch...@chris-wilson.co.uk> > > <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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx