Re: [PATCH] drm/i915: Fix subslice configuration on Gen9LP

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

On 22/08/2018 16:08, Lionel Landwerlin wrote:
> On 22/08/2018 15:29, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@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?

Regards,

Tvrtko

> 
>> +
>>           rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
>> -        rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]) <<
>> -            GEN8_RPCS_SS_CNT_SHIFT;
>> +        rpcs |= val << GEN8_RPCS_SS_CNT_SHIFT;
>>           rpcs |= GEN8_RPCS_ENABLE;
>>       }
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx