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

[Shankar, Uma]

>-----Original Message-----
>From: Sharma, Shashank
>Sent: Tuesday, January 8, 2019 11:26 AM
>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 1/8/2019 11:10 AM, Shankar, Uma wrote:
>>
>>> -----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.
>This is how the data is arranged in a normal CEA DB and Extended CEA DB
>+--------------------+   +-----------------+
>|Tag|Length= 3     |   |Tag|Length=3     |
>+--------------------+   +-----------------+
>| Data               |   |Extended tag     |
>+--------------------+   +-----------------+
>| Data               |   |Data             |
>+--------------------+   +-----------------+
>| Data               |   |Data             |
>+--------------------+   +-----------------+
>
>This function cea_db_payload_len() was written before the CEA extended blocks
>were introduced, so it is unaware of Extended tag code, and assumes its a part of
>data. But in case of extended tag block the actual data length is 3 -1 = 2. You can
>have a look at how we are parsing the YCBCR 4:2:0 blocks. That's why I made this
>comment "may be a good time to add cea_extended_db_payload_len() function"
>or enhance the existing function to reflect the extended tag.

Oh ok got it, thanks for the explanation. Will update the patch accordingly.

Regards,
Uma Shankar

>- Shashank
>>
>> 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