| Message ID | 1524005930-22974-1-git-send-email-yunwei.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 4/17/2018 3:58 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) > v9: > - Changed naming and label fixes. (Oscar) > - Store only the selector instead of whole MCR. (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 | 35 ++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_device_info.h | 3 +++ > drivers/gpu/drm/i915/intel_engine_cs.c | 14 ++++++++----- > 3 files changed, 47 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c > index a32ba72..1a4288f 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -719,6 +719,39 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv) > return 0; > } > > +static void sanitize_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; > + > + /* WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl */ > + if (INTEL_GEN(dev_priv) >= 10) > + mcr |= mcr_slice_subslice_select; Blank line here. The I915_WRITE is both for the WA and for the sanitation of the register. > + I915_WRITE(GEN8_MCR_SELECTOR, mcr); > + > + info->default_mcr_ss_select = mcr_slice_subslice_select; > +} > + > /** > * intel_device_info_runtime_init - initialize runtime info > * @info: intel device info struct > @@ -851,6 +884,8 @@ 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); > > + sanitize_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..2c47a62 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; > > + /* default selected slice/subslice in MCR packet control */ > + u32 default_mcr_ss_select; > + default_mcr_s_ss_select? (yes, I know we are not coherent with the meaning of 's' and 'ss' in many other places). > 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..1ba2826 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -831,18 +831,22 @@ 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((mcr & mcr_slice_subslice_mask) != > + dev_priv->info.default_mcr_ss_select); > mcr &= ~mcr_slice_subslice_mask; > mcr |= mcr_slice_subslice_select; > I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); > > ret = I915_READ_FW(reg); > > + /* > + * HW expects MCR to be programed to a enabled slice/subslice pair > + * before any MMIO read into slice/subslice register > + */ The comment above makes more sense in sanitize_mcr, together with the WA label. You can make it a bit more verbose with the info in the commit message. Something like this: /* * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl * Before any MMIO read into slice/subslice specific registers, MCR * packet control register needs to be programmed to point to any * enabled s/ss pair. Otherwise, incorrect values will be returned. * This means each subsequent MMIO read will be forwarded to an * specific s/ss combination, but this is OK since these registers * are consistent across s/ss in almost all cases. In the rare * occasions, such as INSTDONE, where this value is dependent * on s/ss combo, the read shoud be done with read_subslice_reg. */ I don't think any other comment is required here. > mcr &= ~mcr_slice_subslice_mask; > + mcr |= dev_priv->info.default_mcr_ss_select; > + > I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); > > intel_uncore_forcewake_put__locked(dev_priv, fw_domains);
Quoting Oscar Mateo (2018-04-18 17:30:41) > > > On 4/17/2018 3:58 PM, Yunwei Zhang wrote: > > + /* > > + * HW expects MCR to be programed to a enabled slice/subslice pair > > + * before any MMIO read into slice/subslice register > > + */ > > The comment above makes more sense in sanitize_mcr, together with the WA > label. You can make it a bit more verbose with the info in the commit > message. Something like this: > > /* > * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl > * Before any MMIO read into slice/subslice specific registers, MCR > * packet control register needs to be programmed to point to any > * enabled s/ss pair. Otherwise, incorrect values will be returned. > * This means each subsequent MMIO read will be forwarded to an > * specific s/ss combination, but this is OK since these registers > * are consistent across s/ss in almost all cases. In the rare > * occasions, such as INSTDONE, where this value is dependent > * on s/ss combo, the read shoud be done with read_subslice_reg. > */ > > I don't think any other comment is required here. Apart from the answer to the earlier question, what mmio read after this point? If all slice/subslice register access is through this function, what are you trying to protect? Very curious. -Chris
On 4/18/2018 9:38 AM, Chris Wilson wrote: > Quoting Oscar Mateo (2018-04-18 17:30:41) >> >> On 4/17/2018 3:58 PM, Yunwei Zhang wrote: >>> + /* >>> + * HW expects MCR to be programed to a enabled slice/subslice pair >>> + * before any MMIO read into slice/subslice register >>> + */ >> The comment above makes more sense in sanitize_mcr, together with the WA >> label. You can make it a bit more verbose with the info in the commit >> message. Something like this: >> >> /* >> * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl >> * Before any MMIO read into slice/subslice specific registers, MCR >> * packet control register needs to be programmed to point to any >> * enabled s/ss pair. Otherwise, incorrect values will be returned. >> * This means each subsequent MMIO read will be forwarded to an >> * specific s/ss combination, but this is OK since these registers >> * are consistent across s/ss in almost all cases. In the rare >> * occasions, such as INSTDONE, where this value is dependent >> * on s/ss combo, the read shoud be done with read_subslice_reg. >> */ >> >> I don't think any other comment is required here. > Apart from the answer to the earlier question, what mmio read after this > point? If all slice/subslice register access is through this function, > what are you trying to protect? Very curious. > -Chris The problem is that the BSpec does not have a comprehensive list of registers that live in the slice/subslice, so it's difficult to know when this is going to become a problem. For example, I know from previous experience that the MOCS tables are replicated across slices (that's why we couldn't let UMD decide when to shutdown them: because the MOCS tables get lost as soon as you reconfigure the number of s/ss. The only way to do this in by poking in the context, so that the MOCS gets reprogrammed immediately after).
On 4/18/2018 9:45 AM, Oscar Mateo wrote: > > > On 4/18/2018 9:38 AM, Chris Wilson wrote: >> Quoting Oscar Mateo (2018-04-18 17:30:41) >>> >>> On 4/17/2018 3:58 PM, Yunwei Zhang wrote: >>>> + /* >>>> + * HW expects MCR to be programed to a enabled slice/subslice >>>> pair >>>> + * before any MMIO read into slice/subslice register >>>> + */ >>> The comment above makes more sense in sanitize_mcr, together with >>> the WA >>> label. You can make it a bit more verbose with the info in the commit >>> message. Something like this: >>> >>> /* >>> * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl >>> * Before any MMIO read into slice/subslice specific registers, MCR >>> * packet control register needs to be programmed to point to any >>> * enabled s/ss pair. Otherwise, incorrect values will be returned. >>> * This means each subsequent MMIO read will be forwarded to an >>> * specific s/ss combination, but this is OK since these registers >>> * are consistent across s/ss in almost all cases. In the rare >>> * occasions, such as INSTDONE, where this value is dependent >>> * on s/ss combo, the read shoud be done with read_subslice_reg. >>> */ >>> >>> I don't think any other comment is required here. >> Apart from the answer to the earlier question, what mmio read after this >> point? If all slice/subslice register access is through this function, >> what are you trying to protect? Very curious. >> -Chris > > The problem is that the BSpec does not have a comprehensive list of > registers that live in the slice/subslice, so it's difficult to know > when this is going to become a problem. For example, I know from > previous experience that the MOCS tables are replicated across slices > (that's why we couldn't let UMD decide when to shutdown them: because > the MOCS tables get lost as soon as you reconfigure the number of > s/ss. The only way to do this in by poking in the context, so that the > MOCS gets reprogrammed immediately after). This hasn't been an issue until know because the hardware would route your read to a valid s/ss combo whenever the MCR select was 0s. Apparently, this is not the case anymore...
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index a32ba72..1a4288f 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -719,6 +719,39 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv) return 0; } +static void sanitize_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; + + /* WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl */ + if (INTEL_GEN(dev_priv) >= 10) + mcr |= mcr_slice_subslice_select; + I915_WRITE(GEN8_MCR_SELECTOR, mcr); + + info->default_mcr_ss_select = mcr_slice_subslice_select; +} + /** * intel_device_info_runtime_init - initialize runtime info * @info: intel device info struct @@ -851,6 +884,8 @@ 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); + sanitize_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..2c47a62 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; + /* default selected slice/subslice in MCR packet control */ + u32 default_mcr_ss_select; + 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..1ba2826 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -831,18 +831,22 @@ 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((mcr & mcr_slice_subslice_mask) != + dev_priv->info.default_mcr_ss_select); mcr &= ~mcr_slice_subslice_mask; mcr |= mcr_slice_subslice_select; I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); ret = I915_READ_FW(reg); + /* + * HW expects MCR to be programed to a enabled slice/subslice pair + * before any MMIO read into slice/subslice register + */ mcr &= ~mcr_slice_subslice_mask; + mcr |= dev_priv->info.default_mcr_ss_select; + I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr); intel_uncore_forcewake_put__locked(dev_priv, fw_domains);