Re: [v9 03/13] drm: Parse HDR metadata info from EDID

From: "Shankar, Uma" <uma.shankar@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: "dcastagna@chromium.org" <dcastagna@chromium.org>,
	"jonas@kwiboo.se" <jonas@kwiboo.se>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"seanpaul@chromium.org" <seanpaul@chromium.org>,
	"Syrjala, Ville" <ville.syrjala@intel.com>,
	"Lankhorst, Maarten" <maarten.lankhorst@intel.com>
Subject: Re: [v9 03/13] drm: Parse HDR metadata info from EDID
Date: Tue, 14 May 2019 13:26:55 +0000	[thread overview]
Message-ID: <E7C9878FBA1C6D42A1CA3F62AEB6945F820257D3@BGSMSX104.gar.corp.intel.com> (raw)
In-Reply-To: <20190514124032.GY24299@intel.com>

>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Tuesday, May 14, 2019 12:49 AM
>> >To: Shankar, Uma <uma.shankar@intel.com>
>> >Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
>> >dcastagna@chromium.org; jonas@kwiboo.se; emil.l.velikov@gmail.com;
>> >seanpaul@chromium.org; Syrjala, Ville <ville.syrjala@intel.com>;
>> >Lankhorst, Maarten <maarten.lankhorst@intel.com>
>> >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@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 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@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>
>--
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx