| Message ID | 1523999112-15449-1-git-send-email-yunwei.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 4/17/2018 2:05 PM, Yunwei Zhang wrote: > 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) > v7: > - Rebased. > v8: > - Reviewed by Oscar > - Store default MCR value instead of calculate on the run. (Oscar) > > 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> > Reviewed-by: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/intel_device_info.c | 33 ++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_device_info.h | 3 +++ > drivers/gpu/drm/i915/intel_engine_cs.c | 14 +++++++++++--- > 3 files changed, 47 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c > index a32ba72..2243a23 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -719,6 +719,36 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv) > return 0; > } > > +static void wa_init_mcr(struct intel_device_info *info) mcr_sanitize? > +{ > + struct drm_i915_private *dev_priv = > + container_of(info, struct drm_i915_private, info); > + u32 mcr; > + u32 mcr_slice_subslice_mask; > + u32 mcr_slice_subslice_select; > + u32 slice = fls(info->sseu.slice_mask); > + u32 subslice = fls(info->sseu.subslice_mask[slice]); > + > + if (INTEL_GEN(dev_priv) >= 11) { > + mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK | > + GEN11_MCR_SUBSLICE_MASK; > + mcr_slice_subslice_select = GEN11_MCR_SLICE(slice) | > + GEN11_MCR_SUBSLICE(subslice); > + } else { > + mcr_slice_subslice_mask = GEN8_MCR_SLICE_MASK | > + GEN8_MCR_SUBSLICE_MASK; > + mcr_slice_subslice_select = GEN8_MCR_SLICE(slice) | > + GEN8_MCR_SUBSLICE(subslice); > + } > + > + mcr = I915_READ(GEN8_MCR_SELECTOR); > + mcr &= ~mcr_slice_subslice_mask; Until here you are not applying any WA, only sanitizing what the MCR contains. The real WA is in the two following lines. That's where the WaProgramMgsrForCorrectSliceSpecificMmioReads label should be (and maybe a small comment noting that we are selecting a kind of random slice/subslice combination to make sure MMIO reads in a certaing range are valid). > + if (INTEL_GEN(dev_priv) >= 10) > + mcr |= mcr_slice_subslice_select; > + > + info->mcr = mcr; And now you also want to write the HW register back, otherwise you are not applying the WA! > +} > + > /** > * intel_device_info_runtime_init - initialize runtime info > * @info: intel device info struct > @@ -851,6 +881,9 @@ void intel_device_info_runtime_init(struct intel_device_info *info) > else if (INTEL_INFO(dev_priv)->gen >= 11) > gen11_sseu_info_init(dev_priv); > > + /* WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl */ > + wa_init_mcr(info); > + > /* Initialize command stream timestamp frequency */ > info->cs_timestamp_frequency_khz = read_timestamp_frequency(dev_priv); > } > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > index 933e316..5449a15 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -176,6 +176,9 @@ struct intel_device_info { > /* Slice/subslice/EU info */ > struct sseu_dev_info sseu; > > + /* MCR packet control */ > + u32 mcr; Use a better name for this, like default_mcr. I'll explain why in a second... > + > u32 cs_timestamp_frequency_khz; > > struct color_luts { > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index 1a83707..08798f2 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -831,18 +831,26 @@ 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. > + * before GEN10 or to a enabled s/ss post GEN10 after reading out the > + * registers. > */ > - WARN_ON_ONCE(mcr & mcr_slice_subslice_mask); > + WARN_ON_ONCE(mcr != dev_priv->info.mcr); ... if you call it default_mcr, you can skip the comment completely. It's very clear you are just making sure the MCR has the expected value. > 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,icl No need for this label here, because this is not were the WA should be applied!! Hopefully, you already applied it before now. > + * expects mcr to be programed to a enabled slice/subslice pair > + * before any MMIO read into slice/subslice register > + */ > + mcr = dev_priv->info.mcr; Same here: with a better naming, it's very clear you are just setting the MCR to the default value, so there is no need for any extra comment (and remove the following blank line, to show this goes together with the I915_WRITE_FW). > + > I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); > > intel_uncore_forcewake_put__locked(dev_priv, fw_domains);
On 4/17/2018 2:34 PM, Oscar Mateo wrote: > > > On 4/17/2018 2:05 PM, Yunwei Zhang wrote: >> 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) >> v7: >> - Rebased. >> v8: >> - Reviewed by Oscar >> - Store default MCR value instead of calculate on the run. (Oscar) >> >> 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> >> Reviewed-by: Oscar Mateo <oscar.mateo@intel.com> >> --- >> drivers/gpu/drm/i915/intel_device_info.c | 33 >> ++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_device_info.h | 3 +++ >> drivers/gpu/drm/i915/intel_engine_cs.c | 14 +++++++++++--- >> 3 files changed, 47 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_device_info.c >> b/drivers/gpu/drm/i915/intel_device_info.c >> index a32ba72..2243a23 100644 >> --- a/drivers/gpu/drm/i915/intel_device_info.c >> +++ b/drivers/gpu/drm/i915/intel_device_info.c >> @@ -719,6 +719,36 @@ static u32 read_timestamp_frequency(struct >> drm_i915_private *dev_priv) >> return 0; >> } >> +static void wa_init_mcr(struct intel_device_info *info) > > mcr_sanitize? > >> +{ >> + struct drm_i915_private *dev_priv = >> + container_of(info, struct drm_i915_private, info); >> + u32 mcr; >> + u32 mcr_slice_subslice_mask; >> + u32 mcr_slice_subslice_select; >> + u32 slice = fls(info->sseu.slice_mask); >> + u32 subslice = fls(info->sseu.subslice_mask[slice]); >> + >> + if (INTEL_GEN(dev_priv) >= 11) { >> + mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK | >> + GEN11_MCR_SUBSLICE_MASK; >> + mcr_slice_subslice_select = GEN11_MCR_SLICE(slice) | >> + GEN11_MCR_SUBSLICE(subslice); >> + } else { >> + mcr_slice_subslice_mask = GEN8_MCR_SLICE_MASK | >> + GEN8_MCR_SUBSLICE_MASK; >> + mcr_slice_subslice_select = GEN8_MCR_SLICE(slice) | >> + GEN8_MCR_SUBSLICE(subslice); >> + } >> + >> + mcr = I915_READ(GEN8_MCR_SELECTOR); >> + mcr &= ~mcr_slice_subslice_mask; > > Until here you are not applying any WA, only sanitizing what the MCR > contains. The real WA is in the two following lines. That's where the > WaProgramMgsrForCorrectSliceSpecificMmioReads label should be (and > maybe a small comment noting that we are selecting a kind of random > slice/subslice combination to make sure MMIO reads in a certaing range > are valid). > >> + if (INTEL_GEN(dev_priv) >= 10) >> + mcr |= mcr_slice_subslice_select; >> + >> + info->mcr = mcr; > An extra thought: technically, you don't care about the whole MCR, only the bits in mcr_slice_subslice_mask. I doubt the other bits can change but, if you want to be thorough, store only those bits and avoid making assumptions about the rest in read_subslice_reg. > And now you also want to write the HW register back, otherwise you are > not applying the WA! > >> +} >> + >> /** >> * intel_device_info_runtime_init - initialize runtime info >> * @info: intel device info struct >> @@ -851,6 +881,9 @@ void intel_device_info_runtime_init(struct >> intel_device_info *info) >> else if (INTEL_INFO(dev_priv)->gen >= 11) >> gen11_sseu_info_init(dev_priv); >> + /* WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl */ >> + wa_init_mcr(info); >> + >> /* Initialize command stream timestamp frequency */ >> info->cs_timestamp_frequency_khz = >> read_timestamp_frequency(dev_priv); >> } >> diff --git a/drivers/gpu/drm/i915/intel_device_info.h >> b/drivers/gpu/drm/i915/intel_device_info.h >> index 933e316..5449a15 100644 >> --- a/drivers/gpu/drm/i915/intel_device_info.h >> +++ b/drivers/gpu/drm/i915/intel_device_info.h >> @@ -176,6 +176,9 @@ struct intel_device_info { >> /* Slice/subslice/EU info */ >> struct sseu_dev_info sseu; >> + /* MCR packet control */ >> + u32 mcr; > > Use a better name for this, like default_mcr. I'll explain why in a > second... > >> + >> u32 cs_timestamp_frequency_khz; >> struct color_luts { >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c >> b/drivers/gpu/drm/i915/intel_engine_cs.c >> index 1a83707..08798f2 100644 >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >> @@ -831,18 +831,26 @@ 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. >> + * before GEN10 or to a enabled s/ss post GEN10 after reading >> out the >> + * registers. >> */ >> - WARN_ON_ONCE(mcr & mcr_slice_subslice_mask); >> + WARN_ON_ONCE(mcr != dev_priv->info.mcr); > > ... if you call it default_mcr, you can skip the comment completely. > It's very clear you are just making sure the MCR has the expected value. > >> 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,icl > > No need for this label here, because this is not were the WA should be > applied!! Hopefully, you already applied it before now. > >> + * expects mcr to be programed to a enabled slice/subslice pair >> + * before any MMIO read into slice/subslice register >> + */ >> + mcr = dev_priv->info.mcr; > > Same here: with a better naming, it's very clear you are just setting > the MCR to the default value, so there is no need for any extra > comment (and remove the following blank line, to show this goes > together with the I915_WRITE_FW). > >> + >> I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); >> intel_uncore_forcewake_put__locked(dev_priv, fw_domains); >
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index a32ba72..2243a23 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -719,6 +719,36 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv) return 0; } +static void wa_init_mcr(struct intel_device_info *info) +{ + struct drm_i915_private *dev_priv = + container_of(info, struct drm_i915_private, info); + u32 mcr; + u32 mcr_slice_subslice_mask; + u32 mcr_slice_subslice_select; + u32 slice = fls(info->sseu.slice_mask); + u32 subslice = fls(info->sseu.subslice_mask[slice]); + + if (INTEL_GEN(dev_priv) >= 11) { + mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK | + GEN11_MCR_SUBSLICE_MASK; + mcr_slice_subslice_select = GEN11_MCR_SLICE(slice) | + GEN11_MCR_SUBSLICE(subslice); + } else { + mcr_slice_subslice_mask = GEN8_MCR_SLICE_MASK | + GEN8_MCR_SUBSLICE_MASK; + mcr_slice_subslice_select = GEN8_MCR_SLICE(slice) | + GEN8_MCR_SUBSLICE(subslice); + } + + mcr = I915_READ(GEN8_MCR_SELECTOR); + mcr &= ~mcr_slice_subslice_mask; + if (INTEL_GEN(dev_priv) >= 10) + mcr |= mcr_slice_subslice_select; + + info->mcr = mcr; +} + /** * intel_device_info_runtime_init - initialize runtime info * @info: intel device info struct @@ -851,6 +881,9 @@ void intel_device_info_runtime_init(struct intel_device_info *info) else if (INTEL_INFO(dev_priv)->gen >= 11) gen11_sseu_info_init(dev_priv); + /* WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl */ + wa_init_mcr(info); + /* Initialize command stream timestamp frequency */ info->cs_timestamp_frequency_khz = read_timestamp_frequency(dev_priv); } diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index 933e316..5449a15 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -176,6 +176,9 @@ struct intel_device_info { /* Slice/subslice/EU info */ struct sseu_dev_info sseu; + /* MCR packet control */ + u32 mcr; + u32 cs_timestamp_frequency_khz; struct color_luts { diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 1a83707..08798f2 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -831,18 +831,26 @@ 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. + * before GEN10 or to a enabled s/ss post GEN10 after reading out the + * registers. */ - WARN_ON_ONCE(mcr & mcr_slice_subslice_mask); + WARN_ON_ONCE(mcr != dev_priv->info.mcr); 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,icl + * expects mcr to be programed to a enabled slice/subslice pair + * before any MMIO read into slice/subslice register + */ + mcr = dev_priv->info.mcr; + I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); intel_uncore_forcewake_put__locked(dev_priv, fw_domains);