dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/edid: parse CEA blocks embedded in DisplayID
@ 2019-06-19 18:09 Andres Rodriguez
  0 siblings, 0 replies; 3+ messages in thread
From: Andres Rodriguez @ 2019-06-19 18:09 UTC (permalink / raw)
  To: dri-devel; +Cc: Andres Rodriguez, Dave Airlie, Jani Nikula, stable

DisplayID blocks allow embedding of CEA blocks. The payloads are
identical to traditional top level CEA extension blocks, but the header
is slightly different.

This change allows the CEA parser to find a CEA block inside a DisplayID
block. Additionally, it adds support for parsing the embedded CTA
header. No further changes are necessary due to payload parity.

This change fixes audio support for the Valve Index HMD.

Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.15
---
 drivers/gpu/drm/drm_edid.c  | 81 ++++++++++++++++++++++++++++++++-----
 include/drm/drm_displayid.h | 10 +++++
 2 files changed, 80 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 649cfd8b4200..a4e3f6b22dbb 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1339,6 +1339,7 @@ MODULE_PARM_DESC(edid_fixup,
 
 static void drm_get_displayid(struct drm_connector *connector,
 			      struct edid *edid);
+static int validate_displayid(u8 *displayid, int length, int idx);
 
 static int drm_edid_block_checksum(const u8 *raw_edid)
 {
@@ -2883,16 +2884,46 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
 	return edid_ext;
 }
 
-static u8 *drm_find_cea_extension(const struct edid *edid)
-{
-	return drm_find_edid_extension(edid, CEA_EXT);
-}
 
 static u8 *drm_find_displayid_extension(const struct edid *edid)
 {
 	return drm_find_edid_extension(edid, DISPLAYID_EXT);
 }
 
+static u8 *drm_find_cea_extension(const struct edid *edid)
+{
+	int ret;
+	int idx = 1;
+	int length = EDID_LENGTH;
+	struct displayid_block *block;
+	u8 *cea;
+	u8 *displayid;
+
+	/* Look for a top level CEA extension block */
+	cea = drm_find_edid_extension(edid, CEA_EXT);
+	if (cea)
+		return cea;
+
+	/* CEA blocks can also be found embedded in a DisplayID block */
+	displayid = drm_find_displayid_extension(edid);
+	if (!displayid)
+		return NULL;
+
+	ret = validate_displayid(displayid, length, idx);
+	if (ret)
+		return NULL;
+
+	idx += sizeof(struct displayid_hdr);
+	for_each_displayid_db(displayid, block, idx, length) {
+		if (block->tag == DATA_BLOCK_CTA) {
+			cea = (u8 *)block;
+			break;
+		}
+	}
+
+	return cea;
+}
+
 /*
  * Calculate the alternate clock for the CEA mode
  * (60Hz vs. 59.94Hz etc.)
@@ -3616,13 +3647,38 @@ cea_revision(const u8 *cea)
 static int
 cea_db_offsets(const u8 *cea, int *start, int *end)
 {
-	/* Data block offset in CEA extension block */
-	*start = 4;
-	*end = cea[2];
-	if (*end == 0)
-		*end = 127;
-	if (*end < 4 || *end > 127)
-		return -ERANGE;
+
+	/* DisplayID CTA extension blocks and top-level CEA EDID
+	 * block header definitions differ in the following bytes:
+	 *   1) Byte 2 of the header specifies length differently,
+	 *   2) Byte 3 is only present in the CEA top level block.
+	 *
+	 * The different definitions for byte 2 follow.
+	 *
+	 * DisplayID CTA extension block defines byte 2 as:
+	 *   Number of payload bytes
+	 *
+	 * CEA EDID block defines byte 2 as:
+	 *   Byte number (decimal) within this block where the 18-byte
+	 *   DTDs begin. If no non-DTD data is present in this extension
+	 *   block, the value should be set to 04h (the byte after next).
+	 *   If set to 00h, there are no DTDs present in this block and
+	 *   no non-DTD data.
+	 */
+	if (cea[0] == DATA_BLOCK_CTA) {
+		*start = 3;
+		*end = *start + cea[2];
+	} else if (cea[0] == CEA_EXT) {
+		/* Data block offset in CEA extension block */
+		*start = 4;
+		*end = cea[2];
+		if (*end == 0)
+			*end = 127;
+		if (*end < 4 || *end > 127)
+			return -ERANGE;
+	} else
+		return -ENOTSUPP;
+
 	return 0;
 }
 
@@ -5240,6 +5296,9 @@ static int drm_parse_display_id(struct drm_connector *connector,
 		case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
 			/* handled in mode gathering code. */
 			break;
+		case DATA_BLOCK_CTA:
+			/* handled in the cea parser code. */
+			break;
 		default:
 			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);
 			break;
diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
index c0d4df6a606f..9d3b745c3107 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -40,6 +40,7 @@
 #define DATA_BLOCK_DISPLAY_INTERFACE 0x0f
 #define DATA_BLOCK_STEREO_DISPLAY_INTERFACE 0x10
 #define DATA_BLOCK_TILED_DISPLAY 0x12
+#define DATA_BLOCK_CTA 0x81
 
 #define DATA_BLOCK_VENDOR_SPECIFIC 0x7f
 
@@ -90,4 +91,13 @@ struct displayid_detailed_timing_block {
 	struct displayid_block base;
 	struct displayid_detailed_timings_1 timings[0];
 };
+
+#define for_each_displayid_db(displayid, block, idx, length) \
+	for ((block) = (struct displayid_block *)&(displayid)[idx]; \
+	     (idx) + sizeof(struct displayid_block) <= (length) && \
+	     (idx) + sizeof(struct displayid_block) + (block)->num_bytes <= (length) && \
+	     (block)->num_bytes > 0; \
+	     (idx) += (block)->num_bytes + sizeof(struct displayid_block), \
+	     (block) = (struct displayid_block *)&(displayid)[idx])
+
 #endif
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] drm/edid: parse CEA blocks embedded in DisplayID
  2019-06-12 20:02 Andres Rodriguez
@ 2019-06-13  9:16 ` Jani Nikula
  0 siblings, 0 replies; 3+ messages in thread
From: Jani Nikula @ 2019-06-13  9:16 UTC (permalink / raw)
  To: dri-devel; +Cc: Andres Rodriguez

On Wed, 12 Jun 2019, Andres Rodriguez <andresx7@gmail.com> wrote:
> DisplayID blocks allow embedding of CEA blocks. The payloads are
> identical to traditional top level CEA extension blocks, but the header
> is slightly different.
>
> This change allows the CEA parser to find a CEA block inside a DisplayID
> block. Additionally, it adds support for parsing the embedded CTA
> header. No further changes are necessary due to payload parity.
>
> This change enables audio support for the Valve Index HMD.
>
> Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 79 +++++++++++++++++++++++++++++++++----
>  include/drm/drm_displayid.h |  1 +
>  2 files changed, 72 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 649cfd8b4200..3e71c6ae48d4 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1339,6 +1339,8 @@ MODULE_PARM_DESC(edid_fixup,
>  
>  static void drm_get_displayid(struct drm_connector *connector,
>  			      struct edid *edid);
> +static u8 *drm_find_displayid_extension(const struct edid *edid);
> +static int validate_displayid(u8 *displayid, int length, int idx);
>  
>  static int drm_edid_block_checksum(const u8 *raw_edid)
>  {
> @@ -2885,7 +2887,40 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
>  
>  static u8 *drm_find_cea_extension(const struct edid *edid)
>  {
> -	return drm_find_edid_extension(edid, CEA_EXT);
> +	int ret;
> +	int idx = 1;
> +	int length = EDID_LENGTH;
> +	struct displayid_block *block;
> +	u8 *cea = NULL;
> +	u8 *displayid = NULL;

Unnecessary initializations.

> +
> +	cea = drm_find_edid_extension(edid, CEA_EXT);
> +
> +	/* CEA blocks can also be found embedded in a DisplayID block */
> +	if (!cea) {

It's customary to return early to reduce indent on the following code,
i.e.

	if (cea)
		return cea;

> +		displayid = drm_find_displayid_extension(edid);
> +		if (!displayid)
> +			return NULL;
> +
> +		ret = validate_displayid(displayid, length, idx);
> +		if (ret)
> +			return NULL;
> +
> +		idx += sizeof(struct displayid_hdr);
> +		while (block = (struct displayid_block *)&displayid[idx],
> +		       idx + sizeof(struct displayid_block) <= length &&
> +		       idx + sizeof(struct displayid_block) + block->num_bytes <= length &&
> +		       block->num_bytes > 0) {

I'm sure there's a for loop in there somewhere desperate to get out. The
above is unnecessarily tricky.

> +			idx += block->num_bytes + sizeof(struct displayid_block);
> +			switch (block->tag) {
> +			case DATA_BLOCK_CTA:
> +				cea = (u8 *)block;
> +				break;
> +			}

Looks like an if statement here. Why do you continue the while loop
after you've found the block?

BR,
Jani.

> +		}
> +	}
> +
> +	return cea;
>  }
>  
>  static u8 *drm_find_displayid_extension(const struct edid *edid)
> @@ -3616,13 +3651,38 @@ cea_revision(const u8 *cea)
>  static int
>  cea_db_offsets(const u8 *cea, int *start, int *end)
>  {
> -	/* Data block offset in CEA extension block */
> -	*start = 4;
> -	*end = cea[2];
> -	if (*end == 0)
> -		*end = 127;
> -	if (*end < 4 || *end > 127)
> -		return -ERANGE;
> +
> +	/* DisplayID CTA extension blocks and top-level CEA EDID
> +	 * blocks headers differ in two byte definitions:
> +	 *   1) Byte 2 of the header specifies length differently,
> +	 *   2) Byte 3 is only present in the CEA top level block.
> +	 *
> +	 * The different definitions for byte 2 follow.
> +	 *
> +	 * DisplayID CTA extension block defines byte 2 as:
> +	 *   Number of payload bytes
> +	 *
> +	 * CEA EDID block defines byte 2 as:
> +	 *   Byte number (decimal) within this block where the 18-byte
> +	 *   DTDs begin. If no non-DTD data is present in this extension
> +	 *   block, the value should be set to 04h (the byte after next).
> +	 *   If set to 00h, there are no DTDs present in this block and
> +	 *   no non-DTD data.
> +	 */
> +	if (cea[0] == DATA_BLOCK_CTA) {
> +		*start = 3;
> +		*end = *start + cea[2];
> +	} else if (cea[0] == CEA_EXT) {
> +		*start = 4;
> +		*end = cea[2];
> +		/* Data block offset in CEA extension block */
> +		if (*end == 0)
> +			*end = 127;
> +		if (*end < 4 || *end > 127)
> +			return -ERANGE;
> +	} else
> +		return -ENOTSUPP;
> +
>  	return 0;
>  }
>  
> @@ -5240,6 +5300,9 @@ static int drm_parse_display_id(struct drm_connector *connector,
>  		case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
>  			/* handled in mode gathering code. */
>  			break;
> +		case DATA_BLOCK_CTA:
> +			/* handled in the cea parser code. */
> +			break;
>  		default:
>  			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);
>  			break;
> diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
> index c0d4df6a606f..c7af857f4764 100644
> --- a/include/drm/drm_displayid.h
> +++ b/include/drm/drm_displayid.h
> @@ -40,6 +40,7 @@
>  #define DATA_BLOCK_DISPLAY_INTERFACE 0x0f
>  #define DATA_BLOCK_STEREO_DISPLAY_INTERFACE 0x10
>  #define DATA_BLOCK_TILED_DISPLAY 0x12
> +#define DATA_BLOCK_CTA 0x81
>  
>  #define DATA_BLOCK_VENDOR_SPECIFIC 0x7f

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH] drm/edid: parse CEA blocks embedded in DisplayID
@ 2019-06-12 20:02 Andres Rodriguez
  2019-06-13  9:16 ` Jani Nikula
  0 siblings, 1 reply; 3+ messages in thread
From: Andres Rodriguez @ 2019-06-12 20:02 UTC (permalink / raw)
  To: dri-devel; +Cc: Andres Rodriguez

DisplayID blocks allow embedding of CEA blocks. The payloads are
identical to traditional top level CEA extension blocks, but the header
is slightly different.

This change allows the CEA parser to find a CEA block inside a DisplayID
block. Additionally, it adds support for parsing the embedded CTA
header. No further changes are necessary due to payload parity.

