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

["Summers, Stuart" <stuart.summers@intel.com>]

On Tue, 2025-01-21 at 11:10 -0800, 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.

Yeah makes sense to me thanks. The error here also calls out CI which
is nice but doesn't specifically limit execution otherwise.

And if we do run into a problem like this we can address at that time.

Reviewed-by: Stuart Summers <stuart.summers@intel.com>

> 
> John.
> 
> > 
> > Thanks,
> > Stuart
> > 
> > > +
> > >          if (gt_to_xe(gt)->info.platform == XE_PVC)
> > >                  return 8;
> > >          else if (GRAPHICS_VERx100(gt_to_xe(gt)) >= 1250)
>