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).
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;
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
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
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;
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
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.
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
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;
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. */
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;
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
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.
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
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,
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; }
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
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
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)
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.
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
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
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
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;
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; }
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
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
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
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.
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
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
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Damien Lespiau -
Daniel Vetter -
David Herrmann -
Joakim Plate -
Ville Syrjälä