On Wed, 12 Jun 2019, Andres Rodriguez <andre...@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 <andre...@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

Reply via email to