[Intel-gfx] [PATCH v9 2/2] drm/i915: Implement WaProgramMgsrForL3BankSpecificMmioReads

[Oscar Mateo]

On 4/18/2018 9:40 AM, Oscar Mateo wrote:
>
>
> On 4/17/2018 3:59 PM, Yunwei Zhang wrote:
>> L3Bank could be fused off in hardware for debug purpose, and it
>> is possible that subslice is enabled while its corresponding L3Bank 
>> pairs
>> are disabled. In such case, if MCR packet control register(0xFDC) is
>> programed to point to a disabled bank pair, a MMIO read into L3Bank 
>> range
>> will return 0 instead of correct values.
>>
>> However, this is not going to be the case in any production silicon.
>> Therefore, we only check at initialization and issue a warning should
>> this really happen.
>>
>> References: HSDES#1405586840
>>
>> v2:
>>   - use fls instead of find_last_bit (Chris)
>>   - use is_power_of_2() instead of counting bit set (Chris)
>> v3:
>>   - rebase on latest tip
>> v5:
>>   - Added references (Mika)
>>   - Move local variable into scope where they are used (Ursulin)
>>   - use a new local variable to reduce long line of code (Ursulin)
>> v6:
>>   - Some coding style and use more local variables for clearer
>>     logic (Ursulin)
>> v7:
>>   - Rebased.
>> v8:
>>   - Reviewed by Oscar.
>> v9:
>>   - Fixed label location. (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/i915_reg.h          |  4 ++++
>>   drivers/gpu/drm/i915/intel_device_info.c | 23 +++++++++++++++++++++++
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index fb10602..6c9c01b 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -2709,6 +2709,10 @@ enum i915_power_well_id {
>>   #define   GEN10_F2_SS_DIS_SHIFT        18
>>   #define   GEN10_F2_SS_DIS_MASK        (0xf << GEN10_F2_SS_DIS_SHIFT)
>>   +#define    GEN10_MIRROR_FUSE3        _MMIO(0x9118)
>> +#define GEN10_L3BANK_PAIR_COUNT     4
>> +#define GEN10_L3BANK_MASK   0x0F
>> +
>>   #define GEN8_EU_DISABLE0        _MMIO(0x9134)
>>   #define   GEN8_EU_DIS0_S0_MASK        0xffffff
>>   #define   GEN8_EU_DIS0_S1_SHIFT        24
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
>> b/drivers/gpu/drm/i915/intel_device_info.c
>> index 1a4288f..530b6ba 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -729,6 +729,29 @@ static void sanitize_mcr(struct 
>> intel_device_info *info)
>>       u32 slice = fls(info->sseu.slice_mask);
>>       u32 subslice = fls(info->sseu.subslice_mask[slice]);
>>   +    /*
>> +     * WaProgramMgsrForL3BankSpecificMmioReads: cnl,icl
>> +     * L3Banks could be fused off in single slice scenario, however, if
>> +     * more than one slice is enabled, this should not happen.
>> +     */

Maybe a better explanation is warranted:

/*
  * WaProgramMgsrForL3BankSpecificMmioReads: cnl,icl
  * L3Banks could be fused off in single slice scenario. If that is the 
case,
  * we might need to program MCR select to a valid L3Bank by default,
  * to make sure we correctly read certain registers later on
* (in the range 0xB100 - 0xB3FF).
  * This might be incompatible with 
WaProgramMgsrForCorrectSliceSpecificMmioReads.
  * Fortunately, this should not happen in production hardware, so we only
  * assert that this is the case (instead of implementing something more 
complex
  * that requires checking the range of every MMIO read).
  */

>> +    if (is_power_of_2(info->sseu.slice_mask)) {
>
> This WA is only required for GEN >= 10. In other GENs, 
> GEN10_MIRROR_FUSE3 does not even exist!
>
>> +        /*
>> +         * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches
>> +         * enabled subslice, no need to redirect MCR packet
>> +         */
>> +        u32 fuse3 = I915_READ(GEN10_MIRROR_FUSE3);
>> +        u8 ss_mask = info->sseu.subslice_mask[slice];
>> +
>> +        u8 enabled_mask = (ss_mask | ss_mask >> 4) & 0xf;
>> +        u8 disabled_mask = fuse3 & 0xf;
>> +
>
> You defined GEN10_L3BANK_MASK. Might as well use it :)
>
>> +        /*
>> +         * Production silicon should have matched L3Bank and
>> +         * subslice enabled
>> +         */
>> +        WARN_ON((enabled_mask & disabled_mask) != enabled_mask);
>> +    }
>> +
>>       if (INTEL_GEN(dev_priv) >= 11) {
>>           mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
>>                         GEN11_MCR_SUBSLICE_MASK;
>