[Intel-gfx] [v5 02/13] drm: Parse HDR metadata info from EDID

[Shankar, Uma]

>-----Original Message-----
>From: Sharma, Shashank
>Sent: Friday, March 15, 2019 12:56 PM
>To: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org; dri-
>devel at lists.freedesktop.org
>Cc: Lankhorst, Maarten <maarten.lankhorst at intel.com>; Syrjala, Ville
><ville.syrjala at intel.com>; emil.l.velikov at gmail.com; brian.starkey at arm.com;
>Liviu.Dudau at arm.com
>Subject: RE: [v5 02/13] drm: Parse HDR metadata info from EDID
>
>
>
>> -----Original Message-----
>> From: Shankar, Uma
>> Sent: Monday, March 11, 2019 9:28 AM
>> To: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
>> Cc: Lankhorst, Maarten <maarten.lankhorst at intel.com>; Syrjala, Ville
>> <ville.syrjala at intel.com>; Sharma, Shashank
>> <shashank.sharma at intel.com>; emil.l.velikov at gmail.com;
>> brian.starkey at arm.com; Liviu.Dudau at arm.com; Shankar, Uma
>> <uma.shankar at intel.com>
>> Subject: [v5 02/13] drm: Parse HDR metadata info from EDID
>>
>> HDR metadata block is introduced in CEA-861.3 spec.
>> Parsing the same to get the panel's HDR metadata.
>>
>> v2: Rebase and added Ville's POC changes to the patch.
>>
>> v3: No Change
>>
>> v4: Addressed Shashank's review comments
>>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 52
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 52 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index
>> 5f14253..c5a81b8 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2834,6 +2834,7 @@ static int drm_cvt_modes(struct drm_connector
>> *connector,
>>  #define VIDEO_BLOCK     0x02
>>  #define VENDOR_BLOCK    0x03
>>  #define SPEAKER_BLOCK	0x04
>> +#define HDR_STATIC_METADATA_BLOCK	0x6
>>  #define USE_EXTENDED_TAG 0x07
>>  #define EXT_VIDEO_CAPABILITY_BLOCK 0x00
>>  #define EXT_VIDEO_DATA_BLOCK_420	0x0E
>> @@ -3581,6 +3582,12 @@ static int add_3d_struct_modes(struct
>> drm_connector *connector, u16 structure,  }
>>
>>  static int
>> +cea_db_payload_len_ext(const u8 *db)
>> +{
>> +	return (db[0] & 0x1f) - 1;
>You might have already checked with checkpatch, but can we cross check the space
>before '-' ?

Yes, this seems ok. Space is there before operator.

>> +}
>> +
>> +static int
>>  cea_db_extended_tag(const u8 *db)
>>  {
>>  	return db[1];
>> @@ -3816,6 +3823,49 @@ static void
>> fixup_detailed_cea_mode_clock(struct
>> drm_display_mode *mode)
>>  	mode->clock = clock;
>>  }
>>
>> +static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db) {
>> +	if (cea_db_tag(db) != USE_EXTENDED_TAG)
>> +		return false;
>> +
>> +	if (db[1] != HDR_STATIC_METADATA_BLOCK)
>same here,  looks like we need spaces around !=, or may be this is due to my mail
>client  ?

This seems to be issue with mail client. Looks like it's changing alignment on outlook.
The changes are ok and spaces are there before operators. 

Executed and checked with checkpatch as well. There are some other issues flagged with
--strict option, will be fixing those as part of v6.

Regards,
Uma Shankar

>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static uint8_t eotf_supported(const u8 *edid_ext) {
>> +
>> +	return edid_ext[2] &
>> +		(BIT(HDMI_EOTF_TRADITIONAL_GAMMA_SDR) |
>> +		 BIT(HDMI_EOTF_TRADITIONAL_GAMMA_HDR) |
>Again, space before the '|' sign, please excuse me if this is false alarm ...

Same here.

>> +		 BIT(HDMI_EOTF_SMPTE_ST2084));
>> +
>> +}
>> +
>> +static uint8_t hdr_metadata_type(const u8 *edid_ext) {
>> +
>> +	return edid_ext[3] &
>> +		BIT(HDMI_STATIC_METADATA_TYPE1);
>> +}
>> +
>> +static void
>> +drm_parse_hdr_metadata_block(struct drm_connector *connector, const
>> +u8
>> +*db) {
>Shouldn't this variable go to line above ?
>> +	uint16_t len;
>> +
>> +	len = cea_db_payload_len_ext(db);
>> +	connector->hdr_metadata.eotf = eotf_supported(db);
>> +	connector->hdr_metadata.metadata_type = hdr_metadata_type(db);
>> +
>> +	if (len >= 5)
>> +		connector->hdr_metadata.max_fall = db[5];
>> +	if (len >= 4)
>> +		connector->hdr_metadata.max_cll = db[4]; }
>This '}' certainly should not be here.

Same here as well.

>> +
>>  static void
>>  drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8
>> *db)  { @@
>> -4443,6 +4493,8 @@ static void drm_parse_cea_ext(struct drm_connector
>> *connector,
>>  			drm_parse_y420cmdb_bitmap(connector, db);
>>  		if (cea_db_is_vcdb(db))
>>  			drm_parse_vcdb(connector, db);
>> +		if (cea_db_is_hdmi_hdr_metadata_block(db))
>> +			drm_parse_hdr_metadata_block(connector, db);
>- Shashank
>>  	}
>>  }
>>
>> --
>> 1.9.1