[Intel-gfx] [PATCH 055/190] drm/i915: Unify intel_logical_ring_emit and intel_ring_emit

[Dave Gordon]

On 11/01/16 09:17, Chris Wilson wrote:
> Both perform the same actions with more or less indirection, so just
> unify the code.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem.c            |   2 +-
>   drivers/gpu/drm/i915/i915_gem_context.c    |   8 +-
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |  34 ++++-----
>   drivers/gpu/drm/i915/i915_gem_gtt.c        |  26 +++----
>   drivers/gpu/drm/i915/intel_display.c       |  26 +++----
>   drivers/gpu/drm/i915/intel_lrc.c           | 114 ++++++++++++++---------------
>   drivers/gpu/drm/i915/intel_lrc.h           |  26 -------
>   drivers/gpu/drm/i915/intel_mocs.c          |  30 ++++----
>   drivers/gpu/drm/i915/intel_overlay.c       |  42 +++++------
>   drivers/gpu/drm/i915/intel_ringbuffer.c    | 101 ++++++++++++-------------
>   drivers/gpu/drm/i915/intel_ringbuffer.h    |  21 ++----
>   11 files changed, 194 insertions(+), 236 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c2a1ec8abc11..247731672cb1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4068,7 +4068,7 @@ err:
>
>   int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice)
>   {
> -	struct intel_engine_cs *ring = req->ring;
> +	struct intel_ringbuffer *ring = req->ringbuf;

NAK. (regardless of the fact that I'd like them unified!)

Until you have purged the last use of the name "ring" as a reference to 
an engine, adding new things called "ring" but of a different type will 
be too confusing.

The variable, at least for now, should be called "ringbuf", which makes 
it obvious that it caches the 'req->ringbuf' field and NOT the
         struct intel_engine_cs *ring;
that is also found in struct drm_i915_gem_request.

You can only start reusing names with a new meaning /after/ the old 
meaning has been eliminated from the code, but some interval for 
everyone's mental cache to be updated. But probably better never to 
reuse an old name for a different thing, why not just make up a new one, 
as we did with "engine".

>   	struct drm_i915_private *dev_priv = req->i915;
>   	u32 *remap_info = dev_priv->l3_parity.remap_info[slice];
>   	int i, ret;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 3e3b4bf3fed1..d58de7e084dc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -519,7 +519,7 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
>   static inline int
>   mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
>   {
> -	struct intel_engine_cs *ring = req->ring;
> +	struct intel_ringbuffer *ring = req->ringbuf;
>   	u32 flags = hw_flags | MI_MM_SPACE_GTT;
>   	const int num_rings =
>   		/* Use an extended w/a on ivb+ if signalling from other rings */
> @@ -534,7 +534,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
>   	 * itlb_before_ctx_switch.
>   	 */
>   	if (IS_GEN6(req->i915)) {
> -		ret = ring->flush(req, I915_GEM_GPU_DOMAINS, 0);
> +		ret = req->ring->flush(req, I915_GEM_GPU_DOMAINS, 0);

Hmm ... what is this "ring"? Oh, this one's not a ringbuffer, it's an 
ENGINE!

>   		if (ret)
>   			return ret;
>   	}
> @@ -562,7 +562,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
>
>   			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
>   			for_each_ring(signaller, req->i915, i) {
> -				if (signaller == ring)
> +				if (signaller == req->ring)

another engine

>   					continue;
>
>   				intel_ring_emit_reg(ring, RING_PSMI_CTL(signaller->mmio_base));
> @@ -587,7 +587,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
>
>   			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
>   			for_each_ring(signaller, req->i915, i) {
> -				if (signaller == ring)
> +				if (signaller == req->ring)

and this one too

>   					continue;
>
>   				intel_ring_emit_reg(ring, RING_PSMI_CTL(signaller->mmio_base));
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 78b462956c78..603a247ac333 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1146,14 +1146,12 @@ i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params)
>   }
>
>   static int
> -i915_reset_gen7_sol_offsets(struct drm_device *dev,
> -			    struct drm_i915_gem_request *req)
> +i915_reset_gen7_sol_offsets(struct drm_i915_gem_request *req)
>   {
> -	struct intel_engine_cs *ring = req->ring;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_ringbuffer *ring = req->ringbuf;

But this 'ring' is a ringbuffer ...

>   	int ret, i;
>
> -	if (!IS_GEN7(dev) || ring != &dev_priv->ring[RCS]) {
> +	if (!IS_GEN7(req->i915) || req->ring->id != RCS) {

... and this one, in the same function, is an engine!

>   		DRM_DEBUG("sol reset is gen7/rcs only\n");
>   		return -EINVAL;
>   	}

So please submit a version that does just what it says ('cos I think 
unification would be good) but without the confusing repurposing of 
local variable names.

.Dave.