[i-g-t] lib/xe: set hwconfig NULL for unsupported platforms

[Upadhyay, Tejas]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Tuesday, December 10, 2024 2:08 AM
> To: Harrison, John C <john.c.harrison at intel.com>
> Cc: Upadhyay, Tejas <tejas.upadhyay at intel.com>; igt-
> dev at lists.freedesktop.org; intel-xe at lists.freedesktop.org; Maslak, Jan
> <jan.maslak at intel.com>
> Subject: Re: [i-g-t] lib/xe: set hwconfig NULL for unsupported platforms
> 
> On Mon, Dec 09, 2024 at 11:20:07AM -0800, John Harrison wrote:
> > On 12/9/2024 00:57, Tejas Upadhyay wrote:
> > > There are hardware platforms which are not supporting hwconfig
> > > table, for example ADLS. Querying hwconfig on unsupported platforms
> > > leads to assert and failure for some of tests like,
> > > ./build/tests/xe_module_load --r load
> > The module reload test should not be querying the hwconfig table?!
> 
> See the Fixes: line; that patch made it so that anything calling
> xe_device_get() will fetch and cache the hwconfig (and assert if it's
> missing).  xe_module load calls __drm_open_driver ->
> __drm_open_driver_another -> xe_device_get so it does indeed fetch the
> hwconfig table now and hit the assertion added by that patch.
> 
> I don't know if always grabbing and caching the hwconfig was actually a
> good idea or not.  As we've seen here, the hwconfig may legitimately be
> NULL, so as we remove the assertion here, we still need to do
> NULL-checks at the point the hwconfig gets used to truly be safe.
> 
> >
> > But the hwconfig test itself should be failing on platforms which do not
> > have a table. That is intentional. There is no platform which is POR for the
> > Xe driver which does not support hwconfig tables. So the test is supposed
> to
> > fail if it ever does not get a valid table.
> 
> The Xe driver is only officially supported on the Xe2 platforms and
> beyond, but it can still load and run on some pre-Xe2 platforms that
> people were using as development platforms; some of those don't support
> hwconfig.  And even though our CI setup is only trying to test a subset
> of those platforms that do contain hwconfig, note that our DG2 setups
> also have an ADL-S igpu that also gets probed during module load, and
> that's what leads to the CI failures that were reported here.
> 
> 
> Maybe XeKMD is mature enough now (and we have enough real platforms for
> testing now) that we could prune the list of platforms that Xe is
> willing to load on with unofficial support.  E.g., drop the unofficial
> support for TGL, RKL, and ADL-S since those platforms don't have
> hwconfig and I don't think any of our developers are still using them as
> development vehicles at this point.  That's probably a separate
> discussion from the patch at hand though.
> 

We can go with this patch or skip hwconfig considerations for unsupported platforms like KMD is doing, but in the end it will also need same kind of changes.

Or if we plan to remove pre-xe2 platforms completely from CI then no change needed. 

Tejas

> 
> Matt
> 
> >
> > John.
> >
> > >
> > > https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/3683
> > >
> > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > Cc: Jan Maslak <jan.maslak at intel.com>
> > > Fixes: 37a230e50 ("lib/xe_query: add hwconfig to xe_device")
> > > Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
> > > ---
> > >   lib/xe/xe_query.c | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> > > index f3731d9d3..8f0a5392c 100644
> > > --- a/lib/xe/xe_query.c
> > > +++ b/lib/xe/xe_query.c
> > > @@ -51,7 +51,8 @@ static uint32_t *xe_query_hwconfig_new(int fd,
> uint32_t *hwconfig_size)
> > >   	/* Perform the initial query to get the size */
> > >   	igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_XE_DEVICE_QUERY, &query),
> 0);
> > > -	igt_assert_neq(query.size, 0);
> > > +	if (!query.size)
> > > +		return NULL;
> > >   	hwconfig = malloc(query.size);
> > >   	igt_assert(hwconfig);
> > > @@ -858,7 +859,8 @@ uint32_t *xe_hwconfig_lookup_value(int fd, enum
> intel_hwconfig attribute, uint32
> > >   	igt_assert(xe_dev);
> > >   	hwconfig = xe_dev->hwconfig;
> > > -	igt_assert(hwconfig);
> > > +	if (!hwconfig)
> > > +		return NULL;
> > >   	/* Extract the value from the hwconfig */
> > >   	pos = 0;
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation