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

["Ser, Simon" <simon.ser@intel.com>]

On Tue, 2019-07-02 at 17:05 +0300, Ville Syrjälä wrote:
> 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.

Indeed

> > 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() ?

That's a good point. I just defined hdmi_ieee_oui as a constant
instead, which I thought would be nice, but in the end also brings
`extern` and memcpy.

> > +{
> > +	/* 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.

Ack

> 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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev