| Message ID | 1439304272-9645-3-git-send-email-arun.siluvery@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On 12/08/2015 16:41, 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). > > 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! > Hi Dave, This patch can be ignored if we use below patch, [Intel-gfx] [PATCH] lib/rendercopy_gen9: Setup Push constant pointer before sending BTP commands http://lists.freedesktop.org/archives/intel-gfx/2015-August/073483.html I think the correct option would be to ignore this patch. regards Arun > .Dave. > >
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
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7150
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK 302/302 302/302
SNB 315/315 315/315
IVB 336/336 336/336
BYT -2 283/283 281/283
HSW 378/378 378/378
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*BYT igt@gem_partial_pwrite_pread@reads-uncached PASS(1) FAIL(1)
*BYT igt@gem_tiled_partial_pwrite_pread@reads PASS(1) FAIL(1)
Note: You need to pay more attention to line start with '*'
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 7456bd2..4d32b67 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1720,6 +1720,10 @@ enum skl_disp_power_wells { #define FW_BLC 0x020d8 #define FW_BLC2 0x020dc #define FW_BLC_SELF 0x020e0 /* 915+ only */ +#define CS_RCS_BE 0x20D8 +#define CS_RCS_DISABLE_GATHER_AT_SHADER (1<<7) +#define RS_CHICKEN 0x20DC +#define RS_CHICKEN_DISABLE_GATHER_AT_SHADER (1<<2) #define FW_BLC_SELF_EN_MASK (1<<31) #define FW_BLC_SELF_FIFO_MASK (1<<16) /* 945 only */ #define FW_BLC_SELF_EN (1<<15) /* 945 only */ @@ -5834,6 +5838,7 @@ enum skl_disp_power_wells { # define GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC ((1<<10) | (1<<26)) # define GEN9_RHWO_OPTIMIZATION_DISABLE (1<<14) #define COMMON_SLICE_CHICKEN2 0x7014 +#define GEN9_DISABLE_GATHER_SET_SHADER_SLICE (1<<12) # define GEN8_CSC2_SBE_VUE_CACHE_CONSERVATIVE (1<<0) #define HIZ_CHICKEN 0x7018 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; }
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(+)