HDMI stereo support v4

HDMI stereo support v4

[Damien Lespiau]

Next installment of the HDMI stereo 3D series, following:
  http://lists.freedesktop.org/archives/dri-devel/2013-September/044885.html

Quite a few changes here, and it's starting to "feel right":
  - We now expose each stereo layout as a separate mode, distinct from the 2D
    mode, instead of piggybacking on the 2D mode flags.
  - 2 bugs fixed:
    - We also expose 3D layouts of modes with alternate clocks
    - The VIC field in the AVI infoframe is now correctly set to the
      corresponding 2D mode
  - We forbid the use of frambuffers with 2 buffers to not set any precedent
    before we can explore the interactions of such framebuffers with the rest
    of the DRM API
  - Rename SET_CAP to SET_CLIENT_CAP

[PATCH 01/12] drm: Move the GET_CAP macros next to the corresponding ioctl structure

[Damien Lespiau]

It's a tiny bit more logical to find the different capabilities you can
use with the GET_CAP ioctl next to the structure rather than putting
them at the end of the file.

v2: Tab align the litterals (David Herrmann)
v3: Make it clearer that DRM_PRIME_CAP_EXPORT/IMPORT are flags of
    DRM_CAP_PRIME.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com> (for v2)
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 include/uapi/drm/drm.h | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 272580c..3ad0640 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -611,6 +611,15 @@ struct drm_gem_open {
 	__u64 size;
 };
 
+#define DRM_CAP_DUMB_BUFFER		0x1
+#define DRM_CAP_VBLANK_HIGH_CRTC	0x2
+#define DRM_CAP_DUMB_PREFERRED_DEPTH	0x3
+#define DRM_CAP_DUMB_PREFER_SHADOW	0x4
+#define DRM_CAP_PRIME			0x5
+#define  DRM_PRIME_CAP_IMPORT		0x1
+#define  DRM_PRIME_CAP_EXPORT		0x2
+#define DRM_CAP_TIMESTAMP_MONOTONIC	0x6
+
 /** DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
 	__u64 capability;
@@ -774,16 +783,6 @@ struct drm_event_vblank {
 	__u32 reserved;
 };
 
-#define DRM_CAP_DUMB_BUFFER 0x1
-#define DRM_CAP_VBLANK_HIGH_CRTC 0x2
-#define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
-#define DRM_CAP_DUMB_PREFER_SHADOW 0x4
-#define DRM_CAP_PRIME 0x5
-#define DRM_CAP_TIMESTAMP_MONOTONIC 0x6
-
-#define DRM_PRIME_CAP_IMPORT 0x1
-#define DRM_PRIME_CAP_EXPORT 0x2
-
 /* typedef area */
 #ifndef __KERNEL__
 typedef struct drm_clip_rect drm_clip_rect_t;
-- 
1.8.3.1

[PATCH 02/12] drm: Add a SET_CLIENT_CAP ioctl

[Damien Lespiau]

This ioctl can be used to turn some knobs in a DRM driver. The client
can ask the DRM core for an alternate view of the reality: it can be
useful to be able to instruct the core that the DRM client can handle
new functionnality that would otherwise break current ABI.

v2: Rename to ioctl from SET_CAP to SET_CLIENT_CAP (Chris Wilson)

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/drm_drv.c   | 1 +
 drivers/gpu/drm/drm_ioctl.c | 9 +++++++++
 include/drm/drmP.h          | 2 ++
 include/uapi/drm/drm.h      | 7 +++++++
 4 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 288da3d..9c67c57 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -69,6 +69,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_CLIENT, drm_getclient, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_STATS, drm_getstats, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_CAP, drm_getcap, DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_MASTER),
 
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_setunique, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index cffc7c0..29f660e 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -300,6 +300,15 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 }
 
 /**
+ * Set device/driver capabilities
+ */
+int
+drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
+{
+	return -EINVAL;
+}
+
+/**
  * Setversion ioctl.
  *
  * \param inode device inode.
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 39911dc..36c43ce 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1273,6 +1273,8 @@ extern int drm_getstats(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
 extern int drm_getcap(struct drm_device *dev, void *data,
 		      struct drm_file *file_priv);
+extern int drm_setclientcap(struct drm_device *dev, void *data,
+			    struct drm_file *file_priv);
 extern int drm_setversion(struct drm_device *dev, void *data,
 			  struct drm_file *file_priv);
 extern int drm_noop(struct drm_device *dev, void *data,
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 3ad0640..9564c55 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -626,6 +626,12 @@ struct drm_get_cap {
 	__u64 value;
 };
 
+/** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
+struct drm_set_client_cap {
+	__u64 capability;
+	__u64 value;
+};
+
 #define DRM_CLOEXEC O_CLOEXEC
 struct drm_prime_handle {
 	__u32 handle;
@@ -658,6 +664,7 @@ struct drm_prime_handle {
 #define DRM_IOCTL_GEM_FLINK		DRM_IOWR(0x0a, struct drm_gem_flink)
 #define DRM_IOCTL_GEM_OPEN		DRM_IOWR(0x0b, struct drm_gem_open)
 #define DRM_IOCTL_GET_CAP		DRM_IOWR(0x0c, struct drm_get_cap)
+#define DRM_IOCTL_SET_CLIENT_CAP	DRM_IOW( 0x0d, struct drm_set_client_cap)
 
 #define DRM_IOCTL_SET_UNIQUE		DRM_IOW( 0x10, struct drm_unique)
 #define DRM_IOCTL_AUTH_MAGIC		DRM_IOW( 0x11, struct drm_auth)
-- 
1.8.3.1

[PATCH 03/12] drm: Add HDMI stereo 3D flags to struct drm_mode_modeinfo

[Damien Lespiau]

HDMI 1.4a defines a few layouts that we'd like to expose. This commits
add new modeinfo flags that can be used to list the supported stereo
layouts (when querying the list of modes) and to set a given stereo 3D
mode (when setting a mode).

v2: Add a drm_mode_is_stereo() helper

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 include/drm/drm_crtc.h      | 14 ++++++++++++++
 include/uapi/drm/drm_mode.h | 36 ++++++++++++++++++++++--------------
 2 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f660ac5..bf242ac 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -179,6 +179,20 @@ struct drm_display_mode {
 	int hsync;		/* in kHz */
 };
 
+#define DRM_MODE_FLAG_3D_MASK	(DRM_MODE_FLAG_3D_FRAME_PACKING		| \
+				 DRM_MODE_FLAG_3D_FIELD_ALTERNATIVE	| \
+				 DRM_MODE_FLAG_3D_LINE_ALTERNATIVE	| \
+				 DRM_MODE_FLAG_3D_SIDE_BY_SIDE_FULL	| \
+				 DRM_MODE_FLAG_3D_L_DEPTH		| \
+				 DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH | \
+				 DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	| \
+				 DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF)
+
+static inline bool drm_mode_is_stereo(const struct drm_display_mode *mode)
+{
+	return mode->flags & DRM_MODE_FLAG_3D_MASK;
+}
+
 enum drm_connector_status {
 	connector_status_connected = 1,
 	connector_status_disconnected = 2,
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 53db7ce..045046f 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -44,20 +44,28 @@
 
 /* Video mode flags */
 /* bit compatible with the xorg definitions. */
-#define DRM_MODE_FLAG_PHSYNC	(1<<0)
-#define DRM_MODE_FLAG_NHSYNC	(1<<1)
-#define DRM_MODE_FLAG_PVSYNC	(1<<2)
-#define DRM_MODE_FLAG_NVSYNC	(1<<3)
-#define DRM_MODE_FLAG_INTERLACE	(1<<4)
-#define DRM_MODE_FLAG_DBLSCAN	(1<<5)
-#define DRM_MODE_FLAG_CSYNC	(1<<6)
-#define DRM_MODE_FLAG_PCSYNC	(1<<7)
-#define DRM_MODE_FLAG_NCSYNC	(1<<8)
-#define DRM_MODE_FLAG_HSKEW	(1<<9) /* hskew provided */
-#define DRM_MODE_FLAG_BCAST	(1<<10)
-#define DRM_MODE_FLAG_PIXMUX	(1<<11)
-#define DRM_MODE_FLAG_DBLCLK	(1<<12)
-#define DRM_MODE_FLAG_CLKDIV2	(1<<13)
+#define DRM_MODE_FLAG_PHSYNC			(1<<0)
+#define DRM_MODE_FLAG_NHSYNC			(1<<1)
+#define DRM_MODE_FLAG_PVSYNC			(1<<2)
+#define DRM_MODE_FLAG_NVSYNC			(1<<3)
+#define DRM_MODE_FLAG_INTERLACE			(1<<4)
+#define DRM_MODE_FLAG_DBLSCAN			(1<<5)
+#define DRM_MODE_FLAG_CSYNC			(1<<6)
+#define DRM_MODE_FLAG_PCSYNC			(1<<7)
+#define DRM_MODE_FLAG_NCSYNC			(1<<8)
+#define DRM_MODE_FLAG_HSKEW			(1<<9) /* hskew provided */
+#define DRM_MODE_FLAG_BCAST			(1<<10)
+#define DRM_MODE_FLAG_PIXMUX			(1<<11)
+#define DRM_MODE_FLAG_DBLCLK			(1<<12)
+#define DRM_MODE_FLAG_CLKDIV2			(1<<13)
+#define DRM_MODE_FLAG_3D_FRAME_PACKING		(1<<14)
+#define DRM_MODE_FLAG_3D_FIELD_ALTERNATIVE	(1<<15)
+#define DRM_MODE_FLAG_3D_LINE_ALTERNATIVE	(1<<16)
+#define DRM_MODE_FLAG_3D_SIDE_BY_SIDE_FULL	(1<<17)
+#define DRM_MODE_FLAG_3D_L_DEPTH		(1<<18)
+#define DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(1<<19)
+#define DRM_MODE_FLAG_3D_TOP_AND_BOTTOM		(1<<20)
+#define DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(1<<21)
 
 /* DPMS flags */
 /* bit compatible with the xorg definitions. */
-- 
1.8.3.1

[PATCH 04/12] drm: Add a STEREO_3D capability to the SET_CLIENT_CAP ioctl

[Damien Lespiau]

This capability allows user space to control the delivery of modes with
the 3D flags set. This is to not play games with current user space
users not knowing anything about stereo 3D flags and that could try
to set a mode with one or several of those bits set.

So, the plan is to remove the stereo modes from the list of modes we
give to DRM clients by default, and let them through if we are being
told otherwise.

stereo_allowed is bound to the drm_file structure to make it a
per-client setting, not a global one.

v2: Replace clearing 3D flags by discarding the stereo modes now that
    they are regular modes.
v3: SET_CAP -> SET_CLIENT_CAP rename (Chris Wilson)

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/drm_crtc.c  | 19 ++++++++++++++++++-
 drivers/gpu/drm/drm_ioctl.c | 14 +++++++++++++-
 include/drm/drmP.h          |  3 +++
 include/uapi/drm/drm.h      |  9 +++++++++
 4 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a691764..8cbb119 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1568,6 +1568,19 @@ out:
 	return ret;
 }
 
+static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
+					 const struct drm_file *file_priv)
+{
+	/*
+	 * If user-space hasn't configured the driver to expose the stereo 3D
+	 * modes, don't expose them.
+	 */
+	if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode))
+		return false;
+
+	return true;
+}
+
 /**
  * drm_mode_getconnector - get connector configuration
  * @dev: drm device for the ioctl
@@ -1633,7 +1646,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 
 	/* delayed so we get modes regardless of pre-fill_modes state */
 	list_for_each_entry(mode, &connector->modes, head)
-		mode_count++;
+		if (drm_mode_expose_to_userspace(mode, file_priv))
+			mode_count++;
 
 	out_resp->connector_id = connector->base.id;
 	out_resp->connector_type = connector->connector_type;
@@ -1655,6 +1669,9 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 		copied = 0;
 		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
 		list_for_each_entry(mode, &connector->modes, head) {
+			if (!drm_mode_expose_to_userspace(mode, file_priv))
+				continue;
+
 			drm_crtc_convert_to_umode(&u_mode, mode);
 			if (copy_to_user(mode_ptr + copied,
 					 &u_mode, sizeof(u_mode))) {
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 29f660e..8e012a2 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -305,7 +305,19 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 int
 drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 {
-	return -EINVAL;
+	struct drm_set_client_cap *req = data;
+
+	switch (req->capability) {
+	case DRM_CLIENT_CAP_STEREO_3D:
+		if (req->value > 1)
+			return -EINVAL;
+		file_priv->stereo_allowed = req->value;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 /**
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 36c43ce..ad42aea 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -431,6 +431,9 @@ struct drm_file {
 	struct drm_master *master; /* master this node is currently associated with
 				      N.B. not always minor->master */
 
+	/* true when the client has asked us to expose stereo 3D mode flags */
+	bool stereo_allowed;
+
 	/**
 	 * fbs - List of framebuffers associated with this file.
 	 *
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 9564c55..afac66c 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -626,6 +626,15 @@ struct drm_get_cap {
 	__u64 value;
 };
 
+/**
+ * DRM_CLIENT_CAP_STEREO_3D
+ *
+ * if set to 1, the DRM core will expose the stereo 3D capabilities of the
+ * monitor by advertising the supported 3D layouts in the flags of struct
+ * drm_mode_modeinfo.
+ */
+#define DRM_CLIENT_CAP_STEREO_3D	1
+
 /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
 struct drm_set_client_cap {
 	__u64 capability;
-- 
1.8.3.1

[PATCH 05/12] drm/edid: Expose mandatory stereo modes for HDMI sinks

[Damien Lespiau]

For now, let's just look at the 3D_present flag of the CEA HDMI vendor
block to detect if the sink supports a small list of then mandatory 3D
formats.

See the HDMI 1.4a 3D extraction for detail:
  http://www.hdmi.org/manufacturer/specification.aspx

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 108 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 101 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a207cc3..78009d1 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2550,13 +2550,93 @@ do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
 	return modes;
 }
 
+struct stereo_mandatory_mode {
+	int width, height, freq;
+	unsigned int interlace_flag, layouts;
+};
+
+static const struct stereo_mandatory_mode stereo_mandatory_modes[] = {
+	{ 1920, 1080, 24, 0,
+	  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING },
+	{ 1920, 1080, 50, DRM_MODE_FLAG_INTERLACE,
+	  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF },
+	{ 1920, 1080, 60, DRM_MODE_FLAG_INTERLACE,
+	  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF },
+	{ 1280, 720,  50, 0,
+	  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING },
+	{ 1280, 720,  60, 0,
+	  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING }
+};
+
+static bool
+stereo_match_mandatory(const struct drm_display_mode *mode,
+		       const struct stereo_mandatory_mode *stereo_mode)
+{
+	unsigned int interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
+
+	return mode->hdisplay == stereo_mode->width &&
+	       mode->vdisplay == stereo_mode->height &&
+	       interlaced == stereo_mode->interlace_flag &&
+	       drm_mode_vrefresh(mode) == stereo_mode->freq;
+}
+
+static const struct stereo_mandatory_mode *
+hdmi_find_stereo_mandatory_mode(struct drm_display_mode *mode)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(stereo_mandatory_modes); i++)
+		if (stereo_match_mandatory(mode, &stereo_mandatory_modes[i]))
+			return &stereo_mandatory_modes[i];
+
+	return NULL;
+}
+
+static int add_hdmi_mandatory_stereo_modes(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_display_mode *mode, *new_mode;
+	struct list_head stereo_modes;
+	int modes = 0;
+
+	INIT_LIST_HEAD(&stereo_modes);
+
+	list_for_each_entry(mode, &connector->probed_modes, head) {
+		const struct stereo_mandatory_mode *mandatory;
+		u32 stereo_layouts, layout;
+
+		mandatory = hdmi_find_stereo_mandatory_mode(mode);
+		if (!mandatory)
+			continue;
+
+		stereo_layouts = mandatory->layouts;
+                do {
+                        layout = 1 << (ffs(stereo_layouts) - 1);
+			stereo_layouts &= ~layout;
+
+			new_mode = drm_mode_duplicate(dev, mode);
+			if (!new_mode)
+				continue;
+
+			new_mode->flags |= layout;
+			list_add_tail(&new_mode->head, &stereo_modes);
+			modes++;
+		} while (stereo_layouts);
+	}
+
+	list_splice_tail(&stereo_modes, &connector->probed_modes);
+
+	return modes;
+}
+
 /*
  * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
  * @connector: connector corresponding to the HDMI sink
  * @db: start of the CEA vendor specific block
  * @len: length of the CEA block payload, ie. one can access up to db[len]
  *
- * Parses the HDMI VSDB looking for modes to add to @connector.
+ * Parses the HDMI VSDB looking for modes to add to @connector. This function
+ * also adds the stereo 3d modes when applicable.
  */
 static int
 do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
@@ -2582,10 +2662,15 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
 
 	/* the declared length is not long enough for the 2 first bytes
 	 * of additional video format capabilities */
-	offset += 2;
-	if (len < (8 + offset))
+	if (len < (8 + offset + 2))
 		goto out;
 
+	/* 3D_Present */
+	offset++;
+	if (db[8 + offset] & (1 << 7))
+		modes += add_hdmi_mandatory_stereo_modes(connector);
+
+	offset++;
 	vic_len = db[8 + offset] >> 5;
 
 	for (i = 0; i < vic_len && len >= (9 + offset + i); i++) {
@@ -2665,8 +2750,8 @@ static int
 add_cea_modes(struct drm_connector *connector, struct edid *edid)
 {
 	const u8 *cea = drm_find_cea_extension(edid);
-	const u8 *db;
-	u8 dbl;
+	const u8 *db, *hdmi = NULL;
+	u8 dbl, hdmi_len;
 	int modes = 0;
 
 	if (cea && cea_revision(cea) >= 3) {
@@ -2681,11 +2766,20 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid)
 
 			if (cea_db_tag(db) == VIDEO_BLOCK)
 				modes += do_cea_modes(connector, db + 1, dbl);
-			else if (cea_db_is_hdmi_vsdb(db))
-				modes += do_hdmi_vsdb_modes(connector, db, dbl);
+			else if (cea_db_is_hdmi_vsdb(db)) {
+				hdmi = db;
+				hdmi_len = dbl;
+			}
 		}
 	}
 
+	/*
+	 * We parse the HDMI VSDB after having added the cea modes as we will
+	 * be patching their flags when the sink supports stereo 3D.
+	 */
+	if (hdmi)
+		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len);
+
 	return modes;
 }
 
-- 
1.8.3.1

[PATCH 06/12] drm: Extract add_hdmi_mode() out of do_hdmi_vsdb_modes()

[Damien Lespiau]

So we respect a nice design of having similar functions at the same
level, in this case:

do_hdmi_vsdb_modes()
  - add_hdmi_mandatory_stereo_modes()
  - add_hdmi_mode()

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 78009d1..e016a5d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2629,6 +2629,26 @@ static int add_hdmi_mandatory_stereo_modes(struct drm_connector *connector)
 	return modes;
 }
 
+static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_display_mode *newmode;
+
+	vic--; /* VICs start at 1 */
+	if (vic >= ARRAY_SIZE(edid_4k_modes)) {
+		DRM_ERROR("Unknown HDMI VIC: %d\n", vic);
+		return 0;
+	}
+
+	newmode = drm_mode_duplicate(dev, &edid_4k_modes[vic]);
+	if (!newmode)
+		return 0;
+
+	drm_mode_probed_add(connector, newmode);
+
+	return 1;
+}
+
 /*
  * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
  * @connector: connector corresponding to the HDMI sink
@@ -2641,7 +2661,6 @@ static int add_hdmi_mandatory_stereo_modes(struct drm_connector *connector)
 static int
 do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
 {
-	struct drm_device *dev = connector->dev;
 	int modes = 0, offset = 0, i;
 	u8 vic_len;
 
@@ -2674,23 +2693,10 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
 	vic_len = db[8 + offset] >> 5;
 
 	for (i = 0; i < vic_len && len >= (9 + offset + i); i++) {
-		struct drm_display_mode *newmode;
 		u8 vic;
 
 		vic = db[9 + offset + i];
-
-		vic--; /* VICs start at 1 */
-		if (vic >= ARRAY_SIZE(edid_4k_modes)) {
-			DRM_ERROR("Unknown HDMI VIC: %d\n", vic);
-			continue;
-		}
-
-		newmode = drm_mode_duplicate(dev, &edid_4k_modes[vic]);
-		if (!newmode)
-			continue;
-
-		drm_mode_probed_add(connector, newmode);
-		modes++;
+		modes += add_hdmi_mode(connector, vic);
 	}
 
 out:
-- 
1.8.3.1

[PATCH 07/12] drm: Reject modes with more than 1 stereo flags set

[Damien Lespiau]

When setting a stereo 3D mode, there can be only one bit set describing
the layout of the frambuffer(s). So reject invalid modes early.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 8cbb119..39f60ec 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1306,6 +1306,10 @@ static int drm_crtc_convert_umode(struct drm_display_mode *out,
 	if (in->clock > INT_MAX || in->vrefresh > INT_MAX)
 		return -ERANGE;
 
+	/* At most, 1 set bit describing the 3D layout of the mode */
+	if (hweight32(in->flags & DRM_MODE_FLAG_3D_MASK) > 1)
+		return -EINVAL;
+
 	out->clock = in->clock;
 	out->hdisplay = in->hdisplay;
 	out->hsync_start = in->hsync_start;
-- 
1.8.3.1

[PATCH 08/12] drm: Set the relevant infoframe field when scanning out a 3D mode

[Damien Lespiau]

When scanning out a 3D mode on HDMI, we need to send an HDMI infoframe
with the corresponding layout to the sink.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e016a5d..9a36b6e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3364,6 +3364,18 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 }
 EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
 
