Re: [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit

From: Dave Gordon <david.s.gordon@intel.com>
To: Arun Siluvery <arun.siluvery@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Ben Widawsky <benjamin.widawsky@intel.com>,
	Mika Kuoppala <mika.kuoppala@intel.com>
Subject: Re: [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit
Date: Wed, 12 Aug 2015 16:41:13 +0100	[thread overview]
Message-ID: <55CB6919.8090809@intel.com> (raw)
In-Reply-To: <1439304272-9645-3-git-send-email-arun.siluvery@linux.intel.com>

On 11/08/15 15:44, Arun Siluvery wrote:
>  From Gen9, Push constant instruction parsing behaviour varies according to
> whether set shader is enabled or not. If we want legacy behaviour then it
> can be achieved by disabling set shader.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89959
>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h         |  5 +++++
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++++++
>   2 files changed, 15 insertions(+)

[snip]

> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index cf61262..7d284ed 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -983,6 +983,16 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
>   		tmp |= HDC_FORCE_CSR_NON_COHERENT_OVR_DISABLE;
>   	WA_SET_BIT_MASKED(HDC_CHICKEN0, tmp);
>
> +	/* Chicken bits to disable set shader is in multiple places,
> +	 * set bits in all required registers to disable it correctly
> +	 */
> +	WA_SET_BIT_MASKED(COMMON_SLICE_CHICKEN2, GEN9_DISABLE_GATHER_SET_SHADER_SLICE);
> +	if ((IS_SKYLAKE(dev) && INTEL_REVID(dev) <= SKL_REVID_D0) ||
> +	    (IS_BROXTON(dev) && INTEL_REVID(dev) == BXT_REVID_A0))
> +		WA_SET_BIT_MASKED(RS_CHICKEN, RS_CHICKEN_DISABLE_GATHER_AT_SHADER);
> +	else
> +		WA_SET_BIT_MASKED(CS_RCS_BE, CS_RCS_DISABLE_GATHER_AT_SHADER);
> +
>   	return 0;
>   }

This workaround isn't tagged with a specific /* WaXyz:chip */ comment.
Also, the style isn't consistent with the other paragraphs earlier in 
this function: those have braces round the body part even when there's 
only one line of code, possibly to make it clear where the WA comment
applies (of course, this is why the buggy WA_REG() macro wasn't spotted 
earlier).

So, maybe prettify this a bit, if possible? The code actually looks 
correct, just ugly.

Oh, and keep patch 1 even if you decide to abandon this one!

.Dave.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx