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

[Melissa Wen <mwen@igalia.com>]

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.

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);
 			}
 		}
 
-- 
2.53.0