+static enum hdmi_3d_structure
+s3d_structure_from_display_mode(const struct drm_display_mode *mode)
+{
+	u32 s3d_mode = (mode->flags & DRM_MODE_FLAG_3D_MASK) >> 14;
+	int set = ffs(s3d_mode) - 1;
+
+	if (set == 7)
+		return HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF;
+
+	return set;
+}
+
 /**
  * drm_hdmi_vendor_infoframe_from_display_mode() - fill an HDMI infoframe with
  * data from a DRM display mode
@@ -3381,20 +3393,29 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
 					    const struct drm_display_mode *mode)
 {
 	int err;
+	u32 s3d_flags;
 	u8 vic;
 
 	if (!frame || !mode)
 		return -EINVAL;
 
 	vic = drm_match_hdmi_mode(mode);
-	if (!vic)
+	s3d_flags = mode->flags & DRM_MODE_FLAG_3D_MASK;
+
+	if (!vic && !s3d_flags)
+		return -EINVAL;
+
+	if (vic && s3d_flags)
 		return -EINVAL;
 
 	err = hdmi_vendor_infoframe_init(frame);
 	if (err < 0)
 		return err;
 
-	frame->vic = vic;
+	if (vic)
+		frame->vic = vic;
+	else
+		frame->s3d_struct = s3d_structure_from_display_mode(mode);
 
 	return 0;
 }
-- 
1.8.3.1

[PATCH 09/12] drm: Track the number of buffers that compose a framebuffer

[Damien Lespiau]

DRM has supported multiple-buffers fbs since the introduction of ADDFB2.
Let's track the number of buffers in a drm_framebuffer in the common
structure so we can use it in the DRM core.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/drm_crtc_helper.c | 20 ++++++++++++++++++++
 include/drm/drm_crtc.h            |  9 +++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 6a64749..32985c0 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -901,6 +901,25 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode)
 }
 EXPORT_SYMBOL(drm_helper_connector_dpms);
 
+static int drm_helper_mode_fb_num_buffers(struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	int count = 0, i, j;
+
+	for (i = 0; i < 4; i++) {
+		if (!mode_cmd->handles[i])
+			continue;
+
+		for (j = 0; j < i; j++)
+			if (mode_cmd->handles[i] == mode_cmd->handles[j])
+				break;
+
+		if (j == i)
+			count++;
+	}
+
+	return count;
+}
+
 int drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
 				   struct drm_mode_fb_cmd2 *mode_cmd)
 {
@@ -915,6 +934,7 @@ int drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
 	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &fb->depth,
 				    &fb->bits_per_pixel);
 	fb->pixel_format = mode_cmd->pixel_format;
+	fb->num_buffers = drm_helper_mode_fb_num_buffers(mode_cmd);
 
 	return 0;
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index bf242ac..e685baf 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -286,6 +286,15 @@ struct drm_framebuffer {
 	int flags;
 	uint32_t pixel_format; /* fourcc format */
 	struct list_head filp_head;
+	/*
+	 * A framebuffer can be made of several planes (eg. planar YUV
+	 * formats). These planes can either share the same buffer (in which
+	 * case 'offsets' will tell us where they are within that buffer) or
+	 * be in separate buffers (in which case offsets[i] will generally be
+	 * 0). We track in the common DRM code how many different buffers the
+	 * framebuffer consists of.
+	 */
+	int num_buffers;
 	/* if you are using the helper */
 	void *helper_private;
 };
-- 
1.8.3.1

[PATCH 10/12] drm: Don't allow multiple buffers fb with stereo modes

[Damien Lespiau]

There are a few things to be flushed out if we want to allow multiple
buffers stereo framebuffers:
  - What with drm_planes? what semantics do they follow, what is the
    hardware able to do with them?
  - How do we define which buffer if the right/left one
  - Do we allow flips between 1 buffer fbs and 2 buffers fbs (No.)

So for now, and until I get access to hardware that can do that, let's
just disallow 2 buffers stereo framebuffers to not introduce ABI we
wouldn't be able to change afterwards.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 39f60ec..91d1c4b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2131,6 +2131,17 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			goto out;
 		}
 
+		/*
+		 * Do not allow the use of framebuffers consisting of multiple
+		 * buffers with stereo modes until all the details API details
+		 * are fleshed out (eg. interaction with drm_planes, switch
+		 * between a 1 buffers and a 2 buffers fb, ...)
+		 */
+		if (fb->num_buffers > 1 && drm_mode_is_stereo(mode)) {
+			ret = -EINVAL;
+			goto out;
+		}
+
 		drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
 
 		hdisplay = mode->hdisplay;
-- 
1.8.3.1

[PATCH 11/12] drm: Make drm_match_cea_mode() return the underlying 2D VIC for 3d modes

[Damien Lespiau]

When scanning out a stereo mode, the AVI infoframe vic field has to be
the underlyng 2D VIC. Before that commit, we weren't matching the CEA
mode because of the extra stereo flag and then were setting the VIC
field in the AVI infoframe to 0.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/drm_edid.c  |  4 ++--
 drivers/gpu/drm/drm_modes.c | 18 ++++++++++++------
 include/drm/drm_crtc.h      |  2 +-
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 9a36b6e..6b74698 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2401,7 +2401,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
 
 		if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) ||
 		     KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) &&
-		    drm_mode_equal_no_clocks(to_match, cea_mode))
+		    drm_mode_equal_no_clocks_no_stereo(to_match, cea_mode))
 			return mode + 1;
 	}
 	return 0;
@@ -2450,7 +2450,7 @@ static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
 
 		if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) ||
 		     KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) &&
-		    drm_mode_equal_no_clocks(to_match, hdmi_mode))
+		    drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode))
 			return mode + 1;
 	}
 	return 0;
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 504a602..76adee0 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -851,12 +851,16 @@ bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_displ
 	} else if (mode1->clock != mode2->clock)
 		return false;
 
-	return drm_mode_equal_no_clocks(mode1, mode2);
+	if ((mode1->flags & DRM_MODE_FLAG_3D_MASK) !=
+	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
+		return false;
+
+	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
 }
 EXPORT_SYMBOL(drm_mode_equal);
 
 /**
- * drm_mode_equal_no_clocks - test modes for equality
+ * drm_mode_equal_no_clocks_no_stereo - test modes for equality
  * @mode1: first mode
  * @mode2: second mode
  *
@@ -864,12 +868,13 @@ EXPORT_SYMBOL(drm_mode_equal);
  * None.
  *
  * Check to see if @mode1 and @mode2 are equivalent, but
- * don't check the pixel clocks.
+ * don't check the pixel clocks nor the stereo layout.
  *
  * RETURNS:
  * True if the modes are equal, false otherwise.
  */
-bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2)
+bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
+					const struct drm_display_mode *mode2)
 {
 	if (mode1->hdisplay == mode2->hdisplay &&
 	    mode1->hsync_start == mode2->hsync_start &&
@@ -881,12 +886,13 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
 	    mode1->vsync_end == mode2->vsync_end &&
 	    mode1->vtotal == mode2->vtotal &&
 	    mode1->vscan == mode2->vscan &&
-	    mode1->flags == mode2->flags)
+	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
+	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
 		return true;
 
 	return false;
 }
