[Intel-gfx] [v2 03/14] drm: Parse HDR metadata info from EDID

[Shankar, Uma]

>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of
>Sharma, Shashank
>Sent: Thursday, December 20, 2018 11:47 PM
>To: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org;
>dri-devel at lists.freedesktop.org
>Cc: Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
><maarten.lankhorst at intel.com>
>Subject: Re: [v2 03/14] drm: Parse HDR metadata info from EDID
>
>Regards
>
>Shashank
>
>
>On 12/12/2018 2:08 AM, Uma Shankar wrote:
>> 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.
>>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c | 45
>+++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 45 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 106fd38..d12b74e 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -3818,6 +3818,49 @@ static void fixup_detailed_cea_mode_clock(struct
>drm_display_mode *mode)
>>   	mode->clock = clock;
>>   }
>I guess the previous patch (or a art of previous patch) should be merged in this
>patch.

Sure, will update this.

>> +static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db) {
>> +	if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG)
>> +		return false;
>> +
>> +	if (db[1] != HDR_STATIC_METADATA_BLOCK)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static uint16_t eotf_supported(const u8 *edid_ext)
>Why uint16 ? why not uint8_t ? this is clearly one byte.

Ok, will change this.

>> +{
>> +
>> +	return edid_ext[2] &
>> +		(BIT(HDMI_EOTF_TRADITIONAL_GAMMA_SDR) |
>Should we bother about SDR bit at all ? Why not just check HDR bits ?

As per spec, all of these are EOTF types. So lets update whatever is supported.
User should put correct EOTF for HDR from this list.

>> +		 BIT(HDMI_EOTF_TRADITIONAL_GAMMA_HDR) |
>> +		 BIT(HDMI_EOTF_SMPTE_ST2084));
>> +
>> +}
>> +
>> +static uint16_t hdr_metadata_type(const u8 *edid_ext) {
>Same uint8_t stuff

Ok. Will update.

>> +
>> +	return edid_ext[3] &
>> +		BIT(HDMI_STATIC_METADATA_TYPE1);
>> +}
>> +
>> +static void
>> +drm_parse_hdr_metadata_block(struct drm_connector *connector, const
>> +u8 *db) {
>> +	uint16_t len;
>> +
>> +	len = cea_db_payload_len(db);
>Length is  incorrect here for extended tag blocks, as this function doesn't account
>the extended tag byte's size. So the payload length is looking for is len - 1. May be
>a good time to add
>cea_extended_db_payload_len() ?

As per spec, the definition says length after this byte so it factors the extended tag byte
well, IIUC. Please correct me if my understanding is wrong.

Regards,
Uma Shankar

>> +	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]; }
>> +
>>   static void
>>   drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
>>   {
>> @@ -4468,6 +4511,8 @@ static void drm_parse_cea_ext(struct drm_connector
>*connector,
>>   			drm_parse_hdmi_forum_vsdb(connector, db);
>>   		if (cea_db_is_y420cmdb(db))
>>   			drm_parse_y420cmdb_bitmap(connector, db);
>> +		if (cea_db_is_hdmi_hdr_metadata_block(db))
>> +			drm_parse_hdr_metadata_block(connector, db);
>>   	}
>>   }
>>
>- Shashank
>
>_______________________________________________
>dri-devel mailing list
>dri-devel at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel