Parse the list of additional 3D modes

Parse the list of additional 3D modes

[Thomas Wood]

Hi,

The following two patches add support for parsing the list of additional 3D
modes at the end of the vendor specific data block. The first splits the VIC
display mode lookup into a separate function so that it can be reused. The
second patch parses the list, adding any support modes to the connector.

Regards,

Thomas

[PATCH 1/2] drm/edid: split VIC display mode lookup into a separate function

[Thomas Wood]

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 67 +++++++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 52e060e..1dd82cd 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2557,25 +2557,40 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid)
 	return modes;
 }
 
-static int
-do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
+static struct drm_display_mode *
+drm_display_mode_from_vic_index(struct drm_connector *connector,
+				const u8 *video_db, u8 video_len,
+				u8 video_index)
 {
 	struct drm_device *dev = connector->dev;
-	const u8 *mode;
+	struct drm_display_mode *newmode;
 	u8 cea_mode;
-	int modes = 0;
 
-	for (mode = db; mode < db + len; mode++) {
-		cea_mode = (*mode & 127) - 1; /* CEA modes are numbered 1..127 */
-		if (cea_mode < ARRAY_SIZE(edid_cea_modes)) {
-			struct drm_display_mode *newmode;
-			newmode = drm_mode_duplicate(dev,
-						     &edid_cea_modes[cea_mode]);
-			if (newmode) {
-				newmode->vrefresh = 0;
-				drm_mode_probed_add(connector, newmode);
-				modes++;
-			}
+	if (video_db == NULL || video_index > video_len)
+		return NULL;
+
+	/* CEA modes are numbered 1..127 */
+	cea_mode = (video_db[video_index] & 127) - 1;
+	if (cea_mode >= ARRAY_SIZE(edid_cea_modes))
+		return NULL;
+
+	newmode = drm_mode_duplicate(dev, &edid_cea_modes[cea_mode]);
+	newmode->vrefresh = 0;
+
+	return newmode;
+}
+
+static int
+do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
+{
+	int i, modes = 0;
+
+	for (i = 0; i < len; i++) {
+		struct drm_display_mode *mode;
+		mode = drm_display_mode_from_vic_index(connector, db, len, i);
+		if (mode) {
+			drm_mode_probed_add(connector, mode);
+			modes++;
 		}
 	}
 
@@ -2669,21 +2684,13 @@ static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
 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 << 0)) {
-		newmode = drm_mode_duplicate(dev, &edid_cea_modes[cea_mode]);
+		newmode = drm_display_mode_from_vic_index(connector, video_db,
+							  video_len,
+							  video_index);
 		if (newmode) {
 			newmode->flags |= DRM_MODE_FLAG_3D_FRAME_PACKING;
 			drm_mode_probed_add(connector, newmode);
@@ -2691,7 +2698,9 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
 		}
 	}
 	if (structure & (1 << 6)) {
-		newmode = drm_mode_duplicate(dev, &edid_cea_modes[cea_mode]);
+		newmode = drm_display_mode_from_vic_index(connector, video_db,
+							  video_len,
+							  video_index);
 		if (newmode) {
 			newmode->flags |= DRM_MODE_FLAG_3D_TOP_AND_BOTTOM;
 			drm_mode_probed_add(connector, newmode);
@@ -2699,7 +2708,9 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
 		}
 	}
 	if (structure & (1 << 8)) {
-		newmode = drm_mode_duplicate(dev, &edid_cea_modes[cea_mode]);
+		newmode = drm_display_mode_from_vic_index(connector, video_db,
+							  video_len,
+							  video_index);
 		if (newmode) {
 			newmode->flags |= DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF;
 			drm_mode_probed_add(connector, newmode);
-- 
1.8.4.2

Re: [PATCH 1/2] drm/edid: split VIC display mode lookup into a separate function

[Ville Syrjälä]

On Thu, Nov 28, 2013 at 05:12:46PM +0000, Thomas Wood wrote:
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 67 +++++++++++++++++++++++++++-------------------
>  1 file changed, 39 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 52e060e..1dd82cd 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2557,25 +2557,40 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid)
>  	return modes;
>  }
>  
> -static int
> -do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
> +static struct drm_display_mode *
> +drm_display_mode_from_vic_index(struct drm_connector *connector,
> +				const u8 *video_db, u8 video_len,
> +				u8 video_index)
>  {
>  	struct drm_device *dev = connector->dev;
> -	const u8 *mode;
> +	struct drm_display_mode *newmode;
>  	u8 cea_mode;
> -	int modes = 0;
>  
> -	for (mode = db; mode < db + len; mode++) {
> -		cea_mode = (*mode & 127) - 1; /* CEA modes are numbered 1..127 */
> -		if (cea_mode < ARRAY_SIZE(edid_cea_modes)) {
> -			struct drm_display_mode *newmode;
> -			newmode = drm_mode_duplicate(dev,
> -						     &edid_cea_modes[cea_mode]);
> -			if (newmode) {
> -				newmode->vrefresh = 0;
> -				drm_mode_probed_add(connector, newmode);
> -				modes++;
> -			}
> +	if (video_db == NULL || video_index > video_len)
> +		return NULL;

Hmm, this seems wrong. video_len is the length of the payload only,
so the check should be >=. But this bug is already present in
the current code in add_3d_struct_modes(). So maybe include another
patch to fix that up first.

The rest looks OK to me.

> +
> +	/* CEA modes are numbered 1..127 */
> +	cea_mode = (video_db[video_index] & 127) - 1;
> +	if (cea_mode >= ARRAY_SIZE(edid_cea_modes))
> +		return NULL;
> +
> +	newmode = drm_mode_duplicate(dev, &edid_cea_modes[cea_mode]);
> +	newmode->vrefresh = 0;
> +
> +	return newmode;
> +}
> +
> +static int
> +do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
> +{
> +	int i, modes = 0;
> +
> +	for (i = 0; i < len; i++) {
> +		struct drm_display_mode *mode;
> +		mode = drm_display_mode_from_vic_index(connector, db, len, i);
> +		if (mode) {
> +			drm_mode_probed_add(connector, mode);
> +			modes++;
>  		}
>  	}
>  
> @@ -2669,21 +2684,13 @@ static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
>  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 << 0)) {
> -		newmode = drm_mode_duplicate(dev, &edid_cea_modes[cea_mode]);
> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
> +							  video_len,
> +							  video_index);
>  		if (newmode) {
>  			newmode->flags |= DRM_MODE_FLAG_3D_FRAME_PACKING;
>  			drm_mode_probed_add(connector, newmode);
> @@ -2691,7 +2698,9 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>  		}
>  	}
>  	if (structure & (1 << 6)) {
> -		newmode = drm_mode_duplicate(dev, &edid_cea_modes[cea_mode]);
> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
> +							  video_len,
> +							  video_index);
>  		if (newmode) {
>  			newmode->flags |= DRM_MODE_FLAG_3D_TOP_AND_BOTTOM;
>  			drm_mode_probed_add(connector, newmode);
> @@ -2699,7 +2708,9 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>  		}
>  	}
>  	if (structure & (1 << 8)) {
> -		newmode = drm_mode_duplicate(dev, &edid_cea_modes[cea_mode]);
> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
> +							  video_len,
> +							  video_index);
>  		if (newmode) {
>  			newmode->flags |= DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF;
>  			drm_mode_probed_add(connector, newmode);
> -- 
> 1.8.4.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

[PATCH 2/2] drm/edid: parse the list of additional 3D modes

[Thomas Wood]

Parse 2D_VIC_order_X and 3D_Structure_X from the list at the end of the
HDMI Vendor Specific Data Block.

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 1dd82cd..eb6b363 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2808,6 +2808,56 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
 						     video_len, i);
 	}
 