-EXPORT_SYMBOL(drm_mode_equal_no_clocks);
+EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
 
 /**
  * drm_mode_validate_size - make sure modes adhere to size constraints
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e685baf..f0c5530 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -943,7 +943,7 @@ extern void drm_mode_config_reset(struct drm_device *dev);
 extern void drm_mode_config_cleanup(struct drm_device *dev);
 extern void drm_mode_set_name(struct drm_display_mode *mode);
 extern bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2);
-extern bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2);
+extern bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2);
 extern int drm_mode_width(const struct drm_display_mode *mode);
 extern int drm_mode_height(const struct drm_display_mode *mode);
 
-- 
1.8.3.1

[PATCH 12/12] drm: Carry over the stereo flags when adding the alternate mode

[Damien Lespiau]

This allows to expose the alternate clock versions of the stereo modes.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 6b74698..55dac77 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2504,6 +2504,9 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid)
 		if (!newmode)
 			continue;
 
+		/* Carry over the stereo flags */
+		newmode->flags |= mode->flags & DRM_MODE_FLAG_3D_MASK;
+
 		/*
 		 * The current mode could be either variant. Make
 		 * sure to pick the "other" clock for the new mode.
-- 
1.8.3.1

Re: [PATCH 05/12] drm/edid: Expose mandatory stereo modes for HDMI sinks

[Ville Syrjälä]

On Mon, Sep 16, 2013 at 06:48:48PM +0100, Damien Lespiau wrote:
> For now, let's just look at the 3D_present flag of the CEA HDMI vendor
> block to detect if the sink supports a small list of then mandatory 3D
> formats.
> 
> See the HDMI 1.4a 3D extraction for detail:
>   http://www.hdmi.org/manufacturer/specification.aspx
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 108 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 101 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a207cc3..78009d1 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2550,13 +2550,93 @@ do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
>  	return modes;
>  }
>  
> +struct stereo_mandatory_mode {
> +	int width, height, freq;

Can we call it vrefresh like in drm_display_mode?

> +	unsigned int interlace_flag, layouts;

What's the benefit of separating the two?

> +};
> +
> +static const struct stereo_mandatory_mode stereo_mandatory_modes[] = {
> +	{ 1920, 1080, 24, 0,
> +	  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING },
> +	{ 1920, 1080, 50, DRM_MODE_FLAG_INTERLACE,
> +	  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF },
> +	{ 1920, 1080, 60, DRM_MODE_FLAG_INTERLACE,
> +	  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF },
> +	{ 1280, 720,  50, 0,
> +	  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING },
> +	{ 1280, 720,  60, 0,
> +	  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING }
> +};
> +
> +static bool
> +stereo_match_mandatory(const struct drm_display_mode *mode,
> +		       const struct stereo_mandatory_mode *stereo_mode)
> +{
> +	unsigned int interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
> +
> +	return mode->hdisplay == stereo_mode->width &&
> +	       mode->vdisplay == stereo_mode->height &&
> +	       interlaced == stereo_mode->interlace_flag &&
> +	       drm_mode_vrefresh(mode) == stereo_mode->freq;
> +}
> +
> +static const struct stereo_mandatory_mode *
> +hdmi_find_stereo_mandatory_mode(struct drm_display_mode *mode)
                                   ^

Can be const.

> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(stereo_mandatory_modes); i++)
> +		if (stereo_match_mandatory(mode, &stereo_mandatory_modes[i]))
> +			return &stereo_mandatory_modes[i];
> +
> +	return NULL;
> +}
> +
> +static int add_hdmi_mandatory_stereo_modes(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_display_mode *mode, *new_mode;

'mode' can be const, 'new_mode' could be moved into tighter scope.

> +	struct list_head stereo_modes;
> +	int modes = 0;
> +
> +	INIT_LIST_HEAD(&stereo_modes);
> +
> +	list_for_each_entry(mode, &connector->probed_modes, head) {
> +		const struct stereo_mandatory_mode *mandatory;
> +		u32 stereo_layouts, layout;
> +
> +		mandatory = hdmi_find_stereo_mandatory_mode(mode);
> +		if (!mandatory)
> +			continue;
> +
> +		stereo_layouts = mandatory->layouts;
> +                do {
   ^^^^^^^^^^^^^^^^
> +                        layout = 1 << (ffs(stereo_layouts) - 1);
   ^^^^^^^^^^^^^^^^^^^^^^^^

-ENOTABS

> +			stereo_layouts &= ~layout;
> +
> +			new_mode = drm_mode_duplicate(dev, mode);
> +			if (!new_mode)
> +				continue;
> +
> +			new_mode->flags |= layout;
> +			list_add_tail(&new_mode->head, &stereo_modes);
> +			modes++;
> +		} while (stereo_layouts);
> +	}
> +
> +	list_splice_tail(&stereo_modes, &connector->probed_modes);
> +
> +	return modes;
> +}
> +
>  /*
>   * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
>   * @connector: connector corresponding to the HDMI sink
>   * @db: start of the CEA vendor specific block
>   * @len: length of the CEA block payload, ie. one can access up to db[len]
>   *
> - * Parses the HDMI VSDB looking for modes to add to @connector.
> + * Parses the HDMI VSDB looking for modes to add to @connector. This function
> + * also adds the stereo 3d modes when applicable.
>   */
>  static int
>  do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
> @@ -2582,10 +2662,15 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
>  
>  	/* the declared length is not long enough for the 2 first bytes
>  	 * of additional video format capabilities */
> -	offset += 2;
> -	if (len < (8 + offset))
> +	if (len < (8 + offset + 2))
>  		goto out;
>  
> +	/* 3D_Present */
> +	offset++;
> +	if (db[8 + offset] & (1 << 7))
> +		modes += add_hdmi_mandatory_stereo_modes(connector);

Hmm. I was thinking this is a bit soon since we haven't added the 4k
modes, nor the alternate clock modes yet. But I guess the 4k modes
aren't relevant here, and the alternate modes vs. 3D modes steps can
be done in either order. Or did I miss something here?

> +
> +	offset++;
>  	vic_len = db[8 + offset] >> 5;
>  
>  	for (i = 0; i < vic_len && len >= (9 + offset + i); i++) {
> @@ -2665,8 +2750,8 @@ static int
>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
>  {
>  	const u8 *cea = drm_find_cea_extension(edid);
> -	const u8 *db;
> -	u8 dbl;
> +	const u8 *db, *hdmi = NULL;
> +	u8 dbl, hdmi_len;
>  	int modes = 0;
>  
>  	if (cea && cea_revision(cea) >= 3) {
> @@ -2681,11 +2766,20 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid)
>  
>  			if (cea_db_tag(db) == VIDEO_BLOCK)
>  				modes += do_cea_modes(connector, db + 1, dbl);
> -			else if (cea_db_is_hdmi_vsdb(db))
> -				modes += do_hdmi_vsdb_modes(connector, db, dbl);
> +			else if (cea_db_is_hdmi_vsdb(db)) {
> +				hdmi = db;
> +				hdmi_len = dbl;
> +			}
>  		}
>  	}
>  
> +	/*
> +	 * We parse the HDMI VSDB after having added the cea modes as we will
> +	 * be patching their flags when the sink supports stereo 3D.
> +	 */
> +	if (hdmi)
> +		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len);
> +
>  	return modes;
>  }
>  
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 08/12] drm: Set the relevant infoframe field when scanning out a 3D mode

[Ville Syrjälä]

On Mon, Sep 16, 2013 at 06:48:51PM +0100, Damien Lespiau wrote:
> When scanning out a 3D mode on HDMI, we need to send an HDMI infoframe
> with the corresponding layout to the sink.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e016a5d..9a36b6e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3364,6 +3364,18 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>  }
>  EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
>  
> +static enum hdmi_3d_structure
> +s3d_structure_from_display_mode(const struct drm_display_mode *mode)
> +{
> +	u32 s3d_mode = (mode->flags & DRM_MODE_FLAG_3D_MASK) >> 14;
> +	int set = ffs(s3d_mode) - 1;
> +
> +	if (set == 7)
> +		return HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF;

This function feels a bit too subtle. I would perhaps go w/ just a
switch statement. Or maybe leave a hole for 7 in the DRM_MODE_FLAG_3D
flags. And if we run out of bits before 3d_structure=7 gets defined we
just repurpose the bit for something else. But maybe that's equally
subtle.

Hmm. The spec is quite poor too. In one place it says the quincunx modes
are valid for 3d_structure=8 (and 15 is reserved), but in another place it
says 3d_structure=15 is when the quincunx stuff applies. But I guss we
can just keep ignoring the 3d_structure > 8 case for now.

> +
> +	return set;
> +}
> +
>  /**
>   * drm_hdmi_vendor_infoframe_from_display_mode() - fill an HDMI infoframe with
>   * data from a DRM display mode
> @@ -3381,20 +3393,29 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>  					    const struct drm_display_mode *mode)
>  {
>  	int err;
> +	u32 s3d_flags;
>  	u8 vic;
>  
>  	if (!frame || !mode)
>  		return -EINVAL;
>  
>  	vic = drm_match_hdmi_mode(mode);
> -	if (!vic)
> +	s3d_flags = mode->flags & DRM_MODE_FLAG_3D_MASK;
> +
> +	if (!vic && !s3d_flags)
> +		return -EINVAL;
> +
> +	if (vic && s3d_flags)
>  		return -EINVAL;

Could be just '!vic == !s3d_flags' or w/ !! on both sides if we want
to stick to positive thinking.

But maybe it's me who's getting into the subtle territory here :)

Other than the bikesheds it looks OK to me:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	err = hdmi_vendor_infoframe_init(frame);
>  	if (err < 0)
>  		return err;
>  
> -	frame->vic = vic;
> +	if (vic)
> +		frame->vic = vic;
> +	else
> +		frame->s3d_struct = s3d_structure_from_display_mode(mode);
>  
>  	return 0;
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 05/12] drm/edid: Expose mandatory stereo modes for HDMI sinks

[Damien Lespiau]

On Mon, Sep 16, 2013 at 09:29:31PM +0300, Ville Syrjälä wrote:
> >  static int
> >  do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
> > @@ -2582,10 +2662,15 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
> >  
> >  	/* the declared length is not long enough for the 2 first bytes
> >  	 * of additional video format capabilities */
> > -	offset += 2;
> > -	if (len < (8 + offset))
> > +	if (len < (8 + offset + 2))
> >  		goto out;
> >  
> > +	/* 3D_Present */
> > +	offset++;
> > +	if (db[8 + offset] & (1 << 7))
> > +		modes += add_hdmi_mandatory_stereo_modes(connector);
> 
> Hmm. I was thinking this is a bit soon since we haven't added the 4k
> modes, nor the alternate clock modes yet. But I guess the 4k modes
> aren't relevant here, and the alternate modes vs. 3D modes steps can
> be done in either order. Or did I miss something here?

You're not missing anything. It's indeed fine to add the mandatory modes
before the 4k ones because the mandatory modes are either 1080p, 1080i
or 720p. As for alternate clocks, I opted to re-use the current code
with one tiny adjustement, so the alternate clocks logic stays in one
place.

-- 
Damien

Re: [Intel-gfx] [PATCH 10/12] drm: Don't allow multiple buffers fb with stereo modes

[Ville Syrjälä]

On Mon, Sep 16, 2013 at 06:48:53PM +0100, Damien Lespiau wrote:
> There are a few things to be flushed out if we want to allow multiple
> buffers stereo framebuffers:
>   - What with drm_planes? what semantics do they follow, what is the
>     hardware able to do with them?
>   - How do we define which buffer if the right/left one
>   - Do we allow flips between 1 buffer fbs and 2 buffers fbs (No.)
> 
> So for now, and until I get access to hardware that can do that, let's
> just disallow 2 buffers stereo framebuffers to not introduce ABI we
> wouldn't be able to change afterwards.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 39f60ec..91d1c4b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2131,6 +2131,17 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  			goto out;
>  		}
>  
> +		/*
> +		 * Do not allow the use of framebuffers consisting of multiple
> +		 * buffers with stereo modes until all the details API details
> +		 * are fleshed out (eg. interaction with drm_planes, switch
> +		 * between a 1 buffers and a 2 buffers fb, ...)
> +		 */
> +		if (fb->num_buffers > 1 && drm_mode_is_stereo(mode)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}

This would prevent planar buffers in stereo modes. I'm think we just
ignore the matter for now and let drivers deal with it. We don't have
enough handles anyway for planar stereo, so maybe we even want to add
separate left/right fb attachment properties to the planes instead of
tying it up in inside a single fb. Or we cook up addfb3 when we hit
this problem for real. I think we'd anyway need some kind of flag for
the fb if it contains both left and right buffers.

> +
>  		drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
>  
>  		hdisplay = mode->hdisplay;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 11/12] drm: Make drm_match_cea_mode() return the underlying 2D VIC for 3d modes

[Ville Syrjälä]

On Mon, Sep 16, 2013 at 06:48:54PM +0100, Damien Lespiau wrote:
> When scanning out a stereo mode, the AVI infoframe vic field has to be
> the underlyng 2D VIC. Before that commit, we weren't matching the CEA
> mode because of the extra stereo flag and then were setting the VIC
> field in the AVI infoframe to 0.

I think this is where we hit problems. How do we identify the 2D VIC
when the timings of the adjusted mode got mangled already to include
both eyes?

> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  |  4 ++--
>  drivers/gpu/drm/drm_modes.c | 18 ++++++++++++------
>  include/drm/drm_crtc.h      |  2 +-
>  3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 9a36b6e..6b74698 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2401,7 +2401,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
>  
>  		if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) ||
>  		     KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) &&
> -		    drm_mode_equal_no_clocks(to_match, cea_mode))
> +		    drm_mode_equal_no_clocks_no_stereo(to_match, cea_mode))
>  			return mode + 1;
>  	}
>  	return 0;
> @@ -2450,7 +2450,7 @@ static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
>  
>  		if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) ||
>  		     KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) &&
> -		    drm_mode_equal_no_clocks(to_match, hdmi_mode))
> +		    drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode))
>  			return mode + 1;
>  	}
>  	return 0;
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 504a602..76adee0 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -851,12 +851,16 @@ bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_displ
>  	} else if (mode1->clock != mode2->clock)
>  		return false;
>  
> -	return drm_mode_equal_no_clocks(mode1, mode2);
> +	if ((mode1->flags & DRM_MODE_FLAG_3D_MASK) !=
> +	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
> +		return false;
> +
> +	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
>  }
>  EXPORT_SYMBOL(drm_mode_equal);
>  
>  /**
> - * drm_mode_equal_no_clocks - test modes for equality
> + * drm_mode_equal_no_clocks_no_stereo - test modes for equality
>   * @mode1: first mode
>   * @mode2: second mode
>   *
> @@ -864,12 +868,13 @@ EXPORT_SYMBOL(drm_mode_equal);
>   * None.
>   *
>   * Check to see if @mode1 and @mode2 are equivalent, but
> - * don't check the pixel clocks.
> + * don't check the pixel clocks nor the stereo layout.
>   *
>   * RETURNS:
>   * True if the modes are equal, false otherwise.
>   */
> -bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2)
> +bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
> +					const struct drm_display_mode *mode2)
>  {
>  	if (mode1->hdisplay == mode2->hdisplay &&
>  	    mode1->hsync_start == mode2->hsync_start &&
> @@ -881,12 +886,13 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>  	    mode1->vsync_end == mode2->vsync_end &&
>  	    mode1->vtotal == mode2->vtotal &&
>  	    mode1->vscan == mode2->vscan &&
> -	    mode1->flags == mode2->flags)
> +	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
> +	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
>  		return true;
>  
>  	return false;
>  }
> -EXPORT_SYMBOL(drm_mode_equal_no_clocks);
> +EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
>  
>  /**
>   * drm_mode_validate_size - make sure modes adhere to size constraints
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index e685baf..f0c5530 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -943,7 +943,7 @@ extern void drm_mode_config_reset(struct drm_device *dev);
>  extern void drm_mode_config_cleanup(struct drm_device *dev);
>  extern void drm_mode_set_name(struct drm_display_mode *mode);
>  extern bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2);
> -extern bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2);
> +extern bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2);
>  extern int drm_mode_width(const struct drm_display_mode *mode);
>  extern int drm_mode_height(const struct drm_display_mode *mode);
>  
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 11/12] drm: Make drm_match_cea_mode() return the underlying 2D VIC for 3d modes

[Damien Lespiau]

On Tue, Sep 17, 2013 at 11:22:26AM +0300, Ville Syrjälä wrote:
> On Mon, Sep 16, 2013 at 06:48:54PM +0100, Damien Lespiau wrote:
> > When scanning out a stereo mode, the AVI infoframe vic field has to be
> > the underlyng 2D VIC. Before that commit, we weren't matching the CEA
> > mode because of the extra stereo flag and then were setting the VIC
> > field in the AVI infoframe to 0.
> 
> I think this is where we hit problems. How do we identify the 2D VIC
> when the timings of the adjusted mode got mangled already to include
> both eyes?

Oh, indeed, that wouldn't work. Note that Daniel had the same remark as
you regarding the mangling of the modes, "leaking" how we implement the
modes (in theory hw could need a different way to be programmed). I
wondered about that as well and now that we have a couple of good
reasons, in kernel timing adjustment for stereo modes will happen in a
v5 of this series.

-- 
Damien

Re: [Intel-gfx] [PATCH 10/12] drm: Don't allow multiple buffers fb with stereo modes

[Damien Lespiau]

On Tue, Sep 17, 2013 at 11:20:46AM +0300, Ville Syrjälä wrote:
 > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -2131,6 +2131,17 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> >  			goto out;
> >  		}
> >  
> > +		/*
> > +		 * Do not allow the use of framebuffers consisting of multiple
> > +		 * buffers with stereo modes until all the details API details
> > +		 * are fleshed out (eg. interaction with drm_planes, switch
> > +		 * between a 1 buffers and a 2 buffers fb, ...)
> > +		 */
> > +		if (fb->num_buffers > 1 && drm_mode_is_stereo(mode)) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> 
> This would prevent planar buffers in stereo modes. I'm think we just
> ignore the matter for now and let drivers deal with it. We don't have
> enough handles anyway for planar stereo, so maybe we even want to add
> separate left/right fb attachment properties to the planes instead of
> tying it up in inside a single fb. Or we cook up addfb3 when we hit
> this problem for real. I think we'd anyway need some kind of flag for
> the fb if it contains both left and right buffers.

