Re: [PATCH v5 2/2] drm/i915: Implement WaProgramMgsrForL3BankSpecificMmioReads

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 28/03/2018 10:39, Tvrtko Ursulin wrote:
> 
> On 27/03/2018 23:14, 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)
>>
>> 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>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h        |  4 ++++
>>   drivers/gpu/drm/i915/intel_engine_cs.c | 20 ++++++++++++++++++++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 1bca695f..4f2f5e1 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -2691,6 +2691,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_engine_cs.c 
>> b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index 4c78d1e..7be7a75 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -811,6 +811,26 @@ static u32 calculate_mcr(struct drm_i915_private 
>> *dev_priv, u32 mcr)
>>   static void wa_init_mcr(struct drm_i915_private *dev_priv)
>>   {
>>       u32 mcr;
>> +    const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu);
> 
> Another style nitpick - sorry I did not notice it before - we typically 
> order assignments from functions arguments to locals first, then pure 
> locals. Also we typically try to make the declaration block start wide 
> and then narrow.
> 
>> +
>> +    /* If more than one slice are enabled, L3Banks should be all 
>> enabled */
> 
> L3Banks should be all enabled, or enabled for all enabled slices? (That 
> comment below says "should have _matched_".
> 
>> +    if (is_power_of_2(sseu->slice_mask)) {
>> +        /*
>> +         * WaProgramMgsrForL3BankSpecificMmioReads:
>> +         * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches
>> +         * enabled subslice, no need to redirect MCR packet
>> +         */
> 
> This comment implies there will be some action taken depending on this 
> conditional relating to the MCR, but there is nothing there?
> 
> It is not clear to me what should and whether perhaps this comment 
> should be pulled up and merged with the one above the conditional.
> 
>> +        u32 slice = fls(sseu->slice_mask);
>> +        u32 fuse3 = I915_READ(GEN10_MIRROR_FUSE3);
>> +        u8 ss_mask = sseu->subslice_mask[slice];
> 
> Insert blank line after declarations.
> 
> Also, is it correct to only consider the subslice mask of the last slice 
> for this check?
> 
>> +        /*
>> +         * Production silicon should have matched L3Bank and
>> +         * subslice enabled
>> +         */
>> +        WARN_ON(!((fuse3 & GEN10_L3BANK_MASK) &
>> +              ((ss_mask | ss_mask >> GEN10_L3BANK_PAIR_COUNT) & > 
>> +               GEN10_L3BANK_MASK)));
> 
> Mask in fuse3 is the disabled mask right, since BSpec calls them "L3 
> Bank Disable Select"?
> 
> Should you not be checking that none of the enabled slices have L3Bank 
> disabled, while the above looks like it can miss a partial mismatch? 
> Something like this:
> 
> u8 enabled_mask = (ss_mask | ss_mask >> 4) & 0xf;
> u8 disabled_mask = fuse3 & 0xf;
> 
> WARN_ON((enabled_mask & disabled_mask) != enabled_mask);

Oops no, that's wrong, should be "(enabled_mask & ~disabled_mask) != 
enabled_mask)" I think. Which is then the same as WARN_ON(enabled_mask & 
disabled_mask) - aka same bit is both enabled and disabled - so maybe I 
got the meaning of "L3 Bank Disable Select" wrong.

Regards,

Tvrtko

> 
>> +    }
>>       mcr = I915_READ(GEN8_MCR_SELECTOR);
>>       mcr = calculate_mcr(dev_priv, mcr);
>>
> 
> Regards,
> 
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx