>-----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. 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. 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
