Re: [v2 01/14] drm: Add HDR source metadata property

["Sharma, Shashank" <shashank.sharma@xxxxxxxxx>]

Regards

Shashank


On 12/12/2018 2:08 AM, Uma Shankar wrote:
> This patch adds a blob property to get HDR metadata
> information from userspace. This will be send as part
> of AVI Infoframe to panel.
>
> v2: Rebase and modified the metadata structure elements
> as per Ville's POC changes.
>
> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> ---
>   drivers/gpu/drm/drm_connector.c |  6 ++++++
>   include/drm/drm_connector.h     | 10 ++++++++++
>   include/drm/drm_mode_config.h   |  6 ++++++
>   include/linux/hdmi.h            | 10 ++++++++++
>   include/uapi/drm/drm_mode.h     | 16 ++++++++++++++++
>   5 files changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index da8ae80..361fcda 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1027,6 +1027,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
>   		return -ENOMEM;
>   	dev->mode_config.non_desktop_property = prop;
>   
> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> +				   "HDR_SOURCE_METADATA", 0);
I see that the compositor would be using this blob to setup the output 
HDR curve post blending, which would be in most of cases, sink HDR. So 
we should ideally call it HDR_OUTPUT_METADATA or output_hdr_metadata.
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.hdr_source_metadata_property = prop;
> +
>   	return 0;
>   }
>   
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 9be2181..2ee45dc 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -520,6 +520,13 @@ struct drm_connector_state {
>   	 * and the connector bpc limitations obtained from edid.
>   	 */
>   	u8 max_bpc;
> +
> +	/**
> +	 * @metadata_blob_ptr:
> +	 * DRM blob property for HDR metadata
> +	 */
> +	struct drm_property_blob *hdr_source_metadata_blob_ptr;
Same goes again here, output_hdr_metadata_blob ?
> +	u8 hdr_metadata_changed : 1;
>   };
>   
>   /**
> @@ -1154,6 +1161,9 @@ struct drm_connector {
>   	 * &drm_mode_config.connector_free_work.
>   	 */
>   	struct llist_node free_node;
> +
> +	/* HDR metdata */
> +	struct hdr_static_metadata hdr_metadata;
I think while designing this framework, we should leave the scope for 
dynamic metadata, which would be following up soon. So we should have a 
union inside struct hdr_metedata, which will have option for both static 
and dynamic type of metadata. I will add details to the place where you 
are adding definition of hdr_static_metadata().
>   };
>   
>   #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 69ccd27..4b3211b 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -836,6 +836,12 @@ struct drm_mode_config {
>   	 */
>   	struct drm_property *writeback_out_fence_ptr_property;
>   
> +	/*
> +	 * hdr_metadata_property: Connector property containing hdr metatda
> +	 * This will be provided by userspace compositors based on HDR content
> +	 */
> +	struct drm_property *hdr_source_metadata_property;
Again, source->output
> +
>   	/* dumb ioctl parameters */
>   	uint32_t preferred_depth, prefer_shadow;
>   
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index d2bacf5..bed3e08 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -137,6 +137,16 @@ enum hdmi_content_type {
>   	HDMI_CONTENT_TYPE_GAME,
>   };
>   
> +enum hdmi_metadata_type {
> +	HDMI_STATIC_METADATA_TYPE1 = 1,
> +};
> +
> +enum hdmi_eotf {
> +	HDMI_EOTF_TRADITIONAL_GAMMA_SDR,
> +	HDMI_EOTF_TRADITIONAL_GAMMA_HDR,
> +	HDMI_EOTF_SMPTE_ST2084,
I guess we are not bothering about HLG now, which is fine actually, less 
complicated for now.
> +};
> +
>   struct hdmi_avi_infoframe {
>   	enum hdmi_infoframe_type type;
>   	unsigned char version;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index a439c2e..5012af2 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -630,6 +630,22 @@ struct drm_color_lut {
>   	__u16 reserved;
>   };
>   
> +/* HDR Metadata */
> +struct hdr_static_metadata {
> +	uint8_t eotf;
> +	uint8_t metadata_type;
> +	struct {
> +		uint16_t x, y;
> +		} display_primaries[3];
> +	struct {
> +		uint16_t x, y;
> +		} white_point;
> +	uint16_t max_mastering_display_luminance;
> +	uint16_t min_mastering_display_luminance;
> +	uint16_t max_fall;
> +	uint16_t max_cll;
> +};
How about defining struct hdr_metadata {
    uint8_t size;
    union {
        struct hdr_static_metadata *static;
        struct hdr_metadata_dynamic *dynamic;
    } metadata;
};
So that its easier when we are extending support for dynamic HDR ?

- Shashank
> +
>   #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>   #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
>   #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel