Re: [PATCH i-g-t 1/8] lib/kms: add and use igt_crtc_debugfs_dir()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 13, 2026 at 11:31:53AM +0200, Jani Nikula wrote:
> Add igt_crtc_debugfs_dir() for getting the debugfs for a CRTC, and use
> it where applicable. As a side effect, this fixes several cases where
> pipe is used instead of CRTC index for the debugfs dir.
> 
> Note: Looks like it's difficult to add any igt_crtc_t based interfaces
> in igt_debugfs.h due to header dependencies, so add it to igt_kms.h
> along with the rest of the crtc based helpers.

I think that's the better option. This keeps the
debugfs/etc low level and thus potentially reusable
even for tools/etc that should not suck in the whole
igt library. See eg
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/issues/16

> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  lib/i915/intel_fbc.c                   | 4 ++--
>  lib/igt_kms.c                          | 7 ++++++-
>  lib/igt_kms.h                          | 2 ++
>  tests/intel/kms_frontbuffer_tracking.c | 4 +---
>  tests/nouveau_crc.c                    | 4 +---
>  5 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/i915/intel_fbc.c b/lib/i915/intel_fbc.c
> index 9ea5e3cf68fd..b418c95e66bd 100644
> --- a/lib/i915/intel_fbc.c
> +++ b/lib/i915/intel_fbc.c
> @@ -27,7 +27,7 @@ bool intel_fbc_supported(igt_crtc_t *crtc)
>  	char buf[FBC_STATUS_BUF_LEN];
>  	int dir;
>  
> -	dir = igt_debugfs_crtc_dir(crtc->display->drm_fd, crtc->crtc_index, O_DIRECTORY);
> +	dir = igt_crtc_debugfs_dir(crtc, O_DIRECTORY);
>  	igt_require_fd(dir);
>  	igt_debugfs_simple_read(dir, "i915_fbc_status", buf, sizeof(buf));
>  	close(dir);
> @@ -44,7 +44,7 @@ static bool _intel_fbc_is_enabled(igt_crtc_t *crtc, int log_level, char *last_fb
>  	bool print = true;
>  	int dir;
>  
> -	dir = igt_debugfs_crtc_dir(crtc->display->drm_fd, crtc->crtc_index, O_DIRECTORY);
> +	dir = igt_crtc_debugfs_dir(crtc, O_DIRECTORY);
>  	igt_require_fd(dir);
>  	igt_debugfs_simple_read(dir, "i915_fbc_status", buf, sizeof(buf));
>  	close(dir);
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 0b05ddd72977..6ccc9d0e6221 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -5254,6 +5254,11 @@ igt_pipe_crc_t *igt_crtc_crc_new_nonblock(igt_crtc_t *crtc, const char *source)
>  	return igt_pipe_crc_new_nonblock(crtc->display->drm_fd, crtc->crtc_index, source);
>  }
>  
> +int igt_crtc_debugfs_dir(igt_crtc_t *crtc, int mode)
> +{
> +	return igt_debugfs_crtc_dir(crtc->display->drm_fd, crtc->crtc_index, mode);
> +}
> +
>  const char *igt_crtc_name(igt_crtc_t *crtc)
>  {
>  	if (crtc == NULL)
> @@ -6638,7 +6643,7 @@ unsigned int igt_get_crtc_current_bpc(igt_crtc_t *crtc)
>  	int fd, res;
>  	unsigned int current;
>  
> -	fd = igt_debugfs_crtc_dir(drmfd, crtc->pipe, O_RDONLY);
> +	fd = igt_crtc_debugfs_dir(crtc, O_RDONLY);

Why do we even have these 'mode' arguments to igt_debugfs_*_dir()?

This guy forgets to set O_DIRECTORY (which would make the open 
fail of the path is not a directory), and everyone else forgets
to set O_RDONLY (but O_RDONLY==0 so doesn't really matter I guess).

I think we should get rid of these 'mode' arguments and
just make the debugfs code do the right thing all by itself.

>  	igt_assert(fd >= 0);
>  
>  	if (is_intel_device(drmfd))
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 081613a4b949..2492dac77e4f 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -595,6 +595,8 @@ typedef struct _igt_pipe_crc igt_pipe_crc_t;
>  igt_pipe_crc_t *igt_crtc_crc_new(igt_crtc_t *crtc, const char *source);
>  igt_pipe_crc_t *igt_crtc_crc_new_nonblock(igt_crtc_t *crtc, const char *source);
>  
> +int igt_crtc_debugfs_dir(igt_crtc_t *crtc, int mode);
> +
>  igt_crtc_t *igt_output_get_driving_crtc(igt_output_t *output);
>  const char *igt_output_name(igt_output_t *output);
>  bool kmstest_mode_is_valid(const drmModeModeInfo *mode);
> diff --git a/tests/intel/kms_frontbuffer_tracking.c b/tests/intel/kms_frontbuffer_tracking.c
> index 12f51047d286..e83465c0d478 100644
> --- a/tests/intel/kms_frontbuffer_tracking.c
> +++ b/tests/intel/kms_frontbuffer_tracking.c
> @@ -1534,10 +1534,8 @@ static void set_mode_for_params(struct modeset_params *params)
>  static void __debugfs_read_crtc(const char *param, char *buf, int len)
>  {
>  	int dir;
> -	enum pipe pipe;
>  
> -	pipe = prim_mode_params.crtc->pipe;
> -	dir = igt_debugfs_crtc_dir(drm.fd, pipe, O_DIRECTORY);
> +	dir = igt_crtc_debugfs_dir(prim_mode_params.crtc, O_DIRECTORY);
>  	igt_require_fd(dir);
>  	igt_debugfs_simple_read(dir, param, buf, len);
>  	close(dir);
> diff --git a/tests/nouveau_crc.c b/tests/nouveau_crc.c
> index c080ded3e621..29e2d84e76ac 100644
> --- a/tests/nouveau_crc.c
> +++ b/tests/nouveau_crc.c
> @@ -370,9 +370,7 @@ int igt_main()
>  			igt_plane_set_fb(data.primary, &data.default_fb);
>  			igt_display_commit(&data.display);
>  
> -			dir = igt_debugfs_crtc_dir(data.drm_fd,
> -						   data.crtc->pipe,
> -						   O_DIRECTORY);
> +			dir = igt_crtc_debugfs_dir(data.crtc, O_DIRECTORY);
>  			igt_require_fd(dir);
>  			data.nv_crc_dir = openat(dir, "nv_crc", O_DIRECTORY);
>  			close(dir);
> -- 
> 2.47.3

-- 
Ville Syrjälä
Intel