Re: [PATCH i-g-t v1] tests/intel/xe_pm: Introduce i2c subtests

[Raag Jadav <raag.jadav@intel.com>]

On Mon, Mar 24, 2025 at 09:39:15AM -0500, Lucas De Marchi wrote:
> On Fri, Mar 21, 2025 at 11:02:05PM +0530, Raag Jadav wrote:
> > Introduce subtests for i2c adapter which is used to control on-board
> > OEM sensors on selected devices. This will test D3hot/D3cold transition
> > after i2c adapter access for the devices that support it.
> > 
> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > ---
> > tests/intel/xe_pm.c | 95 +++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 95 insertions(+)
> > 
> > diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c
> > index 19b0d5ef7..233e63d17 100644
> > --- a/tests/intel/xe_pm.c
> > +++ b/tests/intel/xe_pm.c
> > @@ -11,12 +11,18 @@
> >  * Test category: functionality test
> >  */
> > 
> > +#include <dirent.h>
> > #include <limits.h>
> > #include <fcntl.h>
> > #include <string.h>
> > +#include <sys/ioctl.h>
> > +
> > +#include <linux/i2c-dev.h>
> > +#include <linux/i2c.h>
> > 
> > #include "igt.h"
> > #include "lib/igt_device.h"
> > +#include "lib/igt_kmod.h"
> > #include "lib/igt_pm.h"
> > #include "lib/igt_sysfs.h"
> > #include "lib/igt_syncobj.h"
> > @@ -774,6 +780,88 @@ static void test_mocs_suspend_resume(device_t device, enum igt_suspend_state s_s
> > 	}
> > }
> > 
> > +static int find_i2c_index(device_t device, int sysfs_fd)
> 
> s/index/adapter/
> 
> 
> I'm very confused here... how is this related to power management to be
> a subtest in xe_pm?

Ah, this will make more sense when we have driver patches from Heikki.
This is to provide CI coverage for them.

> Back in the days when I was working with IOT, it used to be the case
> that distros weren't enabling spidev/i2c-dev. Did things change and we
> can use them in what's used for CI?

My understanding is that the edge folks will be having their own distro
for the customers.

> > +{
> > +	int adapter_fd, i2c_index = -1;
> > +	struct dirent *dirent;
> > +	char adapter[32];
> > +	DIR *dir;
> > +
> > +	/* Make sure the /dev/i2c-* files exist */
> > +	igt_require(igt_kmod_load("i2c-dev", NULL) == 0);
> > +
> > +	snprintf(adapter, sizeof(adapter) - 1, "%s.%hu", "device/i2c_designware",
> > +		 (device.pci_xe->bus << 8) | (device.pci_xe->dev));
> > +	adapter_fd = openat(sysfs_fd, adapter, O_RDONLY);
> > +	igt_require_fd(adapter_fd);
> > +
> > +	dir = fdopendir(adapter_fd);
> > +	igt_assert(dir);
> > +
> > +	/* Find the i2c device node index */
> > +	while ((dirent = readdir(dir))) {
> > +		if (strncmp(dirent->d_name, "i2c-", 4) == 0) {
> > +			sscanf(dirent->d_name, "i2c-%d", &i2c_index);
> > +			break;
> > +		}
> > +	}
> > +
> > +	closedir(dir);
> > +	close(adapter_fd);
> > +	return i2c_index;
> > +}
> > +
> > +/**
> > + * SUBTEST: %s-i2c
> > + * Description:
> > + * 	Validate whether the device is able to suspend after i2c adapter access.
> > + * Functionality: pm-d3
> > + * GPU requirements: D3 feature should be supported
> > + *
> > + * arg[1]:
> > + *
> > + * @d3hot:	d3hot
> > + * @d3cold:	d3cold
> > + */
> > +static bool i2c_test(device_t device, int sysfs_fd)
> > +{
> > +	/* AMC slave details */
> > +	uint8_t addr = 0x40, reg = 0x0, buf;
> 
> as part of the AMC protocol, it'd be better to have proper defines.

You mean macros? I thought we'd have something more "typed" ;)

> > +	int i2c_index, i2c_fd;
> > +	char i2c_dev[16];
> > +	struct i2c_msg msgs[] = {
> > +		{
> > +			.addr = addr,
> > +			.flags = 0,
> > +			.len = sizeof(reg),
> > +			.buf = &reg,
> > +		}, {
> > +			.addr = addr,
> > +			.flags = I2C_M_RD,
> > +			.len = sizeof(buf),
> > +			.buf = &buf,
> > +		}
> > +	};
> > +	struct i2c_rdwr_ioctl_data msgset = {
> > +		.msgs = msgs,
> > +		.nmsgs = ARRAY_SIZE(msgs),
> > +	};
> > +
> > +	i2c_index = find_i2c_index(device, sysfs_fd);
> > +	igt_assert(i2c_index >= 0);
> > +
> > +	snprintf(i2c_dev, sizeof(i2c_dev) - 1, "/dev/i2c-%hhd", i2c_index);
> > +	i2c_fd = open(i2c_dev, O_RDWR);
> > +	igt_assert_fd(i2c_fd);
> 
> 
> wouldn't this fail if we don't have the needed kernel config or

We wouldn't be at this point without the config.

Hint: igt_require(igt_kmod_load("i2c-dev", NULL) == 0)

> > +
> > +	/* Perform an i2c transaction to trigger adapter wake */
> > +	igt_info("Accessing slave 0x%hhx on %s\n", addr, i2c_dev);
> > +	igt_assert(igt_ioctl(i2c_fd, I2C_RDWR, &msgset) >= 0);
> 
> or... the platform doesn't support talking to amc. Usually these cases
> are skips, not failures.

Yep.

Hint: igt_require_fd(adapter_fd)

Raag