Re: [PATCH i-g-t v2] lib/intel_chipset: Extend intel_get_pci_device to PCI accelerator class

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 16 Jun 2026 08:15:46 -0700, Kamil Konieczny wrote:
>

Hi Kamil,

> On 2026-06-15 at 13:47:23 -0700, Ashutosh Dixit wrote:
> > In addition to PCI display class devices, Intel devices can also be PCI
> > accelerator class devices. Extend intel_get_pci_device() to also find these
> > devices.
>
> Please also write about which tool needs it.

Why? Reviewers can cscope and find which tools are using
intel_get_pci_device(). There are many tools in tools/ directory using that
function, it's not just intel_reg, though that is the main reason this
patch is being floated.

>
> Btw intel_reg should work with --bdf option, did you checked
> that it will work with your case without this patch?

Yes, it works with "--pci-slot' option. This patch is to make intel_reg to
work even without that option. Just as it does today with discrete display
class PCI devices (discrete gpu's), so we want to make it work the same way
even for accelerator class PCI devices.

>
> >
> > v2: Introduce intel_get_pci_device_disp for those tools who specifically
> >     want only display class devices (Kamiil)

This function is renamed to intel_get_pci_device_display() in v3.

> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >  lib/intel_chipset.c             | 82 +++++++++++++++++++++++----------
> >  lib/intel_chipset.h             |  1 +
> >  tools/intel_audio_dump.c        |  2 +-
> >  tools/intel_backlight.c         |  2 +-
> >  tools/intel_display_bandwidth.c |  4 +-
> >  tools/intel_display_poller.c    |  4 +-
> >  tools/intel_panel_fitter.c      |  2 +-
> >  tools/intel_watermark.c         |  2 +-
> >  8 files changed, 66 insertions(+), 33 deletions(-)
>
> Why there are changes to all tools but not to intel_reg?

Because in this patch, intel_get_pci_device() searches over all relevant
Intel devices, which means display devices (which was done previously) and
accelerator devices (which is new).

So intel_reg just calls intel_get_pci_device() as before and is
unchanged. See below.

>
> >
> > diff --git a/lib/intel_chipset.c b/lib/intel_chipset.c
> > index 760faede24..ff5f6c0313 100644
> > --- a/lib/intel_chipset.c
> > +++ b/lib/intel_chipset.c
> > @@ -63,16 +63,34 @@
> >   */
> >  enum pch_type intel_pch;
> >
> > -/**
> > - * intel_get_pci_device:
> > - *
> > - * Looks up the main graphics pci device using libpciaccess.
> > - *
> > - * Returns:
> > - * The pci_device, exits the program on any failures.
> > - */
> > -struct pci_device *
> > -intel_get_pci_device(void)
> > +static struct pci_device *intel_pci_device_by_class(uint8_t class)
>
> Maybe use two params? So this will be more flexible,
> provided that class zero is non-device.

Which two params?

>
> > +{
> > +	struct pci_device *pci_dev;
> > +	struct pci_device_iterator *iter;
> > +	struct pci_id_match match;
> > +	int error;
> > +
> > +	error = igt_pci_system_init();
> > +	igt_fail_on_f(error != 0, "Couldn't initialize PCI system\n");
> > +
> > +	match.vendor_id = 0x8086; /* Intel */
> > +	match.device_id = PCI_MATCH_ANY;
> > +	match.subvendor_id = PCI_MATCH_ANY;
> > +	match.subdevice_id = PCI_MATCH_ANY;
> > +
> > +	match.device_class = class << 16;
> > +	match.device_class_mask = 0xff << 16;
> > +
> > +	match.match_data = 0;
> > +
> > +	iter = pci_id_match_iterator_create(&match);
> > +	pci_dev = pci_device_next(iter);
> > +	pci_iterator_destroy(iter);
> > +
> > +	return pci_dev;
> > +}
> > +
> > +static struct pci_device *__intel_get_pci_device(bool accel)
>
> Please do not add bool params, they are confusing.

But that is the use case this patch implements? If not bool then what do
we want?

>
> >  {
> >	struct pci_device *pci_dev;
> >	int error;
> > @@ -85,22 +103,12 @@ intel_get_pci_device(void)
> >	 * walk the entire PCI bus for a matching device. */
> >	pci_dev = pci_device_find_by_slot(0, 0, 2, 0);
> >	if (pci_dev == NULL || pci_dev->vendor_id != 0x8086) {
> > -		struct pci_device_iterator *iter;
> > -		struct pci_id_match match;
> > +		/* PCI display class (0x3) devices */
> > +		pci_dev = intel_pci_device_by_class(0x3);
> >
> > -		match.vendor_id = 0x8086; /* Intel */
> > -		match.device_id = PCI_MATCH_ANY;
> > -		match.subvendor_id = PCI_MATCH_ANY;
> > -		match.subdevice_id = PCI_MATCH_ANY;
> > -
> > -		match.device_class = 0x3 << 16;
> > -		match.device_class_mask = 0xff << 16;
> > -
> > -		match.match_data = 0;
> > -
> > -		iter = pci_id_match_iterator_create(&match);
> > -		pci_dev = pci_device_next(iter);
> > -		pci_iterator_destroy(iter);
> > +		if (!pci_dev && accel)
> > +			/* PCI accelerator class (0x12) devices */
> > +			pci_dev = intel_pci_device_by_class(0x12);
> >	}
> >	igt_require_f(pci_dev, "Couldn't find Intel graphics card\n");
> >
> > @@ -114,6 +122,30 @@ intel_get_pci_device(void)
> >	return pci_dev;
> >  }
> >
> > +/**
> > + * intel_get_pci_device:
> > + *
> > + * Looks up display and accelerator class pci devices using libpciaccess
> > + *
> > + * Returns: The pci_device, exits the program on any failures
> > + */
> > +struct pci_device *intel_get_pci_device(void)
>
> Please do not change this function, or remove it, your choice.
> Imho better to add:
>
> struct pci_device *intel_get_pci_device_display(void)
> struct pci_device *intel_get_pci_device_accelerator(void)
>
> or
>
> struct pci_device *intel_get_pci_display(void)
> struct pci_device *intel_get_pci_accelerator(void)
>
> +cc Jani
>
> and a helper, which will get display, and if not available,
> tries to get accelerator.

Current need is as follows:

Some tools (e.g. intel_reg) need to work with all relevant Intel
devices. They can just use intel_get_pci_device(). Or we can rename
intel_get_pci_device() to intel_get_pci_device_all().

If for some reason, some tools want to specifically not work with
accelerator devices (i.e. they only want to work with display devices),
they use intel_get_pci_device_display().

We want either (a) all (accelerator + display), which is implemented by
intel_get_pci_device(), or we want (b) display, which is implemented by
intel_get_pci_device_display().

Therefore there is no use case today for
intel_get_pci_device_accelerator(). If such a need arises in the future,
this should be implemented at that time. Linux kernel practice is not
implement cases till they are actually needed, because what needs will
arise in the future cannot be predicted. So the same practice is followed
here also for IGT.

I actually prefer v1 of the patch which doesn't even add
intel_get_pci_device_display(). (And therefore there are no changes to the
tools there). Everybody just uses intel_get_pci_device() as before and the
function supports all valid Intel devices. The display tools will just not
work as expected on accelerator class devices, but the tools are not run
automatically in CI and people running the display tools manually will know
not to run them on accelerator class devices. But anyway, we can have
intel_get_pci_device_display() if we are picky and that is done in v2 and
v3 of the patch.

Thanks.
--
Ashutosh

>
> > +{
> > +	return __intel_get_pci_device(true);
> > +}
> > +
> > +/**
> > + * intel_get_pci_device_disp:
> > + *
> > + * Looks up display class pci device using libpciaccess
> > + *
> > + * Returns: The pci_device, exits the program on any failures
> > + */
> > +struct pci_device *intel_get_pci_device_disp(void)
> > +{
> > +	return __intel_get_pci_device(false);
> > +}
> > +
> >  static uint32_t __i915_get_drm_devid(int fd)
> >  {
> >	struct drm_i915_getparam gp;
> > diff --git a/lib/intel_chipset.h b/lib/intel_chipset.h
> > index 3fcc5b18d6..ddacbb72b4 100644
> > --- a/lib/intel_chipset.h
> > +++ b/lib/intel_chipset.h
> > @@ -36,6 +36,7 @@
> >  #define BIT(x) (1ul <<(x))
> >
> >  struct pci_device *intel_get_pci_device(void);
> > +struct pci_device *intel_get_pci_device_disp(void);
> >  uint32_t intel_get_drm_devid(int fd);
> >
> >  struct intel_device_info {
> > diff --git a/tools/intel_audio_dump.c b/tools/intel_audio_dump.c
> > index 287dbd4759..1ecac37a21 100644
> > --- a/tools/intel_audio_dump.c
> > +++ b/tools/intel_audio_dump.c
> > @@ -2468,7 +2468,7 @@ int main(int argc, char **argv)
> >	struct pci_device *pci_dev;
> >	struct intel_mmio_data mmio_data;
> >
> > -	pci_dev = intel_get_pci_device();
> > +	pci_dev = intel_get_pci_device_disp();
> >	devid = pci_dev->device_id; /* XXX not true when mapping! */
> >
> >	do_self_tests();
> > diff --git a/tools/intel_backlight.c b/tools/intel_backlight.c
> > index edf060224f..c71862f3c2 100644
> > --- a/tools/intel_backlight.c
> > +++ b/tools/intel_backlight.c
> > @@ -41,7 +41,7 @@ int main(int argc, char** argv)
> >	struct intel_mmio_data mmio_data;
> >	uint32_t current, max;
> >
> > -	intel_mmio_use_pci_bar(&mmio_data, intel_get_pci_device());
> > +	intel_mmio_use_pci_bar(&mmio_data, intel_get_pci_device_disp());
> >
> >	current = INREG(BLC_PWM_CPU_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
> >	max = INREG(BLC_PWM_PCH_CTL2) >> 16;
> > diff --git a/tools/intel_display_bandwidth.c b/tools/intel_display_bandwidth.c
> > index b798f8b652..204b71e810 100644
> > --- a/tools/intel_display_bandwidth.c
> > +++ b/tools/intel_display_bandwidth.c
> > @@ -152,14 +152,14 @@ int main(int argc, char *argv[])
> >		}
> >	}
> >
> > -	devid = intel_get_pci_device()->device_id;
> > +	devid = intel_get_pci_device_disp()->device_id;
> >
> >	if (!has_de_power2(devid)) {
> >		fprintf(stderr, "Display bandwidth counter not available\n");
> >		return 2;
> >	}
> >
> > -	intel_register_access_init(&mmio_data, intel_get_pci_device(), 0);
> > +	intel_register_access_init(&mmio_data, intel_get_pci_device_disp(), 0);
> >
> >	if (has_de_power2_abox0_abox1(devid))
> >		measure_de_power2_abox0_abox1(devid, sleep_duration);
> > diff --git a/tools/intel_display_poller.c b/tools/intel_display_poller.c
> > index 6338f22223..c2b85667ec 100644
> > --- a/tools/intel_display_poller.c
> > +++ b/tools/intel_display_poller.c
> > @@ -1508,7 +1508,7 @@ int main(int argc, char *argv[])
> >		}
> >	}
> >
> > -	devid = intel_get_pci_device()->device_id;
> > +	devid = intel_get_pci_device_disp()->device_id;
> >
> >	/*
> >	 * check if the requires registers are
> > @@ -1677,7 +1677,7 @@ int main(int argc, char *argv[])
> >		break;
> >	}
> >
> > -	intel_register_access_init(&mmio_data, intel_get_pci_device(), 0);
> > +	intel_register_access_init(&mmio_data, intel_get_pci_device_disp(), 0);
> >
> >	if (auto_scanline_offset)
> >		scanline_offset = default_scanline_offset(devid, pipe);
> > diff --git a/tools/intel_panel_fitter.c b/tools/intel_panel_fitter.c
> > index d9555d5c67..175b4fbd53 100644
> > --- a/tools/intel_panel_fitter.c
> > +++ b/tools/intel_panel_fitter.c
> > @@ -280,7 +280,7 @@ int main (int argc, char *argv[])
> >	       "with overscan compensation properties: it is just a temporary "
> >	       "solution that may or may not work. Use it at your own risk.\n");
> >
> > -	pci_dev = intel_get_pci_device();
> > +	pci_dev = intel_get_pci_device_disp();
> >	intel_register_access_init(&mmio_data, pci_dev, 0);
> >	devid = pci_dev->device_id;
> >
> > diff --git a/tools/intel_watermark.c b/tools/intel_watermark.c
> > index 4ebd6eb930..d0678e6e16 100644
> > --- a/tools/intel_watermark.c
> > +++ b/tools/intel_watermark.c
> > @@ -328,7 +328,7 @@ static void skl_wm_dump(void)
> >	uint32_t wm_linetime[num_pipes];
> >	uint32_t arb_ctl, arb_ctl2, wm_dbg;
> >
> > -	intel_register_access_init(&mmio_data, intel_get_pci_device(), 0);
> > +	intel_register_access_init(&mmio_data, intel_get_pci_device_disp(), 0);
> >
> >	arb_ctl = read_reg(0x45000);
> >	arb_ctl2 = read_reg(0x45004);
> > --
> > 2.54.0
> >