Re: [Intel-gfx] [PATCH v9 1/2] drm/i915: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads
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... ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v9 1/2] drm/i915: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads
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). ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v9 1/2] drm/i915: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v9 1/2] drm/i915: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads
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
Cc: Michel Thierry
Cc: Joonas Lahtinen
Cc: Chris Wilson
Cc: Mika Kuoppala
Cc: Tvrtko Ursulin
Signed-off-by: Yunwei Zhang
Reviewed-by: Oscar Mateo
---
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__l
[Intel-gfx] [PATCH v9 1/2] drm/i915: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads
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
Cc: Michel Thierry
Cc: Joonas Lahtinen
Cc: Chris Wilson
Cc: Mika Kuoppala
Cc: Tvrtko Ursulin
Signed-off-by: Yunwei Zhang
Reviewed-by: Oscar Mateo
---
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;
+ 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);
