[PATCH i-g-t v2 06/15] igt_kms: Change PIPE_ANY behavior to mean unassigned

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

None of the tests requires that a output bound to PIPE_ANY is assigned,
so don't do it. Fix the display commit to iterate over crtc's instead
of outputs to properly disable pipes without outputs.

This also means that output->valid is only set after connecting a
output to a pipe, so no longer depend on it in for_each_connected_output
and similar macros.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 lib/igt_kms.c | 246 ++++++++++++++++++++++++++++------------------------------
 lib/igt_kms.h |  18 ++++-
 2 files changed, 135 insertions(+), 129 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index ce8aa2455348..88cae7d51787 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -869,18 +869,20 @@ static bool _kmstest_connector_config(int drm_fd, uint32_t connector_id,
 	 */
 	_kmstest_connector_config_crtc_mask(drm_fd, connector, config);
 
+	if (!kmstest_get_connector_default_mode(drm_fd, connector,
+						&config->default_mode))
+		goto err3;
+
+	config->connector = connector;
+
 	crtc_idx_mask &= config->valid_crtc_idx_mask;
 	if (!crtc_idx_mask)
-		goto err3;
+		/* Keep config->connector */
+		goto err2;
 
 	config->pipe = ffs(crtc_idx_mask) - 1;
 
-	if (!kmstest_get_connector_default_mode(drm_fd, connector,
-						&config->default_mode))
-		goto err3;
-
 	config->encoder = _kmstest_connector_config_find_encoder(drm_fd, connector, config->pipe);
-	config->connector = connector;
 	config->crtc = drmModeGetCrtc(drm_fd, resources->crtcs[config->pipe]);
 
 	drmModeFreeResources(resources);
@@ -940,8 +942,13 @@ bool kmstest_probe_connector_config(int drm_fd, uint32_t connector_id,
 void kmstest_free_connector_config(struct kmstest_connector_config *config)
 {
 	drmModeFreeCrtc(config->crtc);
+	config->crtc = NULL;
+
 	drmModeFreeEncoder(config->encoder);
+	config->encoder = NULL;
+
 	drmModeFreeConnector(config->connector);
+	config->connector = NULL;
 }
 
 /**
@@ -1197,8 +1204,7 @@ static void igt_output_refresh(igt_output_t *output)
 	/* we mask out the pipes already in use */
 	crtc_idx_mask = output->pending_crtc_idx_mask & ~display->pipes_in_use;
 
-	if (output->valid)
-		kmstest_free_connector_config(&output->config);
+	kmstest_free_connector_config(&output->config);
 
 	ret = kmstest_get_connector_config(display->drm_fd,
 					   output->id,
@@ -1209,19 +1215,19 @@ static void igt_output_refresh(igt_output_t *output)
 	else
 		output->valid = false;
 
-	if (!output->valid)
-		return;
-
-	if (output->use_override_mode)
-		output->config.default_mode = output->override_mode;
-
-	if (!output->name) {
+	if (!output->name && output->config.connector) {
 		drmModeConnector *c = output->config.connector;
 
 		igt_assert_neq(asprintf(&output->name, "%s-%d", kmstest_connector_type_str(c->connector_type), c->connector_type_id),
 			       -1);
 	}
 
+	if (!output->valid)
+		return;
+
+	if (output->use_override_mode)
+		output->config.default_mode = output->override_mode;
+
 	LOG(display, "%s: Selecting pipe %s\n", output->name,
 	    kmstest_pipe_name(output->config.pipe));
 
@@ -1259,10 +1265,10 @@ get_crtc_property(int drm_fd, uint32_t crtc_id, const char *name,
 }
 
 static void
-igt_crtc_set_property(igt_output_t *output, uint32_t prop_id, uint64_t value)
+igt_crtc_set_property(igt_pipe_t *pipe, uint32_t prop_id, uint64_t value)
 {
-	drmModeObjectSetProperty(output->display->drm_fd,
-		output->config.crtc->crtc_id, DRM_MODE_OBJECT_CRTC, prop_id, value);
+	drmModeObjectSetProperty(pipe->display->drm_fd,
+		pipe->crtc_id, DRM_MODE_OBJECT_CRTC, prop_id, value);
 }
 
 /*
@@ -1325,15 +1331,37 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 		int p = IGT_PLANE_2;
 		int j, type;
 		uint8_t n_planes = 0;
+		uint64_t prop_value;
 
 		pipe->crtc_id = resources->crtcs[i];
 		pipe->display = display;
 		pipe->pipe = i;
 
+		get_crtc_property(display->drm_fd, pipe->crtc_id,
+				    "background_color",
+				    &pipe->background_property,
+				    &prop_value,
+				    NULL);
+		pipe->background = (uint32_t)prop_value;
+		get_crtc_property(display->drm_fd, pipe->crtc_id,
+				  "DEGAMMA_LUT",
+				  &pipe->degamma_property,
+				  NULL,
+				  NULL);
+		get_crtc_property(display->drm_fd, pipe->crtc_id,
+				  "CTM",
+				  &pipe->ctm_property,
+				  NULL,
+				  NULL);
+		get_crtc_property(display->drm_fd, pipe->crtc_id,
+				  "GAMMA_LUT",
+				  &pipe->gamma_property,
+				  NULL,
+				  NULL);
+
 		/* add the planes that can be used with that pipe */
 		for (j = 0; j < plane_resources->count_planes; j++) {
 			drmModePlane *drm_plane;
-			uint64_t prop_value;
 
 			drm_plane = drmModeGetPlane(display->drm_fd,
 						    plane_resources->planes[j]);
@@ -1440,47 +1468,17 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 	igt_assert(display->outputs);
 
 	for (i = 0; i < display->n_outputs; i++) {
-		int j;
 		igt_output_t *output = &display->outputs[i];
 
 		/*
-		 * We're free to select any pipe to drive that output until
-		 * a constraint is set with igt_output_set_pipe().
+		 * We don't assign each output a pipe unless
+		 * a pipe is set with igt_output_set_pipe().
 		 */
-		output->pending_crtc_idx_mask = -1UL;
+		output->pending_crtc_idx_mask = 0;
 		output->id = resources->connectors[i];
 		output->display = display;
 
 		igt_output_refresh(output);
-
-		for (j = 0; j < display->n_pipes; j++) {
-			uint64_t prop_value;
-			igt_pipe_t *pipe = &display->pipes[j];
-
-			if (output->config.crtc) {
-				get_crtc_property(display->drm_fd, output->config.crtc->crtc_id,
-						   "background_color",
-						   &pipe->background_property,
-						   &prop_value,
-						   NULL);
-				pipe->background = (uint32_t)prop_value;
-				get_crtc_property(display->drm_fd, output->config.crtc->crtc_id,
-						  "DEGAMMA_LUT",
-						  &pipe->degamma_property,
-						  NULL,
-						  NULL);
-				get_crtc_property(display->drm_fd, output->config.crtc->crtc_id,
-						  "CTM",
-						  &pipe->ctm_property,
-						  NULL,
-						  NULL);
-				get_crtc_property(display->drm_fd, output->config.crtc->crtc_id,
-						  "GAMMA_LUT",
-						  &pipe->gamma_property,
-						  NULL,
-						  NULL);
-			}
-		}
 	}
 
 	drmModeFreePlaneResources(plane_resources);
@@ -1545,7 +1543,7 @@ static void igt_display_refresh(igt_display_t *display)
         for (i = 0; i < display->n_outputs; i++) {
                 igt_output_t *a = &display->outputs[i];
 
-                if (a->pending_crtc_idx_mask == -1UL)
+                if (!a->pending_crtc_idx_mask)
                         continue;
 
                 for (j = 0; j < display->n_outputs; j++) {
@@ -1554,9 +1552,6 @@ static void igt_display_refresh(igt_display_t *display)
                         if (i == j)
                                 continue;
 
-                        if (b->pending_crtc_idx_mask == -1UL)
-                                continue;
-
                         igt_assert_f(a->pending_crtc_idx_mask !=
                                      b->pending_crtc_idx_mask,
                                      "%s and %s are both trying to use pipe %s\n",
@@ -1565,25 +1560,9 @@ static void igt_display_refresh(igt_display_t *display)
                 }
         }
 
-	/*
-	 * The pipe allocation has to be done in two phases:
-	 *   - first, try to satisfy the outputs where a pipe has been specified
-	 *   - then, allocate the outputs with PIPE_ANY
-	 */
 	for (i = 0; i < display->n_outputs; i++) {
 		igt_output_t *output = &display->outputs[i];
 
-		if (output->pending_crtc_idx_mask == -1UL)
-			continue;
-
-		igt_output_refresh(output);
-	}
-	for (i = 0; i < display->n_outputs; i++) {
-		igt_output_t *output = &display->outputs[i];
-
-		if (output->pending_crtc_idx_mask != -1UL)
-			continue;
-
 		igt_output_refresh(output);
 	}
 }
@@ -1593,12 +1572,11 @@ static igt_pipe_t *igt_output_get_driving_pipe(igt_output_t *output)
 	igt_display_t *display = output->display;
 	enum pipe pipe;
 
-	if (output->pending_crtc_idx_mask == -1UL) {
+	if (!output->pending_crtc_idx_mask) {
 		/*
-		 * The user hasn't specified a pipe to use, take the one
-		 * configured by the last refresh()
+		 * The user hasn't specified a pipe to use, return none.
 		 */
-		pipe = output->config.pipe;
+		return NULL;
 	} else {
 		/*
 		 * Otherwise, return the pending pipe (ie the pipe that should
@@ -1628,6 +1606,21 @@ static igt_plane_t *igt_pipe_get_plane(igt_pipe_t *pipe, enum igt_plane plane)
 	return &pipe->planes[idx];
 }
 
+static igt_output_t *igt_pipe_get_output(igt_pipe_t *pipe)
+{
+	igt_display_t *display = pipe->display;
+	int i;
+
+	for (i = 0; i < display->n_outputs; i++) {
+		igt_output_t *output = &display->outputs[i];
+
+		if (output->pending_crtc_idx_mask == (1 << pipe->pipe))
+			return output;
+	}
+
+	return NULL;
+}
+
 bool igt_pipe_get_property(igt_pipe_t *pipe, const char *name,
 			   uint32_t *prop_id, uint64_t *value,
 			   drmModePropertyPtr *prop)
@@ -1741,10 +1734,10 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_output_t *output,
  * tests that expect a specific error code).
  */
 static int igt_drm_plane_commit(igt_plane_t *plane,
-				igt_output_t *output,
+				igt_pipe_t *pipe,
 				bool fail_on_error)
 {
-	igt_display_t *display = output->display;
+	igt_display_t *display = pipe->display;
 	uint32_t fb_id, crtc_id;
 	int ret;
 	uint32_t src_x;
@@ -1763,13 +1756,12 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
 		   !plane->rotation_changed);
 
 	fb_id = igt_plane_get_fb_id(plane);
-	crtc_id = output->config.crtc->crtc_id;
+	crtc_id = pipe->crtc_id;
 
 	if ((plane->fb_changed || plane->size_changed) && fb_id == 0) {
 		LOG(display,
-		    "%s: SetPlane pipe %s, plane %d, disabling\n",
-		    igt_output_name(output),
-		    kmstest_pipe_name(output->config.pipe),
+		    "SetPlane pipe %s, plane %d, disabling\n",
+		    kmstest_pipe_name(pipe->pipe),
 		    plane->index);
 
 		ret = drmModeSetPlane(display->drm_fd,
@@ -1797,10 +1789,9 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
 		crtc_h = plane->crtc_h;
 
 		LOG(display,
-		    "%s: SetPlane %s.%d, fb %u, src = (%d, %d) "
+		    "SetPlane %s.%d, fb %u, src = (%d, %d) "
 			"%ux%u dst = (%u, %u) %ux%u\n",
-		    igt_output_name(output),
-		    kmstest_pipe_name(output->config.pipe),
+		    kmstest_pipe_name(pipe->pipe),
 		    plane->index,
 		    fb_id,
 		    src_x >> 16, src_y >> 16, src_w >> 16, src_h >> 16,
@@ -1841,11 +1832,11 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
  * code).
  */
 static int igt_cursor_commit_legacy(igt_plane_t *cursor,
-				    igt_output_t *output,
+				    igt_pipe_t *pipe,
 				    bool fail_on_error)
 {
-	igt_display_t *display = output->display;
-	uint32_t crtc_id = output->config.crtc->crtc_id;
+	igt_display_t *display = pipe->display;
+	uint32_t crtc_id = pipe->crtc_id;
 	int ret;
 
 	if (cursor->fb_changed) {
@@ -1853,9 +1844,8 @@ static int igt_cursor_commit_legacy(igt_plane_t *cursor,
 
 		if (gem_handle) {
 			LOG(display,
-			    "%s: SetCursor pipe %s, fb %u %dx%d\n",
-			    igt_output_name(output),
-			    kmstest_pipe_name(output->config.pipe),
+			    "SetCursor pipe %s, fb %u %dx%d\n",
+			    kmstest_pipe_name(pipe->pipe),
 			    gem_handle,
 			    cursor->crtc_w, cursor->crtc_h);
 
@@ -1865,9 +1855,8 @@ static int igt_cursor_commit_legacy(igt_plane_t *cursor,
 					       cursor->crtc_h);
 		} else {
 			LOG(display,
-			    "%s: SetCursor pipe %s, disabling\n",
-			    igt_output_name(output),
-			    kmstest_pipe_name(output->config.pipe));
+			    "SetCursor pipe %s, disabling\n",
+			    kmstest_pipe_name(pipe->pipe));
 
 			ret = drmModeSetCursor(display->drm_fd, crtc_id,
 					       0, 0, 0);
@@ -1883,9 +1872,8 @@ static int igt_cursor_commit_legacy(igt_plane_t *cursor,
 		int y = cursor->crtc_y;
 
 		LOG(display,
-		    "%s: MoveCursor pipe %s, (%d, %d)\n",
-		    igt_output_name(output),
-		    kmstest_pipe_name(output->config.pipe),
+		    "MoveCursor pipe %s, (%d, %d)\n",
+		    kmstest_pipe_name(pipe->pipe),
 		    x, y);
 
 		ret = drmModeMoveCursor(display->drm_fd, crtc_id, x, y);
@@ -1902,10 +1890,11 @@ static int igt_cursor_commit_legacy(igt_plane_t *cursor,
  * (setmode).
  */
 static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
-					   igt_output_t *output,
+					   igt_pipe_t *pipe,
 					   bool fail_on_error)
 {
 	struct igt_display *display = primary->pipe->display;
+	igt_output_t *output = igt_pipe_get_output(pipe);
 	drmModeModeInfo *mode;
 	uint32_t fb_id, crtc_id;
 	int ret;
@@ -1920,7 +1909,7 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
 		!primary->size_changed && !primary->panning_changed)
 		return 0;
 
-	crtc_id = output->config.crtc->crtc_id;
+	crtc_id = pipe->crtc_id;
 	fb_id = igt_plane_get_fb_id(primary);
 	if (fb_id)
 		mode = igt_output_get_mode(output);
@@ -1932,7 +1921,7 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
 		    "%s: SetCrtc pipe %s, fb %u, panning (%d, %d), "
 		    "mode %dx%d\n",
 		    igt_output_name(output),
-		    kmstest_pipe_name(output->config.pipe),
+		    kmstest_pipe_name(pipe->pipe),
 		    fb_id,
 		    primary->pan_x, primary->pan_y,
 		    mode->hdisplay, mode->vdisplay);
@@ -1946,9 +1935,8 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
 				     mode);
 	} else {
 		LOG(display,
-		    "%s: SetCrtc pipe %s, disabling\n",
-		    igt_output_name(output),
-		    kmstest_pipe_name(output->config.pipe));
+		    "SetCrtc pipe %s, disabling\n",
+		    kmstest_pipe_name(pipe->pipe));
 
 		ret = drmModeSetCrtc(display->drm_fd,
 				     crtc_id,
@@ -1976,17 +1964,17 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
  * which API is used to do the programming.
  */
 static int igt_plane_commit(igt_plane_t *plane,
-			    igt_output_t *output,
+			    igt_pipe_t *pipe,
 			    enum igt_commit_style s,
 			    bool fail_on_error)
 {
 	if (plane->is_cursor && s == COMMIT_LEGACY) {
-		return igt_cursor_commit_legacy(plane, output, fail_on_error);
+		return igt_cursor_commit_legacy(plane, pipe, fail_on_error);
 	} else if (plane->is_primary && s == COMMIT_LEGACY) {
-		return igt_primary_plane_commit_legacy(plane, output,
+		return igt_primary_plane_commit_legacy(plane, pipe,
 						       fail_on_error);
 	} else {
-			return igt_drm_plane_commit(plane, output, fail_on_error);
+			return igt_drm_plane_commit(plane, pipe, fail_on_error);
 	}
 }
 
@@ -2000,31 +1988,28 @@ static int igt_plane_commit(igt_plane_t *plane,
  * further programming will take place, which may result in some changes
  * taking effect and others not taking effect.
  */
-static int igt_output_commit(igt_output_t *output,
-			     enum igt_commit_style s,
-			     bool fail_on_error)
+static int igt_pipe_commit(igt_pipe_t *pipe,
+			   enum igt_commit_style s,
+			   bool fail_on_error)
 {
-	igt_display_t *display = output->display;
-	igt_pipe_t *pipe;
+	igt_display_t *display = pipe->display;
 	int i;
 	int ret;
 	bool need_wait_for_vblank = false;
 
-	pipe = igt_output_get_driving_pipe(output);
-
 	if (pipe->background_changed) {
-		igt_crtc_set_property(output, pipe->background_property,
+		igt_crtc_set_property(pipe, pipe->background_property,
 			pipe->background);
 
 		pipe->background_changed = false;
 	}
 
 	if (pipe->color_mgmt_changed) {
-		igt_crtc_set_property(output, pipe->degamma_property,
+		igt_crtc_set_property(pipe, pipe->degamma_property,
 				      pipe->degamma_blob);
-		igt_crtc_set_property(output, pipe->ctm_property,
+		igt_crtc_set_property(pipe, pipe->ctm_property,
 				      pipe->ctm_blob);
-		igt_crtc_set_property(output, pipe->gamma_property,
+		igt_crtc_set_property(pipe, pipe->gamma_property,
 				      pipe->gamma_blob);
 
 		pipe->color_mgmt_changed = false;
@@ -2036,7 +2021,7 @@ static int igt_output_commit(igt_output_t *output,
 		if (plane->fb_changed || plane->position_changed || plane->size_changed)
 			need_wait_for_vblank = true;
 
-		ret = igt_plane_commit(plane, output, s, fail_on_error);
+		ret = igt_plane_commit(plane, pipe, s, fail_on_error);
 		CHECK_RETURN(ret, fail_on_error);
 	}
 
@@ -2060,6 +2045,9 @@ static void igt_atomic_prepare_crtc_commit(igt_output_t *output, drmModeAtomicRe
 
 	igt_pipe_t *pipe_obj = igt_output_get_driving_pipe(output);
 
+	if (!pipe_obj)
+		return;
+
 	if (pipe_obj->background_changed)
 		igt_atomic_populate_crtc_req(req, output, IGT_CRTC_BACKGROUND, pipe_obj->background);
 
@@ -2125,6 +2113,8 @@ static int igt_atomic_commit(igt_display_t *display)
 
 
 		pipe_obj = igt_output_get_driving_pipe(output);
+		if (!pipe_obj)
+			continue;
 
 		pipe = pipe_obj->pipe;
 
@@ -2167,14 +2157,14 @@ static int do_display_commit(igt_display_t *display,
 
 	}
 
-	for (i = 0; i < display->n_outputs; i++) {
-		igt_output_t *output = &display->outputs[i];
+	for_each_pipe(display, i) {
+		igt_pipe_t *pipe_obj = &display->pipes[i];
+		igt_output_t *output = igt_pipe_get_output(pipe_obj);
 
-		if (!output->valid)
-			continue;
+		if (output && output->valid)
+			valid_outs++;
 
-		valid_outs++;
-		ret = igt_output_commit(output, s, fail_on_error);
+		ret = igt_pipe_commit(pipe_obj, s, fail_on_error);
 		CHECK_RETURN(ret, fail_on_error);
 	}
 
@@ -2282,9 +2272,9 @@ void igt_output_set_pipe(igt_output_t *output, enum pipe pipe)
 {
 	igt_display_t *display = output->display;
 
-	if (pipe == PIPE_ANY) {
+	if (pipe == PIPE_NONE) {
 		LOG(display, "%s: set_pipe(any)\n", igt_output_name(output));
-		output->pending_crtc_idx_mask = -1UL;
+		output->pending_crtc_idx_mask = 0;
 	} else {
 		LOG(display, "%s: set_pipe(%s)\n", igt_output_name(output),
 		    kmstest_pipe_name(pipe));
@@ -2297,6 +2287,8 @@ igt_plane_t *igt_output_get_plane(igt_output_t *output, enum igt_plane plane)
 	igt_pipe_t *pipe;
 
 	pipe = igt_output_get_driving_pipe(output);
+	igt_assert(pipe);
+
 	return igt_pipe_get_plane(pipe, plane);
 }
 
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index dc6be5e53b74..3531dc20b6e0 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -334,17 +334,31 @@ void igt_fb_set_size(struct igt_fb *fb, igt_plane_t *plane,
 
 void igt_wait_for_vblank(int drm_fd, enum pipe pipe);
 
+static inline bool igt_output_is_connected(igt_output_t *output)
+{
+	/* Something went wrong during probe? */
+	if (!output->config.connector)
+		return false;
+
+	if (output->config.connector->connection == DRM_MODE_CONNECTED)
+		return true;
+
+	return false;
+}
+
 static inline bool igt_pipe_connector_valid(enum pipe pipe,
 					    igt_output_t *output)
 {
-	return output->valid && (output->config.valid_crtc_idx_mask & (1 << pipe));
+	return igt_output_is_connected(output) &&
+	       (output->config.valid_crtc_idx_mask & (1 << pipe));
 }
 
 #define for_each_if(condition) if (!(condition)) {} else
 
 #define for_each_connected_output(display, output)		\
 	for (int i__ = 0;  i__ < (display)->n_outputs; i__++)	\
-		for_each_if (((output = &(display)->outputs[i__]), output->valid))
+		for_each_if (((output = &(display)->outputs[i__]), \
+			      igt_output_is_connected(output)))
 
 #define for_each_pipe(display, pipe)					\
 	for (pipe = 0; pipe < igt_display_get_n_pipes(display); pipe++)	\
-- 
2.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx