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

[Kamil Konieczny]

Hi Upadhyay,,
On 2024-12-11 at 09:13:50 +0000, Upadhyay, Tejas wrote:
> Going ahead with merge of this patch. 
> 
> Tejas
> 

Merged,

Regards,
Kamil

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