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

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

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)