[Intel-gfx] [PATCH] drm/i915/icl: Fix context slice count configuration

[Tvrtko Ursulin]

On 22/08/2018 17:33, Lionel Landwerlin wrote:
> On 22/08/2018 17:18, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> Bitfield width for configuring the active slice count has grown in Gen11
>> so we need to program the GEN8_R_PWR_CLK_STATE accordingly.
>>
>> Current code was always requesting eight times the number of slices (due
>> writting to a bitfield starting three bits higher than it should). These
>> requests were luckily a) capped by the hardware to the available 
>> number of
>> slices, and b) we haven't yet exported the code to ask for reduced slice
>> configurations.
>>
>> Due both of the above there was no impact from this incorrect programming
>> but we should still fix it.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Bspec: 12247
>> Reported-by: tony.ye at intel.com
>> Suggested-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> Cc: tony.ye at intel.com
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h  |  2 ++
>>   drivers/gpu/drm/i915/intel_lrc.c | 12 ++++++++----
>>   2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 59d06d0055bb..640f7b774a26 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -344,6 +344,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t 
>> reg)
>>   #define   GEN8_RPCS_S_CNT_ENABLE    (1 << 18)
>>   #define   GEN8_RPCS_S_CNT_SHIFT        15
>>   #define   GEN8_RPCS_S_CNT_MASK        (0x7 << GEN8_RPCS_S_CNT_SHIFT)
>> +#define   GEN11_RPCS_S_CNT_SHIFT    12
>> +#define   GEN11_RPCS_S_CNT_MASK        (0x3f << GEN11_RPCS_S_CNT_SHIFT)
>>   #define   GEN8_RPCS_SS_CNT_ENABLE    (1 << 11)
>>   #define   GEN8_RPCS_SS_CNT_SHIFT    8
>>   #define   GEN8_RPCS_SS_CNT_MASK        (0x7 << GEN8_RPCS_SS_CNT_SHIFT)
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>> b/drivers/gpu/drm/i915/intel_lrc.c
>> index 36050f085071..43b8b0675ba0 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -2501,10 +2501,14 @@ make_rpcs(struct drm_i915_private *dev_priv)
>>        * enablement.
>>       */
>>       if (INTEL_INFO(dev_priv)->sseu.has_slice_pg) {
>> -        rpcs |= GEN8_RPCS_S_CNT_ENABLE;
>> -        rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.slice_mask) <<
>> -            GEN8_RPCS_S_CNT_SHIFT;
>> -        rpcs |= GEN8_RPCS_ENABLE;
>> +        rpcs = hweight8(INTEL_INFO(dev_priv)->sseu.slice_mask);
>> +
>> +        if (INTEL_GEN(dev_priv) >= 11)
>> +            rpcs <<= GEN11_RPCS_S_CNT_SHIFT;
>> +        else
>> +            rpcs <<= GEN8_RPCS_S_CNT_SHIFT;
>> +
>> +        rpcs |= GEN8_RPCS_ENABLE | GEN8_RPCS_S_CNT_ENABLE;
> 
> 
> I don't know if you saw that wording in the documentation :
> 
>   "
> 
> Note: In ICL, software programs this register as if GT consists of 2 
> slices with 4 subslices in each slice. Hardware maps this to the 1 
> slice/8-subslice physical layout.
> 
> "
> 
> 
> My understanding is that it would make this function a bit more 
> complicated ;)
> 
> It also implies that on ICL you cannot select 3 subslices, which is 
> unfortunately what Tony was trying to do.
> 
> Maybe some opens need to be raised as to what's possible on ICL.

I interpreted the note in my head as "In ICL, _if_ _the_ software 
programs.." so did not see a problem. Thought that would be just some 
hidden/under the covers remapping hw would do. But I see now that was 
wrong, and you are most likely right. I'll try to do some digging to 
understand this better.

But for the second part of it, I don't see why 1x3 configuration would 
be illegal. If software must assume hw is 2x4, even if in reality it is 
1x8, then 1x3 would still be legal, no?

I thought the cause of the hang on ICL was that when Tony was asking for 
1x3, the code was actually programming a request for 8x3 - which is 
illegal (as in slice count must be 1 to enable subslice pg) and so would 
fail to actually turn of the unwanted subslices.

But then I also though on ICL we deal with masks and not counts when 
programming the hardware. Since apparently it is counts both for slices 
and subslices, I am mightily confused as to how media-driver would even 
theoretically be able to turn off a _specific_ (sub)slice?!

Regards,

Tvrtko

> 
>>       }
>>       if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx