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
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;
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)
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. */
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;
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; }
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
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.
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.
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:
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;
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; }
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
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; };
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;
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
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.
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.
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
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.
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
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?
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 :(
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
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);
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;@@ -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;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
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.
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.
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.
dri-devel@lists.freedesktop.org
-
Damien Lespiau -
Daniel Vetter -
Ville Syrjälä