Re: [PATCH v6] drm/i915: Emit to ringbuffer directly

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> 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@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.ursulin@intel.com>
>> > Cc: Chris Wilson <chris@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 */
>

Going through request feels right. For ring_emit
we could use shorter:

cs_emit and cs_advance.

They are rings but for users at this level the distinction
feels unimportant.

Just my few bikesheds.

-Mika

> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx