Re: [PATCH i-g-t v7] lib/igt_kms: Add COMMIT_ATOMIC to igt_display_commit2()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Pratik,

I'm really looking forward to get this merged.
Just a few comments on the plane commit part below.

Cheers,

-
Lionel

On 07/03/16 10:18, Pratik Vishwakarma wrote:
From: Mayuresh Gharpure <mayuresh.s.gharpure@xxxxxxxxx>

Co-Author : Marius Vlad <marius.c.vlad@xxxxxxxxx>
Co-Author : Pratik Vishwakarma <pratik.vishwakarma@xxxxxxxxx>

So far we have had only two commit styles, COMMIT_LEGACY
and COMMIT_UNIVERSAL. This patch adds another commit style
COMMIT_ATOMIC which makes use of drmModeAtomicCommit()

v2: (Marius)
	i)Set CRTC_ID to zero while disabling plane
	ii)Modified the log message in igt_atomic_prepare_plane_commit
	https://patchwork.freedesktop.org/patch/71945/

v3: (Marius)Set FB_ID to zero while disabling plane
	https://patchwork.freedesktop.org/patch/72179/

v4: (Maarten) Corrected the typo in commit message
	https://patchwork.freedesktop.org/patch/72598/

v5: Added check for DRM_CLIENT_CAP_ATOMIC in igt_display_init
     (Marius)
	i)Removed unused props from igt_display_init
	ii)Removed unused ret. Changed function to void
	iii)Declare the variable before checking if we have
	DRM_CLIENT_CAP_ATOMIC.
	https://patchwork.freedesktop.org/patch/73505/

v6: (Jani) Corrected typo in commit message
	https://patchwork.freedesktop.org/patch/74468/

v7: (Vlad & Maarten) Added is_atomic check for DRM_CLIENT_CAP_ATOMIC
	in igt_display_init and igt_atomic_commit

Signed-off-by: Mayuresh Gharpure <mayuresh.s.gharpure@xxxxxxxxx>
Signed-off-by: Pratik Vishwakarma <pratik.vishwakarma@xxxxxxxxx>
---
  lib/igt_kms.c | 321 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
  lib/igt_kms.h |  72 ++++++++++++-
  2 files changed, 391 insertions(+), 2 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 5a61def..1f738e1 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -145,6 +145,120 @@ const unsigned char* igt_kms_get_base_edid(void)
   *
   * Returns: an alternate edid block
   */
+static const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
+	"SRC_X",
+	"SRC_Y",
+	"SRC_W",
+	"SRC_H",
+	"CRTC_X",
+	"CRTC_Y",
+	"CRTC_W",
+	"CRTC_H",
+	"FB_ID",
+	"CRTC_ID",
+	"type",
+	"rotation"
+};
+
+static const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
+	"background_color"
+};
+
+static const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
+	"scaling mode",
+	"DPMS"
+};
+
+/*
+ * Retrieve all the properies specified in props_name and store them into
+ * plane->atomic_props_plane.
+ */
+static void
+igt_atomic_fill_plane_props(igt_display_t *display, igt_plane_t *plane,
+			int num_props, const char **prop_names)
+{
+	drmModeObjectPropertiesPtr props;
+	int i, j, fd;
+
+	fd = display->drm_fd;
+
+	props = drmModeObjectGetProperties(fd, plane->drm_plane->plane_id, DRM_MODE_OBJECT_PLANE);
+	igt_assert(props);
+
+	for (i = 0; i < props->count_props; i++) {
+		drmModePropertyPtr prop =
+			drmModeGetProperty(fd, props->props[i]);
+
+		for (j = 0; j < num_props; j++) {
+			if (strcmp(prop->name, prop_names[j]) != 0)
+				continue;
+
+			plane->atomic_props_plane[j] = props->props[i];
+			break;
+		}
+
+		drmModeFreeProperty(prop);
+	}
+
+	drmModeFreeObjectProperties(props);
+}
+
+/*
+ * Retrieve all the properies specified in props_name and store them into
+ * config->atomic_props_crtc and config->atomic_props_connector.
+ */
+static void
+igt_atomic_fill_props(igt_display_t *display, igt_output_t *output,
+			int num_crtc_props, const char **crtc_prop_names,
+			int num_connector_props, const char **conn_prop_names)
+{
+	drmModeObjectPropertiesPtr props;
+	int i, j, fd;
+
+	fd = display->drm_fd;
+
+	props = drmModeObjectGetProperties(fd, output->config.crtc->crtc_id, DRM_MODE_OBJECT_CRTC);
+	igt_assert(props);
+
+	for (i = 0; i < props->count_props; i++) {
+		drmModePropertyPtr prop =
+			drmModeGetProperty(fd, props->props[i]);
+
+		for (j = 0; j < num_crtc_props; j++) {
+			if (strcmp(prop->name, crtc_prop_names[j]) != 0)
+				continue;
+
+			output->config.atomic_props_crtc[j] = props->props[i];
+			break;
+		}
+
+		drmModeFreeProperty(prop);
+	}
+
+	drmModeFreeObjectProperties(props);
+	props = NULL;
+	props = drmModeObjectGetProperties(fd, output->config.connector->connector_id, DRM_MODE_OBJECT_CONNECTOR);
+	igt_assert(props);
+
+	for (i = 0; i < props->count_props; i++) {
+		drmModePropertyPtr prop =
+			drmModeGetProperty(fd, props->props[i]);
+
+		for (j = 0; j < num_connector_props; j++) {
+			if (strcmp(prop->name, conn_prop_names[j]) != 0)
+				continue;
+
+			output->config.atomic_props_connector[j] = props->props[i];
+			break;
+		}
+
+		drmModeFreeProperty(prop);
+	}
+
+	drmModeFreeObjectProperties(props);
+
+}
+
  const unsigned char* igt_kms_get_alt_edid(void)
  {
  	update_edid_csum(alt_edid);
@@ -1002,6 +1116,8 @@ static void igt_output_refresh(igt_output_t *output)
  	    kmstest_pipe_name(output->config.pipe));
display->pipes_in_use |= 1 << output->config.pipe;
+	igt_atomic_fill_props(display, output, IGT_NUM_CRTC_PROPS, igt_crtc_prop_names,
+		IGT_NUM_CONNECTOR_PROPS, igt_connector_prop_names);
  }
static bool
@@ -1071,6 +1187,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
  	drmModeRes *resources;
  	drmModePlaneRes *plane_resources;
  	int i;
+	int ret = 0;
memset(display, 0, sizeof(igt_display_t)); @@ -1088,6 +1205,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
  	display->n_pipes = resources->count_crtcs;
drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
+	ret = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1);
  	plane_resources = drmModeGetPlaneResources(display->drm_fd);
  	igt_assert(plane_resources);
@@ -1144,6 +1262,10 @@ void igt_display_init(igt_display_t *display, int drm_fd) plane->pipe = pipe;
  			plane->drm_plane = drm_plane;
+			if (ret == 0) {
+				display->is_atomic = true;
+				igt_atomic_fill_plane_props(display, plane, IGT_NUM_PLANE_PROPS, igt_plane_prop_names);
+			}
get_plane_property(display->drm_fd, drm_plane->plane_id,
  					   "rotation",
@@ -1399,6 +1521,111 @@ static uint32_t igt_plane_get_fb_gem_handle(igt_plane_t *plane)
  	igt_assert(r == 0);	\
  }
+
+
+
+/*
+ * Add position and fb changes of a plane to the atomic property set
+ */
+static void
+igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_output_t *output,
+	drmModeAtomicReq *req)
+{
+
+	igt_display_t *display = output->display;
+	uint32_t fb_id, crtc_id;
+	uint32_t src_x;
+	uint32_t src_y;
+	uint32_t src_w;
+	uint32_t src_h;
+	int32_t crtc_x;
+	int32_t crtc_y;
+	uint32_t crtc_w;
+	uint32_t crtc_h;
+
+	igt_assert(plane->drm_plane);
+
+	/* it's an error to try an unsupported feature */
+	igt_assert(igt_plane_supports_rotation(plane) ||
+			!plane->rotation_changed);
+
+	fb_id = igt_plane_get_fb_id(plane);
+	crtc_id = output->config.crtc->crtc_id;
+
+	if ((plane->fb_changed || plane->size_changed) && fb_id == 0) {
+
+		LOG(display,
+		    "%s: populating plane data: pipe %s, plane %d, disabling\n",
+		     igt_output_name(output),
+		     kmstest_pipe_name(output->config.pipe),
+		     plane->index);
+
+		/* populate plane req */
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID, 0);
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_FB_ID, 0);
+
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_X, IGT_FIXED(0, 0));
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_Y, IGT_FIXED(0, 0));
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_W, IGT_FIXED(0, 0));
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_H, IGT_FIXED(0, 0));
+
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_X, 0);
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_Y, 0);
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_W, 0);
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_H, 0);

Why resetting all these properties? It seems unnecessary.
If there is a reason for this, you might want to synchronize the fields of igt_plane_t.

+
+	} else if (plane->fb_changed || plane->position_changed ||
+			plane->size_changed) {
+
+		src_x = IGT_FIXED(plane->fb->src_x, 0); /* src_x */
+		src_y = IGT_FIXED(plane->fb->src_y, 0); /* src_y */
+		src_w = IGT_FIXED(plane->fb->src_w, 0); /* src_w */
+		src_h = IGT_FIXED(plane->fb->src_h, 0); /* src_h */
+		crtc_x = plane->crtc_x;
+		crtc_y = plane->crtc_y;
+		crtc_w = plane->crtc_w;
+		crtc_h = plane->crtc_h;
+
+		LOG(display,
+		    "%s: populating plane data: %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),
+		    plane->index,
+		    fb_id,
+		    src_x >> 16, src_y >> 16, src_w >> 16, src_h >> 16,
+		    crtc_x, crtc_y, crtc_w, crtc_h);
+
+
+		/* populate plane req */
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID, crtc_id);
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_FB_ID, fb_id);
+
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_X, src_x);
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_Y, src_y);
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_W, src_w);
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_H, src_h);
+
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_X, crtc_x);
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_Y, crtc_y);
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_W, crtc_w);
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_H, crtc_h);
+
+		if (plane->rotation_changed)
+			igt_atomic_populate_plane_req(req, plane,
+				IGT_PLANE_ROTATION, plane->rotation);
What happens if only the rotation has changed?
This condition should be outside the if (plane->fb_changed || plane->position_changed || plane->size_changed).

+
+	}
+
+	plane->fb_changed = false;
+	plane->position_changed = false;
+	plane->size_changed = false;

Overall I think this function would be a lot clearer if you just had a test for each of the *_changed properties and just submitted the associated properties in each case.
+
+	return;
+}
+
+
+
  /*
   * Commit position and fb changes to a DRM plane via the SetPlane ioctl; if the
   * DRM call to program the plane fails, we'll either fail immediately (for
@@ -1651,7 +1878,7 @@ static int igt_plane_commit(igt_plane_t *plane,
  		return igt_primary_plane_commit_legacy(plane, output,
  						       fail_on_error);
  	} else {
-		return igt_drm_plane_commit(plane, output, fail_on_error);
+			return igt_drm_plane_commit(plane, output, fail_on_error);
  	}
  }
@@ -1705,6 +1932,90 @@ static int igt_output_commit(igt_output_t *output,
  	return 0;
  }
+
+/*
+ * Add crtc property changes to the atomic property set
+ */
+static void igt_atomic_prepare_crtc_commit(igt_output_t *output, drmModeAtomicReq *req)
+{
+
+	igt_pipe_t *pipe_obj = igt_output_get_driving_pipe(output);
+
+	if (pipe_obj->background_changed)
+		igt_atomic_populate_crtc_req(req, output, IGT_CRTC_BACKGROUND, pipe_obj->background);
+
+	/*
+	 *	TODO: Add all crtc level properties here
+	 */
+
+}
+
+/*
+ * Add connector property changes to the atomic property set
+ */
+static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
+{
+
+	struct kmstest_connector_config *config = &output->config;
+
+	if (config->connector_scaling_mode_changed)
+		igt_atomic_populate_connector_req(req, output, IGT_CONNECTOR_SCALING_MODE, config->connector_scaling_mode);
+
+	if (config->connector_dpms_changed)
+		igt_atomic_populate_connector_req(req, output, IGT_CONNECTOR_DPMS, config->connector_dpms);
+	/*
+	 *	TODO: Add all other connector level properties here
+	 */
+
+}
+
+/*
+ * Commit all the changes of all the planes,crtcs, connectors
+ * atomically using drmModeAtomicCommit()
+ */
+static int igt_atomic_commit(igt_display_t *display)
+{
+
+	int ret = 0;
+	int i = 0;
+	drmModeAtomicReq *req;
+	if (display->is_atomic != true)
+		return -EINVAL;
+	req = drmModeAtomicAlloc();
+	drmModeAtomicSetCursor(req, 0);
+
+	for (i = 0; i < display->n_outputs; i++) {
+		igt_output_t *output = &display->outputs[i];
+		igt_pipe_t *pipe;
+
+		if (!output->valid)
+			continue;
+
+		/*
+		 * Add CRTC Properties to the property set
+		 */
+		igt_atomic_prepare_crtc_commit(output, req);
+
+		/*
+		 * Add Connector Properties to the property set
+		 */
+		igt_atomic_prepare_connector_commit(output, req);
+
+
+		pipe = igt_output_get_driving_pipe(output);
+
+		for (i = 0; i < pipe->n_planes; i++) {
+			igt_plane_t *plane = &pipe->planes[i];
+			igt_atomic_prepare_plane_commit(plane, output, req);
+		}
+
+	}
+
+	ret = drmModeAtomicCommit(display->drm_fd, req, 0, NULL);
+	drmModeAtomicFree(req);
+	return ret;
+
+}
  /*
   * Commit all plane changes across all outputs of the display.
   *
@@ -1724,6 +2035,14 @@ static int do_display_commit(igt_display_t *display,
igt_display_refresh(display); + if (s == COMMIT_ATOMIC) {
+
+		ret = igt_atomic_commit(display);
+		CHECK_RETURN(ret, fail_on_error);
+		return 0;
+
+	}
+
  	for (i = 0; i < display->n_outputs; i++) {
  		igt_output_t *output = &display->outputs[i];
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 5744ed0..d6e2f12 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -106,11 +106,28 @@ int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id);
  void kmstest_set_vt_graphics_mode(void);
  void kmstest_restore_vt_mode(void);
+enum igt_atomic_crtc_properties {
+       IGT_CRTC_BACKGROUND = 0,
+       IGT_NUM_CRTC_PROPS
+};
+
+enum igt_atomic_connector_properties {
+       IGT_CONNECTOR_SCALING_MODE = 0,
+       IGT_CONNECTOR_DPMS,
+       IGT_NUM_CONNECTOR_PROPS
+};
+
  struct kmstest_connector_config {
  	drmModeCrtc *crtc;
  	drmModeConnector *connector;
  	drmModeEncoder *encoder;
  	drmModeModeInfo default_mode;
+	uint64_t connector_scaling_mode;
+	bool connector_scaling_mode_changed;
+	uint64_t connector_dpms;
+	bool connector_dpms_changed;
+	uint32_t atomic_props_crtc[IGT_NUM_CRTC_PROPS];
+	uint32_t atomic_props_connector[IGT_NUM_CONNECTOR_PROPS];
  	int crtc_idx;
  	int pipe;
  };
@@ -163,9 +180,28 @@ uint32_t kmstest_find_crtc_for_connector(int fd, drmModeRes *res,
  enum igt_commit_style {
  	COMMIT_LEGACY = 0,
  	COMMIT_UNIVERSAL,
-	/* We'll add atomic here eventually. */
+	COMMIT_ATOMIC,
  };
+enum igt_atomic_plane_properties {
+       IGT_PLANE_SRC_X = 0,
+       IGT_PLANE_SRC_Y,
+       IGT_PLANE_SRC_W,
+       IGT_PLANE_SRC_H,
+
+       IGT_PLANE_CRTC_X,
+       IGT_PLANE_CRTC_Y,
+       IGT_PLANE_CRTC_W,
+       IGT_PLANE_CRTC_H,
+
+       IGT_PLANE_FB_ID,
+       IGT_PLANE_CRTC_ID,
+       IGT_PLANE_TYPE,
+       IGT_PLANE_ROTATION,
+       IGT_NUM_PLANE_PROPS
+};
+
+
  typedef struct igt_display igt_display_t;
  typedef struct igt_pipe igt_pipe_t;
  typedef uint32_t igt_fixed_t;			/* 16.16 fixed point */
@@ -207,6 +243,7 @@ typedef struct {
  	/* panning offset within the fb */
  	unsigned int pan_x, pan_y;
  	igt_rotation_t rotation;
+	uint32_t atomic_props_plane[IGT_NUM_PLANE_PROPS];
  } igt_plane_t;
struct igt_pipe {
@@ -242,6 +279,7 @@ struct igt_display {
  	igt_output_t *outputs;
  	igt_pipe_t pipes[I915_MAX_PIPES];
  	bool has_universal_planes;
+	bool is_atomic;
  };
void igt_display_init(igt_display_t *display, int drm_fd);
@@ -288,6 +326,38 @@ void igt_wait_for_vblank(int drm_fd, enum pipe pipe);
#define IGT_FIXED(i,f) ((i) << 16 | (f)) +/**
+ * igt_atomic_populate_plane_req:
+ * @req: A pointer to drmModeAtomicReq
+ * @plane: A pointer igt_plane_t
+ * @prop: one of igt_atomic_plane_properties
+ * @value: the value to add
+ */
+#define igt_atomic_populate_plane_req(req, plane, prop, value) \
+	igt_assert_lt(0, drmModeAtomicAddProperty(req, plane->drm_plane->plane_id,\
+						  plane->atomic_props_plane[prop], value))
+
+/**
+ * igt_atomic_populate_crtc_req:
+ * @req: A pointer to drmModeAtomicReq
+ * @output: A pointer igt_output_t
+ * @prop: one of igt_atomic_crtc_properties
+ * @value: the value to add
+ */
+#define igt_atomic_populate_crtc_req(req, output, prop, value) \
+	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.crtc->crtc_id,\
+						  output->config.atomic_props_crtc[prop], value))
+/**
+ * igt_atomic_populate_connector_req:
+ * @req: A pointer to drmModeAtomicReq
+ * @output: A pointer igt_output_t
+ * @prop: one of igt_atomic_connector_properties
+ * @value: the value to add
+ */
+#define igt_atomic_populate_connector_req(req, output, prop, value) \
+	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,\
+						  output->config.atomic_props_connector[prop], value))
+
  void igt_enable_connectors(void);
  void igt_reset_connectors(void);

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux