Re: [PATCH 4/5] drm/i915: Let number of workarounds more clear

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

Rodrigo Vivi <rodrigo.vivi@intel.com> writes:

> This helps when including or removing cs workarounds.
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 7c3d17a..39fbea6 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -694,6 +694,7 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>  	int ret;
>  	struct drm_device *dev = ring->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int wa_amount;
>  
>  	/*
>  	 * workarounds applied in this fn are part of register state context,
> @@ -704,10 +705,11 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>  	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
>  
>  	/*
> -	 * update the number of dwords required based on the
> -	 * actual number of workarounds applied
> +	 * update the number of workarounds when adding or removing was
> +	 * in order the have propper dwords
>  	 */
> -	ret = intel_ring_begin(ring, 24);
> +	wa_amount = 8;
> +	ret = intel_ring_begin(ring, 3 * wa_amount);
>  	if (ret)
>  		return ret;
>  

I have a bit mixed feelings with this patch as I have tripped
around here myself recently.

I think we should just drop this patch and use:
ret = intel_ring_begin(ring, 3 * <amount>) on the subsequent
patches that modify the amount.

This way the intel_ring_begin will always be the one that
shows in a diff. And raises alarm if it doesnt. We don't want
this to be too transparent and looking too easy for the next reader.
As the reviewer is the only and last line of defense ensuring
symmetry between intel_ring_begin and amount of emits.

-Mika

> @@ -775,6 +777,7 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
>  	int ret;
>  	struct drm_device *dev = ring->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int wa_amount;
>  
>  	/*
>  	 * workarounds applied in this fn are part of register state context,
> @@ -784,7 +787,12 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
>  	dev_priv->num_wa_regs = 0;
>  	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
>  
> -	ret = intel_ring_begin(ring, 12);
> +	/*
> +	 * update the number of workarounds when adding or removing was
> +	 * in order the have propper dwords
> +	 */
> +	wa_amount = 4;
> +	ret = intel_ring_begin(ring, 3 * wa_amount);
>  	if (ret)
>  		return ret;
>  
> -- 
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx