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

Submitted by Shankar, Uma on Dec. 11, 2018, 8:38 p.m.

Details

Message ID 1544560702-16447-4-git-send-email-uma.shankar@intel.com
State New
Headers show
Series "Add HDR Metadata Parsing and handling in DRM layer" ( rev: 2 ) in DRI devel

Commit Message

Shankar, Uma Dec. 11, 2018, 8:38 p.m.
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@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

Patch hide | download patch | download mbox

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;
 }
 
+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)
+{
+
+	return edid_ext[2] &
+		(BIT(HDMI_EOTF_TRADITIONAL_GAMMA_SDR) |
+		 BIT(HDMI_EOTF_TRADITIONAL_GAMMA_HDR) |
+		 BIT(HDMI_EOTF_SMPTE_ST2084));
+
+}
+
+static uint16_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)
+{
+	uint16_t len;
+
+	len = cea_db_payload_len(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];
+}
+
 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);
 	}
 }
 

Comments

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@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.
>   
> +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.
> +{
> +
> +	return edid_ext[2] &
> +		(BIT(HDMI_EOTF_TRADITIONAL_GAMMA_SDR) |
Should we bother about SDR bit at all ? Why not just check HDR bits ?
> +		 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
> +
> +	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() ?
> +	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
>-----Original Message-----

>From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of

>Sharma, Shashank

>Sent: Thursday, December 20, 2018 11:47 PM

>To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;

>dri-devel@lists.freedesktop.org

>Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten

><maarten.lankhorst@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@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@lists.freedesktop.org

>https://lists.freedesktop.org/mailman/listinfo/dri-devel
Regards

Shashank


On 1/8/2019 11:10 AM, Shankar, Uma wrote:
>
>> -----Original Message-----
>> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of
>> Sharma, Shashank
>> Sent: Thursday, December 20, 2018 11:47 PM
>> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
>> dri-devel@lists.freedesktop.org
>> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
>> <maarten.lankhorst@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@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>-----Original Message-----

>From: Sharma, Shashank

>Sent: Tuesday, January 8, 2019 11:26 AM

>To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;

>dri-devel@lists.freedesktop.org

>Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten

><maarten.lankhorst@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@lists.freedesktop.org] On

>>> Behalf Of Sharma, Shashank

>>> Sent: Thursday, December 20, 2018 11:47 PM

>>> To: Shankar, Uma <uma.shankar@intel.com>;

>>> intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org

>>> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten

>>> <maarten.lankhorst@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@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@lists.freedesktop.org

>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel