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

[Martin Peres]

On 03/06/2019 16:28, Ser, Simon wrote:
> 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.

ACK

> 
>>> +	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.

I see... Well, sounds like an acceptable reasoning. Is it documented on
top of the union already?

Regardless:

Reviewed-by: Martin Peres <martin.peres at linux.intel.com>
> 
>>>  	} 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);
>>>  
>>>