Re: [PATCH i-g-t v5 8/8] lib/igt_kms: add macros to iterate color pipelines and colorops

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 9/2/2026 11:28 PM, Melissa Wen wrote:
> Walking a plane's color pipelines, and the colorop chain of a given
> pipeline, is open-coded in eight places across lib and the colorop tests.
> Add for_each_color_pipeline() to iterate over all color pipelines
> supported by a plane and for_each_colorop_in_pipeline() to iterate over
> all colorops of a pipeline.
> 
> The chain walk in igt_fill_plane_color_pipelines() now stores the next
> colorop reached by the NEXT property (which is immutable and doesn't
> change after being discovered) and, as a consequence, igt_find_colorop()
> becomes unused and is removed.

LGTM
Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>

> 
> Suggested-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
> 
> v4:
> - new patch, suggested by Jani
> v5:
> - store next colorop, now igt_colorop_next and igt_find_colorop are
>    unnecessary
> - use igt_unique for macro iterator
> ---
>   lib/igt_kms.c              | 76 ++++++++++++--------------------------
>   lib/igt_kms.h              | 32 ++++++++++++++--
>   tests/kms_colorop.c        | 17 +++------
>   tests/kms_colorop_helper.c | 40 +++++++-------------
>   tests/kms_properties.c     | 17 +++------
>   5 files changed, 77 insertions(+), 105 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index cabe60a31..6329e42eb 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -794,28 +794,6 @@ igt_plane_rotations(igt_display_t *display, igt_plane_t *plane,
>   	return rotations;
>   }
>   
> -/**
> - * igt_find_colorop:
> - * @display: display on which to look for colorop.
> - * @id: DRM object id of the colorop.
> - *
> - * Returns: An igt_colorop_t if found, or NULL otherwise.
> - */
> -igt_colorop_t *igt_find_colorop(igt_display_t *display, uint32_t id)
> -{
> -	int i;
> -
> -	/* find corresponding igt_colorop */
> -	for (i = 0; i < display->n_colorops; ++i) {
> -		igt_colorop_t *colorop = &display->colorops[i];
> -
> -		if (colorop->id == id)
> -			return colorop;
> -	}
> -
> -	return NULL;
> -}
> -
>   /*
>    * Retrieve all the properies specified in props_name and store them into
>    * colorop->props.
> @@ -873,28 +851,34 @@ igt_fill_plane_color_pipelines(igt_display_t *display, igt_plane_t *plane,
>   
>   	for (i = 0; i < prop->count_enums; i++) {
>   		if (prop->enums[i].value) {
> -			igt_colorop_t *colorop = &display->colorops[display->n_colorops++];
> +			igt_colorop_t *colorop;
>   
>   			igt_assert(display->n_colorops < MAX_NUM_COLOROPS);
> +			colorop = &display->colorops[display->n_colorops++];
>   
>   			igt_fill_colorop(display, plane, colorop, prop->enums[i].value, prop->enums[i].name);
>   			plane->color_pipelines[plane->num_color_pipelines++] = colorop;
>   
>   			/* get all NEXT colorops */
>   			colorop_id = igt_colorop_get_prop(display, colorop,
> -							IGT_COLOROP_NEXT);
> +							  IGT_COLOROP_NEXT);
>   			while (colorop_id) {
> -				colorop = &display->colorops[display->n_colorops++];
> +				igt_colorop_t *next;
> +
>   				igt_assert(display->n_colorops < MAX_NUM_COLOROPS);
> -				igt_fill_colorop(display, plane, colorop, colorop_id, NULL);
> +				next = &display->colorops[display->n_colorops++];
> +				igt_fill_colorop(display, plane, next, colorop_id, NULL);
> +
> +				colorop->next = next;
> +				colorop = next;
> +
>   				colorop_id = igt_colorop_get_prop(display, colorop,
> -								IGT_COLOROP_NEXT);
> +								  IGT_COLOROP_NEXT);
>   			}
>   		}
>   	}
>   
>   	igt_assert(plane->num_color_pipelines < IGT_NUM_PLANE_COLOR_PIPELINES);
> -
>   }
>   
>   /*
> @@ -3810,13 +3794,14 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_crtc_t *crtc,
>    * Add colorop properties
>    */
>   static void
> -igt_atomic_prepare_colorop_commit(igt_colorop_t *colorop, igt_crtc_t *crtc,
> +igt_atomic_prepare_colorop_commit(igt_colorop_t *color_pipeline, igt_crtc_t *crtc,
>   				  drmModeAtomicReq *req)
>   {
>   	igt_display_t *display = crtc->display;
> -	int i, next_val;
> +	igt_colorop_t *colorop;
> +	int i;
>   
> -	while (colorop) {
> +	for_each_colorop_in_pipeline(color_pipeline, colorop) {
>   		LOG(display,
>   		    "populating colorop data: %s.%d\n",
>   		    igt_crtc_name(crtc),
> @@ -3838,11 +3823,6 @@ igt_atomic_prepare_colorop_commit(igt_colorop_t *colorop, igt_crtc_t *crtc,
>   							colorop->props[i],
>   							colorop->values[i]));
>   		}
> -
> -		/* get next colorop */
> -		next_val = igt_colorop_get_prop(display, colorop,
> -						IGT_COLOROP_NEXT);
> -		colorop = igt_find_colorop(display, next_val);
>   	}
>   }
>   
> @@ -4415,18 +4395,16 @@ bool igt_plane_check_prop_is_mutable(igt_plane_t *plane,
>    */
>   bool igt_plane_is_valid_colorop(igt_plane_t *plane, igt_colorop_t *colorop)
>   {
> -	int i;
> -	bool found = false;
> +	igt_colorop_t *color_pipeline;
>   
> -	for (i = 0; i < plane->num_color_pipelines; i++) {
> -		if (plane->color_pipelines[i] == colorop) {
> -			found = true;
> -			break;
> -		}
> +	for_each_color_pipeline(plane, color_pipeline) {
> +		if (color_pipeline == colorop)
> +			return true;
>   	}
>   
> -	return found;
> +	return false;
>   }
> +
>   /**
>    * igt_plane_set_color_pipeline:
>    * @plane: Target plane.
> @@ -4932,15 +4910,9 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
>   				 * so already-committed property values aren't re-emitted on
>   				 * the next commit.
>   				 */
> -				colorop = plane->assigned_color_pipeline;
> -				while (colorop) {
> -					uint32_t next_val;
> -
> +				for_each_colorop_in_pipeline(plane->assigned_color_pipeline,
> +							     colorop)
>   					colorop->changed = 0;
> -					next_val = igt_colorop_get_prop(display, colorop,
> -									IGT_COLOROP_NEXT);
> -					colorop = igt_find_colorop(display, next_val);
> -				}
>   
>   				fd = plane->values[IGT_PLANE_IN_FENCE_FD];
>   				if (fd != -1)
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 23615dd91..0187ad9a1 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -415,17 +415,18 @@ static inline bool igt_rotation_90_or_270(igt_rotation_t rotation)
>   }
>   
>   typedef struct _igt_plane igt_plane_t;
> +typedef struct _igt_colorop igt_colorop_t;
>   
> -typedef struct {
> +typedef struct _igt_colorop {
>   	uint32_t id;
>   	igt_plane_t *plane;
> +	igt_colorop_t *next;
>   
>   	char name[DRM_PROP_NAME_LEN];
>   
>   	uint64_t changed;
>   	uint32_t props[IGT_NUM_COLOROP_PROPS];
>   	uint64_t values[IGT_NUM_COLOROP_PROPS];
> -
>   } igt_colorop_t;
>   
>   struct igt_format_mods {
> @@ -1027,6 +1028,31 @@ uint64_t igt_colorop_get_prop(igt_display_t *display, igt_colorop_t *colorop, en
>   		igt_colorop_set_prop_changed(colorop, prop); \
>   	} while (0)
>   
> +/**
> + * for_each_color_pipeline:
> + * @plane: plane to which the color pipelines belong
> + * @color_pipeline: the color pipeline to iterate
> + *
> + * Iterates through all color pipelines supported by @plane. If no color
> + * pipeline is supported, nothing happens.
> + */
> +#define for_each_color_pipeline(plane, color_pipeline) \
> +	for (int igt_unique(__i) = 0; \
> +	     igt_unique(__i) < (plane)->num_color_pipelines && \
> +	     ((color_pipeline) = (plane)->color_pipelines[igt_unique(__i)], true); \
> +	     igt_unique(__i)++)
> +
> +/**
> + * for_each_colorop_in_pipeline:
> + * @pipeline: the first colorop in a color pipeline
> + * @colorop: the colorop to iterate
> + *
> + * Iterates through a color pipeline, starting from an initial colorop pointer
> + * and extending to all subsequent colorops pointed to by next.
> + */
> +#define for_each_colorop_in_pipeline(pipeline, colorop) \
> +	for ((colorop) = (pipeline); (colorop); \
> +	     (colorop) = (colorop)->next)
>   
>   extern bool igt_colorop_has_prop_enum_value(igt_colorop_t *colorop,
>   					    enum igt_atomic_colorop_properties prop,
> @@ -1380,8 +1406,6 @@ uint64_t igt_get_writeback_fb_id(igt_output_t *output);
>   void igt_detach_crtc(igt_display_t *display, igt_output_t *output);
>   void igt_get_and_wait_out_fence(igt_output_t *output);
>   
> -igt_colorop_t *igt_find_colorop(igt_display_t *display, uint32_t id);
> -
>   bool igt_wait_for_connector_status(int drm_fd, unsigned int connector_id, double timeout,
>   				   int drm_mode);
>   int igt_get_connected_connectors(int drm_fd, uint32_t **connector_ids);
> diff --git a/tests/kms_colorop.c b/tests/kms_colorop.c
> index 2bee1eecd..1585c0ae2 100644
> --- a/tests/kms_colorop.c
> +++ b/tests/kms_colorop.c
> @@ -365,10 +365,8 @@ static void colorop_plane_test(igt_display_t *display,
>   static void check_plane_colorop_ids(igt_display_t *display)
>   {
>   	igt_plane_t *plane;
> -	int colorop_idx;
> -	igt_colorop_t *next;
> +	igt_colorop_t *colorop, *color_pipeline;
>   	igt_crtc_t *crtc;
> -	int prop_val = 0;
>   
>   	/* Use hash tables to track drm_planes and unique IDs */
>   	GHashTable *plane_set = g_hash_table_new(g_direct_hash, g_direct_equal);
> @@ -383,18 +381,15 @@ static void check_plane_colorop_ids(igt_display_t *display)
>   
>   			g_hash_table_add(plane_set, GINT_TO_POINTER(plane->drm_plane->plane_id));
>   
> -			for (colorop_idx = 0; colorop_idx < plane->num_color_pipelines; colorop_idx++) {
> -				next = plane->color_pipelines[colorop_idx];
> -				while (next) {
> +			for_each_color_pipeline(plane, color_pipeline) {
> +				for_each_colorop_in_pipeline(color_pipeline, colorop) {
>   					/* Check if the ID already exists in the set */
> -					if (g_hash_table_contains(id_set, GINT_TO_POINTER(next->id))) {
> +					if (g_hash_table_contains(id_set, GINT_TO_POINTER(colorop->id))) {
>   						igt_fail_on_f(true, "Duplicate colorop ID %u found on plane %d\n",
> -						next->id, plane->drm_plane->plane_id);
> +						colorop->id, plane->drm_plane->plane_id);
>   					}
>   
> -					g_hash_table_add(id_set, GINT_TO_POINTER(next->id));
> -					prop_val = igt_colorop_get_prop(display, next, IGT_COLOROP_NEXT);
> -					next = igt_find_colorop(display, prop_val);
> +					g_hash_table_add(id_set, GINT_TO_POINTER(colorop->id));
>   				}
>   			}
>   		}
> diff --git a/tests/kms_colorop_helper.c b/tests/kms_colorop_helper.c
> index a109c1b06..34b0f4901 100644
> --- a/tests/kms_colorop_helper.c
> +++ b/tests/kms_colorop_helper.c
> @@ -278,29 +278,25 @@ static bool can_use_colorop(igt_display_t *display, igt_colorop_t *colorop, kms_
>    * colorops[] to it.
>    */
>   static bool map_to_pipeline(igt_display_t *display,
> -			    igt_colorop_t *colorop,
> +			    igt_colorop_t *color_pipeline,
>   			    kms_colorop_t *colorops[])
>   {
> -	igt_colorop_t *next = colorop;
> +	igt_colorop_t *colorop;
>   	kms_colorop_t *current_op;
>   	int i = 0;
> -	int prop_val = 0;
>   
>   	current_op = colorops[i];
>   	i++;
>   	igt_require(current_op);
>   
> -	while (next) {
> -		if (can_use_colorop(display, next, current_op)) {
> -			current_op->colorop = next;
> +	for_each_colorop_in_pipeline(color_pipeline, colorop) {
> +		if (can_use_colorop(display, colorop, current_op)) {
> +			current_op->colorop = colorop;
>   			current_op = colorops[i];
>   			i++;
>   			if (!current_op)
>   				break;
>   		}
> -		prop_val = igt_colorop_get_prop(display, next,
> -						IGT_COLOROP_NEXT);
> -		next = igt_find_colorop(display, prop_val);
>   	}
>   
>   	if (current_op) {
> @@ -320,18 +316,16 @@ igt_colorop_t *get_color_pipeline(igt_display_t *display,
>   			          igt_plane_t *plane,
>   				  kms_colorop_t *colorops[])
>   {
> -	igt_colorop_t *colorop = NULL;
> -	int i;
> +	igt_colorop_t *color_pipeline;
>   
>   	/* go through all color pipelines */
> -	for (i = 0; i < plane->num_color_pipelines; ++i) {
> -		if (map_to_pipeline(display, plane->color_pipelines[i], colorops)) {
> -			colorop = plane->color_pipelines[i];
> -			break;
> +	for_each_color_pipeline(plane, color_pipeline) {
> +		if (map_to_pipeline(display, color_pipeline, colorops)) {
> +			return color_pipeline;
>   		}
>   	}
>   
> -	return colorop;
> +	return NULL;
>   }
>   
>   static void fill_custom_1dlut(igt_display_t *display, kms_colorop_t *colorop)
> @@ -426,8 +420,7 @@ void set_color_pipeline(igt_display_t *display,
>   			kms_colorop_t *colorops[],
>   			igt_colorop_t *color_pipeline)
>   {
> -	igt_colorop_t *next;
> -	int prop_val = 0;
> +	igt_colorop_t *colorop;
>   	int i;
>   
>   	igt_plane_set_color_pipeline(plane, color_pipeline);
> @@ -436,17 +429,12 @@ void set_color_pipeline(igt_display_t *display,
>   		set_colorop(display, colorops[i]);
>   
>   	/* set unused ops in pipeline to bypass */
> -	next = color_pipeline;
>   	i = 0;
> -	while (next) {
> -		if (!colorops[i] || colorops[i]->colorop != next)
> -			igt_colorop_set_prop_value(next, IGT_COLOROP_BYPASS, 1);
> +	for_each_colorop_in_pipeline(color_pipeline, colorop) {
> +		if (!colorops[i] || colorops[i]->colorop != colorop)
> +			igt_colorop_set_prop_value(colorop, IGT_COLOROP_BYPASS, 1);
>   		else
>   			i++;
> -
> -		prop_val = igt_colorop_get_prop(display, next,
> -						IGT_COLOROP_NEXT);
> -		next = igt_find_colorop(display, prop_val);
>   	}
>   }
>   
> diff --git a/tests/kms_properties.c b/tests/kms_properties.c
> index 76aadd162..ba07d68d6 100644
> --- a/tests/kms_properties.c
> +++ b/tests/kms_properties.c
> @@ -239,9 +239,7 @@ static void run_colorop_property_tests(igt_display_t *display,
>   {
>   	struct igt_fb fb, afb;
>   	igt_plane_t *plane;
> -	igt_colorop_t *colorop;
> -	int i;
> -	int colorop_id = 0;
> +	igt_colorop_t *colorop, *color_pipeline;
>   
>   	prepare_crtc(display, crtc, output,
>   		     &fb);
> @@ -268,23 +266,18 @@ static void run_colorop_property_tests(igt_display_t *display,
>   		}
>   
>   		/* iterate over all color pipelines on plane */
> -		for (i = 0; i < plane->num_color_pipelines; ++i) {
> -			/* iterate over all colorops in pipeline*/
> -			colorop = plane->color_pipelines[i];
> -			igt_plane_set_color_pipeline(plane, colorop);
> +		for_each_color_pipeline(plane, color_pipeline) {
> +			igt_plane_set_color_pipeline(plane, color_pipeline);
>   			igt_display_commit2(display, COMMIT_ATOMIC);
>   
> -			while (colorop) {
> +			/* iterate over all colorops in pipeline*/
> +			for_each_colorop_in_pipeline(color_pipeline, colorop) {
>   				igt_info("Testing colorop properties on %s.#%d.#%d-%s (output: %s)\n",
>   					  igt_crtc_name(crtc), plane->index,
>   					  colorop->id,
>   					  kmstest_plane_type_name(plane->type), output->name);
>   				test_properties(display->drm_fd, DRM_MODE_OBJECT_COLOROP, colorop->id,
>   						atomic, display->has_plane_color_pipeline);
> -
> -				colorop_id = igt_colorop_get_prop(display, colorop,
> -								IGT_COLOROP_NEXT);
> -				colorop = igt_find_colorop(display, colorop_id);
>   			}
>   		}
>