Re: [PATCH i-g-t v5] lib/drmtest: Create proper error report when open driver fails

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 07 May 2026, Kamil Konieczny <kamil.konieczny@linux.intel.com> wrote:
> Hi Jani,
> On 2026-05-07 at 12:20:32 +0300, Jani Nikula wrote:
>> On Wed, 06 May 2026, Kamil Konieczny <kamil.konieczny@linux.intel.com> wrote:
>> > When first try for opening driver fails then opening function is
>> > searching for next available device. When that also fails
>> > reporting is printing last errno which could mislead developer:
>> >
>> > $ build/tests/kms_dp_aux_dev
>> > IGT-Version: 2.3-NO-GIT (x86_64) (Linux: 6.17.0-19-generic x86_64)
>> > Test requirement not met in function drm_open_driver, file ../lib/drmtest.c:754:
>> > Test requirement: !(fd<0)
>> > No known gpu found for chipset flags 0x4294965755 (any)
>> > Last errno: 2, No such file or directory
>> > SKIP (0.006s)
>> >
>> > ls /dev/dri
>> > by-path  card1  renderD128
>> >
>> > The real problem here is lack of permissions as there was a card
>> > but program could not open it. Break looking for a card as soon
>> > as an error is different from ENOENT. It will create a proper
>> > error report which will print:
>> >
>> > Last errno: 13, Permission denied
>> >
>> > v3: break only when errno actually happens in open() (Kamil)
>> > v4: removed 'else', removed errno printing (Zbigniew)
>> > v5: use explicit strerror() (Kamil)
>> >
>> > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> > Cc: Karthik B S <karthik.b.s@intel.com>
>> > Cc: "Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>
>> > Cc: Jani Nikula <jani.nikula@intel.com>
>> > Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> > ---
>> >  lib/drmtest.c          | 26 +++++++++++++++++++++-----
>> >  lib/drmtest.h          |  2 +-
>> >  lib/igt_sriov_device.c |  2 +-
>> >  3 files changed, 23 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/lib/drmtest.c b/lib/drmtest.c
>> > index 4a788ea7a..084737edf 100644
>> > --- a/lib/drmtest.c
>> > +++ b/lib/drmtest.c
>> > @@ -323,6 +323,7 @@ static void log_opened_device_path(const char *device_path)
>> >   * __drm_open_device:
>> >   * @name: DRM node name
>> >   * @chipset: OR'd flags for chipset to be opened
>> > + * @error: pointer for saving errno when open() fails
>> >   *
>> >   * Open a drm legacy device node with given @name and compatible with given
>> >   * @chipset flag.
>> > @@ -334,8 +335,10 @@ static void log_opened_device_path(const char *device_path)
>> >   * name, even when driver was excluded by ANY or is not listed in known drivers.
>> >   *
>> >   * Returns: DRM file descriptor or -1 on error
>> > + *
>> > + * When open() succeeds, sets @error to zero otherwise to errno
>> >   */
>> > -int __drm_open_device(const char *name, unsigned int chipset)
>> > +int __drm_open_device(const char *name, unsigned int chipset, int *error)
>> 
>> Why can't you just return -errno? I think this is unnecessarily
>> complicated.
>
> This function returns -1 also in other cases, when open succeeds
> but chipset or IGT_FORCE_DRIVER was not matched. 
>
> It could be negative number meaning an error when it will use
> few more internal error codes like:
>
> -190  chipset not matched
> -191  forced driver not matched
> -errno -1...-133 for open() failed
>
> There are few places where there are checks for -1, they
> will need a change. When using that solution we could not call
> strerror(errnum), or should we create igterror(errnum)?

Do we need to differentiate on where the error originated really? Just
return sensible errno codes on all issues?

BR,
Jani.

>
> Regards,
> Kamil
>
>> 
>> BR,
>> Jani.
>> 
>> >  {
>> >  	const char *forced;
>> >  	char dev_name[16] = "";
>> > @@ -343,8 +346,15 @@ int __drm_open_device(const char *name, unsigned int chipset)
>> >  	int fd;
>> >  
>> >  	fd = open(name, O_RDWR);
>> > -	if (fd == -1)
>> > +	if (fd == -1) {
>> > +		if (error)
>> > +			*error = errno;
>> > +
>> >  		return -1;
>> > +	}
>> > +
>> > +	if (error)
>> > +		*error = 0;
>> >  
>> >  	if (__get_drm_device_name(fd, dev_name, sizeof(dev_name) - 1) == -1)
>> >  		goto err;
>> > @@ -426,6 +436,7 @@ static bool _is_already_opened(const char *path, int as_idx)
>> >  static int __search_and_open(const char *base, int offset, unsigned int chipset, int as_idx)
>> >  {
>> >  	const char *forced;
>> > +	int err;
>> >  
>> >  	forced = forced_driver();
>> >  	if (forced)
>> > @@ -440,9 +451,14 @@ static int __search_and_open(const char *base, int offset, unsigned int chipset,
>> >  		if (_is_already_opened(name, as_idx))
>> >  			continue;
>> >  
>> > -		fd = __drm_open_device(name, chipset);
>> > +		fd = __drm_open_device(name, chipset, &err);
>> >  		if (fd != -1)
>> >  			return fd;
>> > +
>> > +		if (err) {
>> > +			igt_debug("Error at open %s %s\n", name, strerror(err));
>> > +			break;
>> > +		}
>> >  	}
>> >  
>> >  	return -1;
>> > @@ -500,13 +516,13 @@ static int __open_driver_exact(const char *name, unsigned int chipset)
>> >  {
>> >  	int fd;
>> >  
>> > -	fd = __drm_open_device(name, chipset);
>> > +	fd = __drm_open_device(name, chipset, NULL);
>> >  	if (fd != -1)
>> >  		return fd;
>> >  
>> >  	drm_load_module(chipset);
>> >  
>> > -	return __drm_open_device(name, chipset);
>> > +	return __drm_open_device(name, chipset, NULL);
>> >  }
>> >  
>> >  /*
>> > diff --git a/lib/drmtest.h b/lib/drmtest.h
>> > index 37874d729..126feffda 100644
>> > --- a/lib/drmtest.h
>> > +++ b/lib/drmtest.h
>> > @@ -117,7 +117,7 @@ unsigned int drm_get_chipset(int fd);
>> >   */
>> >  #define IS_ALIGNED(v, a)	(((v) & ((typeof(v))(a) - 1)) == 0)
>> >  
>> > -int __drm_open_device(const char *name, unsigned int chipset);
>> > +int __drm_open_device(const char *name, unsigned int chipset, int *error);
>> >  void drm_load_module(unsigned int chipset);
>> >  int drm_open_driver_another(int idx, int chipset);
>> >  int drm_open_driver(int chipset);
>> > diff --git a/lib/igt_sriov_device.c b/lib/igt_sriov_device.c
>> > index 1f4c3ac04..21d23298b 100644
>> > --- a/lib/igt_sriov_device.c
>> > +++ b/lib/igt_sriov_device.c
>> > @@ -284,7 +284,7 @@ int igt_sriov_open_vf_drm_device(int pf, unsigned int vf_num)
>> >  	if (!found)
>> >  		return -1;
>> >  
>> > -	fd = __drm_open_device(dev_name, DRIVER_ANY);
>> > +	fd = __drm_open_device(dev_name, DRIVER_ANY, NULL);
>> >  	if (fd >= 0 && is_xe_device(fd))
>> >  		xe_device_get(fd);
>> 
>> -- 
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel