[igt-dev] [PATCH i-g-t 2/3] lib/igt_edid: add support for Speaker Allocation Data blocks

[Ser, Simon]

On Mon, 2019-06-03 at 16:13 +0300, Martin Peres wrote:
> On 27/05/2019 15:03, Simon Ser wrote:
> > Speaker Allocation Data blocks describe which speakers are present in the
> > display device.
> > 
> > This block is required to make DisplayPort audio work.
> > 
> > Signed-off-by: Simon Ser <simon.ser at intel.com>
> > ---
> >  lib/igt_edid.c | 12 ++++++++++++
> >  lib/igt_edid.h | 18 ++++++++++++++++++
> >  2 files changed, 30 insertions(+)
> > 
> > diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> > index fbdb0c06b8d7..e71136f48e14 100644
> > --- a/lib/igt_edid.c
> > +++ b/lib/igt_edid.c
> > @@ -348,6 +348,18 @@ 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_speaker_alloc(struct edid_cea_data_block *block,
> > +					     const struct cea_speaker_alloc *speakers)
> > +{
> > +	size_t size;
> > +
> > +	size = sizeof(struct cea_speaker_alloc);
> > +	edid_cea_data_block_init(block, EDID_CEA_DATA_SPEAKER_ALLOC, size);
> > +	memcpy(block->data.speakers, speakers, size);
> > +
> > +	return sizeof(struct edid_cea_data_block) + size;
> > +}
> > +
> >  void edid_ext_set_cea(struct edid_ext *ext, size_t data_blocks_size,
> >  		      uint8_t flags)
> >  {
> > diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> > index 7edd7e38f41e..39d1842d32df 100644
> > --- a/lib/igt_edid.h
> > +++ b/lib/igt_edid.h
> > @@ -195,6 +195,21 @@ struct cea_vsd {
> >  	char data[];
> >  };
> >  
> > +enum cea_speaker_alloc_item {
> 
> Is that the official name? Having alloc in the name of an enum is a
> little odd...

Yes, "Speaker Allocation Data" is the official name.

> > +	CEA_SPEAKER_FRONT_LEFT_RIGHT = 1 << 0,
> > +	CEA_SPEAKER_LFE = 1 << 1,
> > +	CEA_SPEAKER_FRONT_CENTER = 1 << 2,
> > +	CEA_SPEAKER_REAR_LEFT_RIGHT = 1 << 3,
> > +	CEA_SPEAKER_REAR_CENTER = 1 << 4,
> > +	CEA_SPEAKER_FRONT_LEFT_RIGHT_CENTER = 1 << 5,
> > +	CEA_SPEAKER_REAR_LEFT_RIGHT_CENTER = 1 << 6,
> > +};
> > +
> > +struct cea_speaker_alloc {
> > +	uint8_t speakers; /* enum cea_speaker_alloc_item */
> > +	uint8_t reserved[2];
> > +} __attribute__((packed));
> > +
> >  enum edid_cea_data_type {
> >  	EDID_CEA_DATA_AUDIO = 1,
> >  	EDID_CEA_DATA_VIDEO = 2,
> > @@ -207,6 +222,7 @@ struct edid_cea_data_block {
> >  	union {
> >  		struct cea_sad sads[0];
> >  		struct cea_vsd vsds[0];
> > +		struct cea_speaker_alloc speakers[0];
> 
> Why [0]? Shouldn't this all be [1]?

EDID allows a variable number of these, so this struct has variable
length. The array must have a zero size otherwise sizeof-based
computations will be off (each of the members of the union have a
different size). The idea is to allow indexing any of these from the
end of the header block.

Alternatively, in all places where we access these fields we could cast
the edid_cea_data_block to a char *, then add sizeof(struct
edid_cea_data_block), then cast to a struct cea_speaker_alloc *. This
sounds a little annoying to write and to read, though.

> >  	} data;
> >  } __attribute__((packed));
> >  
> > @@ -295,6 +311,8 @@ 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_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,
> >  		      uint8_t flags);
> >  
> >