Re: [PATCH] drm/edid: parse CEA blocks embedded in DisplayID

[Jani Nikula <jani.nikula@linux.intel.com>]

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