On Tue, May 14, 2019 at 09:49:03AM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:[email protected]]
> >Sent: Tuesday, May 14, 2019 12:49 AM
> >To: Shankar, Uma <[email protected]>
> >Cc: [email protected]; [email protected];
> >[email protected]; [email protected]; [email protected];
> >[email protected]; Syrjala, Ville <[email protected]>; Lankhorst,
> >Maarten
> ><[email protected]>
> >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 <[email protected]>
> >> Reviewed-by: Shashank Sharma <[email protected]>
> >> ---
> >> 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
> >> [email protected]
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >--
> >Ville Syrjälä
> >Intel
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
