[v8,02/10] drm: Parse HDR metadata info from EDID

Submitted by Shankar, Uma on April 9, 2019, 4:44 p.m.

Details

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

Commit Message

Shankar, Uma April 9, 2019, 4:44 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.

v3: No Change

v4: Addressed Shashank's review comments

v5: Addressed Shashank's comment and added his RB.

v6: Addressed Jonas Karlman review comments.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 2c22ea4..1fc371b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2830,6 +2830,7 @@  static int drm_cvt_modes(struct drm_connector *connector,
 #define VIDEO_BLOCK     0x02
 #define VENDOR_BLOCK    0x03
 #define SPEAKER_BLOCK	0x04
+#define HDR_STATIC_METADATA_BLOCK	0x6
 #define USE_EXTENDED_TAG 0x07
 #define EXT_VIDEO_CAPABILITY_BLOCK 0x00
 #define EXT_VIDEO_DATA_BLOCK_420	0x0E
@@ -3577,6 +3578,12 @@  static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
 }
 
 static int
+cea_db_payload_len_ext(const u8 *db)
+{
+	return (db[0] & 0x1f) - 1;
+}
+
+static int
 cea_db_extended_tag(const u8 *db)
 {
 	return db[1];
@@ -3812,6 +3819,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) != USE_EXTENDED_TAG)
+		return false;
+
+	if (db[1] != HDR_STATIC_METADATA_BLOCK)
+		return false;
+
+	return true;
+}
+
+static uint8_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 uint8_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)
+{
+	u16 len;
+
+	len = cea_db_payload_len_ext(db);
+	connector->hdr_sink_metadata.hdmi_type1.eotf = eotf_supported(db);
+	connector->hdr_sink_metadata.hdmi_type1.metadata_type =
+					hdr_metadata_type(db);
+
+	if (len >= 4)
+		connector->hdr_sink_metadata.hdmi_type1.max_cll = db[4];
+	if (len >= 5)
+		connector->hdr_sink_metadata.hdmi_type1.max_fall = db[5];
+	if (len >= 6)
+		connector->hdr_sink_metadata.hdmi_type1.min_cll = db[6];
+}
+
 static void
 drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
 {
@@ -4439,6 +4489,8 @@  static void drm_parse_cea_ext(struct drm_connector *connector,
 			drm_parse_y420cmdb_bitmap(connector, db);
 		if (cea_db_is_vcdb(db))
 			drm_parse_vcdb(connector, db);
+		if (cea_db_is_hdmi_hdr_metadata_block(db))
+			drm_parse_hdr_metadata_block(connector, db);
 	}
 }
 

Comments

On 4/9/19 12:44 PM, 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.

> 

> v3: No Change

> 

> v4: Addressed Shashank's review comments

> 

> v5: Addressed Shashank's comment and added his RB.

> 

> v6: Addressed Jonas Karlman review comments.

> 

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

> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

> ---

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

>   1 file changed, 52 insertions(+)

> 

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

> index 2c22ea4..1fc371b 100644

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

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

> @@ -2830,6 +2830,7 @@ static int drm_cvt_modes(struct drm_connector *connector,

>   #define VIDEO_BLOCK     0x02

>   #define VENDOR_BLOCK    0x03

>   #define SPEAKER_BLOCK	0x04

> +#define HDR_STATIC_METADATA_BLOCK	0x6

>   #define USE_EXTENDED_TAG 0x07

>   #define EXT_VIDEO_CAPABILITY_BLOCK 0x00

>   #define EXT_VIDEO_DATA_BLOCK_420	0x0E

> @@ -3577,6 +3578,12 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,

>   }

>   

>   static int

> +cea_db_payload_len_ext(const u8 *db)

> +{

> +	return (db[0] & 0x1f) - 1;

> +}

> +

> +static int

>   cea_db_extended_tag(const u8 *db)

>   {

>   	return db[1];

> @@ -3812,6 +3819,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) != USE_EXTENDED_TAG)

> +		return false;

> +

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

> +		return false;


Shouldn't this just be cea_db_extended_tag(db) != HDR_STATIC_METADATA_BLOCK?

Also, the other functions all check that we're given a valid here here with:

if (!cea_db_payload_len_ext(db))
     return false;

Is there any reason this isn't done here?


> +

> +	return true;

> +}

> +

> +static uint8_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 uint8_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)

> +{

> +	u16 len;

> +

> +	len = cea_db_payload_len_ext(db);

> +	connector->hdr_sink_metadata.hdmi_type1.eotf = eotf_supported(db);

> +	connector->hdr_sink_metadata.hdmi_type1.metadata_type =

> +					hdr_metadata_type(db);


While metadata_type is assigned here like it should be, it isn't 
assigned to the outer metadata_type in the hdr_sink_metadata. I also 
can't see anything in the other patches that assigns this anywhere.

Shouldn't it also be set here?

> +

> +	if (len >= 4)

> +		connector->hdr_sink_metadata.hdmi_type1.max_cll = db[4];

> +	if (len >= 5)

> +		connector->hdr_sink_metadata.hdmi_type1.max_fall = db[5];

> +	if (len >= 6)

> +		connector->hdr_sink_metadata.hdmi_type1.min_cll = db[6];

> +}

> +

>   static void

>   drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)

>   {

> @@ -4439,6 +4489,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,

>   			drm_parse_y420cmdb_bitmap(connector, db);

>   		if (cea_db_is_vcdb(db))

>   			drm_parse_vcdb(connector, db);

> +		if (cea_db_is_hdmi_hdr_metadata_block(db))

> +			drm_parse_hdr_metadata_block(connector, db);

>   	}

>   }

>   

> 


Nicholas Kazlauskas