[Intel-gfx] [PATCH] drm: add support for additional stereo 3D modes

2013-10-15 Thread Thomas Wood 
On 11 October 2013 12:12, Ville Syrjälä ville.syrj...@linux.intel.com wrote:
 On Thu, Oct 10, 2013 at 02:19:15PM +0100, Thomas Wood wrote:
 Parse the 3D_Structure_ALL and 3D_MASK fields of the HDMI Vendor
 Specific Data Block to expose more stereo 3D modes.

 Signed-off-by: Thomas Wood thomas.w...@intel.com
 ---
  drivers/gpu/drm/drm_edid.c | 93 
 ++
  1 file changed, 85 insertions(+), 8 deletions(-)

 diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
 index 9e81609..b3949f9 100644
 --- a/drivers/gpu/drm/drm_edid.c
 +++ b/drivers/gpu/drm/drm_edid.c
 @@ -2652,6 +2652,50 @@ static int add_hdmi_mode(struct drm_connector 
 *connector, u8 vic)
   return 1;
  }

 +static int add_3d_struct_modes(struct drm_connector *connector, u16 
 structure,
 +const u8 *video_db, u8 video_len, u8 
 video_index)
 +{
 + struct drm_device *dev = connector-dev;
 + struct drm_display_mode *newmode;
 + int modes = 0;
 + u8 cea_mode;
 +
 + if (video_db == NULL || video_index  video_len)
 + return 0;
 +
 + /* CEA modes are numbered 1..127 */
 + cea_mode = (video_db[video_index]  127) - 1;
 + if (cea_mode = ARRAY_SIZE(edid_cea_modes))
 + return 0;
 +
 + if (structure  1) {

 Could use (1  0) for consistency.

I've updated this for v2.


 I'm also wondering if some displays might include some of the mandatory
 modes in 3D_Structure_ALL, and if so should we filter out the
 duplicates?

As Damien mentioned, duplicates are filtered out later on.


 + if ((multi_present == 1 || multi_present == 2) 
 + len = (9 + offset)) {

 If multi_present==2 and len is too small for the mask, I think we should
 skip adding the modes since the block is clearly incorrect/corrupted.

 So maybe just something like this:
  if ((multi_present == 1  len  (9 + offset)) ||
  (multi_present == 2  len  (11 + offset)))
  goto out;

I've added this check to v2 and also made sure multi_present is either 1 or 2
before continuing.


 I would also add a similar check for HDMI_3D_LEN since that is
 supposed to include the space required by 3D_Structure_ALL and
 3D_MASK. Or you could just check HDMI_3D_LEN against 'len' and bail
 out if that doesn't fit. And then you could just check
 3D_Structure_ALL and 3D_MASK against HDMI_3D_LEN.


I've added a check to ensure the value of HDMI_3D_LEN is large enough to
include 3D_Structure_ALL and 3D_MASK, if they are present.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: add support for additional stereo 3D modes

2013-10-11 Thread Ville Syrjälä 
On Thu, Oct 10, 2013 at 02:19:15PM +0100, Thomas Wood wrote:
 Parse the 3D_Structure_ALL and 3D_MASK fields of the HDMI Vendor
 Specific Data Block to expose more stereo 3D modes.
 
 Signed-off-by: Thomas Wood thomas.w...@intel.com
 ---
  drivers/gpu/drm/drm_edid.c | 93 
 ++
  1 file changed, 85 insertions(+), 8 deletions(-)
 
 diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
 index 9e81609..b3949f9 100644
 --- a/drivers/gpu/drm/drm_edid.c
 +++ b/drivers/gpu/drm/drm_edid.c
 @@ -2652,6 +2652,50 @@ static int add_hdmi_mode(struct drm_connector 
 *connector, u8 vic)
   return 1;
  }
  
 +static int add_3d_struct_modes(struct drm_connector *connector, u16 
 structure,
 +const u8 *video_db, u8 video_len, u8 video_index)
 +{
 + struct drm_device *dev = connector-dev;
 + struct drm_display_mode *newmode;
 + int modes = 0;
 + u8 cea_mode;
 +
 + if (video_db == NULL || video_index  video_len)
 + return 0;
 +
 + /* CEA modes are numbered 1..127 */
 + cea_mode = (video_db[video_index]  127) - 1;
 + if (cea_mode = ARRAY_SIZE(edid_cea_modes))
 + return 0;
 +
 + if (structure  1) {

Could use (1  0) for consistency.

I'm also wondering if some displays might include some of the mandatory
modes in 3D_Structure_ALL, and if so should we filter out the
duplicates?

 + newmode = drm_mode_duplicate(dev, edid_cea_modes[cea_mode]);
 + if (newmode) {
 + newmode-flags |= DRM_MODE_FLAG_3D_FRAME_PACKING;
 + drm_mode_probed_add(connector, newmode);
 + modes++;
 + }
 + }
 + if (structure  (1  6)) {
 + newmode = drm_mode_duplicate(dev, edid_cea_modes[cea_mode]);
 + if (newmode) {
 + newmode-flags |= DRM_MODE_FLAG_3D_TOP_AND_BOTTOM;
 + drm_mode_probed_add(connector, newmode);
 + modes++;
 + }
 + }
 + if (structure  (1  8)) {
 + newmode = drm_mode_duplicate(dev, edid_cea_modes[cea_mode]);
 + if (newmode) {
 + newmode-flags = DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF;
 + drm_mode_probed_add(connector, newmode);
 + modes++;
 + }
 + }
 +
 + return modes;
 +}
 +
  /*
   * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
   * @connector: connector corresponding to the HDMI sink
 @@ -2662,10 +2706,13 @@ static int add_hdmi_mode(struct drm_connector 
 *connector, u8 vic)
   * also adds the stereo 3d modes when applicable.
   */
  static int
 -do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
 +do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
 +const u8 *video_db, u8 video_len)
  {
 - int modes = 0, offset = 0, i;
 + int modes = 0, offset = 0, i, multi_present = 0;
   u8 vic_len;
 + u16 mask;
 + u16 structure_all;
  
   if (len  8)
   goto out;
 @@ -2689,9 +2736,13 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, 
 const u8 *db, u8 len)
  
   /* 3D_Present */
   offset++;
 - if (db[8 + offset]  (1  7))
 + if (db[8 + offset]  (1  7)) {
   modes += add_hdmi_mandatory_stereo_modes(connector);
  
 + /* 3D_Multi_present */
 + multi_present = (db[8 + offset]  0x60)  5;
 + }
 +
   offset++;
   vic_len = db[8 + offset]  5;
  
 @@ -2702,6 +2753,28 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, 
 const u8 *db, u8 len)
   modes += add_hdmi_mode(connector, vic);
   }
  
 + offset += 1 + vic_len;
 +
 + if ((multi_present == 1 || multi_present == 2) 
 + len = (9 + offset)) {

If multi_present==2 and len is too small for the mask, I think we should
skip adding the modes since the block is clearly incorrect/corrupted.

So maybe just something like this:
 if ((multi_present == 1  len  (9 + offset)) ||
 (multi_present == 2  len  (11 + offset)))
 goto out;

I would also add a similar check for HDMI_3D_LEN since that is
supposed to include the space required by 3D_Structure_ALL and 
3D_MASK. Or you could just check HDMI_3D_LEN against 'len' and bail
out if that doesn't fit. And then you could just check
3D_Structure_ALL and 3D_MASK against HDMI_3D_LEN.

 + /* 3D_Structure_ALL */
 + structure_all = (db[8 + offset]  8) | db[9 + offset];
 +
 + /* check if 3D_MASK is present */
 + if (multi_present == 2  len = 11 + offset)
 + mask = (db[10 + offset]  8) | db[11 + offset];
 + else
 + mask = 0x;
 +
 + for (i = 0; i  16; i++) {
 + if (mask  (1  i))
 + modes += add_3d_struct_modes(connector,
 +

Re: [Intel-gfx] [PATCH] drm: add support for additional stereo 3D modes

2013-10-11 Thread Damien Lespiau 
On Fri, Oct 11, 2013 at 02:12:14PM +0300, Ville Syrjälä wrote:
 On Thu, Oct 10, 2013 at 02:19:15PM +0100, Thomas Wood wrote:
  +static int add_3d_struct_modes(struct drm_connector *connector, u16 
  structure,
  +  const u8 *video_db, u8 video_len, u8 video_index)
  +{
  +   struct drm_device *dev = connector-dev;
  +   struct drm_display_mode *newmode;
  +   int modes = 0;
  +   u8 cea_mode;
  +
  +   if (video_db == NULL || video_index  video_len)
  +   return 0;
  +
  +   /* CEA modes are numbered 1..127 */
  +   cea_mode = (video_db[video_index]  127) - 1;
  +   if (cea_mode = ARRAY_SIZE(edid_cea_modes))
  +   return 0;
  +
  +   if (structure  1) {
 
 I'm also wondering if some displays might include some of the mandatory
 modes in 3D_Structure_ALL, and if so should we filter out the
 duplicates?

It can. Looks like drm_mode_connector_list_update() should be taking
care of the duplicated modes.

-- 
Damien

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: add support for additional stereo 3D modes

2013-10-11 Thread Damien Lespiau 
On Thu, Oct 10, 2013 at 02:19:15PM +0100, Thomas Wood wrote:
 + if ((multi_present == 1 || multi_present == 2) 

You could use the awesome binary literals gcc extension here and 0b01
and 0b10 to be even closer to the spec wording.

There's a precedent in drivers/watchdog/sunxi_wdt.c!

-- 
Damien
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm: add support for additional stereo 3D modes

2013-10-10 Thread Thomas Wood 
Parse the 3D_Structure_ALL and 3D_MASK fields of the HDMI Vendor
Specific Data Block to expose more stereo 3D modes.

Signed-off-by: Thomas Wood thomas.w...@intel.com
---
 drivers/gpu/drm/drm_edid.c | 93 ++
 1 file changed, 85 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 9e81609..b3949f9 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2652,6 +2652,50 @@ static int add_hdmi_mode(struct drm_connector 
*connector, u8 vic)
return 1;
 }
 
+static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
+  const u8 *video_db, u8 video_len, u8 video_index)
+{
+   struct drm_device *dev = connector-dev;
+   struct drm_display_mode *newmode;
+   int modes = 0;
+   u8 cea_mode;
+
+   if (video_db == NULL || video_index  video_len)
+   return 0;
+
+   /* CEA modes are numbered 1..127 */
+   cea_mode = (video_db[video_index]  127) - 1;
+   if (cea_mode = ARRAY_SIZE(edid_cea_modes))
+   return 0;
+
+   if (structure  1) {
+   newmode = drm_mode_duplicate(dev, edid_cea_modes[cea_mode]);
+   if (newmode) {
+   newmode-flags |= DRM_MODE_FLAG_3D_FRAME_PACKING;
+   drm_mode_probed_add(connector, newmode);
+   modes++;
+   }
+   }
+   if (structure  (1  6)) {
+   newmode = drm_mode_duplicate(dev, edid_cea_modes[cea_mode]);
+   if (newmode) {
+   newmode-flags |= DRM_MODE_FLAG_3D_TOP_AND_BOTTOM;
+   drm_mode_probed_add(connector, newmode);
+   modes++;
+   }
+   }
+   if (structure  (1  8)) {
+   newmode = drm_mode_duplicate(dev, edid_cea_modes[cea_mode]);
+   if (newmode) {
+   newmode-flags = DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF;
+   drm_mode_probed_add(connector, newmode);
+   modes++;
+   }
+   }
+
+   return modes;
+}
+
 /*
  * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
  * @connector: connector corresponding to the HDMI sink
@@ -2662,10 +2706,13 @@ static int add_hdmi_mode(struct drm_connector 
*connector, u8 vic)
  * also adds the stereo 3d modes when applicable.
  */
 static int
-do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
+do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
+  const u8 *video_db, u8 video_len)
 {
-   int modes = 0, offset = 0, i;
+   int modes = 0, offset = 0, i, multi_present = 0;
u8 vic_len;
+   u16 mask;
+   u16 structure_all;
 
if (len  8)
goto out;
@@ -2689,9 +2736,13 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, 
const u8 *db, u8 len)
 
/* 3D_Present */
offset++;
-   if (db[8 + offset]  (1  7))
+   if (db[8 + offset]  (1  7)) {
modes += add_hdmi_mandatory_stereo_modes(connector);
 
+   /* 3D_Multi_present */
+   multi_present = (db[8 + offset]  0x60)  5;
+   }
+
offset++;
vic_len = db[8 + offset]  5;
 
@@ -2702,6 +2753,28 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, 
const u8 *db, u8 len)
modes += add_hdmi_mode(connector, vic);
}
 
+   offset += 1 + vic_len;
+
+   if ((multi_present == 1 || multi_present == 2) 
+   len = (9 + offset)) {
+   /* 3D_Structure_ALL */
+   structure_all = (db[8 + offset]  8) | db[9 + offset];
+
+   /* check if 3D_MASK is present */
+   if (multi_present == 2  len = 11 + offset)
+   mask = (db[10 + offset]  8) | db[11 + offset];
+   else
+   mask = 0x;
+
+   for (i = 0; i  16; i++) {
+   if (mask  (1  i))
+   modes += add_3d_struct_modes(connector,
+structure_all,
+video_db,
+video_len, i);
+   }
+   }
+
 out:
return modes;
 }
@@ -2759,8 +2832,8 @@ static int
 add_cea_modes(struct drm_connector *connector, struct edid *edid)
 {
const u8 *cea = drm_find_cea_extension(edid);
-   const u8 *db, *hdmi = NULL;
-   u8 dbl, hdmi_len;
+   const u8 *db, *hdmi = NULL, *video = NULL;
+   u8 dbl, hdmi_len, video_len = 0;
int modes = 0;
 
if (cea  cea_revision(cea) = 3) {
@@ -2773,8 +2846,11 @@ add_cea_modes(struct drm_connector *connector, struct 
edid *edid)
db = cea[i];
dbl = cea_db_payload_len(db);
 
-