+	if (hdmi_3d_len <= 4)
+		goto out;
+
+	offset += 4;
+
+	for (i = 0; i < (hdmi_3d_len - 4); i++) {
+		int vic_index;
+		struct drm_display_mode *newmode = NULL;
+		unsigned int newflag = 0;
+
+		if (((db[8 + offset + i] & 0x0f) > 7)
+		    && (i + 1 == hdmi_3d_len - 4))
+			break;
+
+		/* 2D_VIC_order_X */
+		vic_index = db[8 + offset + i] >> 4;
+
+		/* 3D_Structure_X */
+		switch (db[8 + offset + i] & 0x0f) {
+		case 0:
+			newflag = DRM_MODE_FLAG_3D_FRAME_PACKING;
+			break;
+		case 6:
+			newflag = DRM_MODE_FLAG_3D_TOP_AND_BOTTOM;
+			break;
+		case 8:
+			if ((db[9 + offset + i] >> 4) == 1)
+				newflag = DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF;
+			break;
+		}
+
+		if (newflag != 0) {
+			newmode = drm_display_mode_from_vic_index(connector,
+								  video_db,
+								  video_len,
+								  vic_index);
+
+			if (newmode) {
+				newmode->flags |= newflag;
+				drm_mode_probed_add(connector, newmode);
+				modes++;
+			}
+		}
+
+		if ((db[8 + offset + i] & 0x0f) > 7) {
+			/* Optional 3D_Detail_X and reserved */
+			i++;
+		}
+	}
+
 out:
 	return modes;
 }
