Stereo 3D v3

Stereo 3D v3

[Damien Lespiau]

Follow up of:
 http://lists.freedesktop.org/archives/dri-devel/2012-September/028278.html

Let's try again! This time, intead of a magic connector property to select if
we want to return more modeinfo flags to userspace, this series introduces a
new SET_CAP ioctl.

So the flow goes as following:
  1/ the DRM client (limited to the DRM master) can notify it knows about those
     flags through SET_CAP
  2/ we parse the HDMI CEA vendor blog for 3D_Present and patch the mandatory
     modes with the layouts they support (in ->flags)
  3/ we can then handle a modeset with one (and only one) of those stereo 3D
     layout set on the mode. This includes writing a vendor infoframe with
     the information about the layout we're sending

libdrm patches will follow with a new drmSetCap() function and the stereo 3D
flags.

Testing has been done with intel-gpu-tools' testdisplay that can now compose a
left and right image for the "top and bottom", "side by side half" and "frame
packing" stereo 3d layouts (patches posted on intel-gfx).

[PATCH 1/9] 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.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 include/uapi/drm/drm.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 272580c..4b683f9 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -611,6 +611,16 @@ 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_CAP_TIMESTAMP_MONOTONIC 0x6
+
+#define DRM_PRIME_CAP_IMPORT 0x1
+#define DRM_PRIME_CAP_EXPORT 0x2
+
 /** DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
 	__u64 capability;
@@ -774,16 +784,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 2/9] drm: Sort userspace typedefs by ioctl number

[Damien Lespiau]

This way, it's easier to catch ioctl structures that are not yet
typedef'ed and provide a more consistent API.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 include/uapi/drm/drm.h | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 4b683f9..b8604d2 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -786,47 +786,50 @@ struct drm_event_vblank {
 
 /* typedef area */
 #ifndef __KERNEL__
-typedef struct drm_clip_rect drm_clip_rect_t;
-typedef struct drm_drawable_info drm_drawable_info_t;
-typedef struct drm_tex_region drm_tex_region_t;
-typedef struct drm_hw_lock drm_hw_lock_t;
+
+/* Please keep this list sorted by ioctl number */
 typedef struct drm_version drm_version_t;
 typedef struct drm_unique drm_unique_t;
-typedef struct drm_list drm_list_t;
-typedef struct drm_block drm_block_t;
-typedef struct drm_control drm_control_t;
+typedef struct drm_auth drm_auth_t;
+typedef struct drm_irq_busid drm_irq_busid_t;
 typedef enum drm_map_type drm_map_type_t;
 typedef enum drm_map_flags drm_map_flags_t;
-typedef struct drm_ctx_priv_map drm_ctx_priv_map_t;
 typedef struct drm_map drm_map_t;
 typedef struct drm_client drm_client_t;
 typedef enum drm_stat_type drm_stat_type_t;
 typedef struct drm_stats drm_stats_t;
-typedef enum drm_lock_flags drm_lock_flags_t;
-typedef struct drm_lock drm_lock_t;
-typedef enum drm_dma_flags drm_dma_flags_t;
+typedef struct drm_set_version drm_set_version_t;
+typedef struct drm_block drm_block_t;
+typedef struct drm_control drm_control_t;
 typedef struct drm_buf_desc drm_buf_desc_t;
 typedef struct drm_buf_info drm_buf_info_t;
-typedef struct drm_buf_free drm_buf_free_t;
 typedef struct drm_buf_pub drm_buf_pub_t;
 typedef struct drm_buf_map drm_buf_map_t;
+typedef struct drm_buf_free drm_buf_free_t;
+typedef enum drm_dma_flags drm_dma_flags_t;
 typedef struct drm_dma drm_dma_t;
-typedef union drm_wait_vblank drm_wait_vblank_t;
-typedef struct drm_agp_mode drm_agp_mode_t;
+typedef struct drm_ctx_priv_map drm_ctx_priv_map_t;
 typedef enum drm_ctx_flags drm_ctx_flags_t;
 typedef struct drm_ctx drm_ctx_t;
 typedef struct drm_ctx_res drm_ctx_res_t;
 typedef struct drm_draw drm_draw_t;
-typedef struct drm_update_draw drm_update_draw_t;
-typedef struct drm_auth drm_auth_t;
-typedef struct drm_irq_busid drm_irq_busid_t;
-typedef enum drm_vblank_seq_type drm_vblank_seq_type_t;
-
+typedef enum drm_lock_flags drm_lock_flags_t;
+typedef struct drm_lock drm_lock_t;
+typedef struct drm_agp_mode drm_agp_mode_t;
+typedef struct drm_agp_info drm_agp_info_t;
 typedef struct drm_agp_buffer drm_agp_buffer_t;
 typedef struct drm_agp_binding drm_agp_binding_t;
-typedef struct drm_agp_info drm_agp_info_t;
 typedef struct drm_scatter_gather drm_scatter_gather_t;
-typedef struct drm_set_version drm_set_version_t;
+typedef enum drm_vblank_seq_type drm_vblank_seq_type_t;
+typedef union drm_wait_vblank drm_wait_vblank_t;
+typedef struct drm_update_draw drm_update_draw_t;
+
+typedef struct drm_clip_rect drm_clip_rect_t;
+typedef struct drm_drawable_info drm_drawable_info_t;
+typedef struct drm_tex_region drm_tex_region_t;
+typedef struct drm_hw_lock drm_hw_lock_t;
+typedef struct drm_list drm_list_t;
+
 #endif
 
 #endif
-- 
1.8.3.1

[PATCH 3/9] drm: Make sure every ioctl structure has a typedef

[Damien Lespiau]

Just for consistency.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 include/uapi/drm/drm.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index b8604d2..0430fab 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -799,6 +799,11 @@ typedef struct drm_client drm_client_t;
 typedef enum drm_stat_type drm_stat_type_t;
 typedef struct drm_stats drm_stats_t;
 typedef struct drm_set_version drm_set_version_t;
+typedef struct drm_modeset_ctl drm_modeset_ctl_t;
+typedef struct drm_gem_close drm_gem_close_t;
+typedef struct drm_gem_flink drm_gem_flink_t;
+typedef struct drm_gem_open drm_gem_open_t;
+typedef struct drm_get_cap drm_get_cap_t;
 typedef struct drm_block drm_block_t;
 typedef struct drm_control drm_control_t;
 typedef struct drm_buf_desc drm_buf_desc_t;
@@ -815,6 +820,7 @@ typedef struct drm_ctx_res drm_ctx_res_t;
 typedef struct drm_draw drm_draw_t;
 typedef enum drm_lock_flags drm_lock_flags_t;
 typedef struct drm_lock drm_lock_t;
+typedef struct drm_prime_handle drm_prime_handle_t;
 typedef struct drm_agp_mode drm_agp_mode_t;
 typedef struct drm_agp_info drm_agp_info_t;
 typedef struct drm_agp_buffer drm_agp_buffer_t;
@@ -823,6 +829,29 @@ typedef struct drm_scatter_gather drm_scatter_gather_t;
 typedef enum drm_vblank_seq_type drm_vblank_seq_type_t;
 typedef union drm_wait_vblank drm_wait_vblank_t;
 typedef struct drm_update_draw drm_update_draw_t;
+typedef struct drm_mode_card_res drm_mode_card_res_t;
+typedef struct drm_mode_crtc drm_mode_crtc_t;
+typedef struct drm_mode_cursor drm_mode_cursor_t;
+typedef struct drm_mode_crtc_lut drm_mode_crtc_lut_t;
+typedef struct drm_mode_get_encoder drm_mode_get_encoder_t;
+typedef struct drm_mode_get_connector drm_mode_get_connector_t;
+typedef struct drm_mode_mode_cmd drm_mode_mode_cmd_t;
+typedef struct drm_mode_get_property drm_mode_get_property_t;
+typedef struct drm_mode_connector_set_property drm_mode_connector_set_property_t;
+typedef struct drm_mode_get_blob drm_mode_get_blob_t;
+typedef struct drm_mode_fb_cmd drm_mode_fb_cmd_t;
+typedef struct drm_mode_crtc_page_flip drm_mode_crtc_page_flip_t;
+typedef struct drm_mode_fb_dirty_cmd drm_mode_fb_dirty_cmd_t;
+typedef struct drm_mode_create_dumb drm_mode_create_dumb_t;
+typedef struct drm_mode_map_dumb drm_mode_map_dumb_t;
+typedef struct drm_mode_destroy_dumb drm_mode_destroy_dumb_t;
+typedef struct drm_mode_get_plane_res drm_mode_get_plane_res_t;
+typedef struct drm_mode_get_plane drm_mode_get_plane_t;
+typedef struct drm_mode_set_plane drm_mode_set_plane_t;
+typedef struct drm_mode_fb_cmd2 drm_mode_fb_cmd2_t;
+typedef struct drm_mode_obj_get_properties drm_mode_obj_get_properties_t;
+typedef struct drm_mode_obj_set_property drm_mode_obj_set_property_t;
+typedef struct drm_mode_cursor2 drm_mode_cursor2_t;
 
 typedef struct drm_clip_rect drm_clip_rect_t;
 typedef struct drm_drawable_info drm_drawable_info_t;
-- 
1.8.3.1

[PATCH 4/9] drm: Add a SET_CAP ioctl

[Damien Lespiau]

This ioctl can be used to turn some knobs in a DRM driver. This is the
counterpart of GET_CAP and serves a similar role than the various
SETPARAM ioctls that are driver specific, but for DRM core.

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

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 288da3d..7fce2fb 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_CAP, drm_setcap, DRM_MASTER),
 	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..e471cd9 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -300,6 +300,14 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 }
 
 /**
+ * Set device/driver capabilities
+ */
+int drm_setcap(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..b9c321b 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_setcap(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 0430fab..d400e6f 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -627,6 +627,12 @@ struct drm_get_cap {
 	__u64 value;
 };
 
+/** DRM_IOCTL_SET_CAP ioctl argument type */
+struct drm_set_cap {
+	__u64 capability;
+	__u64 value;
+};
+
 #define DRM_CLOEXEC O_CLOEXEC
 struct drm_prime_handle {
 	__u32 handle;
@@ -659,6 +665,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_CAP		DRM_IOW( 0x0d, struct drm_set_cap)
 
 #define DRM_IOCTL_SET_UNIQUE		DRM_IOW( 0x10, struct drm_unique)
 #define DRM_IOCTL_AUTH_MAGIC		DRM_IOW( 0x11, struct drm_auth)
@@ -804,6 +811,7 @@ typedef struct drm_gem_close drm_gem_close_t;
 typedef struct drm_gem_flink drm_gem_flink_t;
 typedef struct drm_gem_open drm_gem_open_t;
 typedef struct drm_get_cap drm_get_cap_t;
+typedef struct drm_set_cap drm_set_cap_t;
 typedef struct drm_block drm_block_t;
 typedef struct drm_control drm_control_t;
 typedef struct drm_buf_desc drm_buf_desc_t;
-- 
1.8.3.1

[PATCH 5/9] 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).

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

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f660ac5..4e3ce16 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -125,6 +125,15 @@ enum drm_mode_status {
 
 #define CRTC_INTERLACE_HALVE_V 0x1 /* halve V values for interlacing */
 
+#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)
+
 struct drm_display_mode {
 	/* Header */
 	struct list_head head;
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 6/9] drm: Add a DRM_CAP_STEREO_3D capability for SET_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 3D flags from the user mode
modeinfo structure 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.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a691764..ff9646f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1257,12 +1257,14 @@ EXPORT_SYMBOL(drm_mode_group_init_legacy_group);
  * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo
  * @out: drm_mode_modeinfo struct to return to the user
  * @in: drm_display_mode to use
+ * @file_priv: drm file from the ioctl call
  *
  * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to
  * the user.
  */
 static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
-				      const struct drm_display_mode *in)
+				      const struct drm_display_mode *in,
+				      const struct drm_file *file_priv)
 {
 	WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX ||
 	     in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX ||
@@ -1287,6 +1289,13 @@ static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
 	out->type = in->type;
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
+
+	/*
+	 * If user-space hasn't configured the driver to expose the stereo 3D
+	 * flags, clear them.
+	 */
+	if (!file_priv->stereo_allowed)
+		out->flags &= ~DRM_MODE_FLAG_3D_MASK;
 }
 
 /**
@@ -1556,7 +1565,8 @@ int drm_mode_getcrtc(struct drm_device *dev,
 
 	if (crtc->enabled) {
 
-		drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->mode);
+		drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->mode,
+					  file_priv);
 		crtc_resp->mode_valid = 1;
 
 	} else {
@@ -1655,7 +1665,7 @@ 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) {
-			drm_crtc_convert_to_umode(&u_mode, mode);
+			drm_crtc_convert_to_umode(&u_mode, mode, file_priv);
 			if (copy_to_user(mode_ptr + copied,
 					 &u_mode, sizeof(u_mode))) {
 				ret = -EFAULT;
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index e471cd9..a716641 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -304,7 +304,19 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
  */
 int drm_setcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 {
-	return -EINVAL;
+	struct drm_set_cap *req = data;
+
+	switch (req->capability) {
+	case DRM_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 b9c321b..0df654c 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 d400e6f..23922b4 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -627,6 +627,15 @@ struct drm_get_cap {
 	__u64 value;
 };
 
+/**
+ * DRM_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_CAP_STEREO_3D	1
+
 /** DRM_IOCTL_SET_CAP ioctl argument type */
 struct drm_set_cap {
 	__u64 capability;
-- 
1.8.3.1

[PATCH 7/9] 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 | 75 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 68 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a207cc3..9d9881b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2550,13 +2550,60 @@ do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
 	return modes;
 }
 
+struct s3d_mandatory_mode {
+	int width, height, freq;
+	unsigned int interlace_flag, formats;
+};
+
+static const struct s3d_mandatory_mode s3d_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 match_s3d_mandatory_mode(const struct drm_display_mode *mode,
+				     const struct s3d_mandatory_mode *s3d_mode)
+{
+	unsigned int interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
+
+	return mode->hdisplay == s3d_mode->width &&
+	       mode->vdisplay == s3d_mode->height &&
+	       interlaced == s3d_mode->interlace_flag &&
+	       drm_mode_vrefresh(mode) == s3d_mode->freq;
+}
+
+static void hdmi_patch_stereo_mode(struct drm_display_mode *mode)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(s3d_mandatory_modes); i++)
+		if (match_s3d_mandatory_mode(mode, &s3d_mandatory_modes[i]))
+			mode->flags |= s3d_mandatory_modes[i].formats;
+}
+
+static void hdmi_patch_stereo_modes(struct drm_connector *connector)
+{
+	struct drm_display_mode *mode;
+
+	list_for_each_entry(mode, &connector->probed_modes, head)
+		hdmi_patch_stereo_mode(mode);
+}
+
 /*
  * 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 flags to already added modes.
  */
 static int
 do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
@@ -2582,10 +2629,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))
+		hdmi_patch_stereo_modes(connector);
+
+	offset++;
 	vic_len = db[8 + offset] >> 5;
 
 	for (i = 0; i < vic_len && len >= (9 + offset + i); i++) {
@@ -2665,8 +2717,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 +2733,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 8/9] 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 ff9646f..f53d199 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1315,6 +1315,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 9/9] 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 9d9881b..8a1ae56 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3325,6 +3325,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
@@ -3342,20 +3354,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

Re: Stereo 3D v3

[Chris Wilson]

On Fri, Sep 06, 2013 at 07:57:16PM +0100, Damien Lespiau wrote:
> Follow up of:
>  http://lists.freedesktop.org/archives/dri-devel/2012-September/028278.html
> 
> Let's try again! This time, intead of a magic connector property to select if
> we want to return more modeinfo flags to userspace, this series introduces a
> new SET_CAP ioctl.
> 
> So the flow goes as following:
>   1/ the DRM client (limited to the DRM master) can notify it knows about those
>      flags through SET_CAP

Is this capability dropped along with a change in master then?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/9] drm: Make sure every ioctl structure has a typedef

[Daniel Vetter]

On Fri, Sep 06, 2013 at 07:57:19PM +0100, Damien Lespiau wrote:
> Just for consistency.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Afaik kernel coding style is to echew typdefs for normal structures as
much as possible. The only exception is for truly opaque types used in
abstractions, like dma_addr_t or pid_t.

All the typedefs we still have here go back to the old days of a drm core
shared between *bsd and linux. Since that's long gone they imo should all
die, but certainly we shouldn't add new ones.

Aside: My patcha apply script will also bitch about new usages of
drm_i915_private_t ;-)

Cheers, Daniel
> ---
>  include/uapi/drm/drm.h | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b8604d2..0430fab 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -799,6 +799,11 @@ typedef struct drm_client drm_client_t;
>  typedef enum drm_stat_type drm_stat_type_t;
>  typedef struct drm_stats drm_stats_t;
>  typedef struct drm_set_version drm_set_version_t;
> +typedef struct drm_modeset_ctl drm_modeset_ctl_t;
> +typedef struct drm_gem_close drm_gem_close_t;
> +typedef struct drm_gem_flink drm_gem_flink_t;
> +typedef struct drm_gem_open drm_gem_open_t;
> +typedef struct drm_get_cap drm_get_cap_t;
>  typedef struct drm_block drm_block_t;
>  typedef struct drm_control drm_control_t;
>  typedef struct drm_buf_desc drm_buf_desc_t;
> @@ -815,6 +820,7 @@ typedef struct drm_ctx_res drm_ctx_res_t;
>  typedef struct drm_draw drm_draw_t;
>  typedef enum drm_lock_flags drm_lock_flags_t;
>  typedef struct drm_lock drm_lock_t;
> +typedef struct drm_prime_handle drm_prime_handle_t;
>  typedef struct drm_agp_mode drm_agp_mode_t;
>  typedef struct drm_agp_info drm_agp_info_t;
>  typedef struct drm_agp_buffer drm_agp_buffer_t;
> @@ -823,6 +829,29 @@ typedef struct drm_scatter_gather drm_scatter_gather_t;
>  typedef enum drm_vblank_seq_type drm_vblank_seq_type_t;
>  typedef union drm_wait_vblank drm_wait_vblank_t;
>  typedef struct drm_update_draw drm_update_draw_t;
> +typedef struct drm_mode_card_res drm_mode_card_res_t;
> +typedef struct drm_mode_crtc drm_mode_crtc_t;
> +typedef struct drm_mode_cursor drm_mode_cursor_t;
> +typedef struct drm_mode_crtc_lut drm_mode_crtc_lut_t;
> +typedef struct drm_mode_get_encoder drm_mode_get_encoder_t;
> +typedef struct drm_mode_get_connector drm_mode_get_connector_t;
> +typedef struct drm_mode_mode_cmd drm_mode_mode_cmd_t;
> +typedef struct drm_mode_get_property drm_mode_get_property_t;
> +typedef struct drm_mode_connector_set_property drm_mode_connector_set_property_t;
> +typedef struct drm_mode_get_blob drm_mode_get_blob_t;
> +typedef struct drm_mode_fb_cmd drm_mode_fb_cmd_t;
> +typedef struct drm_mode_crtc_page_flip drm_mode_crtc_page_flip_t;
> +typedef struct drm_mode_fb_dirty_cmd drm_mode_fb_dirty_cmd_t;
> +typedef struct drm_mode_create_dumb drm_mode_create_dumb_t;
> +typedef struct drm_mode_map_dumb drm_mode_map_dumb_t;
> +typedef struct drm_mode_destroy_dumb drm_mode_destroy_dumb_t;
> +typedef struct drm_mode_get_plane_res drm_mode_get_plane_res_t;
> +typedef struct drm_mode_get_plane drm_mode_get_plane_t;
> +typedef struct drm_mode_set_plane drm_mode_set_plane_t;
> +typedef struct drm_mode_fb_cmd2 drm_mode_fb_cmd2_t;
> +typedef struct drm_mode_obj_get_properties drm_mode_obj_get_properties_t;
> +typedef struct drm_mode_obj_set_property drm_mode_obj_set_property_t;
> +typedef struct drm_mode_cursor2 drm_mode_cursor2_t;
>  
>  typedef struct drm_clip_rect drm_clip_rect_t;
>  typedef struct drm_drawable_info drm_drawable_info_t;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: Stereo 3D v3

[Daniel Vetter]

On Fri, Sep 6, 2013 at 9:11 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Sep 06, 2013 at 07:57:16PM +0100, Damien Lespiau wrote:
>> Follow up of:
>>  http://lists.freedesktop.org/archives/dri-devel/2012-September/028278.html
>>
>> Let's try again! This time, intead of a magic connector property to select if
>> we want to return more modeinfo flags to userspace, this series introduces a
>> new SET_CAP ioctl.
>>
>> So the flow goes as following:
>>   1/ the DRM client (limited to the DRM master) can notify it knows about those
>>      flags through SET_CAP
>
> Is this capability dropped along with a change in master then?

Yeah, I think it would make sense to store this flag in the master
structure. But David Herrmann has some big plans for the drm master
stuff, so would be good to have his opinion on this. David?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/9] drm: Move the GET_CAP macros next to the corresponding ioctl structure

[David Herrmann]

Hi

On Fri, Sep 6, 2013 at 8:57 PM, Damien Lespiau <damien.lespiau@intel.com> wrote:
> 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.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  include/uapi/drm/drm.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 272580c..4b683f9 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -611,6 +611,16 @@ 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_CAP_TIMESTAMP_MONOTONIC 0x6
> +
> +#define DRM_PRIME_CAP_IMPORT 0x1
> +#define DRM_PRIME_CAP_EXPORT 0x2
> +

Makes sense. Would you mind also adding tabs between the name and
literal? Makes it much more readable. And I think it's fine to use
moves to fix coding-style issues.

Thanks
David

>  /** DRM_IOCTL_GET_CAP ioctl argument type */
>  struct drm_get_cap {
>         __u64 capability;
> @@ -774,16 +784,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
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Stereo 3D v3

[David Herrmann]

Hi

On Sun, Sep 8, 2013 at 1:59 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Sep 6, 2013 at 9:11 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> On Fri, Sep 06, 2013 at 07:57:16PM +0100, Damien Lespiau wrote:
>>> Follow up of:
>>>  http://lists.freedesktop.org/archives/dri-devel/2012-September/028278.html
>>>
>>> Let's try again! This time, intead of a magic connector property to select if
>>> we want to return more modeinfo flags to userspace, this series introduces a
>>> new SET_CAP ioctl.
>>>
>>> So the flow goes as following:
>>>   1/ the DRM client (limited to the DRM master) can notify it knows about those
>>>      flags through SET_CAP
>>
>> Is this capability dropped along with a change in master then?
>
> Yeah, I think it would make sense to store this flag in the master
> structure. But David Herrmann has some big plans for the drm master
> stuff, so would be good to have his opinion on this. David?

The series implements SET_CAP as a per _file_ capability set, not per
master. I like it this way. Note that with SET_VERSION, we already
have a per _master_ capability set. Compared to SET_CAP it only allows
incremental capability changes, but that's fine I think.

However, the problem with per-master capabilities (SET_VERSION) is
that we currently have no way to control which master a
graphics-server gets assigned to. If it's started in background, it
will get the same master as the foreground compositor. Therefore, we
don't want per-master client-capabilities. It's wrong and breaks
existing setups (same as SET_VERSION, and everyone knows that). I also
don't see a reason to bind capabilities to a master object.

SET_CAP describes what the *calling client* understands and can work
with. And this is logically bound to drm_file (as it represents a
client). On the other hand, GET_CAP describes what the *device*
understands and provides. This is obviously bound to a "drm_device". A
"drm_master" object allows to split GET_CAP capabilities and resources
across multiple logical master objects. But these resemble a
drm_device much more than a drm_file.

So no, this capability is not dropped with a change in master. It's
independent of the active/bound master. It just describes what a
client sees or not sees.

Regarding my drm_master plans: I don't plan on changing the concept, I
just plan on adding ioctls to control master objects and allow
*multiple* active masters per device, but each with different
resources.
Just as a hint: with the current setup, we only have one master.
Seriously, add debug-prints to drm_master_create() and watch. The
problem is, chances are pretty low that a compositors starts while no
master is active. At least on my system.. here all compositors share a
master-object.

Comments?
David

Re: [PATCH 6/9] drm: Add a DRM_CAP_STEREO_3D capability for SET_CAP ioctl

[David Herrmann]

Hi

On Fri, Sep 6, 2013 at 8:57 PM, Damien Lespiau <damien.lespiau@intel.com> wrote:
> 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 3D flags from the user mode
> modeinfo structure 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.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c  | 16 +++++++++++++---
>  drivers/gpu/drm/drm_ioctl.c | 14 +++++++++++++-
>  include/drm/drmP.h          |  3 +++
>  include/uapi/drm/drm.h      |  9 +++++++++
>  4 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index a691764..ff9646f 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1257,12 +1257,14 @@ EXPORT_SYMBOL(drm_mode_group_init_legacy_group);
>   * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo
>   * @out: drm_mode_modeinfo struct to return to the user
>   * @in: drm_display_mode to use
> + * @file_priv: drm file from the ioctl call
>   *
>   * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to
>   * the user.
>   */
>  static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
> -                                     const struct drm_display_mode *in)
> +                                     const struct drm_display_mode *in,
> +                                     const struct drm_file *file_priv)
>  {
>         WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX ||
>              in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX ||
> @@ -1287,6 +1289,13 @@ static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
>         out->type = in->type;
>         strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>         out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> +
> +       /*
> +        * If user-space hasn't configured the driver to expose the stereo 3D
> +        * flags, clear them.
> +        */
> +       if (!file_priv->stereo_allowed)
> +               out->flags &= ~DRM_MODE_FLAG_3D_MASK;

So just to be clear: Whenever a mode is present with 3D flags, it is
also a valid non-3D mode? Is this guaranteed? Don't you want to add a
mode twice, once without 3D flags and once with? You could then just
skip all 3D modes for clients that don't support it.

I have no idea how the 3D flags are specified, just want to go sure
this doesn't break. So whenever a mode with 3D flags is present on a
device, it can be set by a client dropping the 3D flags and it will be
a valid mono-mode?

Cheers
David

>  }
>
>  /**
> @@ -1556,7 +1565,8 @@ int drm_mode_getcrtc(struct drm_device *dev,
>
>         if (crtc->enabled) {
>
> -               drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->mode);
> +               drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->mode,
> +                                         file_priv);
>                 crtc_resp->mode_valid = 1;
>
>         } else {
> @@ -1655,7 +1665,7 @@ 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) {
> -                       drm_crtc_convert_to_umode(&u_mode, mode);
> +                       drm_crtc_convert_to_umode(&u_mode, mode, file_priv);
>                         if (copy_to_user(mode_ptr + copied,
>                                          &u_mode, sizeof(u_mode))) {
>                                 ret = -EFAULT;
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index e471cd9..a716641 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -304,7 +304,19 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>   */
>  int drm_setcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  {
> -       return -EINVAL;
> +       struct drm_set_cap *req = data;
> +
> +       switch (req->capability) {
> +       case DRM_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 b9c321b..0df654c 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 d400e6f..23922b4 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -627,6 +627,15 @@ struct drm_get_cap {
>         __u64 value;
>  };
>
> +/**
> + * DRM_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_CAP_STEREO_3D      1
> +
>  /** DRM_IOCTL_SET_CAP ioctl argument type */
>  struct drm_set_cap {
>         __u64 capability;
> --
> 1.8.3.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/9] drm: Add a DRM_CAP_STEREO_3D capability for SET_CAP ioctl

[Damien Lespiau]

On Sun, Sep 08, 2013 at 03:50:29PM +0200, David Herrmann wrote:
> Hi
> 
> On Fri, Sep 6, 2013 at 8:57 PM, Damien Lespiau <damien.lespiau@intel.com> wrote:
> > 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 3D flags from the user mode
> > modeinfo structure 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.
> >
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c  | 16 +++++++++++++---
> >  drivers/gpu/drm/drm_ioctl.c | 14 +++++++++++++-
> >  include/drm/drmP.h          |  3 +++
> >  include/uapi/drm/drm.h      |  9 +++++++++
> >  4 files changed, 38 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index a691764..ff9646f 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -1257,12 +1257,14 @@ EXPORT_SYMBOL(drm_mode_group_init_legacy_group);
> >   * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo
> >   * @out: drm_mode_modeinfo struct to return to the user
> >   * @in: drm_display_mode to use
> > + * @file_priv: drm file from the ioctl call
> >   *
> >   * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to
> >   * the user.
> >   */
> >  static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
> > -                                     const struct drm_display_mode *in)
> > +                                     const struct drm_display_mode *in,
> > +                                     const struct drm_file *file_priv)
> >  {
> >         WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX ||
> >              in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX ||
> > @@ -1287,6 +1289,13 @@ static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
> >         out->type = in->type;
> >         strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> >         out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> > +
> > +       /*
> > +        * If user-space hasn't configured the driver to expose the stereo 3D
> > +        * flags, clear them.
> > +        */
> > +       if (!file_priv->stereo_allowed)
> > +               out->flags &= ~DRM_MODE_FLAG_3D_MASK;
> 
> So just to be clear: Whenever a mode is present with 3D flags, it is
> also a valid non-3D mode? Is this guaranteed? Don't you want to add a
> mode twice, once without 3D flags and once with? You could then just
> skip all 3D modes for clients that don't support it.
> 
> I have no idea how the 3D flags are specified, just want to go sure
> this doesn't break. So whenever a mode with 3D flags is present on a
> device, it can be set by a client dropping the 3D flags and it will be
> a valid mono-mode?

So yes, that's exactly what happens right now. I was actually thinking
about a v4 of the series with what you said in the first paragraph: just
add the 3D modes to the list, one mode per 3D layout, and drop those
modes from the list given back to userspace when the stereo_allowed
isn't set. That seems quite a bit better than the convoluted approach
here.

-- 
Damien

Re: Stereo 3D v3

[Damien Lespiau]

On Sun, Sep 08, 2013 at 03:46:43PM +0200, David Herrmann wrote:
> The series implements SET_CAP as a per _file_ capability set, not per
> master. I like it this way. Note that with SET_VERSION, we already
> have a per _master_ capability set. Compared to SET_CAP it only allows
> incremental capability changes, but that's fine I think.
> 
> However, the problem with per-master capabilities (SET_VERSION) is
> that we currently have no way to control which master a
> graphics-server gets assigned to. If it's started in background, it
> will get the same master as the foreground compositor. Therefore, we
> don't want per-master client-capabilities. It's wrong and breaks
> existing setups (same as SET_VERSION, and everyone knows that). I also
> don't see a reason to bind capabilities to a master object.
> 
> SET_CAP describes what the *calling client* understands and can work
> with. And this is logically bound to drm_file (as it represents a
> client). On the other hand, GET_CAP describes what the *device*
> understands and provides. This is obviously bound to a "drm_device". A
> "drm_master" object allows to split GET_CAP capabilities and resources
> across multiple logical master objects. But these resemble a
> drm_device much more than a drm_file.
> 
> So no, this capability is not dropped with a change in master. It's
> independent of the active/bound master. It just describes what a
> client sees or not sees.

Right, that sums it up. Note that while I've made stereo_allowed a per
fd thing (which is what I wanted in that case, alter the reality viewed
by the process opening the file), SET_CAP itself it marked as master
only. This can be changed in the future to provide per-cap access
restrictions if needed.

-- 
Damien

Re: [PATCH 3/9] drm: Make sure every ioctl structure has a typedef

[Damien Lespiau]

On Sun, Sep 08, 2013 at 01:58:29PM +0200, Daniel Vetter wrote:
> On Fri, Sep 06, 2013 at 07:57:19PM +0100, Damien Lespiau wrote:
> > Just for consistency.
> > 
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> Afaik kernel coding style is to echew typdefs for normal structures as
> much as possible. The only exception is for truly opaque types used in
> abstractions, like dma_addr_t or pid_t.
> 
> All the typedefs we still have here go back to the old days of a drm core
> shared between *bsd and linux. Since that's long gone they imo should all
> die, but certainly we shouldn't add new ones.

I figured that since we where talking about user space API, the kernel
rules wouldn't apply there and we could have some consistency, but I
certainly can just drop those patches.

-- 
Damien

Re: [PATCH 3/9] drm: Make sure every ioctl structure has a typedef

[Daniel Vetter]

On Sun, Sep 8, 2013 at 9:36 PM, Damien Lespiau <damien.lespiau@intel.com> wrote:
> On Sun, Sep 08, 2013 at 01:58:29PM +0200, Daniel Vetter wrote:
>> On Fri, Sep 06, 2013 at 07:57:19PM +0100, Damien Lespiau wrote:
>> > Just for consistency.
>> >
>> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>>
>> Afaik kernel coding style is to echew typdefs for normal structures as
>> much as possible. The only exception is for truly opaque types used in
>> abstractions, like dma_addr_t or pid_t.
>>
>> All the typedefs we still have here go back to the old days of a drm core
>> shared between *bsd and linux. Since that's long gone they imo should all
>> die, but certainly we shouldn't add new ones.
>
> I figured that since we where talking about user space API, the kernel
> rules wouldn't apply there and we could have some consistency, but I
> certainly can just drop those patches.

I've thought typedefs are even frowned upon in the ioctl abi -
magically changing sized types cause pain since they need 32bit compat
wrappers. And otherwise I haven't really seen them much for structures
...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: Stereo 3D v3

[Daniel Vetter]

On Sun, Sep 8, 2013 at 5:03 PM, Damien Lespiau <damien.lespiau@intel.com> wrote:
> On Sun, Sep 08, 2013 at 03:46:43PM +0200, David Herrmann wrote:
>> The series implements SET_CAP as a per _file_ capability set, not per
>> master. I like it this way. Note that with SET_VERSION, we already
>> have a per _master_ capability set. Compared to SET_CAP it only allows
>> incremental capability changes, but that's fine I think.
>>
>> However, the problem with per-master capabilities (SET_VERSION) is
>> that we currently have no way to control which master a
>> graphics-server gets assigned to. If it's started in background, it
>> will get the same master as the foreground compositor. Therefore, we
>> don't want per-master client-capabilities. It's wrong and breaks
>> existing setups (same as SET_VERSION, and everyone knows that). I also
>> don't see a reason to bind capabilities to a master object.
>>
>> SET_CAP describes what the *calling client* understands and can work
>> with. And this is logically bound to drm_file (as it represents a
>> client). On the other hand, GET_CAP describes what the *device*
>> understands and provides. This is obviously bound to a "drm_device". A
>> "drm_master" object allows to split GET_CAP capabilities and resources
>> across multiple logical master objects. But these resemble a
>> drm_device much more than a drm_file.
>>
>> So no, this capability is not dropped with a change in master. It's
>> independent of the active/bound master. It just describes what a
>> client sees or not sees.
>
> Right, that sums it up. Note that while I've made stereo_allowed a per
> fd thing (which is what I wanted in that case, alter the reality viewed
> by the process opening the file), SET_CAP itself it marked as master
> only. This can be changed in the future to provide per-cap access
> restrictions if needed.

Ok, I admit defaut, master doesn't make sense here ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: Stereo 3D v3

[Chris Wilson]

On Sun, Sep 08, 2013 at 04:03:52PM +0100, Damien Lespiau wrote:
> On Sun, Sep 08, 2013 at 03:46:43PM +0200, David Herrmann wrote:
> > The series implements SET_CAP as a per _file_ capability set, not per
> > master. I like it this way. Note that with SET_VERSION, we already
> > have a per _master_ capability set. Compared to SET_CAP it only allows
> > incremental capability changes, but that's fine I think.
> > 
> > However, the problem with per-master capabilities (SET_VERSION) is
> > that we currently have no way to control which master a
> > graphics-server gets assigned to. If it's started in background, it
> > will get the same master as the foreground compositor. Therefore, we
> > don't want per-master client-capabilities. It's wrong and breaks
> > existing setups (same as SET_VERSION, and everyone knows that). I also
> > don't see a reason to bind capabilities to a master object.
> > 
> > SET_CAP describes what the *calling client* understands and can work
> > with. And this is logically bound to drm_file (as it represents a
> > client). On the other hand, GET_CAP describes what the *device*
> > understands and provides. This is obviously bound to a "drm_device". A
> > "drm_master" object allows to split GET_CAP capabilities and resources
> > across multiple logical master objects. But these resemble a
> > drm_device much more than a drm_file.
> > 
> > So no, this capability is not dropped with a change in master. It's
> > independent of the active/bound master. It just describes what a
> > client sees or not sees.
> 
> Right, that sums it up. Note that while I've made stereo_allowed a per
> fd thing (which is what I wanted in that case, alter the reality viewed
> by the process opening the file), SET_CAP itself it marked as master
> only. This can be changed in the future to provide per-cap access
> restrictions if needed.

This could be renamed to SET_CLIENT_CAP and also drop the master
requirement. (That some capabilities only affect master ioctls is
irrelevant I think, as the client will be master at that time.)
That would reduce the confusion between the device caps and the session
caps.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/9] drm: Add a DRM_CAP_STEREO_3D capability for SET_CAP ioctl

[Joakim Plate]

David Herrmann <dh.herrmann <at> gmail.com> writes:

> 
> So just to be clear: Whenever a mode is present with 3D flags, it is
> also a valid non-3D mode? Is this guaranteed? 
> 

Well.. Some HDTV's will when they receive a frame packed mode (1080*2+45=2205 
pixels high) . Display just the top part. The bottom part of that is not on 
screen.

So while it will not display it as 3d, it will discard half of the image.

/Joakim

Re: [PATCH 7/9] drm/edid: Expose mandatory stereo modes for HDMI sinks

[Joakim Plate]

Damien Lespiau <damien.lespiau <at> intel.com> writes:

> +static const struct s3d_mandatory_mode s3d_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 }
> +};


I may be missing something here... But..

The frame packed modes are much higher in pixels than this and include frame 
packing.
1080*2+45=2050
720*2+30=1470

Unless you intend to hide the left/right split in mesa or other place, we 
need to get the ability to render to both fields somehow.

Either as the full 2050 pixels high or at 1080*2 and the driver adds the 
blanking.

Also, some logic aught to indicate pixel aspect ratio for the modes since 
they are non square for the half res modes.

/Joakim

Re: [PATCH 7/9] drm/edid: Expose mandatory stereo modes for HDMI sinks

