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