I'm quite happy to ignore 3 planes YUV stereo fbs for now :) (2 planes
YUV stereo fbs still fit!).

Are you fine with this test though, or do you mean ignore the whole
matter of forbidding this case (or just the multiplane stereo fb case)?
I was just thinking that I missed the addition of this check in the page
flip ioctl.

-- 
Damien

Re: HDMI stereo support v4

[Damien Lespiau]

On Mon, Sep 16, 2013 at 06:48:43PM +0100, Damien Lespiau wrote:
> Next installment of the HDMI stereo 3D series, following:
>   http://lists.freedesktop.org/archives/dri-devel/2013-September/044885.html

Seems like a v5 is shapping up. I'll summarise the discussion with
Daniel on IRC yesterday and with Ville on the thread:

  - Let's adjust the timings (when it's needed) in the driver instead of
    having user-space do it. This avoid leaking an implementation
    detail, let user-space use the mode without having to tweak it and
    Ville found that it'd be a bit impractical to match the 2D CEA VIC.
    (Joakim Plate was wondering about this as well).

  - Make drivers opt-in to expose stereo modes, just like today with
    interlaced modes, giving an opportunity for people to make it work
    people exposing the modes.

  - For frame packing (where the framebuffer has to be twice as high +
    vblank) we could also reject framebuffers with the wrong geometry.

-- 
Damien

Re: [PATCH 10/12] drm: Don't allow multiple buffers fb with stereo modes

[Ville Syrjälä]

On Tue, Sep 17, 2013 at 10:03:12AM +0100, Damien Lespiau wrote:
> On Tue, Sep 17, 2013 at 11:20:46AM +0300, Ville Syrjälä wrote:
>  > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -2131,6 +2131,17 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> > >  			goto out;
> > >  		}
> > >  
> > > +		/*
> > > +		 * Do not allow the use of framebuffers consisting of multiple
> > > +		 * buffers with stereo modes until all the details API details
> > > +		 * are fleshed out (eg. interaction with drm_planes, switch
> > > +		 * between a 1 buffers and a 2 buffers fb, ...)
> > > +		 */
> > > +		if (fb->num_buffers > 1 && drm_mode_is_stereo(mode)) {
> > > +			ret = -EINVAL;
> > > +			goto out;
> > > +		}
> > 
> > This would prevent planar buffers in stereo modes. I'm think we just
> > ignore the matter for now and let drivers deal with it. We don't have
> > enough handles anyway for planar stereo, so maybe we even want to add
> > separate left/right fb attachment properties to the planes instead of
> > tying it up in inside a single fb. Or we cook up addfb3 when we hit
> > this problem for real. I think we'd anyway need some kind of flag for
> > the fb if it contains both left and right buffers.
> 
> I'm quite happy to ignore 3 planes YUV stereo fbs for now :) (2 planes
> YUV stereo fbs still fit!).
> 
> Are you fine with this test though, or do you mean ignore the whole
> matter of forbidding this case (or just the multiplane stereo fb case)?
> I was just thinking that I missed the addition of this check in the page
> flip ioctl.

Yeah, I was thinking we that we can ignore this issue for now, and so we
wouldn't need the check. Currently for non-stereo cases the only thing
we check is that there is a valid handle for each plane. If userspace
passes more handles, we simply ignore the extra ones.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 10/12] drm: Don't allow multiple buffers fb with stereo modes

[Daniel Vetter]

On Tue, Sep 17, 2013 at 12:54:09PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 17, 2013 at 10:03:12AM +0100, Damien Lespiau wrote:
> > On Tue, Sep 17, 2013 at 11:20:46AM +0300, Ville Syrjälä wrote:
> >  > +++ b/drivers/gpu/drm/drm_crtc.c
> > > > @@ -2131,6 +2131,17 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> > > >  			goto out;
> > > >  		}
> > > >  
> > > > +		/*
> > > > +		 * Do not allow the use of framebuffers consisting of multiple
> > > > +		 * buffers with stereo modes until all the details API details
> > > > +		 * are fleshed out (eg. interaction with drm_planes, switch
> > > > +		 * between a 1 buffers and a 2 buffers fb, ...)
> > > > +		 */
> > > > +		if (fb->num_buffers > 1 && drm_mode_is_stereo(mode)) {
> > > > +			ret = -EINVAL;
> > > > +			goto out;
> > > > +		}
> > > 
> > > This would prevent planar buffers in stereo modes. I'm think we just
> > > ignore the matter for now and let drivers deal with it. We don't have
> > > enough handles anyway for planar stereo, so maybe we even want to add
> > > separate left/right fb attachment properties to the planes instead of
> > > tying it up in inside a single fb. Or we cook up addfb3 when we hit
> > > this problem for real. I think we'd anyway need some kind of flag for
> > > the fb if it contains both left and right buffers.
> > 
> > I'm quite happy to ignore 3 planes YUV stereo fbs for now :) (2 planes
> > YUV stereo fbs still fit!).
> > 
> > Are you fine with this test though, or do you mean ignore the whole
> > matter of forbidding this case (or just the multiplane stereo fb case)?
> > I was just thinking that I missed the addition of this check in the page
> > flip ioctl.
> 
> Yeah, I was thinking we that we can ignore this issue for now, and so we
> wouldn't need the check. Currently for non-stereo cases the only thing
> we check is that there is a valid handle for each plane. If userspace
> passes more handles, we simply ignore the extra ones.

I guess we should start to check that. For 3d framebuffers with 2
separate buffer handles for each plane I think we need to add another flag
to addfb2, e.g.

#define DRM_MODE_FB_3D_2_FRAMES (1<<1) /* separate left/right buffers, doubles plane count */

and then also throw in the respective check code into the core that
userspace supplies sufficient amounts of buffers in framebuffer_check()
by adjusting drM_format_num_planes and drm_format_plane_cpp.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 10/12] drm: Don't allow multiple buffers fb with stereo modes

[Ville Syrjälä]

