[Intel-gfx] [PATCH] drm/i915: Fix subslice configuration on Gen9LP

[Tvrtko Ursulin]

On 22/08/2018 16:22, Lionel Landwerlin wrote:
> On 22/08/2018 16:17, Tvrtko Ursulin wrote:
>>
>> On 22/08/2018 16:08, Lionel Landwerlin wrote:
>>> On 22/08/2018 15:29, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> According to the documentation, when programming the subslice count 
>>>> power-
>>>> gating configuration register, the value to be written into it on 
>>>> Gen9LP
>>>> should actually in the format of:
>>>>
>>>>    1 slice  = 0x001
>>>>    2 slices = 0x010
>>>>    3 slices = 0x100
>>>
>>>
>>> s/slice/subslice/
>>>
>>>
>>> Also 0b001 etc... Not hexadecimal.
>>
>> Oops, you're right.
>>
>>>>
>>>> And not the popcount of the enabled subslice mask as on other 
>>>> platforms.
>>>>
>>>> So on Gen9LP platforms we have been programming 0x11 into those 
>>>> bits, but
>>>> the documentation does not explain what would that achieve. Could it be
>>>> that we enable only two subslice on three sub-slice parts? Or hardware
>>>> simply ignores it and sticks with the maximum configuration?
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>>> Bspec: 12247
>>>> ---
>>>> Could this actually be true or I am severely misreading the docs? It 
>>>> does
>>>> not sound plausible to me this would have been missed all this time..
>>>>
>>>> How to test in what configuration do these parts run before and 
>>>> after this
>>>> patch?
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++++--
>>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>>>> b/drivers/gpu/drm/i915/intel_lrc.c
>>>> index 36050f085071..cdfa962a1975 100644
>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>> @@ -2508,9 +2508,15 @@ make_rpcs(struct drm_i915_private *dev_priv)
>>>>       }
>>>>       if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
>>>> +        u8 val;
>>>> +
>>>> +        val = hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]);
>>>> +
>>>> +        if (IS_GEN9_LP(dev_priv))
>>>> +            val = BIT(val - 1);
>>>
>>>
>>> Hmm... Are you breaking the 2 subslices setting here then?
>>>
>>> (2 subslices = 0b10 which should be equal to hweight8(subslice_mask) 
>>> if I'm thinking right)
>>
>> No and yes, I think.
>>
>> subslice_mask = 0b011 => hweight = 2 => BIT(2 - 1) = BIT(1) = 0b010 
>> into the register
>>
>> In the same way, all together:
>>
>> subslice_mask = 0b001 => hweight = 1 => BIT(0) = 0b001
>> subslice_mask = 0b011 => hweight = 2 => BIT(1) = 0b010
>> subslice_mask = 0b111 => hweight = 3 => BIT(2) = 0b100
>>
>> Have I made a mistake somewhere?
> 
> 
> Ah, yes! You're right :)
> 
> My eyes got tricked, thanks for finding this out.

At least half of the credit goes to you for linking to 12247 in scope of 
one different thread!

> 
> With the comment fixed :
> 
> 
> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>

Thanks!

Any ideas on how to test this? I'd like to commit message to be more 
precise - have we been running with one slice too few? Or hardware 
ignores the undocumented bit combination? Or even, is the documentation 
perhaps incorrect?!

Regards,

Tvrtko