>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
>Sent: Tuesday, May 14, 2019 12:49 AM
>To: Shankar, Uma <uma.shan...@intel.com>
>Cc: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
>dcasta...@chromium.org; jo...@kwiboo.se; emil.l.veli...@gmail.com;
>seanp...@chromium.org; Syrjala, Ville <ville.syrj...@intel.com>; Lankhorst,
>Maarten
><maarten.lankho...@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.shan...@intel.com>
>> Reviewed-by: Shashank Sharma <shashank.sha...@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.
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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>--
>Ville Syrjälä
>Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