On Tue, Sep 17, 2013 at 12:37:48PM +0200, Daniel Vetter wrote:
> On Tue, Sep 17, 2013 at 12:54:09PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 17, 2013 at 10:03:12AM +0100, Damien Lespiau wrote:
> > > On Tue, Sep 17, 2013 at 11:20:46AM +0300, Ville Syrjälä wrote:
> > >  > +++ b/drivers/gpu/drm/drm_crtc.c
> > > > > @@ -2131,6 +2131,17 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> > > > >  			goto out;
> > > > >  		}
> > > > >  
> > > > > +		/*
> > > > > +		 * Do not allow the use of framebuffers consisting of multiple
> > > > > +		 * buffers with stereo modes until all the details API details
> > > > > +		 * are fleshed out (eg. interaction with drm_planes, switch
> > > > > +		 * between a 1 buffers and a 2 buffers fb, ...)
> > > > > +		 */
> > > > > +		if (fb->num_buffers > 1 && drm_mode_is_stereo(mode)) {
> > > > > +			ret = -EINVAL;
> > > > > +			goto out;
> > > > > +		}
> > > > 
> > > > This would prevent planar buffers in stereo modes. I'm think we just
> > > > ignore the matter for now and let drivers deal with it. We don't have
> > > > enough handles anyway for planar stereo, so maybe we even want to add
> > > > separate left/right fb attachment properties to the planes instead of
> > > > tying it up in inside a single fb. Or we cook up addfb3 when we hit
> > > > this problem for real. I think we'd anyway need some kind of flag for
> > > > the fb if it contains both left and right buffers.
> > > 
> > > I'm quite happy to ignore 3 planes YUV stereo fbs for now :) (2 planes
> > > YUV stereo fbs still fit!).
> > > 
> > > Are you fine with this test though, or do you mean ignore the whole
> > > matter of forbidding this case (or just the multiplane stereo fb case)?
> > > I was just thinking that I missed the addition of this check in the page
> > > flip ioctl.
> > 
> > Yeah, I was thinking we that we can ignore this issue for now, and so we
> > wouldn't need the check. Currently for non-stereo cases the only thing
> > we check is that there is a valid handle for each plane. If userspace
> > passes more handles, we simply ignore the extra ones.
> 
> I guess we should start to check that. For 3d framebuffers with 2
> separate buffer handles for each plane I think we need to add another flag
> to addfb2, e.g.
> 
> #define DRM_MODE_FB_3D_2_FRAMES (1<<1) /* separate left/right buffers, doubles plane count */
> 
> and then also throw in the respective check code into the core that
> userspace supplies sufficient amounts of buffers in framebuffer_check()
> by adjusting drM_format_num_planes and drm_format_plane_cpp.

Well, we shouldn't mix the plane vs. buffers/handles/whatever concepts. The
number of planes is clearly defined by the pixel format. But yes, we should
probably add a drm_format_num_buffers() function to figure out how many buffers
are required.

But the problem is that addfb2 can't supply more than 4. So we need a new
ioctl if we want to collect all that information inside a single drm_fb
object. If we do add another ioctl then I think we should go at least to
16, since we might also want to have separate buffers for each field in
interlaced framebuffers. And then we need to clearly define which handle
is which plane/field/eye. Something like this for example:

eye=left field=top plane=0 [plane=1 ...] [field=bottom plane=0 [plane=1 ...]]
 [eye=right field=top plane=0 [plane=1 ...] [field=bottom plane=0 [plane=1 ...]]]

so you first have all the planes for left/top, then all planes for
left/bottom, then right/top, and finally right/bottom.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 10/12] drm: Don't allow multiple buffers fb with stereo modes

[Damien Lespiau]

On Tue, Sep 17, 2013 at 12:37:48PM +0200, Daniel Vetter wrote:
> I guess we should start to check that. For 3d framebuffers with 2
> separate buffer handles for each plane I think we need to add another flag
> to addfb2, e.g.
> 
> #define DRM_MODE_FB_3D_2_FRAMES (1<<1) /* separate left/right buffers, doubles plane count */
> 
> and then also throw in the respective check code into the core that
> userspace supplies sufficient amounts of buffers in framebuffer_check()
> by adjusting drM_format_num_planes and drm_format_plane_cpp.

Would we really need that flag? we can just count the number of buffers
and decide from that number whether we're in the 1 or 2 buffer(s) case?

-- 
Damien

Re: [PATCH 10/12] drm: Don't allow multiple buffers fb with stereo modes

[Ville Syrjälä]

On Tue, Sep 17, 2013 at 02:20:41PM +0100, Damien Lespiau wrote:
> On Tue, Sep 17, 2013 at 12:37:48PM +0200, Daniel Vetter wrote:
> > I guess we should start to check that. For 3d framebuffers with 2
> > separate buffer handles for each plane I think we need to add another flag
> > to addfb2, e.g.
> > 
> > #define DRM_MODE_FB_3D_2_FRAMES (1<<1) /* separate left/right buffers, doubles plane count */
> > 
> > and then also throw in the respective check code into the core that
> > userspace supplies sufficient amounts of buffers in framebuffer_check()
> > by adjusting drM_format_num_planes and drm_format_plane_cpp.
> 
> Would we really need that flag? we can just count the number of buffers
> and decide from that number whether we're in the 1 or 2 buffer(s) case?

I'd go with explicit flags. We have one for interlaced already. Although
ATM the interlaced flag must mean that the fields are interleaved in the
same buffer. But if we want separate buffers for each field, we need a
flag for that too. Or we just pretend the old interlace flag doesn't
exist yet and steal it for that purpose. I don't think anyone is using
the flag yet. But anyway we'd need a new ioctl :(

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 10/12] drm: Don't allow multiple buffers fb with stereo modes

[Daniel Vetter]

On Tue, Sep 17, 2013 at 3:20 PM, Damien Lespiau
<damien.lespiau@intel.com> wrote:
> On Tue, Sep 17, 2013 at 12:37:48PM +0200, Daniel Vetter wrote:
>> I guess we should start to check that. For 3d framebuffers with 2
>> separate buffer handles for each plane I think we need to add another flag
>> to addfb2, e.g.
>>
>> #define DRM_MODE_FB_3D_2_FRAMES (1<<1) /* separate left/right buffers, doubles plane count */
>>
>> and then also throw in the respective check code into the core that
>> userspace supplies sufficient amounts of buffers in framebuffer_check()
>> by adjusting drM_format_num_planes and drm_format_plane_cpp.
>
> Would we really need that flag? we can just count the number of buffers
> and decide from that number whether we're in the 1 or 2 buffer(s) case?

We do not know at addfb2 time that it'll be a 3d mode buffer, and
drivers might want to know that to internally assign the buffers to
left/right buffer handle pointers. Similar to how we already have a
flag for when the buffer is interlaced, which atm we don't support
(since we only support scan-out of progressive buffers to interlaced
modes, not interlaced to interlaced). So imo a flag here makes sense
so that the driver can properly check constraints and make sense of
the handles.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 10/12] drm: Don't allow multiple buffers fb with stereo modes

[Daniel Vetter]

On Tue, Sep 17, 2013 at 1:02 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> But the problem is that addfb2 can't supply more than 4. So we need a new
> ioctl if we want to collect all that information inside a single drm_fb
> object. If we do add another ioctl then I think we should go at least to
> 16, since we might also want to have separate buffers for each field in
> interlaced framebuffers. And then we need to clearly define which handle
> is which plane/field/eye. Something like this for example:


Blergh, I've thought we have gone with at least 6 buffers for the 3d
case. Meh :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 05/12] drm/edid: Expose mandatory stereo modes for HDMI sinks

[Damien Lespiau]

On Mon, Sep 16, 2013 at 09:29:31PM +0300, Ville Syrjälä wrote:
> > +struct stereo_mandatory_mode {
> > +	int width, height, freq;

[..]

> > +	unsigned int interlace_flag, layouts;
> 
> What's the benefit of separating the two?

Just that we can easily cycle through the layout flags and add a mode
per flag without having to clear the interlace bit. Trade 16 bytes against 
one instruction or so, can do I guess.

-- 
Damien