Re: [PATCH] drm/xe: Upgrade complaint about missing slice info

[John Harrison <john.c.harrison@intel.com>]

Ping @Stuart & @Rodrigo? You still have concerns or can this be merged?

John.


On 1/21/2025 11:10, John Harrison wrote:
> On 1/21/2025 09:42, Summers, Stuart wrote:
>> On Fri, 2025-01-17 at 16:54 -0800, John.C.Harrison@Intel.com wrote:
>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>
>>> The steering code needs to know slice/subslice counts and this
>>> information should be retrieved from the hwconfig table. However,
>>> earlier platforms don't have it, hence the KMD has a fallback path.
>>> Newer platforms really should have the entries and if they are
>>> missing
>>> that is a bug that needs to be fixed in the table.
>>>
>>> So update the complaint to be an error on newer platforms and remove
>>> it completely for older ones that we know are bad (but are not POR
>>> for
>>> the Xe driver anyway). Also, re-word the message a little to make it
>>> clearer what the issue is.
>>>
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_gt_mcr.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c
>>> b/drivers/gpu/drm/xe/xe_gt_mcr.c
>>> index a1676b787fdc..605aad3554e7 100644
>>> --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
>>> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
>>> @@ -341,7 +341,13 @@ static unsigned int dss_per_group(struct xe_gt
>>> *gt)
>>>          return DIV_ROUND_UP(max_subslices, max_slices);
>>>     fallback:
>>> -       xe_gt_dbg(gt, "GuC hwconfig cannot provide dss/slice; using
>>> typical fallback values\n");
>> Agree with Rodrigo that this is still interesting for older platforms
>> to show the expectation.
> But it does not provide any useful information. You might as well just 
> have a loop on initial driver load that prints the warning out ten 
> times for any platform prior to LNL. The comment below has all the 
> information that the above message tells you (and more, because it 
> actually tells you this is expected rather than unexpected). There is 
> zero use in spamming the user with debug messages to say something 
> that is an absolute guarantee on a given platform.
>
>>
>>> +       /*
>>> +        * Some older platforms don't have tables or don't have
>>> complete tables.
>>> +        * Newer platforms should always have the required info.
>>> +        */
>>> +       if (GRAPHICS_VERx100(gt_to_xe(gt)) >= 2000)
>>> +               xe_gt_err(gt, "Slice/Subslice counts missing from
>>> hwconfig table; using typical fallback values\n");
>> I understand the intent here, but IMO it would be better for this to be
>> a warning.
> Whereas, if this is a platform which is supposed to have this 
> information then it is absolutely an error if that information is 
> missing. Something, somewhere is broken and very definitely needs to 
> be fixed. Also, while the fallback should be accurate for the 
> platforms below, that is not guaranteed to be the case in future. In 
> which case, using a fallback may lead to incorrect register accesses. 
> Which, again is very definitely an error.
>
> Either it is a new platform and someone forgot to add that information 
> to the table. Or it is a new platform that does not conform to this 
> way of accessing registers and thus needs a KMD update to support it. 
> Or it is an existing platform that got broken because of some 
> regression bug. Either way, it is something that we need to catch in 
> CI before the cause of the issue makes it out of the door and on to 
> end user systems.
>
> John.
>
>>
>> Thanks,
>> Stuart
>>
>>> +
>>>          if (gt_to_xe(gt)->info.platform == XE_PVC)
>>>                  return 8;
>>>          else if (GRAPHICS_VERx100(gt_to_xe(gt)) >= 1250)
>