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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Jan 24, 2025 at 03:56:54PM -0800, John Harrison wrote:
> Ping @Stuart & @Rodrigo? You still have concerns or can this be merged?

no further concerns from my side

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> 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)
> > 
>