[v2,04/14] drm: Parse Colorimetry data block from EDID

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

Details

Message ID 1544560702-16447-5-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.
CEA 861.3 spec adds colorimetry data block for HDMI.
Parsing the block to get the colorimetry data from
panel.

v2: Rebase

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 24 ++++++++++++++++++++++++
 include/drm/drm_connector.h |  2 ++
 2 files changed, 26 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index d12b74e..344d8c1 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3818,6 +3818,28 @@  static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode)
 	mode->clock = clock;
 }
 
+static bool cea_db_is_hdmi_colorimetry_data_block(const u8 *db)
+{
+	if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG)
+		return false;
+
+	if (db[1] != COLORIMETRY_DATA_BLOCK)
+		return false;
+
+	return true;
+}
+
+static void
+drm_parse_colorimetry_data_block(struct drm_connector *connector, const u8 *db)
+{
+	struct drm_hdmi_info *info = &connector->display_info.hdmi;
+	uint16_t len;
+
+	len = cea_db_payload_len(db);
+	info->colorimetry = db[2];
+}
+
+
 static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db)
 {
 	if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG)
@@ -4513,6 +4535,8 @@  static void drm_parse_cea_ext(struct drm_connector *connector,
 			drm_parse_y420cmdb_bitmap(connector, db);
 		if (cea_db_is_hdmi_hdr_metadata_block(db))
 			drm_parse_hdr_metadata_block(connector, db);
+		if (cea_db_is_hdmi_colorimetry_data_block(db))
+			drm_parse_colorimetry_data_block(connector, db);
 	}
 }
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 2ee45dc..90ce364 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -206,6 +206,8 @@  struct drm_hdmi_info {
 
 	/** @y420_dc_modes: bitmap of deep color support index */
 	u8 y420_dc_modes;
+
+	u8 colorimetry;
 };
 
 /**

Comments

Regards

Shashank


On 12/12/2018 2:08 AM, Uma Shankar wrote:
> CEA 861.3 spec adds colorimetry data block for HDMI.
> Parsing the block to get the colorimetry data from
> panel.
>
> v2: Rebase
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>   drivers/gpu/drm/drm_edid.c  | 24 ++++++++++++++++++++++++
>   include/drm/drm_connector.h |  2 ++
>   2 files changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index d12b74e..344d8c1 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3818,6 +3818,28 @@ static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode)
>   	mode->clock = clock;
>   }
>   
> +static bool cea_db_is_hdmi_colorimetry_data_block(const u8 *db)
> +{
> +	if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG)
> +		return false;
> +
> +	if (db[1] != COLORIMETRY_DATA_BLOCK)
> +	
Again, the COLORIMETRY_DATA_BLOCK definition should be added into this 
patch.
> 	return false;
> +
> +	return true;
> +}
> +
> +static void
> +drm_parse_colorimetry_data_block(struct drm_connector *connector, const u8 *db)
> +{
> +	struct drm_hdmi_info *info = &connector->display_info.hdmi;
> +	uint16_t len;
> +
> +	len = cea_db_payload_len(db);
Again, the return value of this function doesn't account for extended db 
tag byte, so what we are looking for is len -1
> +	info->colorimetry = db[2];
colorimetry block is actually db[2] and db[3].bit7 (which represents 
DCI-P3 space), so we probably want to make info->colorimetryuint16_t
> +}
> +
> +
>   static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db)
>   {
>   	if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG)
> @@ -4513,6 +4535,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>   			drm_parse_y420cmdb_bitmap(connector, db);
>   		if (cea_db_is_hdmi_hdr_metadata_block(db))
>   			drm_parse_hdr_metadata_block(connector, db);
> +		if (cea_db_is_hdmi_colorimetry_data_block(db))
> +			drm_parse_colorimetry_data_block(connector, db);
>   	}
>   }
>   
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 2ee45dc..90ce364 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -206,6 +206,8 @@ struct drm_hdmi_info {
>   
>   	/** @y420_dc_modes: bitmap of deep color support index */
>   	u8 y420_dc_modes;
> +
Please add a one line description about this variable.
> +	u8 colorimetry;
>   };
>   
>   /**
Shashank
>-----Original Message-----

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

>Sharma, Shashank

>Sent: Thursday, December 20, 2018 11:54 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 04/14] drm: Parse Colorimetry data block from EDID

>

>Regards

>

>Shashank

>

>

>On 12/12/2018 2:08 AM, Uma Shankar wrote:

>> CEA 861.3 spec adds colorimetry data block for HDMI.

>> Parsing the block to get the colorimetry data from panel.

>>

>> v2: Rebase

>>

>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>

>> ---

>>   drivers/gpu/drm/drm_edid.c  | 24 ++++++++++++++++++++++++

>>   include/drm/drm_connector.h |  2 ++

>>   2 files changed, 26 insertions(+)

>>

>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c

>> index d12b74e..344d8c1 100644

>> --- a/drivers/gpu/drm/drm_edid.c

>> +++ b/drivers/gpu/drm/drm_edid.c

>> @@ -3818,6 +3818,28 @@ static void fixup_detailed_cea_mode_clock(struct

>drm_display_mode *mode)

>>   	mode->clock = clock;

>>   }

>>

>> +static bool cea_db_is_hdmi_colorimetry_data_block(const u8 *db) {

>> +	if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG)

>> +		return false;

>> +

>> +	if (db[1] != COLORIMETRY_DATA_BLOCK)

>> +

>Again, the COLORIMETRY_DATA_BLOCK definition should be added into this

>patch.


Ok, will do that.

>> 	return false;

>> +

>> +	return true;

>> +}

>> +

>> +static void

>> +drm_parse_colorimetry_data_block(struct drm_connector *connector,

>> +const u8 *db) {

>> +	struct drm_hdmi_info *info = &connector->display_info.hdmi;

>> +	uint16_t len;

>> +

>> +	len = cea_db_payload_len(db);

>Again, the return value of this function doesn't account for extended db tag byte,

>so what we are looking for is len -1


Will update this.

>> +	info->colorimetry = db[2];

>colorimetry block is actually db[2] and db[3].bit7 (which represents

>DCI-P3 space), so we probably want to make info->colorimetryuint16_t


3 BT2020RGB BT2020YCC BT2020cYCC AdobeRGB AdobeYCC601 sYCC601 xvYCC709 xvYCC601
4 F47=0 F46=0 F45=0 F44=0 MD3 MD2 MD1 MD0

This is how db[2] and db[3] is described in spec. There is not much clarity on F47. Not sure if they
are for DCI-P3, can you clarify once.

Regards,
Uma Shankar

>> +}

>> +

>> +

>>   static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db)

>>   {

>>   	if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG) @@ -4513,6

>+4535,8

>> @@ static void drm_parse_cea_ext(struct drm_connector *connector,

>>   			drm_parse_y420cmdb_bitmap(connector, db);

>>   		if (cea_db_is_hdmi_hdr_metadata_block(db))

>>   			drm_parse_hdr_metadata_block(connector, db);

>> +		if (cea_db_is_hdmi_colorimetry_data_block(db))

>> +			drm_parse_colorimetry_data_block(connector, db);

>>   	}

>>   }

>>

>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h

>> index 2ee45dc..90ce364 100644

>> --- a/include/drm/drm_connector.h

>> +++ b/include/drm/drm_connector.h

>> @@ -206,6 +206,8 @@ struct drm_hdmi_info {

>>

>>   	/** @y420_dc_modes: bitmap of deep color support index */

>>   	u8 y420_dc_modes;

>> +

>Please add a one line description about this variable.


Sure will update this.

>> +	u8 colorimetry;

>>   };

>>

>>   /**

>Shashank

>_______________________________________________

>dri-devel mailing list

>dri-devel@lists.freedesktop.org

>https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 1/8/2019 12:10 PM, 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:54 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 04/14] drm: Parse Colorimetry data block from EDID
>>
>> Regards
>>
>> Shashank
>>
>>
>> On 12/12/2018 2:08 AM, Uma Shankar wrote:
>>> CEA 861.3 spec adds colorimetry data block for HDMI.
>>> Parsing the block to get the colorimetry data from panel.
>>>
>>> v2: Rebase
>>>
>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>> ---
>>>    drivers/gpu/drm/drm_edid.c  | 24 ++++++++++++++++++++++++
>>>    include/drm/drm_connector.h |  2 ++
>>>    2 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index d12b74e..344d8c1 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -3818,6 +3818,28 @@ static void fixup_detailed_cea_mode_clock(struct
>> drm_display_mode *mode)
>>>    	mode->clock = clock;
>>>    }
>>>
>>> +static bool cea_db_is_hdmi_colorimetry_data_block(const u8 *db) {
>>> +	if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG)
>>> +		return false;
>>> +
>>> +	if (db[1] != COLORIMETRY_DATA_BLOCK)
>>> +
>> Again, the COLORIMETRY_DATA_BLOCK definition should be added into this
>> patch.
> Ok, will do that.
>
>>> 	return false;
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +static void
>>> +drm_parse_colorimetry_data_block(struct drm_connector *connector,
>>> +const u8 *db) {
>>> +	struct drm_hdmi_info *info = &connector->display_info.hdmi;
>>> +	uint16_t len;
>>> +
>>> +	len = cea_db_payload_len(db);
>> Again, the return value of this function doesn't account for extended db tag byte,
>> so what we are looking for is len -1
> Will update this.
>
>>> +	info->colorimetry = db[2];
>> colorimetry block is actually db[2] and db[3].bit7 (which represents
>> DCI-P3 space), so we probably want to make info->colorimetryuint16_t
> 3 BT2020RGB BT2020YCC BT2020cYCC AdobeRGB AdobeYCC601 sYCC601 xvYCC709 xvYCC601
> 4 F47=0 F46=0 F45=0 F44=0 MD3 MD2 MD1 MD0
>
> This is how db[2] and db[3] is described in spec. There is not much clarity on F47. Not sure if they
> are for DCI-P3, can you clarify once.
Ah, check CEA-861-G table 70, there there is no F47, but I could see 
DCI-P3 in that place, which spec is that you are following right now ?
- Shashank
> Regards,
> Uma Shankar
>
>>> +}
>>> +
>>> +
>>>    static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db)
>>>    {
>>>    	if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG) @@ -4513,6
>> +4535,8
>>> @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>>>    			drm_parse_y420cmdb_bitmap(connector, db);
>>>    		if (cea_db_is_hdmi_hdr_metadata_block(db))
>>>    			drm_parse_hdr_metadata_block(connector, db);
>>> +		if (cea_db_is_hdmi_colorimetry_data_block(db))
>>> +			drm_parse_colorimetry_data_block(connector, db);
>>>    	}
>>>    }
>>>
>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>> index 2ee45dc..90ce364 100644
>>> --- a/include/drm/drm_connector.h
>>> +++ b/include/drm/drm_connector.h
>>> @@ -206,6 +206,8 @@ struct drm_hdmi_info {
>>>
>>>    	/** @y420_dc_modes: bitmap of deep color support index */
>>>    	u8 y420_dc_modes;
>>> +
>> Please add a one line description about this variable.
> Sure will update this.
>
>>> +	u8 colorimetry;
>>>    };
>>>
>>>    /**
>> Shashank
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>-----Original Message-----

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

>Sharma, Shashank

>Sent: Tuesday, January 8, 2019 12:27 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 04/14] drm: Parse Colorimetry data block from EDID

>

>

>

>On 1/8/2019 12:10 PM, 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:54 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 04/14] drm: Parse Colorimetry data block from EDID

>>>

>>> Regards

>>>

>>> Shashank

>>>

>>>

>>> On 12/12/2018 2:08 AM, Uma Shankar wrote:

>>>> CEA 861.3 spec adds colorimetry data block for HDMI.

>>>> Parsing the block to get the colorimetry data from panel.

>>>>

>>>> v2: Rebase

>>>>

>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>

>>>> ---

>>>>    drivers/gpu/drm/drm_edid.c  | 24 ++++++++++++++++++++++++

>>>>    include/drm/drm_connector.h |  2 ++

>>>>    2 files changed, 26 insertions(+)

>>>>

>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c

>>>> index d12b74e..344d8c1 100644

>>>> --- a/drivers/gpu/drm/drm_edid.c

>>>> +++ b/drivers/gpu/drm/drm_edid.c

>>>> @@ -3818,6 +3818,28 @@ static void

>>>> fixup_detailed_cea_mode_clock(struct

>>> drm_display_mode *mode)

>>>>    	mode->clock = clock;

>>>>    }

>>>>

>>>> +static bool cea_db_is_hdmi_colorimetry_data_block(const u8 *db) {

>>>> +	if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG)

>>>> +		return false;

>>>> +

>>>> +	if (db[1] != COLORIMETRY_DATA_BLOCK)

>>>> +

>>> Again, the COLORIMETRY_DATA_BLOCK definition should be added into

>>> this patch.

>> Ok, will do that.

>>

>>>> 	return false;

>>>> +

>>>> +	return true;

>>>> +}

>>>> +

>>>> +static void

>>>> +drm_parse_colorimetry_data_block(struct drm_connector *connector,

>>>> +const u8 *db) {

>>>> +	struct drm_hdmi_info *info = &connector->display_info.hdmi;

>>>> +	uint16_t len;

>>>> +

>>>> +	len = cea_db_payload_len(db);

>>> Again, the return value of this function doesn't account for extended

>>> db tag byte, so what we are looking for is len -1

>> Will update this.

>>

>>>> +	info->colorimetry = db[2];

>>> colorimetry block is actually db[2] and db[3].bit7 (which represents

>>> DCI-P3 space), so we probably want to make info->colorimetryuint16_t

>> 3 BT2020RGB BT2020YCC BT2020cYCC AdobeRGB AdobeYCC601 sYCC601

>xvYCC709

>> xvYCC601

>> 4 F47=0 F46=0 F45=0 F44=0 MD3 MD2 MD1 MD0

>>

>> This is how db[2] and db[3] is described in spec. There is not much

>> clarity on F47. Not sure if they are for DCI-P3, can you clarify once.

>Ah, check CEA-861-G table 70, there there is no F47, but I could see

>DCI-P3 in that place, which spec is that you are following right now ?


I am looking to 861-F. So seems like its updated in 861-G ?

Regards,
Uma Shankar

>- Shashank

>> Regards,

>> Uma Shankar

>>

>>>> +}

>>>> +

>>>> +

>>>>    static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db)

>>>>    {

>>>>    	if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG) @@ -4513,6

>>> +4535,8

>>>> @@ static void drm_parse_cea_ext(struct drm_connector *connector,

>>>>    			drm_parse_y420cmdb_bitmap(connector, db);

>>>>    		if (cea_db_is_hdmi_hdr_metadata_block(db))

>>>>    			drm_parse_hdr_metadata_block(connector, db);

>>>> +		if (cea_db_is_hdmi_colorimetry_data_block(db))

>>>> +			drm_parse_colorimetry_data_block(connector, db);

>>>>    	}

>>>>    }

>>>>

>>>> diff --git a/include/drm/drm_connector.h

>>>> b/include/drm/drm_connector.h index 2ee45dc..90ce364 100644

>>>> --- a/include/drm/drm_connector.h

>>>> +++ b/include/drm/drm_connector.h

>>>> @@ -206,6 +206,8 @@ struct drm_hdmi_info {

>>>>

>>>>    	/** @y420_dc_modes: bitmap of deep color support index */

>>>>    	u8 y420_dc_modes;

>>>> +

>>> Please add a one line description about this variable.

>> Sure will update this.

>>

>>>> +	u8 colorimetry;

>>>>    };

>>>>

>>>>    /**

>>> Shashank

>>> _______________________________________________

>>> dri-devel mailing list

>>> dri-devel@lists.freedesktop.org

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

>

>_______________________________________________

>dri-devel mailing list

>dri-devel@lists.freedesktop.org

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