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

From: Daniel Vetter <daniel@ffwll.ch>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>,
	intel-gfx@lists.freedesktop.org,
	Ben Widawsky <benjamin.widawsky@intel.com>
Subject: Re: [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit
Date: Fri, 14 Aug 2015 10:54:07 +0200	[thread overview]
Message-ID: <20150814085407.GP17734@phenom.ffwll.local> (raw)
In-Reply-To: <55CB6919.8090809@intel.com>

On Wed, Aug 12, 2015 at 04:41:13PM +0100, Dave Gordon wrote:
> 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).

Imo if we want to bikeshed this is should be

	/*
	 * WaBlaFoo:pft
	 * extended comment if need
	 */
	if (IS_FOO(dev))
		WA_SET_();

i.e. follow the style in this patch here for all of them.
-Daniel
> 
> 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx