[igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support

[Petri Latvala]

On Sun, Apr 24, 2022 at 10:44:14PM -0700, Dixit, Ashutosh wrote:
> On Sun, 24 Apr 2022 22:24:06 -0700, Gupta, Anshuman wrote:
> >
> > > -----Original Message-----
> > > From: Dixit, Ashutosh <ashutosh.dixit at intel.com>
> > > Sent: Friday, April 22, 2022 9:38 PM
> > > To: Gupta, Anshuman <anshuman.gupta at intel.com>
> > > Cc: igt-dev at lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi at intel.com>
> > > Subject: Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support
> > >
> > > On Fri, 22 Apr 2022 05:59:12 -0700, Anshuman Gupta wrote:
> > > >
> > > > Currently IGT is lacking IS_DGFX() macro support.
> > > > There are some power features like D3Cold are only supported on
> > > > discrete card. So IGT test/tools specific to D3Cold requires to
> > > > consume IS_DGFX().
> > > > Adding a is_dgfx field in intel_device_info and initializing it for
> > > > DG1. All future discrete platform would require to initialize this
> > > > field.
> > >
> > > #define IS_DGFX(fd) gem_has_lmem(fd) ?
> > >
> > > gem_has_lmem() is already pretty widely used so maybe we should not
> > > introduce another way to achieve the same goal?
> >
> > Before introducing this , I thought on using gem_has_lmem.
> > But I was not sure in case every discrete platform mandatory to have lmem region.
> > If it is guaranteed that every discrete platform will have lmem memory region ?
> > I will  drop  this patch.
> 
> So not sure what will happen in the future but till now and in the
> forseeable future all dGfx platforms have LMEM, so gem_has_lmem seems fine.
> 
> I think if we introduce IS_DGFX() as in this patch, we should have an
> additional patch to convert all tests using gem_has_lmem() to IS_DGFX() so
> that we don't have multiple ways of tests deciding if something should run
> for dGfx.

The difference between checking for device info data and runtime data
is finding out whether something is expected to work on some kernel
versions or not. Runtime data should always be preferred when it's
available. A stupid example from history: DG1 enablement phase when
lmem access wasn't there yet in the kernel. gem_has_lmem() false,
still dGPU. IS_DGFX() doesn't tell that lmem interfaces are usable for
this device, it just tells that this device is supposed to have
gem_has_lmem() = true.

Best practice is to appease the human reader of the test code: Instead
of something like

if (IS_DGFX(i915))

use

if (i915_can_do_d3cold(i915))

where the function is

bool i915_can_do_d3cold(int i915) {
  /* explain why only DGFX, and all DGFX */
  return IS_DGFX(i915);
}

In my opinion nothing should care directly whether a device is DGFX or
not, but specific requirements can be expressed with IS_DGFX just fine
if there's not a good way to ask the kernel if the capability is
present.

If multiple different requirement checks end up being just "return
IS_DGFX(i915)" it doesn't matter, we're still better off for the case
in the future when someone figures out a better way to control whether
the device can do x. Easier to find all the spots that case about x,
but not y.


-- 
Petri Latvala