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

[Sharma, Shashank]

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