On Tue, May 14, 2019 at 09:49:03AM +0000, Shankar, Uma wrote: > > > >-----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! > 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. > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
