From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Subject: Re: [PATCH 4/5] drm/i915: Let number of workarounds
	more	clear
Date: Fri, 26 Sep 2014 15:22:21 +0300
Message-ID: <87ppeilgg2.fsf@gaia.fi.intel.com>
References: <1411172190-1642-1-git-send-email-rodrigo.vivi@intel.com>
 <1411172190-1642-4-git-send-email-rodrigo.vivi@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <intel-gfx-bounces@lists.freedesktop.org>
Received: from mga09.intel.com (mga09.intel.com [134.134.136.24])
 by gabe.freedesktop.org (Postfix) with ESMTP id D94096E09C
 for <intel-gfx@lists.freedesktop.org>; Fri, 26 Sep 2014 05:22:25 -0700 (PDT)
In-Reply-To: <1411172190-1642-4-git-send-email-rodrigo.vivi@intel.com>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
 <mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
 <mailto:intel-gfx-request@lists.freedesktop.org?subject=subscribe>
Errors-To: intel-gfx-bounces@lists.freedesktop.org
Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>
To: intel-gfx@lists.freedesktop.org
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
List-Id: intel-gfx@lists.freedesktop.org

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