>> >> >-----Original Message----- >> >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >> >Sent: Tuesday, May 14, 2019 12:49 AM >> >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; >> >dcastagna@xxxxxxxxxxxx; jonas@xxxxxxxxx; emil.l.velikov@xxxxxxxxx; >> >seanpaul@xxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; >> >Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx> >> >Subject: Re: [v9 03/13] drm: Parse HDR metadata info from EDID >> > >> >On Thu, May 09, 2019 at 12:08:43AM +0530, 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@xxxxxxxxx> >> >> Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> >> >> --- >> >> 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 852bdd8..fe2c29b 100644 >> >> --- a/drivers/gpu/drm/drm_edid.c >> >> +++ b/drivers/gpu/drm/drm_edid.c >> >> @@ -2852,6 +2852,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 >> >> @@ -3599,6 +3600,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]; >> >> @@ -3834,6 +3841,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); >> > >> >Length checks missing for this stuff. >> >> The above 2 are mandatory bytes , so this will always be there in this block. > >You can only make that claim if the EDID is not malicious or corrupted. >Never trust anything coming from an external source! Ok Got it, will have a check to ensure that we have a reliable EDID. >> Byte 5 to 7 are optional and depends on length field. So we should not >> need a length check here for above 2 fields. Hope my understanding is right. >> >> >> + >> >> + 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]; >> > >> >All the length checks seem to be off by one due to cea_db_payload_len_ext(). >> >I think just dropping cea_db_payload_len_ext() would make things >> >better since a) nothing else has that, b) db still points at the >> >start of the whole data block instead of the payload. >> >> The length field adds/includes the extended tag byte while reporting >> the size of block. So we need to remove 1 byte to get the correct size of data. A >value of n = 3 means bytes 5 to 7 are not present. >> >> >The while payload vs. block length thing is already confusing anyway. >> >I have a feeling we should replace cea_db_payload_len() with >> >something that returns the length of the whole data block. That way >> >the db[index] vs. length checks would start to make some sense to the casual >reader. >> >> I hope with above understanding, in db[index], index just tells the >> byte number for a particular field which matches exactly to spec offsets. > >Not quite sure what you're saying. > >What I'm saying is that with eg. n=6 (ie. the full block) your code ends up with len==5, >and then it will not read min_cll even though it should. Hmm yeah, the checks are off. Will fix these and drop the cea_db_payload_len_ext(). Thanks Ville. Regards, Uma Shankar >> Regards, >> Uma Shankar >> >> > >> >> +} >> >> + >> >> static void >> >> drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const >> >> u8 >> >> *db) { @@ -4461,6 +4511,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); >> >> } >> >> } >> >> >> >> -- >> >> 1.9.1 >> >> >> >> _______________________________________________ >> >> dri-devel mailing list >> >> dri-devel@xxxxxxxxxxxxxxxxxxxxx >> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > >> >-- >> >Ville Syrjälä >> >Intel > >-- >Ville Syrjälä >Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
