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

[Upadhyay, Tejas]

Going ahead with merge of this patch. 

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