This change enables audio support for the Valve Index HMD.

Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
---
 drivers/gpu/drm/drm_edid.c  | 79 +++++++++++++++++++++++++++++++++----
 include/drm/drm_displayid.h |  1 +
 2 files changed, 72 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 649cfd8b4200..3e71c6ae48d4 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1339,6 +1339,8 @@ MODULE_PARM_DESC(edid_fixup,
 
 static void drm_get_displayid(struct drm_connector *connector,
 			      struct edid *edid);
+static u8 *drm_find_displayid_extension(const struct edid *edid);
+static int validate_displayid(u8 *displayid, int length, int idx);
 
 static int drm_edid_block_checksum(const u8 *raw_edid)
 {
@@ -2885,7 +2887,40 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
 
 static u8 *drm_find_cea_extension(const struct edid *edid)
 {
-	return drm_find_edid_extension(edid, CEA_EXT);
+	int ret;
+	int idx = 1;
+	int length = EDID_LENGTH;
+	struct displayid_block *block;
+	u8 *cea = NULL;
+	u8 *displayid = NULL;
+
+	cea = drm_find_edid_extension(edid, CEA_EXT);
+
+	/* CEA blocks can also be found embedded in a DisplayID block */
+	if (!cea) {
+		displayid = drm_find_displayid_extension(edid);
+		if (!displayid)
+			return NULL;
+
+		ret = validate_displayid(displayid, length, idx);
+		if (ret)
+			return NULL;
+
+		idx += sizeof(struct displayid_hdr);
+		while (block = (struct displayid_block *)&displayid[idx],
+		       idx + sizeof(struct displayid_block) <= length &&
+		       idx + sizeof(struct displayid_block) + block->num_bytes <= length &&
+		       block->num_bytes > 0) {
+			idx += block->num_bytes + sizeof(struct displayid_block);
+			switch (block->tag) {
+			case DATA_BLOCK_CTA:
+				cea = (u8 *)block;
+				break;
+			}
+		}
+	}
+
+	return cea;
 }
 
 static u8 *drm_find_displayid_extension(const struct edid *edid)
@@ -3616,13 +3651,38 @@ cea_revision(const u8 *cea)
 static int
 cea_db_offsets(const u8 *cea, int *start, int *end)
 {
-	/* Data block offset in CEA extension block */
-	*start = 4;
-	*end = cea[2];
-	if (*end == 0)
-		*end = 127;
-	if (*end < 4 || *end > 127)
-		return -ERANGE;
+
+	/* DisplayID CTA extension blocks and top-level CEA EDID
+	 * blocks headers differ in two byte definitions:
+	 *   1) Byte 2 of the header specifies length differently,
+	 *   2) Byte 3 is only present in the CEA top level block.
+	 *
+	 * The different definitions for byte 2 follow.
+	 *
+	 * DisplayID CTA extension block defines byte 2 as:
+	 *   Number of payload bytes
+	 *
+	 * CEA EDID block defines byte 2 as:
+	 *   Byte number (decimal) within this block where the 18-byte
+	 *   DTDs begin. If no non-DTD data is present in this extension
+	 *   block, the value should be set to 04h (the byte after next).
+	 *   If set to 00h, there are no DTDs present in this block and
+	 *   no non-DTD data.
+	 */
+	if (cea[0] == DATA_BLOCK_CTA) {
+		*start = 3;
+		*end = *start + cea[2];
+	} else if (cea[0] == CEA_EXT) {
+		*start = 4;
+		*end = cea[2];
+		/* Data block offset in CEA extension block */
+		if (*end == 0)
+			*end = 127;
+		if (*end < 4 || *end > 127)
+			return -ERANGE;
+	} else
+		return -ENOTSUPP;
+
 	return 0;
 }
 
@@ -5240,6 +5300,9 @@ static int drm_parse_display_id(struct drm_connector *connector,
 		case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
 			/* handled in mode gathering code. */
 			break;
+		case DATA_BLOCK_CTA:
+			/* handled in the cea parser code. */
+			break;
 		default:
 			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);
 			break;
diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
index c0d4df6a606f..c7af857f4764 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -40,6 +40,7 @@
 #define DATA_BLOCK_DISPLAY_INTERFACE 0x0f
 #define DATA_BLOCK_STEREO_DISPLAY_INTERFACE 0x10
 #define DATA_BLOCK_TILED_DISPLAY 0x12
+#define DATA_BLOCK_CTA 0x81
 
 #define DATA_BLOCK_VENDOR_SPECIFIC 0x7f
 
-- 
2.19.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-06-19 18:09 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-19 18:09 [PATCH] drm/edid: parse CEA blocks embedded in DisplayID Andres Rodriguez
  -- strict thread matches above, loose matches on Subject: below --
2019-06-12 20:02 Andres Rodriguez
2019-06-13  9:16 ` Jani Nikula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).