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

[Melissa Wen <mwen@igalia.com>]

On 17/08/2026 17:57, Jani Nikula wrote:
> On Mon, 17 Aug 2026, Melissa Wen <mwen@igalia.com> wrote:
>> Walking a plane's color pipelines, and the colorop chain of a given
>> pipeline, is open-coded in six 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, together with the igt_colorop_next() helper
>> the latter builds on, and convert the existing walks.
>>
>> The chain walk in igt_fill_plane_color_pipelines() is left as is: it
>> discovers colorops while walking, so igt_find_colorop() cannot resolve
>> them yet.
>>
>> Suggested-by: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>> ---
>>
>> v4:
>> - new patch, suggested by Jani
>> ---
>>   lib/igt_kms.c              | 52 ++++++++++++++++++++------------------
>>   lib/igt_kms.h              |  9 +++++++
>>   tests/kms_colorop.c        | 17 +++++--------
>>   tests/kms_colorop_helper.c | 40 ++++++++++-------------------
>>   tests/kms_properties.c     | 17 ++++---------
>>   5 files changed, 62 insertions(+), 73 deletions(-)
> I like the diffstat, but I like the clarity more. :)
>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index e9a14c991..7a21404b8 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -815,6 +815,21 @@ igt_colorop_t *igt_find_colorop(igt_display_t *display, uint32_t id)
>>   	return NULL;
>>   }
>>   
>> +/**
>> + * igt_colorop_next:
>> + * @display: a pointer to an #igt_display_t structure
>> + * @colorop: Current colorop.
>> + *
>> + * Returns: the colorop pointed to by @colorop's NEXT property, or NULL at the
>> + * end of the chain.
>> + */
>> +igt_colorop_t *igt_colorop_next(igt_display_t *display, igt_colorop_t *colorop)
>> +{
> Hmm. Can't you get at display via colorop->plane->crtc->display? Or can
> some of those be NULL?
>
> Not a big deal to keep it, might be a rabbit hole to ensure the above.

I think currently it can't be NULL.
I can add an assert and remove the display parameters following your 
suggestion.

>
>> +	uint32_t next = igt_colorop_get_prop(display, colorop, IGT_COLOROP_NEXT);
>> +
>> +	return igt_find_colorop(display, next);
>> +}
>> +
>>   /*
>>    * Retrieve all the properies specified in props_name and store them into
>>    * colorop->props.
>> @@ -3809,13 +3824,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(display, color_pipeline, colorop) {
>>   		LOG(display,
>>   		    "populating colorop data: %s.%d\n",
>>   		    igt_crtc_name(crtc),
>> @@ -3837,11 +3853,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);
>>   	}
>>   }
>>   
>> @@ -4414,18 +4425,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.
>> @@ -4925,15 +4934,10 @@ 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(display,
>> +							     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 ace8c2b1f..b4b666796 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -1022,6 +1022,15 @@ uint64_t igt_colorop_get_prop(igt_display_t *display, igt_colorop_t *colorop, en
>>   		igt_colorop_set_prop_changed(colorop, prop); \
>>   	} while (0)
>>   
>> +igt_colorop_t *igt_colorop_next(igt_display_t *display, igt_colorop_t *colorop);
> If this is only ever to be used with the iterator, maybe call it
> __igt_colorop_next(), because otherwise people will start using it
> willy-nilly.

ack

>
>> +
>> +#define for_each_color_pipeline(plane, color_pipeline) \
>> +	for (int i__ = 0; i__ < (plane)->num_color_pipelines && \
>> +	     ((color_pipeline) = (plane)->color_pipelines[i__], true); i__++)
> With this, you could avoid the loop variable:
>
> 	for ((color_pipeline) = &(plane)->color_pipelines[0]; \
> 	     (color_pipeline) < &(plane)->color_pipelines[(plane)->num_color_pipelines]; \
> 	     (color_pipeline)++)
>
> We do this in for_each_output(), for_each_crtc(),
> for_each_connector_mode(), etc. too.

Oh, got it. I'll rearrange it in the next version.

I'll wait a little to see if more comments arrive before sending a next 
version addressing yours.

Thanks for reviewing,

Melissa

>
>> +
>> +#define for_each_colorop_in_pipeline(display, pipeline, colorop) \
>> +	for ((colorop) = (pipeline); (colorop); \
>> +	     (colorop) = igt_colorop_next((display), (colorop)))
>>   
>>   extern bool igt_colorop_has_prop_enum_value(igt_colorop_t *colorop,
>>   					    enum igt_atomic_colorop_properties prop,
>> diff --git a/tests/kms_colorop.c b/tests/kms_colorop.c
>> index 8648bd7ce..ae67d4f4c 100644
>> --- a/tests/kms_colorop.c
>> +++ b/tests/kms_colorop.c
>> @@ -296,10 +296,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);
>> @@ -314,18 +312,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(display, 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 707661378..192502c18 100644
>> --- a/tests/kms_colorop_helper.c
>> +++ b/tests/kms_colorop_helper.c
>> @@ -230,29 +230,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(display, 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) {
>> @@ -272,18 +268,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)
>> @@ -374,8 +368,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);
>> @@ -384,17 +377,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(display, 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 764c77963..1395f10ae 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(display, 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);
>>   			}
>>   		}