Re: [PATCH i-g-t v14 2/5] lib/drmtest: Introduced drm_open_driver_another

[Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>]

Hi Kamil,

On Wednesday, 20 March 2024 16:56:56 CET Kamil Konieczny wrote:
> From: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> 
> For user convenience, introduce drm_open_driver_another as
> a wrapper for __drm_open_driver_another with skip on fail.
> Also add counterpart to open: __drm_close_driver() with
> no warning for non-opened fd.
> 
> v2: rebased (Kamil)
> v6: reword description (Janusz)
> v10: change assert into skip, add TODO note about atomic reset
>   before tests starts for i915 and on exit path (Janusz)
>   add __drm_close_driver() (Kamil)
> 
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: "Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>
> Signed-off-by: "Dominik Karol Piątkowski" <dominik.karol.piatkowski@intel.com>
> Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>  lib/drmtest.c | 109 ++++++++++++++++++++++++++++++++++----------------
>  lib/drmtest.h |   2 +
>  2 files changed, 76 insertions(+), 35 deletions(-)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index 52b5a2020..f8810da43 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -241,6 +241,35 @@ static void modulename_to_chipset(const char *name, unsigned int *chip)
>  	}
>  }
>  
> +static const char *chipset_to_str(int chipset)
> +{
> +	switch (chipset) {
> +	case DRIVER_INTEL:
> +		return "intel";
> +	case DRIVER_V3D:
> +		return "v3d";
> +	case DRIVER_VC4:
> +		return "vc4";
> +	case DRIVER_VGEM:
> +		return "vgem";
> +	case DRIVER_AMDGPU:
> +		return "amdgpu";
> +	case DRIVER_PANFROST:
> +		return "panfrost";
> +	case DRIVER_MSM:
> +		return "msm";
> +	case DRIVER_XE:
> +		return "xe";
> +	case DRIVER_VMWGFX:
> +		return "vmwgfx";
> +	case DRIVER_ANY:
> +		return "any";
> +	default:
> +		return "other";
> +	}
> +}
> +
> +
>  /*
>   * Logs path of opened device. Device path opened for the first time is logged at info level,
>   * subsequent opens (if any) are logged at debug level.
> @@ -563,6 +592,27 @@ int __drm_open_driver_another(int idx, int chipset)
>  	return fd;
>  }
>  
> +/**
> + * drm_open_driver_another:
> + * @idx: index of the device you are opening
> + * @chipset: OR'd flags for each chipset to search, eg. #DRIVER_INTEL
> + *
> + * A wrapper for __drm_open_driver with skip on fail.
> + *
> + * Returns:
> + * An open DRM fd or skips
> + */
> +int drm_open_driver_another(int idx, int chipset)
> +{
> +	int fd = __drm_open_driver_another(idx, chipset);
> +
> +	igt_skip_on_f(fd < 0, "No known gpu found for chipset flags %d (%s)\n",
> +		      chipset, chipset_to_str(chipset));
> +
> +	/* TODO: for i915 and idx > 0 add atomic reset before test */

NIT: ... and after test via exit handler, like drm_open does.
BTW, why idx > 0?

Anyway,
Reviewed-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>

> +	return fd;
> +}
> +
>  /**
>   * __drm_open_driver:
>   * @chipset: OR'd flags for each chipset to search, eg. #DRIVER_INTEL
> @@ -643,34 +693,6 @@ static void cancel_work_at_exit_render(int sig)
>  	at_exit_drm_render_fd = -1;
>  }
>  
> -static const char *chipset_to_str(int chipset)
> -{
> -	switch (chipset) {
> -	case DRIVER_INTEL:
> -		return "intel";
> -	case DRIVER_V3D:
> -		return "v3d";
> -	case DRIVER_VC4:
> -		return "vc4";
> -	case DRIVER_VGEM:
> -		return "vgem";
> -	case DRIVER_AMDGPU:
> -		return "amdgpu";
> -	case DRIVER_PANFROST:
> -		return "panfrost";
> -	case DRIVER_MSM:
> -		return "msm";
> -	case DRIVER_XE:
> -		return "xe";
> -	case DRIVER_VMWGFX:
> -		return "vmwgfx";
> -	case DRIVER_ANY:
> -		return "any";
> -	default:
> -		return "other";
> -	}
> -}
> -
>  static const char *chipset_to_vendor_str(int chipset)
>  {
>  	return chipset == DRIVER_XE ? chipset_to_str(DRIVER_INTEL) : chipset_to_str(chipset);
> @@ -733,12 +755,33 @@ static bool is_valid_fd(int fd)
>  	return false;
>  }
>  
> +/**
> + * __drm_close_driver:
> + * @fd: a drm file descriptor
> + *
> + * Check the given drm file descriptor @fd is valid and if not,
> + * return -1. For valid fd close it and make cleanups.
> + *
> + * Returns: 0 on success or -1 on error.
> + */
> +int __drm_close_driver(int fd)
> +{
> +	if (!is_valid_fd(fd))
> +		return -1;
> +
> +	/* Remove xe_device from cache. */
> +	if (is_xe_device(fd))
> +		xe_device_put(fd);
> +
> +	return close(fd);
> +}
> +
>  /**
>   * drm_close_driver:
>   * @fd: a drm file descriptor
>   *
> - * Check the given drm file descriptor @fd is valid & Close if it is
> - * an valid drm fd.
> + * Check the given drm file descriptor @fd is valid and if not issue warning.
> + * For valid fd close it and make cleanups.
>   *
>   * Returns: 0 on success or -1 on error.
>   */
> @@ -750,11 +793,7 @@ int drm_close_driver(int fd)
>  		return -1;
>  	}
>  
> -	/* Remove xe_device from cache. */
> -	if (is_xe_device(fd))
> -		xe_device_put(fd);
> -
> -	return close(fd);
> +	return __drm_close_driver(fd);
>  }
>  
>  /**
> diff --git a/lib/drmtest.h b/lib/drmtest.h
> index 588d7da17..bbe5f252f 100644
> --- a/lib/drmtest.h
> +++ b/lib/drmtest.h
> @@ -110,12 +110,14 @@ void __set_forced_driver(const char *name);
>  
>  int __drm_open_device(const char *name, unsigned int chipset);
>  void drm_load_module(unsigned int chipset);
> +int drm_open_driver_another(int idx, int chipset);
>  int drm_open_driver(int chipset);
>  int drm_open_driver_master(int chipset);
>  int drm_open_driver_render(int chipset);
>  int __drm_open_driver_another(int idx, int chipset);
>  int __drm_open_driver(int chipset);
>  int __drm_open_driver_render(int chipset);
> +int __drm_close_driver(int fd);
>  int drm_close_driver(int fd);
>  
>  int drm_reopen_driver(int fd);
>