Regards

Shashank


On 1/8/2019 11:10 AM, Shankar, Uma wrote:

-----Original Message-----
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of
Sharma, Shashank
Sent: Thursday, December 20, 2018 11:47 PM
To: Shankar, Uma <uma.shan...@intel.com>; intel-...@lists.freedesktop.org;
dri-devel@lists.freedesktop.org
Cc: Syrjala, Ville <ville.syrj...@intel.com>; Lankhorst, Maarten
<maarten.lankho...@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.shan...@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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to