-- 
1.8.4.2

Re: [PATCH 2/2] drm/edid: parse the list of additional 3D modes

[Ville Syrjälä]

On Thu, Nov 28, 2013 at 05:12:47PM +0000, Thomas Wood wrote:
> Parse 2D_VIC_order_X and 3D_Structure_X from the list at the end of the
> HDMI Vendor Specific Data Block.
> 
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 1dd82cd..eb6b363 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2808,6 +2808,56 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
>  						     video_len, i);
>  	}
>  
> +	if (hdmi_3d_len <= 4)

This number 4 should depend on the value of multi_present, no?

Also we haven't actually verified that all hdmi_3d_len bytes
fit within the data block. If you do that a bit earlier, you
could also skip the explicit 'len' checks for the
multi_present cases since we also check those against
hdmi_3d_len.

> +		goto out;
> +
> +	offset += 4;
> +
> +	for (i = 0; i < (hdmi_3d_len - 4); i++) {
> +		int vic_index;
> +		struct drm_display_mode *newmode = NULL;
> +		unsigned int newflag = 0;
> +
> +		if (((db[8 + offset + i] & 0x0f) > 7)
> +		    && (i + 1 == hdmi_3d_len - 4))
> +			break;

The '(db[8 + offset + i] & 0x0f) > 7' part is repeated a few times.
Maybe add a 'bool detail_present = (db[8 + offset + i] & 0x0f) > 7' or
something to make make things a bit easier to read.

> +
> +		/* 2D_VIC_order_X */
> +		vic_index = db[8 + offset + i] >> 4;
> +
> +		/* 3D_Structure_X */
> +		switch (db[8 + offset + i] & 0x0f) {
> +		case 0:
> +			newflag = DRM_MODE_FLAG_3D_FRAME_PACKING;
> +			break;
> +		case 6:
> +			newflag = DRM_MODE_FLAG_3D_TOP_AND_BOTTOM;
> +			break;
> +		case 8:

Maybe add a /* 3D_Detail_X */ comment here
for consistency with the other comments.

> +			if ((db[9 + offset + i] >> 4) == 1)
> +				newflag = DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF;
> +			break;
> +		}
> +
> +		if (newflag != 0) {
> +			newmode = drm_display_mode_from_vic_index(connector,
> +								  video_db,
> +								  video_len,
> +								  vic_index);
> +
> +			if (newmode) {
> +				newmode->flags |= newflag;
> +				drm_mode_probed_add(connector, newmode);
> +				modes++;
> +			}
> +		}
> +
> +		if ((db[8 + offset + i] & 0x0f) > 7) {
> +			/* Optional 3D_Detail_X and reserved */
> +			i++;
> +		}
> +	}
> +
>  out:
>  	return modes;
>  }
> -- 
> 1.8.4.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC