Re: [v10 03/12] drm: Parse HDR metadata info from EDID

Ville Syrjälä
2019-05-15T19:36:17Z

On Tue, May 14, 2019 at 11:06:25PM +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.
>
> v7: Adressed Ville's review comments and fixed the issue
> with length handling.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
> drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 852bdd8..2e0b5be 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
> @@ -3834,6 +3835,52 @@ 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(db);
> + if (len >= 3) {

I believe in other cases we've put the length check for the
mandatory bytes into the cea_db_is_foo() function. Would be
good to follow the path laid out by existing code here too.

> + 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 +4508,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

--
Ville Syrjälä
Intel