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@xxxxxxxxx> > >Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> > >Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx> > >Signed-off-by: Arun Siluvery <arun.siluvery@xxxxxxxxxxxxxxx> > >--- > > 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
