| Message ID | 1522188856-10683-1-git-send-email-yunwei.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Yunwei Zhang (2018-03-27 23:14:16) > WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO > read into Slice/Subslice specific registers, MCR packet control > register(0xFDC) needs to be programmed to point to any enabled > slice/subslice pair. Otherwise, incorrect value will be returned. > > However, that means each subsequent MMIO read will be forwarded to a > specific slice/subslice combination as read is unicast. This is OK since > slice/subslice specific register values are consistent in almost all cases > across slice/subslice. There are rare occasions such as INSTDONE that this > value will be dependent on slice/subslice combo, in such cases, we need to > program 0xFDC and recover this after. This is already covered by > read_subslice_reg. > > Also, 0xFDC will lose its information after TDR/engine reset/power state > change. > > References: HSD#1405586840, BSID#0575 > > v2: > - use fls() instead of find_last_bit() (Chris) > - added INTEL_SSEU to extract sseu from device info. (Chris) > v3: > - rebase on latest tip > v5: > - Added references (Mika) > - Change the ordered of passing arguments and etc. (Ursulin) > > Cc: Oscar Mateo <oscar.mateo@intel.com> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 39 ++++++++++++++++++++++++++++++++-- > 1 file changed, 37 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index de09fa4..4c78d1e 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type) > } > } > > +static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr) > +{ > + const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu); > + u32 slice = fls(sseu->slice_mask); > + u32 subslice = fls(sseu->subslice_mask[slice]); > + > + mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK); > + mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice); > + > + return mcr; > +} > + > +static void wa_init_mcr(struct drm_i915_private *dev_priv) > +{ > + u32 mcr; > + > + mcr = I915_READ(GEN8_MCR_SELECTOR); > + mcr = calculate_mcr(dev_priv, mcr); > + I915_WRITE(GEN8_MCR_SELECTOR, mcr); > +} > + > static inline uint32_t > read_subslice_reg(struct drm_i915_private *dev_priv, int slice, > int subslice, i915_reg_t reg) > @@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int slice, > intel_uncore_forcewake_get__locked(dev_priv, fw_domains); > > mcr = I915_READ_FW(GEN8_MCR_SELECTOR); > + > /* > * The HW expects the slice and sublice selectors to be reset to 0 > * after reading out the registers. > */ > - WARN_ON_ONCE(mcr & mcr_slice_subslice_mask); > + WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 && > + (mcr & mcr_slice_subslice_mask)); > mcr &= ~mcr_slice_subslice_mask; > mcr |= mcr_slice_subslice_select; > I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); > > ret = I915_READ_FW(reg); > > - mcr &= ~mcr_slice_subslice_mask; > + /* > + * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl > + * expects mcr to be programed to a enabled slice/subslice pair > + * before any MMIO read into slice/subslice register > + */ So the read was above, where we did set the subslice_select appropriately. Here we are resetting back to 0 *after* the read, as the comment before indicates. So what are you trying to accomplish with this patch? Other than leaving the code in conflict with itself. -Chris
On 3/27/2018 3:27 PM, Chris Wilson wrote: > Quoting Yunwei Zhang (2018-03-27 23:14:16) >> WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO >> read into Slice/Subslice specific registers, MCR packet control >> register(0xFDC) needs to be programmed to point to any enabled >> slice/subslice pair. Otherwise, incorrect value will be returned. >> >> However, that means each subsequent MMIO read will be forwarded to a >> specific slice/subslice combination as read is unicast. This is OK since >> slice/subslice specific register values are consistent in almost all cases >> across slice/subslice. There are rare occasions such as INSTDONE that this >> value will be dependent on slice/subslice combo, in such cases, we need to >> program 0xFDC and recover this after. This is already covered by >> read_subslice_reg. >> >> Also, 0xFDC will lose its information after TDR/engine reset/power state >> change. >> >> References: HSD#1405586840, BSID#0575 >> >> v2: >> - use fls() instead of find_last_bit() (Chris) >> - added INTEL_SSEU to extract sseu from device info. (Chris) >> v3: >> - rebase on latest tip >> v5: >> - Added references (Mika) >> - Change the ordered of passing arguments and etc. (Ursulin) >> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> Cc: Michel Thierry <michel.thierry@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >> Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com> >> --- >> drivers/gpu/drm/i915/intel_engine_cs.c | 39 ++++++++++++++++++++++++++++++++-- >> 1 file changed, 37 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c >> index de09fa4..4c78d1e 100644 >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >> @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type) >> } >> } >> >> +static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr) >> +{ >> + const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu); >> + u32 slice = fls(sseu->slice_mask); >> + u32 subslice = fls(sseu->subslice_mask[slice]); >> + >> + mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK); >> + mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice); >> + >> + return mcr; >> +} >> + >> +static void wa_init_mcr(struct drm_i915_private *dev_priv) >> +{ >> + u32 mcr; >> + >> + mcr = I915_READ(GEN8_MCR_SELECTOR); >> + mcr = calculate_mcr(dev_priv, mcr); >> + I915_WRITE(GEN8_MCR_SELECTOR, mcr); >> +} >> + >> static inline uint32_t >> read_subslice_reg(struct drm_i915_private *dev_priv, int slice, >> int subslice, i915_reg_t reg) >> @@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int slice, >> intel_uncore_forcewake_get__locked(dev_priv, fw_domains); >> >> mcr = I915_READ_FW(GEN8_MCR_SELECTOR); >> + >> /* >> * The HW expects the slice and sublice selectors to be reset to 0 >> * after reading out the registers. >> */ >> - WARN_ON_ONCE(mcr & mcr_slice_subslice_mask); >> + WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 && >> + (mcr & mcr_slice_subslice_mask)); >> mcr &= ~mcr_slice_subslice_mask; >> mcr |= mcr_slice_subslice_select; >> I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); >> >> ret = I915_READ_FW(reg); >> >> - mcr &= ~mcr_slice_subslice_mask; >> + /* >> + * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl >> + * expects mcr to be programed to a enabled slice/subslice pair >> + * before any MMIO read into slice/subslice register >> + */ > So the read was above, where we did set the subslice_select > appropriately. Here we are resetting back to 0 *after* the read, as the > comment before indicates. > > So what are you trying to accomplish with this patch? Other than leaving > the code in conflict with itself. > -Chris Hi Chris, The comment mentioned 0xFDC needs to be reset to 0 was before this WA was introduced, in later HW, this WA requires 0xFDC to be programmed to a enabled slice/subslice. What this patch does it to initialize 0xFDC once at the initialization (also it will be called after engine reset/TDR/coming out of c6) and make sure every time it is changed, it will be reprogrammed to a enabled slice/subslice so that a MMIO read will get the correct value. read_subslice_reg changes the 0xFDC value and if it is set to 0, it will cause MMIO read to return invalid value for s/ss specific registers.
Quoting Zhang, Yunwei (2018-03-27 23:49:27) > > > On 3/27/2018 3:27 PM, Chris Wilson wrote: > > Quoting Yunwei Zhang (2018-03-27 23:14:16) > >> WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO > >> read into Slice/Subslice specific registers, MCR packet control > >> register(0xFDC) needs to be programmed to point to any enabled > >> slice/subslice pair. Otherwise, incorrect value will be returned. > >> > >> However, that means each subsequent MMIO read will be forwarded to a > >> specific slice/subslice combination as read is unicast. This is OK since > >> slice/subslice specific register values are consistent in almost all cases > >> across slice/subslice. There are rare occasions such as INSTDONE that this > >> value will be dependent on slice/subslice combo, in such cases, we need to > >> program 0xFDC and recover this after. This is already covered by > >> read_subslice_reg. > >> > >> Also, 0xFDC will lose its information after TDR/engine reset/power state > >> change. > >> > >> References: HSD#1405586840, BSID#0575 > >> > >> v2: > >> - use fls() instead of find_last_bit() (Chris) > >> - added INTEL_SSEU to extract sseu from device info. (Chris) > >> v3: > >> - rebase on latest tip > >> v5: > >> - Added references (Mika) > >> - Change the ordered of passing arguments and etc. (Ursulin) > >> > >> Cc: Oscar Mateo <oscar.mateo@intel.com> > >> Cc: Michel Thierry <michel.thierry@intel.com> > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > >> Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_engine_cs.c | 39 ++++++++++++++++++++++++++++++++-- > >> 1 file changed, 37 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > >> index de09fa4..4c78d1e 100644 > >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c > >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > >> @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type) > >> } > >> } > >> > >> +static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr) > >> +{ > >> + const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu); > >> + u32 slice = fls(sseu->slice_mask); > >> + u32 subslice = fls(sseu->subslice_mask[slice]); > >> + > >> + mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK); > >> + mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice); > >> + > >> + return mcr; > >> +} > >> + > >> +static void wa_init_mcr(struct drm_i915_private *dev_priv) > >> +{ > >> + u32 mcr; > >> + > >> + mcr = I915_READ(GEN8_MCR_SELECTOR); > >> + mcr = calculate_mcr(dev_priv, mcr); > >> + I915_WRITE(GEN8_MCR_SELECTOR, mcr); > >> +} > >> + > >> static inline uint32_t > >> read_subslice_reg(struct drm_i915_private *dev_priv, int slice, > >> int subslice, i915_reg_t reg) > >> @@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int slice, > >> intel_uncore_forcewake_get__locked(dev_priv, fw_domains); > >> > >> mcr = I915_READ_FW(GEN8_MCR_SELECTOR); > >> + > >> /* > >> * The HW expects the slice and sublice selectors to be reset to 0 > >> * after reading out the registers. > >> */ > >> - WARN_ON_ONCE(mcr & mcr_slice_subslice_mask); > >> + WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 && > >> + (mcr & mcr_slice_subslice_mask)); > >> mcr &= ~mcr_slice_subslice_mask; > >> mcr |= mcr_slice_subslice_select; > >> I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); > >> > >> ret = I915_READ_FW(reg); > >> > >> - mcr &= ~mcr_slice_subslice_mask; > >> + /* > >> + * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl > >> + * expects mcr to be programed to a enabled slice/subslice pair > >> + * before any MMIO read into slice/subslice register > >> + */ > > So the read was above, where we did set the subslice_select > > appropriately. Here we are resetting back to 0 *after* the read, as the > > comment before indicates. > > > > So what are you trying to accomplish with this patch? Other than leaving > > the code in conflict with itself. > > -Chris > Hi Chris, > > The comment mentioned 0xFDC needs to be reset to 0 was before this WA > was introduced, in later HW, this WA requires 0xFDC to be programmed to > a enabled slice/subslice. > > What this patch does it to initialize 0xFDC once at the initialization > (also it will be called after engine reset/TDR/coming out of c6) and > make sure every time it is changed, it will be reprogrammed to a enabled > slice/subslice so that a MMIO > read will get the correct value. read_subslice_reg changes the 0xFDC > value and if it is set to 0, it will cause MMIO read to return invalid > value for s/ss specific registers. What mmio read? The only accessor should be this function. And still the two comments are in direct conflict with each other. -Chris
On 3/27/2018 4:13 PM, Chris Wilson wrote: > Quoting Zhang, Yunwei (2018-03-27 23:49:27) >> >> On 3/27/2018 3:27 PM, Chris Wilson wrote: >>> Quoting Yunwei Zhang (2018-03-27 23:14:16) >>>> WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO >>>> read into Slice/Subslice specific registers, MCR packet control >>>> register(0xFDC) needs to be programmed to point to any enabled >>>> slice/subslice pair. Otherwise, incorrect value will be returned. >>>> >>>> However, that means each subsequent MMIO read will be forwarded to a >>>> specific slice/subslice combination as read is unicast. This is OK since >>>> slice/subslice specific register values are consistent in almost all cases >>>> across slice/subslice. There are rare occasions such as INSTDONE that this >>>> value will be dependent on slice/subslice combo, in such cases, we need to >>>> program 0xFDC and recover this after. This is already covered by >>>> read_subslice_reg. >>>> >>>> Also, 0xFDC will lose its information after TDR/engine reset/power state >>>> change. >>>> >>>> References: HSD#1405586840, BSID#0575 >>>> >>>> v2: >>>> - use fls() instead of find_last_bit() (Chris) >>>> - added INTEL_SSEU to extract sseu from device info. (Chris) >>>> v3: >>>> - rebase on latest tip >>>> v5: >>>> - Added references (Mika) >>>> - Change the ordered of passing arguments and etc. (Ursulin) >>>> >>>> Cc: Oscar Mateo <oscar.mateo@intel.com> >>>> Cc: Michel Thierry <michel.thierry@intel.com> >>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >>>> Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_engine_cs.c | 39 ++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 37 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c >>>> index de09fa4..4c78d1e 100644 >>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>>> @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type) >>>> } >>>> } >>>> >>>> +static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr) >>>> +{ >>>> + const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu); >>>> + u32 slice = fls(sseu->slice_mask); >>>> + u32 subslice = fls(sseu->subslice_mask[slice]); >>>> + >>>> + mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK); >>>> + mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice); >>>> + >>>> + return mcr; >>>> +} >>>> + >>>> +static void wa_init_mcr(struct drm_i915_private *dev_priv) >>>> +{ >>>> + u32 mcr; >>>> + >>>> + mcr = I915_READ(GEN8_MCR_SELECTOR); >>>> + mcr = calculate_mcr(dev_priv, mcr); >>>> + I915_WRITE(GEN8_MCR_SELECTOR, mcr); >>>> +} >>>> + >>>> static inline uint32_t >>>> read_subslice_reg(struct drm_i915_private *dev_priv, int slice, >>>> int subslice, i915_reg_t reg) >>>> @@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int slice, >>>> intel_uncore_forcewake_get__locked(dev_priv, fw_domains); >>>> >>>> mcr = I915_READ_FW(GEN8_MCR_SELECTOR); >>>> + >>>> /* >>>> * The HW expects the slice and sublice selectors to be reset to 0 >>>> * after reading out the registers. >>>> */ >>>> - WARN_ON_ONCE(mcr & mcr_slice_subslice_mask); >>>> + WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 && >>>> + (mcr & mcr_slice_subslice_mask)); >>>> mcr &= ~mcr_slice_subslice_mask; >>>> mcr |= mcr_slice_subslice_select; >>>> I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); >>>> >>>> ret = I915_READ_FW(reg); >>>> >>>> - mcr &= ~mcr_slice_subslice_mask; >>>> + /* >>>> + * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl >>>> + * expects mcr to be programed to a enabled slice/subslice pair >>>> + * before any MMIO read into slice/subslice register >>>> + */ >>> So the read was above, where we did set the subslice_select >>> appropriately. Here we are resetting back to 0 *after* the read, as the >>> comment before indicates. >>> >>> So what are you trying to accomplish with this patch? Other than leaving >>> the code in conflict with itself. >>> -Chris >> Hi Chris, >> >> The comment mentioned 0xFDC needs to be reset to 0 was before this WA >> was introduced, in later HW, this WA requires 0xFDC to be programmed to >> a enabled slice/subslice. >> >> What this patch does it to initialize 0xFDC once at the initialization >> (also it will be called after engine reset/TDR/coming out of c6) and >> make sure every time it is changed, it will be reprogrammed to a enabled >> slice/subslice so that a MMIO >> read will get the correct value. read_subslice_reg changes the 0xFDC >> value and if it is set to 0, it will cause MMIO read to return invalid >> value for s/ss specific registers. > What mmio read? The only accessor should be this function. > > And still the two comments are in direct conflict with each other. > -Chris This function is only used in INST_DONE case which you need to iterate through each slice/subslice to check and makes sense to program MCR for each s/ss combination. But there could be inadvertent read into this range without using this function, the value would be wrong without this WA. Anyway, the comment is not right given the latest WA required, I will submit a new patchset to update this comment. Thanks, Yunwei
Quoting Zhang, Yunwei (2018-03-28 16:54:26) > > > On 3/27/2018 4:13 PM, Chris Wilson wrote: > > Quoting Zhang, Yunwei (2018-03-27 23:49:27) > >> > >> On 3/27/2018 3:27 PM, Chris Wilson wrote: > >>> Quoting Yunwei Zhang (2018-03-27 23:14:16) > >>>> WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO > >>>> read into Slice/Subslice specific registers, MCR packet control > >>>> register(0xFDC) needs to be programmed to point to any enabled > >>>> slice/subslice pair. Otherwise, incorrect value will be returned. > >>>> > >>>> However, that means each subsequent MMIO read will be forwarded to a > >>>> specific slice/subslice combination as read is unicast. This is OK since > >>>> slice/subslice specific register values are consistent in almost all cases > >>>> across slice/subslice. There are rare occasions such as INSTDONE that this > >>>> value will be dependent on slice/subslice combo, in such cases, we need to > >>>> program 0xFDC and recover this after. This is already covered by > >>>> read_subslice_reg. > >>>> > >>>> Also, 0xFDC will lose its information after TDR/engine reset/power state > >>>> change. > >>>> > >>>> References: HSD#1405586840, BSID#0575 > >>>> > >>>> v2: > >>>> - use fls() instead of find_last_bit() (Chris) > >>>> - added INTEL_SSEU to extract sseu from device info. (Chris) > >>>> v3: > >>>> - rebase on latest tip > >>>> v5: > >>>> - Added references (Mika) > >>>> - Change the ordered of passing arguments and etc. (Ursulin) > >>>> > >>>> Cc: Oscar Mateo <oscar.mateo@intel.com> > >>>> Cc: Michel Thierry <michel.thierry@intel.com> > >>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >>>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > >>>> Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com> > >>>> --- > >>>> drivers/gpu/drm/i915/intel_engine_cs.c | 39 ++++++++++++++++++++++++++++++++-- > >>>> 1 file changed, 37 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > >>>> index de09fa4..4c78d1e 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c > >>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > >>>> @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type) > >>>> } > >>>> } > >>>> > >>>> +static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr) > >>>> +{ > >>>> + const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu); > >>>> + u32 slice = fls(sseu->slice_mask); > >>>> + u32 subslice = fls(sseu->subslice_mask[slice]); > >>>> + > >>>> + mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK); > >>>> + mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice); > >>>> + > >>>> + return mcr; > >>>> +} > >>>> + > >>>> +static void wa_init_mcr(struct drm_i915_private *dev_priv) > >>>> +{ > >>>> + u32 mcr; > >>>> + > >>>> + mcr = I915_READ(GEN8_MCR_SELECTOR); > >>>> + mcr = calculate_mcr(dev_priv, mcr); > >>>> + I915_WRITE(GEN8_MCR_SELECTOR, mcr); > >>>> +} > >>>> + > >>>> static inline uint32_t > >>>> read_subslice_reg(struct drm_i915_private *dev_priv, int slice, > >>>> int subslice, i915_reg_t reg) > >>>> @@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int slice, > >>>> intel_uncore_forcewake_get__locked(dev_priv, fw_domains); > >>>> > >>>> mcr = I915_READ_FW(GEN8_MCR_SELECTOR); > >>>> + > >>>> /* > >>>> * The HW expects the slice and sublice selectors to be reset to 0 > >>>> * after reading out the registers. > >>>> */ > >>>> - WARN_ON_ONCE(mcr & mcr_slice_subslice_mask); > >>>> + WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 && > >>>> + (mcr & mcr_slice_subslice_mask)); > >>>> mcr &= ~mcr_slice_subslice_mask; > >>>> mcr |= mcr_slice_subslice_select; > >>>> I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); > >>>> > >>>> ret = I915_READ_FW(reg); > >>>> > >>>> - mcr &= ~mcr_slice_subslice_mask; > >>>> + /* > >>>> + * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl > >>>> + * expects mcr to be programed to a enabled slice/subslice pair > >>>> + * before any MMIO read into slice/subslice register > >>>> + */ > >>> So the read was above, where we did set the subslice_select > >>> appropriately. Here we are resetting back to 0 *after* the read, as the > >>> comment before indicates. > >>> > >>> So what are you trying to accomplish with this patch? Other than leaving > >>> the code in conflict with itself. > >>> -Chris > >> Hi Chris, > >> > >> The comment mentioned 0xFDC needs to be reset to 0 was before this WA > >> was introduced, in later HW, this WA requires 0xFDC to be programmed to > >> a enabled slice/subslice. > >> > >> What this patch does it to initialize 0xFDC once at the initialization > >> (also it will be called after engine reset/TDR/coming out of c6) and > >> make sure every time it is changed, it will be reprogrammed to a enabled > >> slice/subslice so that a MMIO > >> read will get the correct value. read_subslice_reg changes the 0xFDC > >> value and if it is set to 0, it will cause MMIO read to return invalid > >> value for s/ss specific registers. > > What mmio read? The only accessor should be this function. > > > > And still the two comments are in direct conflict with each other. > > -Chris > This function is only used in INST_DONE case which you need to iterate > through each slice/subslice to check and makes sense to program MCR for > each s/ss combination. But there could be inadvertent read into this > range without using this function, the value would be wrong without this > WA. Sure, but garbage in, garbage out. If we write an accessor for a register because it requires a workaround, anyone who wants to access the register should use the accessor. Not just leave HW in a random state so that one particular selector works. -Chris
On 3/28/2018 9:03 AM, Chris Wilson wrote: > Quoting Zhang, Yunwei (2018-03-28 16:54:26) >> >> On 3/27/2018 4:13 PM, Chris Wilson wrote: >>> Quoting Zhang, Yunwei (2018-03-27 23:49:27) >>>> On 3/27/2018 3:27 PM, Chris Wilson wrote: >>>>> Quoting Yunwei Zhang (2018-03-27 23:14:16) >>>>>> WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO >>>>>> read into Slice/Subslice specific registers, MCR packet control >>>>>> register(0xFDC) needs to be programmed to point to any enabled >>>>>> slice/subslice pair. Otherwise, incorrect value will be returned. >>>>>> >>>>>> However, that means each subsequent MMIO read will be forwarded to a >>>>>> specific slice/subslice combination as read is unicast. This is OK since >>>>>> slice/subslice specific register values are consistent in almost all cases >>>>>> across slice/subslice. There are rare occasions such as INSTDONE that this >>>>>> value will be dependent on slice/subslice combo, in such cases, we need to >>>>>> program 0xFDC and recover this after. This is already covered by >>>>>> read_subslice_reg. >>>>>> >>>>>> Also, 0xFDC will lose its information after TDR/engine reset/power state >>>>>> change. >>>>>> >>>>>> References: HSD#1405586840, BSID#0575 >>>>>> >>>>>> v2: >>>>>> - use fls() instead of find_last_bit() (Chris) >>>>>> - added INTEL_SSEU to extract sseu from device info. (Chris) >>>>>> v3: >>>>>> - rebase on latest tip >>>>>> v5: >>>>>> - Added references (Mika) >>>>>> - Change the ordered of passing arguments and etc. (Ursulin) >>>>>> >>>>>> Cc: Oscar Mateo <oscar.mateo@intel.com> >>>>>> Cc: Michel Thierry <michel.thierry@intel.com> >>>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>>>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >>>>>> Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/intel_engine_cs.c | 39 ++++++++++++++++++++++++++++++++-- >>>>>> 1 file changed, 37 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c >>>>>> index de09fa4..4c78d1e 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>>>>> @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type) >>>>>> } >>>>>> } >>>>>> >>>>>> +static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr) >>>>>> +{ >>>>>> + const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu); >>>>>> + u32 slice = fls(sseu->slice_mask); >>>>>> + u32 subslice = fls(sseu->subslice_mask[slice]); >>>>>> + >>>>>> + mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK); >>>>>> + mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice); >>>>>> + >>>>>> + return mcr; >>>>>> +} >>>>>> + >>>>>> +static void wa_init_mcr(struct drm_i915_private *dev_priv) >>>>>> +{ >>>>>> + u32 mcr; >>>>>> + >>>>>> + mcr = I915_READ(GEN8_MCR_SELECTOR); >>>>>> + mcr = calculate_mcr(dev_priv, mcr); >>>>>> + I915_WRITE(GEN8_MCR_SELECTOR, mcr); >>>>>> +} >>>>>> + >>>>>> static inline uint32_t >>>>>> read_subslice_reg(struct drm_i915_private *dev_priv, int slice, >>>>>> int subslice, i915_reg_t reg) >>>>>> @@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int slice, >>>>>> intel_uncore_forcewake_get__locked(dev_priv, fw_domains); >>>>>> >>>>>> mcr = I915_READ_FW(GEN8_MCR_SELECTOR); >>>>>> + >>>>>> /* >>>>>> * The HW expects the slice and sublice selectors to be reset to 0 >>>>>> * after reading out the registers. >>>>>> */ >>>>>> - WARN_ON_ONCE(mcr & mcr_slice_subslice_mask); >>>>>> + WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 && >>>>>> + (mcr & mcr_slice_subslice_mask)); >>>>>> mcr &= ~mcr_slice_subslice_mask; >>>>>> mcr |= mcr_slice_subslice_select; >>>>>> I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); >>>>>> >>>>>> ret = I915_READ_FW(reg); >>>>>> >>>>>> - mcr &= ~mcr_slice_subslice_mask; >>>>>> + /* >>>>>> + * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl >>>>>> + * expects mcr to be programed to a enabled slice/subslice pair >>>>>> + * before any MMIO read into slice/subslice register >>>>>> + */ >>>>> So the read was above, where we did set the subslice_select >>>>> appropriately. Here we are resetting back to 0 *after* the read, as the >>>>> comment before indicates. >>>>> >>>>> So what are you trying to accomplish with this patch? Other than leaving >>>>> the code in conflict with itself. >>>>> -Chris >>>> Hi Chris, >>>> >>>> The comment mentioned 0xFDC needs to be reset to 0 was before this WA >>>> was introduced, in later HW, this WA requires 0xFDC to be programmed to >>>> a enabled slice/subslice. >>>> >>>> What this patch does it to initialize 0xFDC once at the initialization >>>> (also it will be called after engine reset/TDR/coming out of c6) and >>>> make sure every time it is changed, it will be reprogrammed to a enabled >>>> slice/subslice so that a MMIO >>>> read will get the correct value. read_subslice_reg changes the 0xFDC >>>> value and if it is set to 0, it will cause MMIO read to return invalid >>>> value for s/ss specific registers. >>> What mmio read? The only accessor should be this function. >>> >>> And still the two comments are in direct conflict with each other. >>> -Chris >> This function is only used in INST_DONE case which you need to iterate >> through each slice/subslice to check and makes sense to program MCR for >> each s/ss combination. But there could be inadvertent read into this >> range without using this function, the value would be wrong without this >> WA. > Sure, but garbage in, garbage out. If we write an accessor for a > register because it requires a workaround, anyone who wants to access > the register should use the accessor. Not just leave HW in a random > state so that one particular selector works. > -Chris But keep in mind this is not only *a* register, it is a range of registers, if we don't use this WA, developer needs to reminded if there is a MMIO read in slice/subslice/l3bank range, they will need to use read_subslice_reg instead of i915_READ. To me, it is just safe to program MCR in the beginning and reprogram it every time we have to change like in INST_DONE case. Thanks, Yunwei
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index de09fa4..4c78d1e 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type) } } +static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr) +{ + const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu); + u32 slice = fls(sseu->slice_mask); + u32 subslice = fls(sseu->subslice_mask[slice]); + + mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK); + mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice); + + return mcr; +} + +static void wa_init_mcr(struct drm_i915_private *dev_priv) +{ + u32 mcr; + + mcr = I915_READ(GEN8_MCR_SELECTOR); + mcr = calculate_mcr(dev_priv, mcr); + I915_WRITE(GEN8_MCR_SELECTOR, mcr); +} + static inline uint32_t read_subslice_reg(struct drm_i915_private *dev_priv, int slice, int subslice, i915_reg_t reg) @@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int slice, intel_uncore_forcewake_get__locked(dev_priv, fw_domains); mcr = I915_READ_FW(GEN8_MCR_SELECTOR); + /* * The HW expects the slice and sublice selectors to be reset to 0 * after reading out the registers. */ - WARN_ON_ONCE(mcr & mcr_slice_subslice_mask); + WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 && + (mcr & mcr_slice_subslice_mask)); mcr &= ~mcr_slice_subslice_mask; mcr |= mcr_slice_subslice_select; I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); ret = I915_READ_FW(reg); - mcr &= ~mcr_slice_subslice_mask; + /* + * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl + * expects mcr to be programed to a enabled slice/subslice pair + * before any MMIO read into slice/subslice register + */ + if (INTEL_GEN(dev_priv) < 10) + mcr &= ~mcr_slice_subslice_mask; + else + mcr = calculate_mcr(dev_priv, mcr); + I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); intel_uncore_forcewake_put__locked(dev_priv, fw_domains); @@ -1307,6 +1339,9 @@ static int cnl_init_workarounds(struct intel_engine_cs *engine) struct drm_i915_private *dev_priv = engine->i915; int ret; + /* WaProgramMgsrForCorrectSliceSpecificMmioReads: cnl */ + wa_init_mcr(dev_priv); + /* WaDisableI2mCycleOnWRPort:cnl (pre-prod) */ if (IS_CNL_REVID(dev_priv, CNL_REVID_B0, CNL_REVID_B0)) I915_WRITE(GAMT_CHKN_BIT_REG,
WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO read into Slice/Subslice specific registers, MCR packet control register(0xFDC) needs to be programmed to point to any enabled slice/subslice pair. Otherwise, incorrect value will be returned. However, that means each subsequent MMIO read will be forwarded to a specific slice/subslice combination as read is unicast. This is OK since slice/subslice specific register values are consistent in almost all cases across slice/subslice. There are rare occasions such as INSTDONE that this value will be dependent on slice/subslice combo, in such cases, we need to program 0xFDC and recover this after. This is already covered by read_subslice_reg. Also, 0xFDC will lose its information after TDR/engine reset/power state change. References: HSD#1405586840, BSID#0575 v2: - use fls() instead of find_last_bit() (Chris) - added INTEL_SSEU to extract sseu from device info. (Chris) v3: - rebase on latest tip v5: - Added references (Mika) - Change the ordered of passing arguments and etc. (Ursulin) Cc: Oscar Mateo <oscar.mateo@intel.com> Cc: Michel Thierry <michel.thierry@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com> --- drivers/gpu/drm/i915/intel_engine_cs.c | 39 ++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-)