[Daniel Vetter]

On Fri, Sep 13, 2013 at 6:10 PM, Joakim Plate <elupus@ecce.se> wrote:
>
> Also, some logic aught to indicate pixel aspect ratio for the modes since
> they are non square for the half res modes.

Atm we completely ignore pixel aspect ratio, also for flatworld CEA
modes. So I don't think we need to concer ourselves here about this,
imo pixel aspect ratio support is orthogonal.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/9] drm/edid: Expose mandatory stereo modes for HDMI sinks

[Damien Lespiau]

On Fri, Sep 13, 2013 at 04:10:24PM +0000, Joakim Plate wrote:
> Damien Lespiau <damien.lespiau <at> intel.com> writes:
> 
> > +static const struct s3d_mandatory_mode s3d_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 }
> > +};
> 
> 
> I may be missing something here... But..

Oops, did not see your answer, please don't forget to include me in Cc:
next time and not just the list.

> The frame packed modes are much higher in pixels than this and include frame 
> packing.
> 1080*2+45=2050
> 720*2+30=1470
> 
> Unless you intend to hide the left/right split in mesa or other place, we 
> need to get the ability to render to both fields somehow.
> 
> Either as the full 2050 pixels high or at 1080*2 and the driver adds the 
> blanking.

Right, so at the moment, my proposition is that userspace is responsible for
giving us a framebuffer with the right dimensions. For instance in
intel-gpu-tools's testdisplay I have:

struct box {
        int x, y, width, height;
};

struct stereo_fb_layout {
        int fb_width, fb_height;
        struct box left, right;
};

