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.widaw...@intel.com>
> >Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> >Cc: Mika Kuoppala <mika.kuopp...@intel.com>
> >Signed-off-by: Arun Siluvery <arun.siluv...@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
