[v6,03/13] drm: Parse Colorimetry data block from EDID

Submitted by Shankar, Uma on March 20, 2019, 10:48 a.m.

Details

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

Commit Message

Shankar, Uma March 20, 2019, 10:48 a.m.
CEA 861.3 spec adds colorimetry data block for HDMI.
Parsing the block to get the colorimetry data from
panel.

v2: Rebase

v3: No Change

v4: Addressed Shashank's review comments. Updated
colorimetry field to 16 bit as DCI-P3 got added
in CEA 861-G spec, as pointed out by Shashank.

v5: Fixed checkpatch warnings with --strict option.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 25 +++++++++++++++++++++++++
 include/drm/drm_connector.h |  3 +++
 2 files changed, 28 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index fd8a621a..676569b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2840,6 +2840,7 @@  static int drm_cvt_modes(struct drm_connector *connector,
 #define VIDEO_BLOCK     0x02
 #define VENDOR_BLOCK    0x03
 #define SPEAKER_BLOCK	0x04
+#define COLORIMETRY_DATA_BLOCK		0x5
 #define HDR_STATIC_METADATA_BLOCK	0x6
 #define USE_EXTENDED_TAG 0x07
 #define EXT_VIDEO_CAPABILITY_BLOCK 0x00
@@ -3829,6 +3830,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) != USE_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;
+	u16 len;
+
+	len = cea_db_payload_len_ext(db);
+	/* As per CEA 861-G spec */
+	info->colorimetry = ((db[3] & (0x1 << 7)) << 1) | db[2];
+}
+
 static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db)
 {
 	if (cea_db_tag(db) != USE_EXTENDED_TAG)
@@ -4498,6 +4521,8 @@  static void drm_parse_cea_ext(struct drm_connector *connector,
 			drm_parse_vcdb(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 29388bd..94f926e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -206,6 +206,9 @@  struct drm_hdmi_info {
 
 	/** @y420_dc_modes: bitmap of deep color support index */
 	u8 y420_dc_modes;
+
+	/* @colorimetry: bitmap of supported colorimetry modes */
+	u16 colorimetry;
 };
 
 /**

Comments

Hi Uma,

On Wed, Mar 20, 2019 at 04:18:16PM +0530, Uma Shankar wrote:
> CEA 861.3 spec adds colorimetry data block for HDMI.
> Parsing the block to get the colorimetry data from
> panel.
> 

While code which parses these parts (patches 2 and 3) out of EDID
could be useful - it doesn't seem to actually be used anywhere in the
kernel (sorry if I missed it), and I'm not sure it will/can be unless
we expose more properties.

The discussion last time leant towards a more useful/generic userspace
library for EDID parsing, so perhaps patches 2 and 3 should be
dropped.

Did you have an intended use for them in the kernel?

Thanks,
-Brian

> v2: Rebase
> 
> v3: No Change
> 
> v4: Addressed Shashank's review comments. Updated
> colorimetry field to 16 bit as DCI-P3 got added
> in CEA 861-G spec, as pointed out by Shashank.
> 
> v5: Fixed checkpatch warnings with --strict option.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 25 +++++++++++++++++++++++++
>  include/drm/drm_connector.h |  3 +++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index fd8a621a..676569b 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2840,6 +2840,7 @@ static int drm_cvt_modes(struct drm_connector *connector,
>  #define VIDEO_BLOCK     0x02
>  #define VENDOR_BLOCK    0x03
>  #define SPEAKER_BLOCK	0x04
> +#define COLORIMETRY_DATA_BLOCK		0x5
>  #define HDR_STATIC_METADATA_BLOCK	0x6
>  #define USE_EXTENDED_TAG 0x07
>  #define EXT_VIDEO_CAPABILITY_BLOCK 0x00
> @@ -3829,6 +3830,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) != USE_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;
> +	u16 len;
> +
> +	len = cea_db_payload_len_ext(db);
> +	/* As per CEA 861-G spec */
> +	info->colorimetry = ((db[3] & (0x1 << 7)) << 1) | db[2];
> +}
> +
>  static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db)
>  {
>  	if (cea_db_tag(db) != USE_EXTENDED_TAG)
> @@ -4498,6 +4521,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  			drm_parse_vcdb(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 29388bd..94f926e 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -206,6 +206,9 @@ struct drm_hdmi_info {
>  
>  	/** @y420_dc_modes: bitmap of deep color support index */
>  	u8 y420_dc_modes;
> +
> +	/* @colorimetry: bitmap of supported colorimetry modes */
> +	u16 colorimetry;
>  };
>  
>  /**
> -- 
> 1.9.1
>
>-----Original Message-----
>From: Brian Starkey [mailto:Brian.Starkey@arm.com]
>Sent: Thursday, March 21, 2019 4:47 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Lankhorst,
>Maarten <maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
>Sharma, Shashank <shashank.sharma@intel.com>; emil.l.velikov@gmail.com; Liviu
>Dudau <Liviu.Dudau@arm.com>; nd <nd@arm.com>
>Subject: Re: [v6 03/13] drm: Parse Colorimetry data block from EDID
>
>Hi Uma,
>
>On Wed, Mar 20, 2019 at 04:18:16PM +0530, Uma Shankar wrote:
>> CEA 861.3 spec adds colorimetry data block for HDMI.
>> Parsing the block to get the colorimetry data from panel.
>>
>
>While code which parses these parts (patches 2 and 3) out of EDID could be useful - it
>doesn't seem to actually be used anywhere in the kernel (sorry if I missed it), and I'm
>not sure it will/can be unless we expose more properties.
>
>The discussion last time leant towards a more useful/generic userspace library for
>EDID parsing, so perhaps patches 2 and 3 should be dropped.
>
>Did you have an intended use for them in the kernel?

The idea was to use them for any kernel defaults or some state and error checking, when we
have more such types exposed. But yeah, may be till we have it I can drop it for now and
re-introduced them later when needed.

Thanks & Regards,
Uma Shankar

>Thanks,
>-Brian
>
>> v2: Rebase
>>
>> v3: No Change
>>
>> v4: Addressed Shashank's review comments. Updated colorimetry field to
>> 16 bit as DCI-P3 got added in CEA 861-G spec, as pointed out by
>> Shashank.
>>
>> v5: Fixed checkpatch warnings with --strict option.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c  | 25 +++++++++++++++++++++++++
>> include/drm/drm_connector.h |  3 +++
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index fd8a621a..676569b 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2840,6 +2840,7 @@ static int drm_cvt_modes(struct drm_connector
>*connector,
>>  #define VIDEO_BLOCK     0x02
>>  #define VENDOR_BLOCK    0x03
>>  #define SPEAKER_BLOCK	0x04
>> +#define COLORIMETRY_DATA_BLOCK		0x5
>>  #define HDR_STATIC_METADATA_BLOCK	0x6
>>  #define USE_EXTENDED_TAG 0x07
>>  #define EXT_VIDEO_CAPABILITY_BLOCK 0x00 @@ -3829,6 +3830,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) != USE_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;
>> +	u16 len;
>> +
>> +	len = cea_db_payload_len_ext(db);
>> +	/* As per CEA 861-G spec */
>> +	info->colorimetry = ((db[3] & (0x1 << 7)) << 1) | db[2]; }
>> +
>>  static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db)  {
>>  	if (cea_db_tag(db) != USE_EXTENDED_TAG) @@ -4498,6 +4521,8 @@ static
>> void drm_parse_cea_ext(struct drm_connector *connector,
>>  			drm_parse_vcdb(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 29388bd..94f926e 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -206,6 +206,9 @@ struct drm_hdmi_info {
>>
>>  	/** @y420_dc_modes: bitmap of deep color support index */
>>  	u8 y420_dc_modes;
>> +
>> +	/* @colorimetry: bitmap of supported colorimetry modes */
>> +	u16 colorimetry;
>>  };
>>
>>  /**
>> --
>> 1.9.1
>>