Re: [PATCH i-g-t,v3 1/2] lib/igt_sysfs: Promote driver rebind function

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Sep 24, 2024 at 09:54:57PM +0200, Francois Dugast wrote:
> Move the rebind function to the library so that it can be used in other
> tests without code duplication. Also extend it to support simple bind
> and unbind.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  lib/igt_sysfs.c         | 54 +++++++++++++++++++++++++++++++++++++++++
>  lib/igt_sysfs.h         |  9 +++++++
>  tests/intel/xe_wedged.c | 36 ++++-----------------------
>  3 files changed, 68 insertions(+), 31 deletions(-)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index aec0bb53d..f8e634f8f 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -1360,3 +1360,57 @@ int xe_sysfs_get_num_tiles(int xe_device)
>  
>  	return num_tiles;
>  }
> +
> +/**
> + * xe_sysfs_driver_do:
> + * @xe_device: fd of the device
> + * @pci_slot: PCI slot of the device
> + * @action: the action to perform through sysfs on the driver
> + *
> + * Use sysfs to perform an action on the driver.
> + *
> + * Returns: fd of the device, which renewed if needed
> + */
> +int xe_sysfs_driver_do(int xe_device, char pci_slot[], enum xe_sysfs_driver_action action)
> +{
> +	int sysfs;
> +
> +	sysfs = open("/sys/bus/pci/drivers/xe", O_DIRECTORY);
> +	igt_assert(sysfs);
> +
> +	switch(action) {
> +	case XE_SYSFS_DRIVER_BIND:
> +		igt_assert(igt_sysfs_set(sysfs, "bind", pci_slot));
> +		close(sysfs);
> +		break;
> +	case XE_SYSFS_DRIVER_TRY_BIND:
> +		igt_sysfs_set(sysfs, "bind", pci_slot);

Perhaps we should avoid the assert here inside and leave it for the
caller so we don't need to duplicate this?

But up to you,

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +		close(sysfs);
> +		break;
> +	case XE_SYSFS_DRIVER_UNBIND:
> +		igt_assert(igt_sysfs_set(sysfs, "unbind", pci_slot));
> +		close(sysfs);
> +		break;
> +	case XE_SYSFS_DRIVER_REBIND:
> +		igt_assert(igt_sysfs_set(sysfs, "unbind", pci_slot));
> +
> +		/*
> +		 * We need to close the client for a proper release, before
> +		 * binding back again.
> +		 */
> +		close(xe_device);
> +
> +		igt_assert(igt_sysfs_set(sysfs, "bind", pci_slot));
> +		close(sysfs);
> +
> +		/* Renew the client connection */
> +		xe_device = drm_open_driver(DRIVER_XE);
> +		igt_assert(xe_device);
> +
> +		break;
> +	default:
> +		igt_assert(!"missing");
> +	}
> +
> +	return xe_device;
> +}
> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> index 2a1e3b1bf..0ee253826 100644
> --- a/lib/igt_sysfs.h
> +++ b/lib/igt_sysfs.h
> @@ -175,4 +175,13 @@ int xe_sysfs_get_num_tiles(int xe_device);
>  char *xe_sysfs_engine_path(int xe_device, int gt, int class, char *path, int pathlen);
>  int xe_sysfs_engine_open(int xe_device, int gt, int class);
>  
> +enum xe_sysfs_driver_action {
> +	XE_SYSFS_DRIVER_BIND,
> +	XE_SYSFS_DRIVER_TRY_BIND,
> +	XE_SYSFS_DRIVER_UNBIND,
> +	XE_SYSFS_DRIVER_REBIND,
> +};
> +
> +int xe_sysfs_driver_do(int xe_device, char pci_slot[], enum xe_sysfs_driver_action action);
> +
>  #endif /* __IGT_SYSFS_H__ */
> diff --git a/tests/intel/xe_wedged.c b/tests/intel/xe_wedged.c
> index a3f7a697f..ba790aa8d 100644
> --- a/tests/intel/xe_wedged.c
> +++ b/tests/intel/xe_wedged.c
> @@ -41,34 +41,6 @@ static void force_wedged(int fd)
>  	sleep(1);
>  }
>  
> -static int rebind_xe(int fd)
> -{
> -	char pci_slot[NAME_MAX];
> -	int sysfs;
> -
> -	igt_device_get_pci_slot_name(fd, pci_slot);
> -
> -	sysfs = open("/sys/bus/pci/drivers/xe", O_DIRECTORY);
> -	igt_assert(sysfs);
> -
> -        igt_assert(igt_sysfs_set(sysfs, "unbind", pci_slot));
> -
> -	/*
> -	 * We need to close the client for a proper release, before
> -	 * binding back again.
> -	 */
> -	close(fd);
> -
> -        igt_assert(igt_sysfs_set(sysfs, "bind", pci_slot));
> -	close(sysfs);
> -
> -	/* Renew the client connection */
> -	fd = drm_open_driver(DRIVER_XE);
> -	igt_assert(fd);
> -
> -	return fd;
> -}
> -
>  static int simple_ioctl(int fd)
>  {
>  	int ret;
> @@ -231,9 +203,11 @@ igt_main
>  {
>  	struct drm_xe_engine_class_instance *hwe;
>  	int fd;
> +	char pci_slot[NAME_MAX];
>  
>  	igt_fixture {
>  		fd = drm_open_driver(DRIVER_XE);
> +		igt_device_get_pci_slot_name(fd, pci_slot);
>  	}
>  
>  	igt_subtest("basic-wedged") {
> @@ -245,7 +219,7 @@ igt_main
>  
>  		force_wedged(fd);
>  		igt_assert_neq(simple_ioctl(fd), 0);
> -		fd = rebind_xe(fd);
> +		fd = xe_sysfs_driver_do(fd, pci_slot, XE_SYSFS_DRIVER_REBIND);
>  		igt_assert_eq(simple_ioctl(fd), 0);
>  		xe_for_each_engine(fd, hwe)
>  			simple_exec(fd, hwe);
> @@ -265,7 +239,7 @@ igt_main
>  		 */
>  		sleep(1);
>  		igt_assert_neq(simple_ioctl(fd), 0);
> -		fd = rebind_xe(fd);
> +		fd = xe_sysfs_driver_do(fd, pci_slot, XE_SYSFS_DRIVER_REBIND);
>  		igt_assert_eq(simple_ioctl(fd), 0);
>  		xe_for_each_engine(fd, hwe)
>  			simple_exec(fd, hwe);
> @@ -289,7 +263,7 @@ igt_main
>  		}
>  
>  		/* Tests might have failed, force a rebind before exiting */
> -		fd = rebind_xe(fd);
> +		fd = xe_sysfs_driver_do(fd, pci_slot, XE_SYSFS_DRIVER_REBIND);
>  
>  		drm_close_driver(fd);
>  	}
> -- 
> 2.43.0
>