[Intel-gfx] [PATCH] drm/i915/icl: Fix context slice count configuration
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Wed Aug 29 12:35:33 UTC 2018
On 29/08/2018 13:29, Tvrtko Ursulin wrote:
>
> On 29/08/2018 13:11, Tvrtko Ursulin wrote:
>>
>>
>> On 29/08/2018 13:09, Tvrtko Ursulin wrote:
>>>
>>> On 29/08/2018 13:02, Tvrtko Ursulin wrote:
>>>>
>>>> On 29/08/2018 12:07, Lionel Landwerlin wrote:
>>>>> On 29/08/2018 11:54, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Happy to r-b this one since we clarified it is fine?
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Tvrtko
>>>>>>
>>>>>>
>>>>> I think there is still an issue here with regard to the subslice
>>>>> programming.
>>>>> You can only program values in [1, 4].
>>>>> But because we expose up to 8 subslices, you need to take that
>>>>> mask and alter the slice mask based on its value.
>>>>
>>>> Hmm true, thanks. I think we need to somehow express this
>>>> restriction on the uAPI level rather than silently mask it. Shall
>>>> we reject attempts to configure subslice mask if hweight(mask) >
>>>> max_subslices / 2 for ICL-LP?
>>>
>>> No wait, I was in the dynamic sseu world, but this also applies to
>>> the current code base..
>>>
>>> When we program SScount in drm-tip with the hweight of 8 we overflow
>>> the bitfield already and splat over SScountEn. Luckily with no ill
>>> effect since we set it anyway.
>>>
>>> So the fix needs to be in make_rpcs today..
>>>
>>> if (GEN11_LP) {
>>> if (hweight(slice_mask) == 1)
>>
>> && hweight(subslice_mask) < max_subslices
>>
>> So to leave the default full config alone.
>
> Which means that drm-tip configures the 1x6x8 part as SScount=0b110,
> which is another undocumented value according to BSpec. Furthermore
> the allowed configuration table suggests only three subslices can be
> enabled when SScountEn is set, so like the situation on BXT/GLK, now I
> need to figure out if we are inadvertently only enabling half of the
> subslices on ICL-LP.
Or maybe make the upper 4 bits of the subslice map to "big subslices"
and lower bottom 4 to "small subslices".
Then have the restrictions checked in ioctl() context_set_param.
-
Lionel
>
> Tvrtko
>
>>> subslice_count /= 2;
>>> else
>>> subslice_en = 0;
>>> }
>>>
>>> I think..
>>>
>>> Regards,
>>>
>>> Tvrtko
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list