Re: [igt-dev] [PATCH i-g-t 2/5] lib/igt_edid: add hdmi_vsd

Ville Syrjälä
2019-07-02T14:05:50Z

On Tue, Jul 02, 2019 at 03:50:35PM +0300, Simon Ser wrote:
> The HDMI Vendor-Specific Data block, defined as an opaque blob in the EDID
> spec, is described in the HDMI spec.

You may want to clarify that this is the HDMI 1.4 thing, not the
HDMI 2.0 "HDMI Forum" thing.

>
> Most of the extension fields are optional, so it doesn't translate well to a C
> struct. For now callers need to manually fill the hdmi_vsd.data field.
>
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> ---
> lib/igt_edid.c | 40 ++++++++++++++++++++++++++++++++--------
> lib/igt_edid.h | 49 +++++++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 77 insertions(+), 12 deletions(-)
>
> diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> index cbb7eefff70d..c93d8fa8bca0 100644
> --- a/lib/igt_edid.c
> +++ b/lib/igt_edid.c
> @@ -314,6 +314,14 @@ void cea_sad_init_pcm(struct cea_sad *sad, int channels,
> sad->bitrate = sample_sizes;
> }
>
> +static void cea_vsd_set_hdmi(struct cea_vsd *vsd)

vsdb is the acronym used by the specs. I would suggest a sed job
over the whole thing to align with that. The name of the function
is a bit vague. ..._set_ieee_oui_hdmi() ?

