Re: [PATCH v4 1/2] lib/drmtest: add __drm_open_driver_path()

[Gustavo Sousa <gustavo.sousa@intel.com>]

Kamil Konieczny <kamil.konieczny@linux.intel.com> writes:

> Hi Jani,
> On 2026-05-26 at 15:07:57 +0300, Jani Nikula wrote:
>> On Tue, 26 May 2026, "Sebastian Brzezinka" <sebastian.brzezinka@intel.com> wrote:
>> > Hi Jani,
>> >
>> > On Tue May 26, 2026 at 1:51 PM CEST, Jani Nikula wrote:
>> >> On Tue, 26 May 2026, Sebastian Brzezinka <sebastian.brzezinka@intel.com> wrote:
>> >>> Add __drm_open_driver_path() as a path-based DRM open helper. It opens
>> >>> the given path with O_RDWR, verifies the resulting fd is a DRM device by
>> >>> issuing DRM_IOCTL_VERSION via __get_drm_device_name() before proceeding
>> >>> (returning -1 and closing on failure), logs the opened device path, and
>> >>> populates the xe_device cache when the device is Xe.
>> >>>
>> >>> The __ prefix signals that the function does not throw assertions or
>> >>> igt_require() calls, consistent with the existing __drm_open_driver*
>> >>> family.
>> >>
>> >> FWIW, I think the __ prefix should indicate "implementation detail,
>> >> don't call directly" or something along those lines.
>> >>
>> >> I think it's problematic to encourage tests to call __ prefixed
>> >> functions.
>
>
>> > In general I agree with you, but in v3 Kamil comment on this:
>> > ```
>> >>>Could you rename it into __drm_open_driver_path?
>> >>>The idea is that all __functions should not throw asserts nor
>> >>>require.
>> > ```
>> >
>> > So I added an explanation in the comment.
>> 
>> I obviously disagree with all of that, but *shrug*.
>
> We could implement separete lib functions for tools but
> that will require duplicate effort.
>
> The idea with __function is that it should be implemented
> without any igt_assert nor igt_require so tools could use them.
> We could go with function() but then developers could
> later on change behaviour and break a tool.
>
> Or we could add some suffix to a name? function_safe()?

Given the number of assertions that already exist under lib/, perhaps
this would be somewhat hard to achieve, but I feel like we should go the
other way around: a suffix to denote that a function uses IGT
assertions.

I.o.w., do_something() would be usable from both test and tools code;
do_something_asserting() would be a version of the same function that
contains assertions.

"asserting" is a bit long for a prefix, we could try to come up with
something better.

--
Gustavo Sousa

>
> Btw why tool/igt_power couldn't just read sysfs? Why does it
> needs to open /dev/dri/card* to get any info? +cc Ville
>
> Regards,
> Kamil
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> -- 
>> Jani Nikula, Intel