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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx