diff mbox series

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

Message ID 1557340733-9629-4-git-send-email-uma.shankar@intel.com (mailing list archive)
State New, archived
Headers show
Series Add HDR Metadata Parsing and handling in DRM layer | expand

Commit Message

Shankar, Uma May 8, 2019, 6:38 p.m. UTC 
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(+)

Comments

Ville Syrjälä May 13, 2019, 7:19 p.m. UTC | #1 
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.

> +
> +	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 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.

> +}
> +
>  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
Shankar, Uma May 14, 2019, 9:49 a.m. UTC | #2 
>-----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.
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
Ville Syrjälä May 14, 2019, 12:40 p.m. UTC | #3 
On Tue, May 14, 2019 at 09:49:03AM +0000, Shankar, Uma wrote:
> 
> 
> >-----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!

> 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@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >--
> >Ville Syrjälä
> >Intel
Shankar, Uma May 14, 2019, 1:26 p.m. UTC | #4 
>>
>> >-----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
diff mbox series

Patch

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);
+
+	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];
+}
+
 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);
 	}
 }