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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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)

Or, we make the name distinctive, "*ring++" would work for that.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx