Re: [Intel-gfx] [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 10.01.2023 10:16, Tvrtko Ursulin wrote:
>
> On 10/01/2023 08:23, Andrzej Hajda wrote:
>>
>>
>> On 09.01.2023 13:24, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Revert to the original explicit approach and document the reasoning
>>> behind it.
>>>
>>> v2:
>>>   * DG2 needs to be covered too. (Matt)
>>>
>>> v3:
>>>   * Full version check for Gen12 to avoid catching all future 
>>> platforms.
>>>     (Matt)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
>>> ---
>>>   drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++++++++++++++--
>>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
>>> b/drivers/gpu/drm/i915/gt/intel_gt.c
>>> index 7eeee5a7cb33..5521fa057aab 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>>> @@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct 
>>> intel_gt *gt)
>>>       unsigned int num = 0;
>>>       unsigned long flags;
>>> -    if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
>>> +    /*
>>> +     * New platforms should not be added with catch-all-newer (>=)
>>> +     * condition so that any later platform added triggers the 
>>> below warning
>>> +     * and in turn mandates a human cross-check of whether the 
>>> invalidation
>>> +     * flows have compatible semantics.
>>> +     *
>>> +     * For instance with the 11.00 -> 12.00 transition three out of 
>>> five
>>> +     * respective engine registers were moved to masked type. Then 
>>> after the
>>> +     * 12.00 -> 12.50 transition multi cast handling is required too.
>>> +     */
>>> +
>>> +    if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
>>> +        GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
>>>           regs = NULL;
>>>           num = ARRAY_SIZE(xehp_regs);
>>> -    } else if (GRAPHICS_VER(i915) == 12) {
>>> +    } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
>>> +           GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
>>
>> MTL support is lost? IP_VER(12, 70)
>
> AFAIU Matt says MTL is still incomplete anyway, so that would be added 
> in an explicit patch here.

I've missed this part, sorry for the noise then :)
And as I see PVC is similar story.

>
>> And again it looks for me inconsistent, some unknown platforms are 
>> covered, for example 12.54, some not, for example 12.11.
>
> .11 and .54 as hypotheticals? You suggest this instead:
>
>     if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
>         GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
>         regs = NULL;
>         num = ARRAY_SIZE(xehp_regs);
>     } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
>            GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
>         regs = gen12_regs;
>         num = ARRAY_SIZE(gen12_regs);
>
> ?

For me this perfectly follows the 'strict' approach :)

>
> It's fine by me if that covers all currently known platforms.

My grep in i915_pci.c agrees.

Regards
Andrzej

>
> Regards,
>
> Tvrtko