diff mbox

drm: add support for additional stereo 3D modes

Message ID 1381411155-2841-1-git-send-email-thomas.wood@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Wood Oct. 10, 2013, 1:19 p.m. UTC
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.wood@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 93 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 85 insertions(+), 8 deletions(-)

Comments

Ville Syrjälä Oct. 11, 2013, 11:12 a.m. UTC | #1
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.wood@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 = 0xffff;
> +
> +		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);
>  
> -			if (cea_db_tag(db) == VIDEO_BLOCK)
> -				modes += do_cea_modes(connector, db + 1, dbl);
> +			if (cea_db_tag(db) == VIDEO_BLOCK) {
> +				video = db + 1;
> +				video_len = dbl;
> +				modes += do_cea_modes(connector, video, dbl);
> +			}
>  			else if (cea_db_is_hdmi_vsdb(db)) {
>  				hdmi = db;
>  				hdmi_len = dbl;
> @@ -2787,7 +2863,8 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid)
>  	 * be patching their flags when the sink supports stereo 3D.
>  	 */
>  	if (hdmi)
> -		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len);
> +		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
> +					    video_len);
>  
>  	return modes;
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Lespiau, Damien Oct. 11, 2013, 12:09 p.m. UTC | #2
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.
Lespiau, Damien Oct. 11, 2013, 3:59 p.m. UTC | #3
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!
Thomas Wood Oct. 15, 2013, 5:25 p.m. UTC | #4
On 11 October 2013 12:12, Ville Syrjälä <ville.syrjala@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.wood@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.
diff mbox

Patch

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 = 0xffff;
+
+		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);
 
-			if (cea_db_tag(db) == VIDEO_BLOCK)
-				modes += do_cea_modes(connector, db + 1, dbl);
+			if (cea_db_tag(db) == VIDEO_BLOCK) {
+				video = db + 1;
+				video_len = dbl;
+				modes += do_cea_modes(connector, video, dbl);
+			}
 			else if (cea_db_is_hdmi_vsdb(db)) {
 				hdmi = db;
 				hdmi_len = dbl;
@@ -2787,7 +2863,8 @@  add_cea_modes(struct drm_connector *connector, struct edid *edid)
 	 * be patching their flags when the sink supports stereo 3D.
 	 */
 	if (hdmi)
-		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len);
+		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
+					    video_len);
 
 	return modes;
 }