> +{
> + /* HDMI's IEEE Registration Identifier */
> + vsd->ieee_oui[0] = 0x03;
> + vsd->ieee_oui[1] = 0x0C;
> + vsd->ieee_oui[2] = 0x00;
> +}
> +
> /**
> * cea_vsd_get_hdmi_default:
> *
> @@ -321,21 +329,22 @@ void cea_sad_init_pcm(struct cea_sad *sad, int channels,
> */
> const struct cea_vsd *cea_vsd_get_hdmi_default(size_t *size)
> {
> - static char raw[sizeof(struct cea_vsd) + 4] = {0};
> + static char raw[CEA_VSD_HDMI_MIN_SIZE + 2] = {0};

A comment explaining the 2 might be helful.

Otherwise
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> struct cea_vsd *vsd;
> + struct hdmi_vsd *hdmi;
>
> *size = sizeof(raw);
>
> /* Magic incantation. Works better if you orient your screen in the
> * direction of the VESA headquarters. */
> vsd = (struct cea_vsd *) raw;
> - vsd->ieee_oui[0] = 0x03;
> - vsd->ieee_oui[1] = 0x0C;
> - vsd->ieee_oui[2] = 0x00;
> - vsd->data[0] = 0x10;
> - vsd->data[1] = 0x00;
> - vsd->data[2] = 0x38;
> - vsd->data[3] = 0x2D;
> + cea_vsd_set_hdmi(vsd);
> + hdmi = &vsd->data.hdmi;
> + hdmi->src_phy_addr[0] = 0x10;
> + hdmi->src_phy_addr[1] = 0x00;
> + /* 2 VSD extension fields */
> + hdmi->flags1 = 0x38;
> + hdmi->max_tdms_clock = 0x2D;
>
> return vsd;
> }
> @@ -371,6 +380,21 @@ size_t edid_cea_data_block_set_vsd(struct edid_cea_data_block *block,
> return sizeof(struct edid_cea_data_block) + vsd_size;
> }
>
> +size_t edid_cea_data_block_set_hdmi_vsd(struct edid_cea_data_block *block,
> + const struct hdmi_vsd *hdmi,
> + size_t hdmi_size)
> +{
> + char raw_vsd[CEA_VSD_HDMI_MAX_SIZE] = {0};
> + struct cea_vsd *vsd = (struct cea_vsd *) raw_vsd;
> +
> + assert(hdmi_size >= HDMI_VSD_MIN_SIZE && hdmi_size <= HDMI_VSD_MAX_SIZE);
> + cea_vsd_set_hdmi(vsd);
> + memcpy(&vsd->data.hdmi, hdmi, hdmi_size);
> +
> + return edid_cea_data_block_set_vsd(block, vsd,
> + CEA_VSD_HEADER_SIZE + hdmi_size);
> +}
> +
> size_t edid_cea_data_block_set_speaker_alloc(struct edid_cea_data_block *block,
> const struct cea_speaker_alloc *speakers)
> {
> diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> index 47581bb778b0..0ac851c0d74a 100644
> --- a/lib/igt_edid.h
> +++ b/lib/igt_edid.h
> @@ -191,12 +191,50 @@ struct cea_sad {
> uint8_t bitrate;
> } __attribute__((packed));
>
> -/* Vendor Specific Data */
> -struct cea_vsd {
> - uint8_t ieee_oui[3];
> - char data[];
> +enum hdmi_vsd_flags1 {
> + HDMI_VSD_DVI_DUAL = 1 << 0,
> + HDMI_VSD_DC_Y444 = 1 << 3, /* supports YCbCr 4:4:4 */
> + HDMI_VSD_DC_30BIT = 1 << 4, /* 30 bits per pixel */
> + HDMI_VSD_DC_36BIT = 1 << 5, /* 36 bits per pixel */
> + HDMI_VSD_DC_48BIT = 1 << 6, /* 48 bits per pixel */
> + HDMI_VSD_SUPPORTS_AI = 1 << 7, /* supports ACP, ISRC1 or ISRC2 packets */
> +};
> +
> +enum hdmi_vsd_flags2 {
> + HDMI_VSD_CNC_GRAPHICS = 1 << 0,
> + HDMI_VSD_CNC_PHOTO = 1 << 1,
> + HDMI_VSD_CNC_CINEMA = 1 << 2,
> + HDMI_VSD_CNC_GAME = 1 << 3,
> + HDMI_VSD_VIDEO_PRESENT = 1 << 5,
> + HDMI_VSD_INTERLACED_LATENCY_PRESENT = 1 << 6,
> + HDMI_VSD_LATENCY_PRESENT = 1 << 7,
> };
>
> +/* HDMI Vendor-Specific Data Block (defined in the HDMI spec) */
> +struct hdmi_vsd {
> + uint8_t src_phy_addr[2]; /* source physical address */
> +
> + /* Extension fields */
> + uint8_t flags1; /* enum hdmi_vsd_flags1 */
> + uint8_t max_tdms_clock; /* multiply by 5MHz */
> + uint8_t flags2; /* enum hdmi_vsd_flags2 */
> + char data[]; /* latency, misc, VIC, 3D */
> +} __attribute__((packed));
> +
> +#define HDMI_VSD_MIN_SIZE 2 /* just the source physical address */
> +#define HDMI_VSD_MAX_SIZE 28
> +#define CEA_VSD_HEADER_SIZE 3 /* IEEE OUI */
> +#define CEA_VSD_HDMI_MIN_SIZE (CEA_VSD_HEADER_SIZE + HDMI_VSD_MIN_SIZE)
> +#define CEA_VSD_HDMI_MAX_SIZE (CEA_VSD_HEADER_SIZE + HDMI_VSD_MAX_SIZE)
> +
> +/* Vendor-Specific Data */
> +struct cea_vsd {
> + uint8_t ieee_oui[3]; /* 24-bit IEEE Registration Identifier, LSB */
> + union {
> + struct hdmi_vsd hdmi;
> + } data;
> +} __attribute__((packed));
> +
> enum cea_speaker_alloc_item {
> CEA_SPEAKER_FRONT_LEFT_RIGHT = 1 << 0,
> CEA_SPEAKER_LFE = 1 << 1,
> @@ -315,6 +353,9 @@ size_t edid_cea_data_block_set_sad(struct edid_cea_data_block *block,
> const struct cea_sad *sads, size_t sads_len);
> size_t edid_cea_data_block_set_vsd(struct edid_cea_data_block *block,
> const struct cea_vsd *vsd, size_t vsd_size);
> +size_t edid_cea_data_block_set_hdmi_vsd(struct edid_cea_data_block *block,
> + const struct hdmi_vsd *hdmi,
> + size_t hdmi_size);
> size_t edid_cea_data_block_set_speaker_alloc(struct edid_cea_data_block *block,
> const struct cea_speaker_alloc *speakers);
> void edid_ext_set_cea(struct edid_ext *ext, size_t data_blocks_size,
> --
> 2.22.0
>
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

--
Ville Syrjälä
Intel