[Intel-gfx] [PATCH v8 1/2] drm/i915: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads
Oscar Mateo
oscar.mateo at intel.com
Tue Apr 17 21:53:37 UTC 2018
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 at intel.com>
>> Cc: Michel Thierry <michel.thierry at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>> Signed-off-by: Yunwei Zhang <yunwei.zhang at intel.com>
>> Reviewed-by: Oscar Mateo <oscar.mateo at 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);
>
More information about the Intel-gfx
mailing list