Re: [PATCH i-g-t 07/12] lib/crc: Convert igt_pipe_crc away from enum pipe

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

On Tue, 20 Jan 2026, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Use the correct crtc_index/crtc_offset rather than the
> incorrect pipe for the pipe crc debugfs stuff. Currently
> the two match 1:1 so no functional change, but if/when we
> change the pipe<->crtc mapping this needs to be done correctly.

Yeah, this aligns with my thinking. If a function uses the passed in
pipe as a crtc index, the function should be explicitly using crtc
(index) in function/parameter naming. If the caller requires a specific
pipe, it's the caller's responsibility to pass in a crtc that matches
the pipe they want, because *that* is the special case.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  lib/igt_pipe_crc.c | 32 ++++++++++++++++----------------
>  lib/igt_pipe_crc.h |  6 ++----
>  2 files changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/lib/igt_pipe_crc.c b/lib/igt_pipe_crc.c
> index 866bf2881cbd..9a79edd51531 100644
> --- a/lib/igt_pipe_crc.c
> +++ b/lib/igt_pipe_crc.c
> @@ -189,7 +189,7 @@ struct _igt_pipe_crc {
>  	int crc_fd;
>  	int flags;
>  
> -	enum pipe pipe;
> +	int crtc_index;
>  	char *source;
>  };
>  
> @@ -215,7 +215,7 @@ void igt_require_pipe_crc(int fd)
>  }
>  
>  static igt_pipe_crc_t *
> -pipe_crc_new(int fd, enum pipe pipe, const char *source, int flags)
> +pipe_crc_new(int fd, int crtc_index, const char *source, int flags)
>  {
>  	igt_pipe_crc_t *pipe_crc;
>  	char buf[128];
> @@ -235,14 +235,14 @@ pipe_crc_new(int fd, enum pipe pipe, const char *source, int flags)
>  	pipe_crc = calloc(1, sizeof(struct _igt_pipe_crc));
>  	igt_assert(pipe_crc);
>  
> -	sprintf(buf, "crtc-%d/crc/control", pipe);
> +	sprintf(buf, "crtc-%d/crc/control", crtc_index);
>  	pipe_crc->ctl_fd = openat(debugfs, buf, O_WRONLY);
>  	igt_assert(pipe_crc->ctl_fd != -1);
>  
>  	pipe_crc->crc_fd = -1;
>  	pipe_crc->fd = fd;
>  	pipe_crc->dir = debugfs;
> -	pipe_crc->pipe = pipe;
> +	pipe_crc->crtc_index = crtc_index;
>  	pipe_crc->source = strdup(env_source);
>  	igt_assert(pipe_crc->source);
>  	pipe_crc->flags = flags;
> @@ -253,39 +253,39 @@ pipe_crc_new(int fd, enum pipe pipe, const char *source, int flags)
>  /**
>   * igt_pipe_crc_new:
>   * @fd: fd of the device
> - * @pipe: display pipe to use as source
> + * @crtc_index: CRTC to use as source
>   * @source: CRC tap point to use as source
>   *
> - * This sets up a new pipe CRC capture object for the given @pipe and @source
> + * This sets up a new pipe CRC capture object for the given @crtc_index and @source
>   * in blocking mode.
>   *
> - * Returns: A pipe CRC object for the given @pipe and @source. The library
> + * Returns: A pipe CRC object for the given @crtc_index and @source. The library
>   * assumes that the source is always available since recent kernels support at
>   * least IGT_PIPE_CRC_SOURCE_AUTO everywhere.
>   */
>  igt_pipe_crc_t *
> -igt_pipe_crc_new(int fd, enum pipe pipe, const char *source)
> +igt_pipe_crc_new(int fd, int crtc_index, const char *source)
>  {
> -	return pipe_crc_new(fd, pipe, source, O_RDONLY);
> +	return pipe_crc_new(fd, crtc_index, source, O_RDONLY);
>  }
>  
>  /**
>   * igt_pipe_crc_new_nonblock:
>   * @fd: fd of the device
> - * @pipe: display pipe to use as source
> + * @crtc_index: CRTC to use as source
>   * @source: CRC tap point to use as source
>   *
> - * This sets up a new pipe CRC capture object for the given @pipe and @source
> + * This sets up a new pipe CRC capture object for the given @crtc_index and @source
>   * in nonblocking mode.
>   *
> - * Returns: A pipe CRC object for the given @pipe and @source. The library
> + * Returns: A pipe CRC object for the given @crtc_index and @source. The library
>   * assumes that the source is always available since recent kernels support at
>   * least IGT_PIPE_CRC_SOURCE_AUTO everywhere.
>   */
>  igt_pipe_crc_t *
> -igt_pipe_crc_new_nonblock(int fd, enum pipe pipe, const char *source)
> +igt_pipe_crc_new_nonblock(int fd, int crtc_index, const char *source)
>  {
> -	return pipe_crc_new(fd, pipe, source, O_RDONLY | O_NONBLOCK);
> +	return pipe_crc_new(fd, crtc_index, source, O_RDONLY | O_NONBLOCK);
>  }
>  
>  /**
> @@ -380,7 +380,7 @@ void igt_pipe_crc_start(igt_pipe_crc_t *pipe_crc)
>  
>  	igt_assert_eq(write(pipe_crc->ctl_fd, src, strlen(src)), strlen(src));
>  
> -	sprintf(buf, "crtc-%d/crc/data", pipe_crc->pipe);
> +	sprintf(buf, "crtc-%d/crc/data", pipe_crc->crtc_index);
>  
>  	igt_set_timeout(10, "Opening crc fd, and poll for first CRC.");
>  	pipe_crc->crc_fd = openat(pipe_crc->dir, buf, pipe_crc->flags);
> @@ -560,7 +560,7 @@ igt_pipe_crc_get_for_frame(int drm_fd, igt_pipe_crc_t *pipe_crc,
>  void
>  igt_pipe_crc_get_current(int drm_fd, igt_pipe_crc_t *pipe_crc, igt_crc_t *crc)
>  {
> -	unsigned vblank = kmstest_get_vblank(drm_fd, pipe_crc->pipe, 0) + 1;
> +	unsigned vblank = kmstest_get_vblank(drm_fd, pipe_crc->crtc_index, 0) + 1;
>  
>  	return igt_pipe_crc_get_for_frame(drm_fd, pipe_crc, vblank, crc);
>  }
> diff --git a/lib/igt_pipe_crc.h b/lib/igt_pipe_crc.h
> index 171f082300ec..f648b2c9231e 100644
> --- a/lib/igt_pipe_crc.h
> +++ b/lib/igt_pipe_crc.h
> @@ -9,8 +9,6 @@
>  #include <stdbool.h>
>  #include <stdint.h>
>  
> -enum pipe;
> -
>  /**
>   * igt_pipe_crc_t:
>   *
> @@ -48,9 +46,9 @@ char *igt_crc_to_string(igt_crc_t *crc);
>  
>  void igt_require_pipe_crc(int fd);
>  igt_pipe_crc_t *
> -igt_pipe_crc_new(int fd, enum pipe pipe, const char *source);
> +igt_pipe_crc_new(int fd, int crtc_index, const char *source);
>  igt_pipe_crc_t *
> -igt_pipe_crc_new_nonblock(int fd, enum pipe pipe, const char *source);
> +igt_pipe_crc_new_nonblock(int fd, int crtc_index, const char *source);
>  void igt_pipe_crc_free(igt_pipe_crc_t *pipe_crc);
>  void igt_pipe_crc_start(igt_pipe_crc_t *pipe_crc);
>  void igt_pipe_crc_stop(igt_pipe_crc_t *pipe_crc);

-- 
Jani Nikula, Intel