static void stereo_fb_layout_from_mode(struct stereo_fb_layout *layout,
                                       drmModeModeInfo *mode)
{
        unsigned int format = mode->flags & DRMTEST_MODE_FLAG_3D_MASK;
        const int hdisplay = mode->hdisplay, vdisplay = mode->vdisplay;

        switch (format) {
	
	[...]

        case DRM_MODE_FLAG_3D_FRAME_PACKING:
        {
                int vactive_space = mode->vtotal - vdisplay;

                layout->fb_width = hdisplay;
                layout->fb_height = 2 * vdisplay + vactive_space;

                box_init(&layout->left,
                         0, 0, hdisplay, vdisplay);
                box_init(&layout->right,
                         0, vdisplay + vactive_space, hdisplay, vdisplay);
                break;
        }

and then adjust the timings if needed:

static void adjust_stereo_timings(drmModeModeInfo *mode)
{
        unsigned int layout = mode->flags & DRMTEST_MODE_FLAG_3D_MASK;
        uint16_t vdisplay, vactive_space;

        switch (layout) {
        case DRM_MODE_FLAG_3D_TOP_AND_BOTTOM:
        case DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF:
                return;
        case DRM_MODE_FLAG_3D_FRAME_PACKING:
                vactive_space = mode->vtotal - mode->vdisplay;
                vdisplay = mode->vdisplay;

                mode->vdisplay += vdisplay + vactive_space;
                mode->vsync_start += vdisplay + vactive_space;
                mode->vsync_end += vdisplay + vactive_space;
                mode->vtotal += vdisplay + vactive_space;
                mode->clock = (mode->vtotal * mode->htotal * mode->vrefresh) /
                              1000;
                return;
[...]

I think it makes quite a bit of sense to have the "underlying 2D mode" in the
mode structure as this 2d mode is relevant to the 3d mode:
  - HDMI stereo modes are defined based on the unerdlying 2D mode. (eg the
    extra, non-mandatory, modes in the EDID have their definitions pointing to
    CEA 2D VICs)
  - HDMI VIC infoframe: one needs to indicate the CEA VIC of this underlying 2d
    mode when setting the stereo mode.

Note that in the future, we also want to allow framebuffers with 2 distinct
buffers for right and left.

> Also, some logic aught to indicate pixel aspect ratio for the modes since 
> they are non square for the half res modes.

As Daniel already answered, aspect ration in CEA modes is an orthogonal issue
(that we want to sort out as well sooner rather than later)

-- 
Damien

Re: [PATCH 6/9] drm: Add a DRM_CAP_STEREO_3D capability for SET_CAP?ioctl

[Damien Lespiau]

On Fri, Sep 13, 2013 at 04:04:02PM +0000, Joakim Plate wrote:
> David Herrmann <dh.herrmann <at> gmail.com> writes:
> 
> > 
> > So just to be clear: Whenever a mode is present with 3D flags, it is
> > also a valid non-3D mode? Is this guaranteed? 
> > 
> 
> Well.. Some HDTV's will when they receive a frame packed mode (1080*2+45=2205 
> pixels high) . Display just the top part. The bottom part of that is not on 
> screen.

I changed this part of the API so there's no mixing the 2d mode
structure with the 3d flags. Now (v4 of the series) userspace receives
one struct drm_mode_modeinfo for the 2D mode (like before) and one
struct drm_mode_modeinfo for each 3D layout (and the rest of the timings
are from the "underlying 2D mode" that userspace can use in conjonction
with the layout bit to compute the stereo framebufer layout)

HTH,

-- 
Damien

Re: [PATCH 7/9] drm/edid: Expose mandatory stereo modes for HDMI sinks

[Ville Syrjälä]

On Mon, Sep 16, 2013 at 06:35:12PM +0100, Damien Lespiau wrote:
> On Fri, Sep 13, 2013 at 04:10:24PM +0000, Joakim Plate wrote:
> > Damien Lespiau <damien.lespiau <at> intel.com> writes:
> > 
> > > +static const struct s3d_mandatory_mode s3d_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 }
> > > +};
> > 
> > 
> > I may be missing something here... But..
> 
> Oops, did not see your answer, please don't forget to include me in Cc:
> next time and not just the list.
> 
> > The frame packed modes are much higher in pixels than this and include frame 
> > packing.
> > 1080*2+45=2050
> > 720*2+30=1470
> > 
> > Unless you intend to hide the left/right split in mesa or other place, we 
> > need to get the ability to render to both fields somehow.
> > 
> > Either as the full 2050 pixels high or at 1080*2 and the driver adds the 
> > blanking.
> 
> Right, so at the moment, my proposition is that userspace is responsible for
> giving us a framebuffer with the right dimensions. For instance in
> intel-gpu-tools's testdisplay I have:
> 
> struct box {
>         int x, y, width, height;
> };
> 
> struct stereo_fb_layout {
>         int fb_width, fb_height;
>         struct box left, right;
> };
> 
> static void stereo_fb_layout_from_mode(struct stereo_fb_layout *layout,
>                                        drmModeModeInfo *mode)
> {
>         unsigned int format = mode->flags & DRMTEST_MODE_FLAG_3D_MASK;
>         const int hdisplay = mode->hdisplay, vdisplay = mode->vdisplay;
> 
>         switch (format) {
> 	
> 	[...]
> 
>         case DRM_MODE_FLAG_3D_FRAME_PACKING:
>         {
>                 int vactive_space = mode->vtotal - vdisplay;
> 
>                 layout->fb_width = hdisplay;
>                 layout->fb_height = 2 * vdisplay + vactive_space;
> 
>                 box_init(&layout->left,
>                          0, 0, hdisplay, vdisplay);
>                 box_init(&layout->right,
>                          0, vdisplay + vactive_space, hdisplay, vdisplay);
>                 break;
>         }
> 
> and then adjust the timings if needed:
> 
> static void adjust_stereo_timings(drmModeModeInfo *mode)
> {
>         unsigned int layout = mode->flags & DRMTEST_MODE_FLAG_3D_MASK;
>         uint16_t vdisplay, vactive_space;
> 
>         switch (layout) {
>         case DRM_MODE_FLAG_3D_TOP_AND_BOTTOM:
>         case DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF:
>                 return;
>         case DRM_MODE_FLAG_3D_FRAME_PACKING:
>                 vactive_space = mode->vtotal - mode->vdisplay;
>                 vdisplay = mode->vdisplay;
> 
>                 mode->vdisplay += vdisplay + vactive_space;
>                 mode->vsync_start += vdisplay + vactive_space;
>                 mode->vsync_end += vdisplay + vactive_space;
>                 mode->vtotal += vdisplay + vactive_space;
>                 mode->clock = (mode->vtotal * mode->htotal * mode->vrefresh) /
>                               1000;

I'm wondering if we should take in the 2D mode timings and do this
adjustment in the kernel instead. Not quite sure what the pros/cons
are for doing it in userland.

Oh and now that vactive is actually part of the framebuffer as well, we
need to be more careful in the kernel how we adjust the mode. I can't
recall if we have special hardware needs wrt. the vertical timings, but
if we do we should probably review them all to avoid adjustments that
would cause issues with stereo modes.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 7/9] drm/edid: Expose mandatory stereo modes for HDMI sinks

[Daniel Vetter]

On Mon, Sep 16, 2013 at 7:35 PM, Damien Lespiau
<damien.lespiau@intel.com> wrote:
> I think it makes quite a bit of sense to have the "underlying 2D mode" in the
> mode structure as this 2d mode is relevant to the 3d mode:
>   - HDMI stereo modes are defined based on the unerdlying 2D mode. (eg the
>     extra, non-mandatory, modes in the EDID have their definitions pointing to
>     CEA 2D VICs)
>   - HDMI VIC infoframe: one needs to indicate the CEA VIC of this underlying 2d
>     mode when setting the stereo mode.
>
> Note that in the future, we also want to allow framebuffers with 2 distinct
> buffers for right and left.

I think this is the right approach. To extract a bit of common code we
could add a bunch more flags to drm_mode_set_crtcinfo so that drivers
don't need to compute the blow-up 3d modes (including the blank in
between the 2 left/right frames) themselves.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/9] drm/edid: Expose mandatory stereo modes for HDMI sinks

[Daniel Vetter]

On Mon, Sep 16, 2013 at 7:56 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> Oh and now that vactive is actually part of the framebuffer as well, we
> need to be more careful in the kernel how we adjust the mode. I can't
> recall if we have special hardware needs wrt. the vertical timings, but
> if we do we should probably review them all to avoid adjustments that
> would cause issues with stereo modes.

I think we need to either adjust the entire mode before handing it to
userspace or if that's not possible we need to reject a modeset if the
implicit vactive/vblank in the side-by-side framebuffer doesn't fit.
On X even that shouldn't be an issue since the ddx could just
duplicate this knowledge, more fun would be wayland or similar generic
display servers. I'd opt to solve this when it's a real problem though
and not before.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/9] drm/edid: Expose mandatory stereo modes for HDMI sinks

[Damien Lespiau]

On Mon, Sep 16, 2013 at 06:35:12PM +0100, Damien Lespiau wrote:
> On Fri, Sep 13, 2013 at 04:10:24PM +0000, Joakim Plate wrote:
> > Damien Lespiau <damien.lespiau <at> intel.com> writes:
> > 
> > > +static const struct s3d_mandatory_mode s3d_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 }
> > > +};
> > 
> > 
> > I may be missing something here... But..
> 
> Oops, did not see your answer, please don't forget to include me in Cc:
> next time and not just the list.
> 
> > The frame packed modes are much higher in pixels than this and include frame 
> > packing.
> > 1080*2+45=2050
> > 720*2+30=1470
> > 
> > Unless you intend to hide the left/right split in mesa or other place, we 
> > need to get the ability to render to both fields somehow.
> > 
> > Either as the full 2050 pixels high or at 1080*2 and the driver adds the 
> > blanking.
> 
> Right, so at the moment, my proposition is that userspace is responsible for
> giving us a framebuffer with the right dimensions. For instance in
> intel-gpu-tools's testdisplay I have:

[...]

> and then adjust the timings if needed:

So, actually it seems that this will change a bit. User space still
needs to compute a correct fb size. In the case of frame packing, note
that user-space also needs to add the vblank lines, because it has to
know where to place the second eye starting at vdisplay + vblank.

The kernel will be in charge of tweaking the timings if needed though,
see:
http://lists.freedesktop.org/archives/dri-devel/2013-September/045386.html

-- 
Damien
~