[v8,01/10] drm: Add HDR source metadata property

Commit Message

Shankar, Uma April 9, 2019, 4:44 p.m. UTC

This patch adds a blob property to get HDR metadata
information from userspace. This will be send as part
of AVI Infoframe to panel.

It also implements get() and set() functions for HDR output
metadata property.The blob data is received from userspace and
saved in connector state, the same is returned as blob in get
property call to userspace.

v2: Rebase and modified the metadata structure elements
as per Ville's POC changes.

v3: No Change

v4: Addressed Shashank's review comments

v5: Rebase.

v6: Addressed Brian Starkey's review comments, defined
new structure with header for dynamic metadata scalability.
Merge get/set property functions for metadata in this patch.

v7: Addressed Jonas Karlman review comments and defined separate
structure for infoframe to better align with CTA 861.G spec. Added
Shashank's RB.

Signed-off-by: Uma Shankar <[e-mail]>
Reviewed-by: Shashank Sharma <[e-mail]>
---
 drivers/gpu/drm/drm_atomic.c      |  2 ++
 drivers/gpu/drm/drm_atomic_uapi.c | 13 +++++++++++++
 drivers/gpu/drm/drm_connector.c   |  6 ++++++
 include/drm/drm_connector.h       | 11 +++++++++++
 include/drm/drm_mode_config.h     |  6 ++++++
 include/linux/hdmi.h              | 10 ++++++++++
 include/uapi/drm/drm_mode.h       | 39 +++++++++++++++++++++++++++++++++++++++
 7 files changed, 87 insertions(+)

Comments

Jonas Karlman May 4, 2019, 10:17 a.m. UTC | #1

On 2019-04-09 18:44, 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.
>
> It also implements get() and set() functions for HDR output
> metadata property.The blob data is received from userspace and
> saved in connector state, the same is returned as blob in get
> property call to userspace.
>
> v2: Rebase and modified the metadata structure elements
> as per Ville's POC changes.
>
> v3: No Change
>
> v4: Addressed Shashank's review comments
>
> v5: Rebase.
>
> v6: Addressed Brian Starkey's review comments, defined
> new structure with header for dynamic metadata scalability.
> Merge get/set property functions for metadata in this patch.
>
> v7: Addressed Jonas Karlman review comments and defined separate
> structure for infoframe to better align with CTA 861.G spec. Added
> Shashank's RB.
>
> Signed-off-by: Uma Shankar <[e-mail]>
> Reviewed-by: Shashank Sharma <[e-mail]>
> ---
>  drivers/gpu/drm/drm_atomic.c      |  2 ++
>  drivers/gpu/drm/drm_atomic_uapi.c | 13 +++++++++++++
>  drivers/gpu/drm/drm_connector.c   |  6 ++++++
>  include/drm/drm_connector.h       | 11 +++++++++++
>  include/drm/drm_mode_config.h     |  6 ++++++
>  include/linux/hdmi.h              | 10 ++++++++++
>  include/uapi/drm/drm_mode.h       | 39 +++++++++++++++++++++++++++++++++++++++
>  7 files changed, 87 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 5eb4013..8b9c126 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -881,6 +881,8 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
>  
>  	drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector->name);
>  	drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)");
> +	drm_printf(p, "\thdr_metadata_changed=%d\n",
> +		   state->hdr_metadata_changed);
>  
>  	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
>  		if (state->writeback_job && state->writeback_job->fb)
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index ea797d4..6d0d161 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -673,6 +673,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  {
>  	struct drm_device *dev = connector->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
> +	bool replaced = false;
> +	int ret;
>  
>  	if (property == config->prop_crtc_id) {
>  		struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val);
> @@ -721,6 +723,14 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		 */
>  		if (state->link_status != DRM_LINK_STATUS_GOOD)
>  			state->link_status = val;
> +	} else if (property == config->hdr_output_metadata_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> +				&state->hdr_output_metadata_blob_ptr,
> +				val,
> +				-1, sizeof(struct hdr_output_metadata),
> +				&replaced);
> +		state->hdr_metadata_changed |= replaced;
> +		return ret;
>  	} else if (property == config->aspect_ratio_property) {
>  		state->picture_aspect_ratio = val;
>  	} else if (property == config->content_type_property) {
> @@ -807,6 +817,9 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		*val = state->colorspace;
>  	} else if (property == connector->scaling_mode_property) {
>  		*val = state->scaling_mode;
> +	} else if (property == config->hdr_output_metadata_property) {
> +		*val = (state->hdr_output_metadata_blob_ptr) ?
> +			state->hdr_output_metadata_blob_ptr->base.id : 0;
>  	} else if (property == connector->content_protection_property) {
>  		*val = state->content_protection;
>  	} else if (property == config->writeback_fb_id_property) {
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 2355124..0bdae90 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1058,6 +1058,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_OUTPUT_METADATA", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.hdr_output_metadata_property = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 02a1312..5343f60 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -599,6 +599,13 @@ struct drm_connector_state {
>  	 * and the connector bpc limitations obtained from edid.
>  	 */
>  	u8 max_bpc;
> +
> +	/**
> +	 * @metadata_blob_ptr:
> +	 * DRM blob property for HDR output metadata
> +	 */
> +	struct drm_property_blob *hdr_output_metadata_blob_ptr;
> +	u8 hdr_metadata_changed : 1;
>  };
>  
>  /**
> @@ -1239,6 +1246,10 @@ struct drm_connector {
>  	 * &drm_mode_config.connector_free_work.
>  	 */
>  	struct llist_node free_node;
> +
> +	/* HDR metdata */
> +	struct hdr_output_metadata hdr_output_metadata;
> +	struct hdr_sink_metadata hdr_sink_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 7f60e8e..ef2656b 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_output_metadata_property;
> +
>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;
>  
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index 927ad64..a065194 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -152,6 +152,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,
> +};
> +
>  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 83cd163..7b6a519 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -630,6 +630,45 @@ struct drm_color_lut {
>  	__u16 reserved;
>  };
>  
> +/* HDR Metadata Infoframe as per 861.G spec */
> +struct hdr_metadata_infoframe {
> +	__u8 eotf;
> +	__u8 metadata_type;
> +	struct {
> +		__u16 x, y;
> +		} display_primaries[3];
> +	struct {
> +		__u16 x, y;
> +		} white_point;
> +	__u16 max_display_mastering_luminance;
> +	__u16 min_display_mastering_luminance;
> +	__u16 max_cll;
> +	__u16 max_fall;
> +};
> +
> +struct hdr_output_metadata {
> +	__u32 metadata_type;
> +	union {
> +		struct hdr_metadata_infoframe hdmi_metadata_type1;
> +	};
> +};
> +
> +/* HDR Metadata as per 861.G spec */
> +struct hdr_static_metadata {
> +	__u8 eotf;
> +	__u8 metadata_type;
> +	__u16 max_cll;
> +	__u16 max_fall;
> +	__u16 min_cll;
> +};
> +
> +struct hdr_sink_metadata {
> +	__u32 metadata_type;
> +	union {
> +		struct hdr_static_metadata hdmi_type1;
> +	};
> +};

These two structs (hdr_static_metadata and hdr_sink_metadata) should probably not be part of uapi,
unless the metadata is intended to be exposed in a HDR_SINK_METADATA property.

The commit "drm/i915: Add HLG EOTF" should probably not use a i915 in prefix as it affects drm core.

The blob_ptr should probably be reference counted in duplicate_state/destroy_state helper similar to [1].
I know too little about drm core in order to know if that is correct, please feel free to pick/squash if this is correct.

[1] https://github.com/Kwiboo/linux-rockchip/commit/5f065ff0bbcca145ee46e9466bb8ca048c7a7b1e

I have tested this patchset on rockchip / dw-hdmi using dw-hdmi [2] and Kodi [3] patches with a successful result of enabling HDR mode on the sink.
There is more work needed to get a full 10-bit pipeline for dw-hdmi in order to make full use of HDR,
but for the purpose of enabling HDR on the sink this patchset seems ready.

[2] https://github.com/Kwiboo/linux-rockchip/commit/d63e38d1cb905e5d885c903286402b202be8541e
[3] https://github.com/Kwiboo/xbmc/commit/c41f85ddaa48995659786ba6d7df6b61c7276aa0

Regards,
Jonas

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

Shankar, Uma May 7, 2019, 9:03 a.m. UTC | #2

>-----Original Message-----
>From: Jonas Karlman [mailto:[e-mail]]
>Sent: Saturday, May 4, 2019 3:48 PM
>To: Shankar, Uma <[e-mail]>; [e-mail]; dri-
>[e-mail]
>Cc: [e-mail]; [e-mail]; [e-mail];
>Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
><[e-mail]>
>Subject: Re: [v8 01/10] drm: Add HDR source metadata property
>
>On 2019-04-09 18:44, 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.
>>
>> It also implements get() and set() functions for HDR output metadata
>> property.The blob data is received from userspace and saved in
>> connector state, the same is returned as blob in get property call to
>> userspace.
>>
>> v2: Rebase and modified the metadata structure elements as per Ville's
>> POC changes.
>>
>> v3: No Change
>>
>> v4: Addressed Shashank's review comments
>>
>> v5: Rebase.
>>
>> v6: Addressed Brian Starkey's review comments, defined new structure
>> with header for dynamic metadata scalability.
>> Merge get/set property functions for metadata in this patch.
>>
>> v7: Addressed Jonas Karlman review comments and defined separate
>> structure for infoframe to better align with CTA 861.G spec. Added
>> Shashank's RB.
>>
>> Signed-off-by: Uma Shankar <[e-mail]>
>> Reviewed-by: Shashank Sharma <[e-mail]>
>> ---
>>  drivers/gpu/drm/drm_atomic.c      |  2 ++
>>  drivers/gpu/drm/drm_atomic_uapi.c | 13 +++++++++++++
>>  drivers/gpu/drm/drm_connector.c   |  6 ++++++
>>  include/drm/drm_connector.h       | 11 +++++++++++
>>  include/drm/drm_mode_config.h     |  6 ++++++
>>  include/linux/hdmi.h              | 10 ++++++++++
>>  include/uapi/drm/drm_mode.h       | 39
>+++++++++++++++++++++++++++++++++++++++
>>  7 files changed, 87 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c
>> b/drivers/gpu/drm/drm_atomic.c index 5eb4013..8b9c126 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -881,6 +881,8 @@ static void
>> drm_atomic_connector_print_state(struct drm_printer *p,
>>
>>  	drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector-
>>name);
>>  	drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name :
>> "(null)");
>> +	drm_printf(p, "\thdr_metadata_changed=%d\n",
>> +		   state->hdr_metadata_changed);
>>
>>  	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
>>  		if (state->writeback_job && state->writeback_job->fb) diff --git
>> a/drivers/gpu/drm/drm_atomic_uapi.c
>> b/drivers/gpu/drm/drm_atomic_uapi.c
>> index ea797d4..6d0d161 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -673,6 +673,8 @@ static int
>> drm_atomic_connector_set_property(struct drm_connector *connector,  {
>>  	struct drm_device *dev = connector->dev;
>>  	struct drm_mode_config *config = &dev->mode_config;
>> +	bool replaced = false;
>> +	int ret;
>>
>>  	if (property == config->prop_crtc_id) {
>>  		struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val); @@ -721,6
>> +723,14 @@ static int drm_atomic_connector_set_property(struct drm_connector
>*connector,
>>  		 */
>>  		if (state->link_status != DRM_LINK_STATUS_GOOD)
>>  			state->link_status = val;
>> +	} else if (property == config->hdr_output_metadata_property) {
>> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>> +				&state->hdr_output_metadata_blob_ptr,
>> +				val,
>> +				-1, sizeof(struct hdr_output_metadata),
>> +				&replaced);
>> +		state->hdr_metadata_changed |= replaced;
>> +		return ret;
>>  	} else if (property == config->aspect_ratio_property) {
>>  		state->picture_aspect_ratio = val;
>>  	} else if (property == config->content_type_property) { @@ -807,6
>> +817,9 @@ static int drm_atomic_connector_set_property(struct drm_connector
>*connector,
>>  		*val = state->colorspace;
>>  	} else if (property == connector->scaling_mode_property) {
>>  		*val = state->scaling_mode;
>> +	} else if (property == config->hdr_output_metadata_property) {
>> +		*val = (state->hdr_output_metadata_blob_ptr) ?
>> +			state->hdr_output_metadata_blob_ptr->base.id : 0;
>>  	} else if (property == connector->content_protection_property) {
>>  		*val = state->content_protection;
>>  	} else if (property == config->writeback_fb_id_property) { diff
>> --git a/drivers/gpu/drm/drm_connector.c
>> b/drivers/gpu/drm/drm_connector.c index 2355124..0bdae90 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1058,6 +1058,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_OUTPUT_METADATA", 0);
>> +	if (!prop)
>> +		return -ENOMEM;
>> +	dev->mode_config.hdr_output_metadata_property = prop;
>> +
>>  	return 0;
>>  }
>>
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 02a1312..5343f60 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -599,6 +599,13 @@ struct drm_connector_state {
>>  	 * and the connector bpc limitations obtained from edid.
>>  	 */
>>  	u8 max_bpc;
>> +
>> +	/**
>> +	 * @metadata_blob_ptr:
>> +	 * DRM blob property for HDR output metadata
>> +	 */
>> +	struct drm_property_blob *hdr_output_metadata_blob_ptr;
>> +	u8 hdr_metadata_changed : 1;
>>  };
>>
>>  /**
>> @@ -1239,6 +1246,10 @@ struct drm_connector {
>>  	 * &drm_mode_config.connector_free_work.
>>  	 */
>>  	struct llist_node free_node;
>> +
>> +	/* HDR metdata */
>> +	struct hdr_output_metadata hdr_output_metadata;
>> +	struct hdr_sink_metadata hdr_sink_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 7f60e8e..ef2656b 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_output_metadata_property;
>> +
>>  	/* dumb ioctl parameters */
>>  	uint32_t preferred_depth, prefer_shadow;
>>
>> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index
>> 927ad64..a065194 100644
>> --- a/include/linux/hdmi.h
>> +++ b/include/linux/hdmi.h
>> @@ -152,6 +152,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,
>> +};
>> +
>>  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 83cd163..7b6a519 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -630,6 +630,45 @@ struct drm_color_lut {
>>  	__u16 reserved;
>>  };
>>
>> +/* HDR Metadata Infoframe as per 861.G spec */ struct
>> +hdr_metadata_infoframe {
>> +	__u8 eotf;
>> +	__u8 metadata_type;
>> +	struct {
>> +		__u16 x, y;
>> +		} display_primaries[3];
>> +	struct {
>> +		__u16 x, y;
>> +		} white_point;
>> +	__u16 max_display_mastering_luminance;
>> +	__u16 min_display_mastering_luminance;
>> +	__u16 max_cll;
>> +	__u16 max_fall;
>> +};
>> +
>> +struct hdr_output_metadata {
>> +	__u32 metadata_type;
>> +	union {
>> +		struct hdr_metadata_infoframe hdmi_metadata_type1;
>> +	};
>> +};
>> +
>> +/* HDR Metadata as per 861.G spec */
>> +struct hdr_static_metadata {
>> +	__u8 eotf;
>> +	__u8 metadata_type;
>> +	__u16 max_cll;
>> +	__u16 max_fall;
>> +	__u16 min_cll;
>> +};
>> +
>> +struct hdr_sink_metadata {
>> +	__u32 metadata_type;
>> +	union {
>> +		struct hdr_static_metadata hdmi_type1;
>> +	};
>> +};
>
>These two structs (hdr_static_metadata and hdr_sink_metadata) should probably not
>be part of uapi, unless the metadata is intended to be exposed in a
>HDR_SINK_METADATA property.

Hmm yeah, currently we are not planning it to expose that. It was just to help userspace
get and parse the EDID and store the details. But yes this doesn't need to be UAPI and
userspace should be free to use whatever way they want to parse and store sink metadata.
Will move it outside of uapi headers.


>The commit "drm/i915: Add HLG EOTF" should probably not use a i915 in prefix as it
>affects drm core.

Ok, will update this.

>
>The blob_ptr should probably be reference counted in duplicate_state/destroy_state
>helper similar to [1].
>I know too little about drm core in order to know if that is correct, please feel free to
>pick/squash if this is correct.

Sure, I will add these changes to the series.

>[1] https://github.com/Kwiboo/linux-
>rockchip/commit/5f065ff0bbcca145ee46e9466bb8ca048c7a7b1e
>
>I have tested this patchset on rockchip / dw-hdmi using dw-hdmi [2] and Kodi [3]
>patches with a successful result of enabling HDR mode on the sink.
>There is more work needed to get a full 10-bit pipeline for dw-hdmi in order to make
>full use of HDR, but for the purpose of enabling HDR on the sink this patchset seems
>ready.

Thanks Jonas for your comments and more importantly for testing and enabling with kodi.

I will rebase the series and send out next version. With the changes already in place in kodi branch,
we should probably be good for merge.

Regards,
Uma Shankar

>[2] https://github.com/Kwiboo/linux-
>rockchip/commit/d63e38d1cb905e5d885c903286402b202be8541e
>[3]
>https://github.com/Kwiboo/xbmc/commit/c41f85ddaa48995659786ba6d7df6b61c72
>76aa0
>
>Regards,
>Jonas
>
>> +
>>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01  #define
>> DRM_MODE_PAGE_FLIP_ASYNC 0x02  #define
>> DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4

Ville Syrjälä May 7, 2019, 10:25 a.m. UTC | #3

On Tue, May 07, 2019 at 09:03:45AM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Jonas Karlman [mailto:[e-mail]]
> >Sent: Saturday, May 4, 2019 3:48 PM
> >To: Shankar, Uma <[e-mail]>; [e-mail]; dri-
> >[e-mail]
> >Cc: [e-mail]; [e-mail]; [e-mail];
> >Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
> ><[e-mail]>
> >Subject: Re: [v8 01/10] drm: Add HDR source metadata property
> >
> >On 2019-04-09 18:44, 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.
> >>
> >> It also implements get() and set() functions for HDR output metadata
> >> property.The blob data is received from userspace and saved in
> >> connector state, the same is returned as blob in get property call to
> >> userspace.
> >>
> >> v2: Rebase and modified the metadata structure elements as per Ville's
> >> POC changes.
> >>
> >> v3: No Change
> >>
> >> v4: Addressed Shashank's review comments
> >>
> >> v5: Rebase.
> >>
> >> v6: Addressed Brian Starkey's review comments, defined new structure
> >> with header for dynamic metadata scalability.
> >> Merge get/set property functions for metadata in this patch.
> >>
> >> v7: Addressed Jonas Karlman review comments and defined separate
> >> structure for infoframe to better align with CTA 861.G spec. Added
> >> Shashank's RB.
> >>
> >> Signed-off-by: Uma Shankar <[e-mail]>
> >> Reviewed-by: Shashank Sharma <[e-mail]>
> >> ---
> >>  drivers/gpu/drm/drm_atomic.c      |  2 ++
> >>  drivers/gpu/drm/drm_atomic_uapi.c | 13 +++++++++++++
> >>  drivers/gpu/drm/drm_connector.c   |  6 ++++++
> >>  include/drm/drm_connector.h       | 11 +++++++++++
> >>  include/drm/drm_mode_config.h     |  6 ++++++
> >>  include/linux/hdmi.h              | 10 ++++++++++
> >>  include/uapi/drm/drm_mode.h       | 39
> >+++++++++++++++++++++++++++++++++++++++
> >>  7 files changed, 87 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c
> >> b/drivers/gpu/drm/drm_atomic.c index 5eb4013..8b9c126 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -881,6 +881,8 @@ static void
> >> drm_atomic_connector_print_state(struct drm_printer *p,
> >>
> >>  	drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector-
> >>name);
> >>  	drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name :
> >> "(null)");
> >> +	drm_printf(p, "\thdr_metadata_changed=%d\n",
> >> +		   state->hdr_metadata_changed);
> >>
> >>  	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
> >>  		if (state->writeback_job && state->writeback_job->fb) diff --git
> >> a/drivers/gpu/drm/drm_atomic_uapi.c
> >> b/drivers/gpu/drm/drm_atomic_uapi.c
> >> index ea797d4..6d0d161 100644
> >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> @@ -673,6 +673,8 @@ static int
> >> drm_atomic_connector_set_property(struct drm_connector *connector,  {
> >>  	struct drm_device *dev = connector->dev;
> >>  	struct drm_mode_config *config = &dev->mode_config;
> >> +	bool replaced = false;
> >> +	int ret;
> >>
> >>  	if (property == config->prop_crtc_id) {
> >>  		struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val); @@ -721,6
> >> +723,14 @@ static int drm_atomic_connector_set_property(struct drm_connector
> >*connector,
> >>  		 */
> >>  		if (state->link_status != DRM_LINK_STATUS_GOOD)
> >>  			state->link_status = val;
> >> +	} else if (property == config->hdr_output_metadata_property) {
> >> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> >> +				&state->hdr_output_metadata_blob_ptr,
> >> +				val,
> >> +				-1, sizeof(struct hdr_output_metadata),
> >> +				&replaced);
> >> +		state->hdr_metadata_changed |= replaced;
> >> +		return ret;
> >>  	} else if (property == config->aspect_ratio_property) {
> >>  		state->picture_aspect_ratio = val;
> >>  	} else if (property == config->content_type_property) { @@ -807,6
> >> +817,9 @@ static int drm_atomic_connector_set_property(struct drm_connector
> >*connector,
> >>  		*val = state->colorspace;
> >>  	} else if (property == connector->scaling_mode_property) {
> >>  		*val = state->scaling_mode;
> >> +	} else if (property == config->hdr_output_metadata_property) {
> >> +		*val = (state->hdr_output_metadata_blob_ptr) ?
> >> +			state->hdr_output_metadata_blob_ptr->base.id : 0;
> >>  	} else if (property == connector->content_protection_property) {
> >>  		*val = state->content_protection;
> >>  	} else if (property == config->writeback_fb_id_property) { diff
> >> --git a/drivers/gpu/drm/drm_connector.c
> >> b/drivers/gpu/drm/drm_connector.c index 2355124..0bdae90 100644
> >> --- a/drivers/gpu/drm/drm_connector.c
> >> +++ b/drivers/gpu/drm/drm_connector.c
> >> @@ -1058,6 +1058,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_OUTPUT_METADATA", 0);
> >> +	if (!prop)
> >> +		return -ENOMEM;
> >> +	dev->mode_config.hdr_output_metadata_property = prop;
> >> +
> >>  	return 0;
> >>  }
> >>
> >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> >> index 02a1312..5343f60 100644
> >> --- a/include/drm/drm_connector.h
> >> +++ b/include/drm/drm_connector.h
> >> @@ -599,6 +599,13 @@ struct drm_connector_state {
> >>  	 * and the connector bpc limitations obtained from edid.
> >>  	 */
> >>  	u8 max_bpc;
> >> +
> >> +	/**
> >> +	 * @metadata_blob_ptr:
> >> +	 * DRM blob property for HDR output metadata
> >> +	 */
> >> +	struct drm_property_blob *hdr_output_metadata_blob_ptr;
> >> +	u8 hdr_metadata_changed : 1;
> >>  };
> >>
> >>  /**
> >> @@ -1239,6 +1246,10 @@ struct drm_connector {
> >>  	 * &drm_mode_config.connector_free_work.
> >>  	 */
> >>  	struct llist_node free_node;
> >> +
> >> +	/* HDR metdata */
> >> +	struct hdr_output_metadata hdr_output_metadata;
> >> +	struct hdr_sink_metadata hdr_sink_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 7f60e8e..ef2656b 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_output_metadata_property;
> >> +
> >>  	/* dumb ioctl parameters */
> >>  	uint32_t preferred_depth, prefer_shadow;
> >>
> >> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index
> >> 927ad64..a065194 100644
> >> --- a/include/linux/hdmi.h
> >> +++ b/include/linux/hdmi.h
> >> @@ -152,6 +152,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,
> >> +};
> >> +
> >>  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 83cd163..7b6a519 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -630,6 +630,45 @@ struct drm_color_lut {
> >>  	__u16 reserved;
> >>  };
> >>
> >> +/* HDR Metadata Infoframe as per 861.G spec */ struct
> >> +hdr_metadata_infoframe {
> >> +	__u8 eotf;
> >> +	__u8 metadata_type;
> >> +	struct {
> >> +		__u16 x, y;
> >> +		} display_primaries[3];
> >> +	struct {
> >> +		__u16 x, y;
> >> +		} white_point;
> >> +	__u16 max_display_mastering_luminance;
> >> +	__u16 min_display_mastering_luminance;
> >> +	__u16 max_cll;
> >> +	__u16 max_fall;
> >> +};
> >> +
> >> +struct hdr_output_metadata {
> >> +	__u32 metadata_type;
> >> +	union {
> >> +		struct hdr_metadata_infoframe hdmi_metadata_type1;
> >> +	};
> >> +};
> >> +
> >> +/* HDR Metadata as per 861.G spec */
> >> +struct hdr_static_metadata {
> >> +	__u8 eotf;
> >> +	__u8 metadata_type;
> >> +	__u16 max_cll;
> >> +	__u16 max_fall;
> >> +	__u16 min_cll;
> >> +};
> >> +
> >> +struct hdr_sink_metadata {
> >> +	__u32 metadata_type;
> >> +	union {
> >> +		struct hdr_static_metadata hdmi_type1;
> >> +	};
> >> +};
> >
> >These two structs (hdr_static_metadata and hdr_sink_metadata) should probably not
> >be part of uapi, unless the metadata is intended to be exposed in a
> >HDR_SINK_METADATA property.
> 
> Hmm yeah, currently we are not planning it to expose that. It was just to help userspace
> get and parse the EDID and store the details. But yes this doesn't need to be UAPI and
> userspace should be free to use whatever way they want to parse and store sink metadata.
> Will move it outside of uapi headers.
> 
> 
> >The commit "drm/i915: Add HLG EOTF" should probably not use a i915 in prefix as it
> >affects drm core.
> 
> Ok, will update this.
> 
> >
> >The blob_ptr should probably be reference counted in duplicate_state/destroy_state
> >helper similar to [1].
> >I know too little about drm core in order to know if that is correct, please feel free to
> >pick/squash if this is correct.
> 
> Sure, I will add these changes to the series.
> 
> >[1] https://github.com/Kwiboo/linux-
> >rockchip/commit/5f065ff0bbcca145ee46e9466bb8ca048c7a7b1e
> >
> >I have tested this patchset on rockchip / dw-hdmi using dw-hdmi [2] and Kodi [3]
> >patches with a successful result of enabling HDR mode on the sink.
> >There is more work needed to get a full 10-bit pipeline for dw-hdmi in order to make
> >full use of HDR, but for the purpose of enabling HDR on the sink this patchset seems
> >ready.
> 
> Thanks Jonas for your comments and more importantly for testing and enabling with kodi.
> 
> I will rebase the series and send out next version. With the changes already in place in kodi branch,
> we should probably be good for merge.

Patch 9 is not ready IMO. I don't see any analysis on why it's OK to
update infoframes without a full modeset. I would just drop that patch
from the initial set and work on it as a followup.

Also did someone analysze how the new dynamic HDR metadata fits in with
the new uapi?

Ville Syrjälä May 7, 2019, 12:07 p.m. UTC | #4

On Tue, Apr 09, 2019 at 10:14:35PM +0530, 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.
> 
> It also implements get() and set() functions for HDR output
> metadata property.The blob data is received from userspace and
> saved in connector state, the same is returned as blob in get
> property call to userspace.
> 
> v2: Rebase and modified the metadata structure elements
> as per Ville's POC changes.
> 
> v3: No Change
> 
> v4: Addressed Shashank's review comments
> 
> v5: Rebase.
> 
> v6: Addressed Brian Starkey's review comments, defined
> new structure with header for dynamic metadata scalability.
> Merge get/set property functions for metadata in this patch.
> 
> v7: Addressed Jonas Karlman review comments and defined separate
> structure for infoframe to better align with CTA 861.G spec. Added
> Shashank's RB.
> 
> Signed-off-by: Uma Shankar <[e-mail]>
> Reviewed-by: Shashank Sharma <[e-mail]>
> ---
>  drivers/gpu/drm/drm_atomic.c      |  2 ++
>  drivers/gpu/drm/drm_atomic_uapi.c | 13 +++++++++++++
>  drivers/gpu/drm/drm_connector.c   |  6 ++++++
>  include/drm/drm_connector.h       | 11 +++++++++++
>  include/drm/drm_mode_config.h     |  6 ++++++
>  include/linux/hdmi.h              | 10 ++++++++++
>  include/uapi/drm/drm_mode.h       | 39 +++++++++++++++++++++++++++++++++++++++
>  7 files changed, 87 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 5eb4013..8b9c126 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -881,6 +881,8 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
>  
>  	drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector->name);
>  	drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)");
> +	drm_printf(p, "\thdr_metadata_changed=%d\n",
> +		   state->hdr_metadata_changed);

Is that flag actually useful?

>  
>  	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
>  		if (state->writeback_job && state->writeback_job->fb)
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index ea797d4..6d0d161 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -673,6 +673,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  {
>  	struct drm_device *dev = connector->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
> +	bool replaced = false;
> +	int ret;
>  
>  	if (property == config->prop_crtc_id) {
>  		struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val);
> @@ -721,6 +723,14 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		 */
>  		if (state->link_status != DRM_LINK_STATUS_GOOD)
>  			state->link_status = val;
> +	} else if (property == config->hdr_output_metadata_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> +				&state->hdr_output_metadata_blob_ptr,
> +				val,
> +				-1, sizeof(struct hdr_output_metadata),
> +				&replaced);
> +		state->hdr_metadata_changed |= replaced;
> +		return ret;
>  	} else if (property == config->aspect_ratio_property) {
>  		state->picture_aspect_ratio = val;
>  	} else if (property == config->content_type_property) {
> @@ -807,6 +817,9 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		*val = state->colorspace;
>  	} else if (property == connector->scaling_mode_property) {
>  		*val = state->scaling_mode;
> +	} else if (property == config->hdr_output_metadata_property) {
> +		*val = (state->hdr_output_metadata_blob_ptr) ?

Pointless parens.

> +			state->hdr_output_metadata_blob_ptr->base.id : 0;
>  	} else if (property == connector->content_protection_property) {
>  		*val = state->content_protection;
>  	} else if (property == config->writeback_fb_id_property) {
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 2355124..0bdae90 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1058,6 +1058,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_OUTPUT_METADATA", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.hdr_output_metadata_property = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 02a1312..5343f60 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -599,6 +599,13 @@ struct drm_connector_state {
>  	 * and the connector bpc limitations obtained from edid.
>  	 */
>  	u8 max_bpc;
> +
> +	/**
> +	 * @metadata_blob_ptr:
> +	 * DRM blob property for HDR output metadata
> +	 */
> +	struct drm_property_blob *hdr_output_metadata_blob_ptr;

I would drop the _blob_ptr suffix. We don't have that for any other
state blob.

> +	u8 hdr_metadata_changed : 1;
>  };
>  
>  /**
> @@ -1239,6 +1246,10 @@ struct drm_connector {
>  	 * &drm_mode_config.connector_free_work.
>  	 */
>  	struct llist_node free_node;
> +
> +	/* HDR metdata */
> +	struct hdr_output_metadata hdr_output_metadata;
> +	struct hdr_sink_metadata hdr_sink_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 7f60e8e..ef2656b 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_output_metadata_property;
> +
>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;
>  
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index 927ad64..a065194 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -152,6 +152,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,
> +};
> +
>  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 83cd163..7b6a519 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -630,6 +630,45 @@ struct drm_color_lut {
>  	__u16 reserved;
>  };
>  
> +/* HDR Metadata Infoframe as per 861.G spec */
> +struct hdr_metadata_infoframe {
> +	__u8 eotf;
> +	__u8 metadata_type;
> +	struct {
> +		__u16 x, y;
> +		} display_primaries[3];
> +	struct {
> +		__u16 x, y;
> +		} white_point;
> +	__u16 max_display_mastering_luminance;
> +	__u16 min_display_mastering_luminance;
> +	__u16 max_cll;
> +	__u16 max_fall;
> +};
> +
> +struct hdr_output_metadata {
> +	__u32 metadata_type;
> +	union {
> +		struct hdr_metadata_infoframe hdmi_metadata_type1;
> +	};
> +};
> +
> +/* HDR Metadata as per 861.G spec */
> +struct hdr_static_metadata {
> +	__u8 eotf;
> +	__u8 metadata_type;
> +	__u16 max_cll;
> +	__u16 max_fall;
> +	__u16 min_cll;
> +};
> +
> +struct hdr_sink_metadata {
> +	__u32 metadata_type;
> +	union {
> +		struct hdr_static_metadata hdmi_type1;
> +	};
> +};
> +
>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
>  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä May 7, 2019, 12:10 p.m. UTC | #5

On Tue, May 07, 2019 at 01:25:42PM +0300, Ville Syrjälä wrote:
> On Tue, May 07, 2019 at 09:03:45AM +0000, Shankar, Uma wrote:
> > 
> > 
> > >-----Original Message-----
> > >From: Jonas Karlman [mailto:[e-mail]]
> > >Sent: Saturday, May 4, 2019 3:48 PM
> > >To: Shankar, Uma <[e-mail]>; [e-mail]; dri-
> > >[e-mail]
> > >Cc: [e-mail]; [e-mail]; [e-mail];
> > >Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
> > ><[e-mail]>
> > >Subject: Re: [v8 01/10] drm: Add HDR source metadata property
> > >
> > >On 2019-04-09 18:44, 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.
> > >>
> > >> It also implements get() and set() functions for HDR output metadata
> > >> property.The blob data is received from userspace and saved in
> > >> connector state, the same is returned as blob in get property call to
> > >> userspace.
> > >>
> > >> v2: Rebase and modified the metadata structure elements as per Ville's
> > >> POC changes.
> > >>
> > >> v3: No Change
> > >>
> > >> v4: Addressed Shashank's review comments
> > >>
> > >> v5: Rebase.
> > >>
> > >> v6: Addressed Brian Starkey's review comments, defined new structure
> > >> with header for dynamic metadata scalability.
> > >> Merge get/set property functions for metadata in this patch.
> > >>
> > >> v7: Addressed Jonas Karlman review comments and defined separate
> > >> structure for infoframe to better align with CTA 861.G spec. Added
> > >> Shashank's RB.
> > >>
> > >> Signed-off-by: Uma Shankar <[e-mail]>
> > >> Reviewed-by: Shashank Sharma <[e-mail]>
> > >> ---
> > >>  drivers/gpu/drm/drm_atomic.c      |  2 ++
> > >>  drivers/gpu/drm/drm_atomic_uapi.c | 13 +++++++++++++
> > >>  drivers/gpu/drm/drm_connector.c   |  6 ++++++
> > >>  include/drm/drm_connector.h       | 11 +++++++++++
> > >>  include/drm/drm_mode_config.h     |  6 ++++++
> > >>  include/linux/hdmi.h              | 10 ++++++++++
> > >>  include/uapi/drm/drm_mode.h       | 39
> > >+++++++++++++++++++++++++++++++++++++++
> > >>  7 files changed, 87 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_atomic.c
> > >> b/drivers/gpu/drm/drm_atomic.c index 5eb4013..8b9c126 100644
> > >> --- a/drivers/gpu/drm/drm_atomic.c
> > >> +++ b/drivers/gpu/drm/drm_atomic.c
> > >> @@ -881,6 +881,8 @@ static void
> > >> drm_atomic_connector_print_state(struct drm_printer *p,
> > >>
> > >>  	drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector-
> > >>name);
> > >>  	drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name :
> > >> "(null)");
> > >> +	drm_printf(p, "\thdr_metadata_changed=%d\n",
> > >> +		   state->hdr_metadata_changed);
> > >>
> > >>  	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
> > >>  		if (state->writeback_job && state->writeback_job->fb) diff --git
> > >> a/drivers/gpu/drm/drm_atomic_uapi.c
> > >> b/drivers/gpu/drm/drm_atomic_uapi.c
> > >> index ea797d4..6d0d161 100644
> > >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > >> @@ -673,6 +673,8 @@ static int
> > >> drm_atomic_connector_set_property(struct drm_connector *connector,  {
> > >>  	struct drm_device *dev = connector->dev;
> > >>  	struct drm_mode_config *config = &dev->mode_config;
> > >> +	bool replaced = false;
> > >> +	int ret;
> > >>
> > >>  	if (property == config->prop_crtc_id) {
> > >>  		struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val); @@ -721,6
> > >> +723,14 @@ static int drm_atomic_connector_set_property(struct drm_connector
> > >*connector,
> > >>  		 */
> > >>  		if (state->link_status != DRM_LINK_STATUS_GOOD)
> > >>  			state->link_status = val;
> > >> +	} else if (property == config->hdr_output_metadata_property) {
> > >> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> > >> +				&state->hdr_output_metadata_blob_ptr,
> > >> +				val,
> > >> +				-1, sizeof(struct hdr_output_metadata),
> > >> +				&replaced);
> > >> +		state->hdr_metadata_changed |= replaced;
> > >> +		return ret;
> > >>  	} else if (property == config->aspect_ratio_property) {
> > >>  		state->picture_aspect_ratio = val;
> > >>  	} else if (property == config->content_type_property) { @@ -807,6
> > >> +817,9 @@ static int drm_atomic_connector_set_property(struct drm_connector
> > >*connector,
> > >>  		*val = state->colorspace;
> > >>  	} else if (property == connector->scaling_mode_property) {
> > >>  		*val = state->scaling_mode;
> > >> +	} else if (property == config->hdr_output_metadata_property) {
> > >> +		*val = (state->hdr_output_metadata_blob_ptr) ?
> > >> +			state->hdr_output_metadata_blob_ptr->base.id : 0;
> > >>  	} else if (property == connector->content_protection_property) {
> > >>  		*val = state->content_protection;
> > >>  	} else if (property == config->writeback_fb_id_property) { diff
> > >> --git a/drivers/gpu/drm/drm_connector.c
> > >> b/drivers/gpu/drm/drm_connector.c index 2355124..0bdae90 100644
> > >> --- a/drivers/gpu/drm/drm_connector.c
> > >> +++ b/drivers/gpu/drm/drm_connector.c
> > >> @@ -1058,6 +1058,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_OUTPUT_METADATA", 0);
> > >> +	if (!prop)
> > >> +		return -ENOMEM;
> > >> +	dev->mode_config.hdr_output_metadata_property = prop;
> > >> +
> > >>  	return 0;
> > >>  }
> > >>
> > >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > >> index 02a1312..5343f60 100644
> > >> --- a/include/drm/drm_connector.h
> > >> +++ b/include/drm/drm_connector.h
> > >> @@ -599,6 +599,13 @@ struct drm_connector_state {
> > >>  	 * and the connector bpc limitations obtained from edid.
> > >>  	 */
> > >>  	u8 max_bpc;
> > >> +
> > >> +	/**
> > >> +	 * @metadata_blob_ptr:
> > >> +	 * DRM blob property for HDR output metadata
> > >> +	 */
> > >> +	struct drm_property_blob *hdr_output_metadata_blob_ptr;
> > >> +	u8 hdr_metadata_changed : 1;
> > >>  };
> > >>
> > >>  /**
> > >> @@ -1239,6 +1246,10 @@ struct drm_connector {
> > >>  	 * &drm_mode_config.connector_free_work.
> > >>  	 */
> > >>  	struct llist_node free_node;
> > >> +
> > >> +	/* HDR metdata */
> > >> +	struct hdr_output_metadata hdr_output_metadata;
> > >> +	struct hdr_sink_metadata hdr_sink_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 7f60e8e..ef2656b 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_output_metadata_property;
> > >> +
> > >>  	/* dumb ioctl parameters */
> > >>  	uint32_t preferred_depth, prefer_shadow;
> > >>
> > >> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index
> > >> 927ad64..a065194 100644
> > >> --- a/include/linux/hdmi.h
> > >> +++ b/include/linux/hdmi.h
> > >> @@ -152,6 +152,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,
> > >> +};
> > >> +
> > >>  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 83cd163..7b6a519 100644
> > >> --- a/include/uapi/drm/drm_mode.h
> > >> +++ b/include/uapi/drm/drm_mode.h
> > >> @@ -630,6 +630,45 @@ struct drm_color_lut {
> > >>  	__u16 reserved;
> > >>  };
> > >>
> > >> +/* HDR Metadata Infoframe as per 861.G spec */ struct
> > >> +hdr_metadata_infoframe {
> > >> +	__u8 eotf;
> > >> +	__u8 metadata_type;
> > >> +	struct {
> > >> +		__u16 x, y;
> > >> +		} display_primaries[3];
> > >> +	struct {
> > >> +		__u16 x, y;
> > >> +		} white_point;
> > >> +	__u16 max_display_mastering_luminance;
> > >> +	__u16 min_display_mastering_luminance;
> > >> +	__u16 max_cll;
> > >> +	__u16 max_fall;
> > >> +};
> > >> +
> > >> +struct hdr_output_metadata {
> > >> +	__u32 metadata_type;
> > >> +	union {
> > >> +		struct hdr_metadata_infoframe hdmi_metadata_type1;
> > >> +	};
> > >> +};
> > >> +
> > >> +/* HDR Metadata as per 861.G spec */
> > >> +struct hdr_static_metadata {
> > >> +	__u8 eotf;
> > >> +	__u8 metadata_type;
> > >> +	__u16 max_cll;
> > >> +	__u16 max_fall;
> > >> +	__u16 min_cll;
> > >> +};
> > >> +
> > >> +struct hdr_sink_metadata {
> > >> +	__u32 metadata_type;
> > >> +	union {
> > >> +		struct hdr_static_metadata hdmi_type1;
> > >> +	};
> > >> +};
> > >
> > >These two structs (hdr_static_metadata and hdr_sink_metadata) should probably not
> > >be part of uapi, unless the metadata is intended to be exposed in a
> > >HDR_SINK_METADATA property.
> > 
> > Hmm yeah, currently we are not planning it to expose that. It was just to help userspace
> > get and parse the EDID and store the details. But yes this doesn't need to be UAPI and
> > userspace should be free to use whatever way they want to parse and store sink metadata.
> > Will move it outside of uapi headers.
> > 
> > 
> > >The commit "drm/i915: Add HLG EOTF" should probably not use a i915 in prefix as it
> > >affects drm core.
> > 
> > Ok, will update this.
> > 
> > >
> > >The blob_ptr should probably be reference counted in duplicate_state/destroy_state
> > >helper similar to [1].
> > >I know too little about drm core in order to know if that is correct, please feel free to
> > >pick/squash if this is correct.
> > 
> > Sure, I will add these changes to the series.
> > 
> > >[1] https://github.com/Kwiboo/linux-
> > >rockchip/commit/5f065ff0bbcca145ee46e9466bb8ca048c7a7b1e
> > >
> > >I have tested this patchset on rockchip / dw-hdmi using dw-hdmi [2] and Kodi [3]
> > >patches with a successful result of enabling HDR mode on the sink.
> > >There is more work needed to get a full 10-bit pipeline for dw-hdmi in order to make
> > >full use of HDR, but for the purpose of enabling HDR on the sink this patchset seems
> > >ready.
> > 
> > Thanks Jonas for your comments and more importantly for testing and enabling with kodi.
> > 
> > I will rebase the series and send out next version. With the changes already in place in kodi branch,
> > we should probably be good for merge.
> 
> Patch 9 is not ready IMO. I don't see any analysis on why it's OK to
> update infoframes without a full modeset. I would just drop that patch
> from the initial set and work on it as a followup.
> 
> Also did someone analysze how the new dynamic HDR metadata fits in with
> the new uapi?

Also I can't see readout + state checker for the HDR infoframe. Am I blind
or is it missing?

Shankar, Uma May 8, 2019, 9:03 a.m. UTC | #6

>-----Original Message-----
>From: Ville Syrjälä [mailto:[e-mail]]
>Sent: Tuesday, May 7, 2019 5:40 PM
>To: Shankar, Uma <[e-mail]>
>Cc: Jonas Karlman <[e-mail]>; [e-mail]; dri-
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; Lankhorst, Maarten <[e-mail]>;
>Syrjala, Ville <[e-mail]>
>Subject: Re: [v8 01/10] drm: Add HDR source metadata property
>
>On Tue, May 07, 2019 at 01:25:42PM +0300, Ville Syrjälä wrote:
>> On Tue, May 07, 2019 at 09:03:45AM +0000, Shankar, Uma wrote:
>> >
>> >
>> > >-----Original Message-----
>> > >From: Jonas Karlman [mailto:[e-mail]]
>> > >Sent: Saturday, May 4, 2019 3:48 PM
>> > >To: Shankar, Uma <[e-mail]>;
>> > >[e-mail]; dri- [e-mail]
>> > >Cc: [e-mail]; [e-mail];
>> > >[e-mail]; Syrjala, Ville <[e-mail]>;
>> > >Lankhorst, Maarten <[e-mail]>
>> > >Subject: Re: [v8 01/10] drm: Add HDR source metadata property
>> > >
>> > >On 2019-04-09 18:44, 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.
>> > >>
>> > >> It also implements get() and set() functions for HDR output
>> > >> metadata property.The blob data is received from userspace and
>> > >> saved in connector state, the same is returned as blob in get
>> > >> property call to userspace.
>> > >>
>> > >> v2: Rebase and modified the metadata structure elements as per
>> > >> Ville's POC changes.
>> > >>
>> > >> v3: No Change
>> > >>
>> > >> v4: Addressed Shashank's review comments
>> > >>
>> > >> v5: Rebase.
>> > >>
>> > >> v6: Addressed Brian Starkey's review comments, defined new
>> > >> structure with header for dynamic metadata scalability.
>> > >> Merge get/set property functions for metadata in this patch.
>> > >>
>> > >> v7: Addressed Jonas Karlman review comments and defined separate
>> > >> structure for infoframe to better align with CTA 861.G spec.
>> > >> Added Shashank's RB.
>> > >>
>> > >> Signed-off-by: Uma Shankar <[e-mail]>
>> > >> Reviewed-by: Shashank Sharma <[e-mail]>
>> > >> ---
>> > >>  drivers/gpu/drm/drm_atomic.c      |  2 ++
>> > >>  drivers/gpu/drm/drm_atomic_uapi.c | 13 +++++++++++++
>> > >>  drivers/gpu/drm/drm_connector.c   |  6 ++++++
>> > >>  include/drm/drm_connector.h       | 11 +++++++++++
>> > >>  include/drm/drm_mode_config.h     |  6 ++++++
>> > >>  include/linux/hdmi.h              | 10 ++++++++++
>> > >>  include/uapi/drm/drm_mode.h       | 39
>> > >+++++++++++++++++++++++++++++++++++++++
>> > >>  7 files changed, 87 insertions(+)
>> > >>
>> > >> diff --git a/drivers/gpu/drm/drm_atomic.c
>> > >> b/drivers/gpu/drm/drm_atomic.c index 5eb4013..8b9c126 100644
>> > >> --- a/drivers/gpu/drm/drm_atomic.c
>> > >> +++ b/drivers/gpu/drm/drm_atomic.c
>> > >> @@ -881,6 +881,8 @@ static void
>> > >> drm_atomic_connector_print_state(struct drm_printer *p,
>> > >>
>> > >>  	drm_printf(p, "connector[%u]: %s\n", connector->base.id,
>> > >>connector- name);
>> > >>  	drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name :
>> > >> "(null)");
>> > >> +	drm_printf(p, "\thdr_metadata_changed=%d\n",
>> > >> +		   state->hdr_metadata_changed);
>> > >>
>> > >>  	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
>> > >>  		if (state->writeback_job && state->writeback_job->fb) diff
>> > >> --git a/drivers/gpu/drm/drm_atomic_uapi.c
>> > >> b/drivers/gpu/drm/drm_atomic_uapi.c
>> > >> index ea797d4..6d0d161 100644
>> > >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> > >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> > >> @@ -673,6 +673,8 @@ static int
>> > >> drm_atomic_connector_set_property(struct drm_connector *connector,  {
>> > >>  	struct drm_device *dev = connector->dev;
>> > >>  	struct drm_mode_config *config = &dev->mode_config;
>> > >> +	bool replaced = false;
>> > >> +	int ret;
>> > >>
>> > >>  	if (property == config->prop_crtc_id) {
>> > >>  		struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val); @@
>> > >> -721,6
>> > >> +723,14 @@ static int drm_atomic_connector_set_property(struct
>> > >> +drm_connector
>> > >*connector,
>> > >>  		 */
>> > >>  		if (state->link_status != DRM_LINK_STATUS_GOOD)
>> > >>  			state->link_status = val;
>> > >> +	} else if (property == config->hdr_output_metadata_property) {
>> > >> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>> > >> +				&state->hdr_output_metadata_blob_ptr,
>> > >> +				val,
>> > >> +				-1, sizeof(struct hdr_output_metadata),
>> > >> +				&replaced);
>> > >> +		state->hdr_metadata_changed |= replaced;
>> > >> +		return ret;
>> > >>  	} else if (property == config->aspect_ratio_property) {
>> > >>  		state->picture_aspect_ratio = val;
>> > >>  	} else if (property == config->content_type_property) { @@
>> > >> -807,6
>> > >> +817,9 @@ static int drm_atomic_connector_set_property(struct
>> > >> +drm_connector
>> > >*connector,
>> > >>  		*val = state->colorspace;
>> > >>  	} else if (property == connector->scaling_mode_property) {
>> > >>  		*val = state->scaling_mode;
>> > >> +	} else if (property == config->hdr_output_metadata_property) {
>> > >> +		*val = (state->hdr_output_metadata_blob_ptr) ?
>> > >> +			state->hdr_output_metadata_blob_ptr->base.id : 0;
>> > >>  	} else if (property == connector->content_protection_property) {
>> > >>  		*val = state->content_protection;
>> > >>  	} else if (property == config->writeback_fb_id_property) { diff
>> > >> --git a/drivers/gpu/drm/drm_connector.c
>> > >> b/drivers/gpu/drm/drm_connector.c index 2355124..0bdae90 100644
>> > >> --- a/drivers/gpu/drm/drm_connector.c
>> > >> +++ b/drivers/gpu/drm/drm_connector.c
>> > >> @@ -1058,6 +1058,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_OUTPUT_METADATA", 0);
>> > >> +	if (!prop)
>> > >> +		return -ENOMEM;
>> > >> +	dev->mode_config.hdr_output_metadata_property = prop;
>> > >> +
>> > >>  	return 0;
>> > >>  }
>> > >>
>> > >> diff --git a/include/drm/drm_connector.h
>> > >> b/include/drm/drm_connector.h index 02a1312..5343f60 100644
>> > >> --- a/include/drm/drm_connector.h
>> > >> +++ b/include/drm/drm_connector.h
>> > >> @@ -599,6 +599,13 @@ struct drm_connector_state {
>> > >>  	 * and the connector bpc limitations obtained from edid.
>> > >>  	 */
>> > >>  	u8 max_bpc;
>> > >> +
>> > >> +	/**
>> > >> +	 * @metadata_blob_ptr:
>> > >> +	 * DRM blob property for HDR output metadata
>> > >> +	 */
>> > >> +	struct drm_property_blob *hdr_output_metadata_blob_ptr;
>> > >> +	u8 hdr_metadata_changed : 1;
>> > >>  };
>> > >>
>> > >>  /**
>> > >> @@ -1239,6 +1246,10 @@ struct drm_connector {
>> > >>  	 * &drm_mode_config.connector_free_work.
>> > >>  	 */
>> > >>  	struct llist_node free_node;
>> > >> +
>> > >> +	/* HDR metdata */
>> > >> +	struct hdr_output_metadata hdr_output_metadata;
>> > >> +	struct hdr_sink_metadata hdr_sink_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 7f60e8e..ef2656b 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_output_metadata_property;
>> > >> +
>> > >>  	/* dumb ioctl parameters */
>> > >>  	uint32_t preferred_depth, prefer_shadow;
>> > >>
>> > >> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index
>> > >> 927ad64..a065194 100644
>> > >> --- a/include/linux/hdmi.h
>> > >> +++ b/include/linux/hdmi.h
>> > >> @@ -152,6 +152,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,
>> > >> +};
>> > >> +
>> > >>  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 83cd163..7b6a519 100644
>> > >> --- a/include/uapi/drm/drm_mode.h
>> > >> +++ b/include/uapi/drm/drm_mode.h
>> > >> @@ -630,6 +630,45 @@ struct drm_color_lut {
>> > >>  	__u16 reserved;
>> > >>  };
>> > >>
>> > >> +/* HDR Metadata Infoframe as per 861.G spec */ struct
>> > >> +hdr_metadata_infoframe {
>> > >> +	__u8 eotf;
>> > >> +	__u8 metadata_type;
>> > >> +	struct {
>> > >> +		__u16 x, y;
>> > >> +		} display_primaries[3];
>> > >> +	struct {
>> > >> +		__u16 x, y;
>> > >> +		} white_point;
>> > >> +	__u16 max_display_mastering_luminance;
>> > >> +	__u16 min_display_mastering_luminance;
>> > >> +	__u16 max_cll;
>> > >> +	__u16 max_fall;
>> > >> +};
>> > >> +
>> > >> +struct hdr_output_metadata {
>> > >> +	__u32 metadata_type;
>> > >> +	union {
>> > >> +		struct hdr_metadata_infoframe hdmi_metadata_type1;
>> > >> +	};
>> > >> +};
>> > >> +
>> > >> +/* HDR Metadata as per 861.G spec */ struct hdr_static_metadata
>> > >> +{
>> > >> +	__u8 eotf;
>> > >> +	__u8 metadata_type;
>> > >> +	__u16 max_cll;
>> > >> +	__u16 max_fall;
>> > >> +	__u16 min_cll;
>> > >> +};
>> > >> +
>> > >> +struct hdr_sink_metadata {
>> > >> +	__u32 metadata_type;
>> > >> +	union {
>> > >> +		struct hdr_static_metadata hdmi_type1;
>> > >> +	};
>> > >> +};
>> > >
>> > >These two structs (hdr_static_metadata and hdr_sink_metadata)
>> > >should probably not be part of uapi, unless the metadata is
>> > >intended to be exposed in a HDR_SINK_METADATA property.
>> >
>> > Hmm yeah, currently we are not planning it to expose that. It was
>> > just to help userspace get and parse the EDID and store the details.
>> > But yes this doesn't need to be UAPI and userspace should be free to use whatever
>way they want to parse and store sink metadata.
>> > Will move it outside of uapi headers.
>> >
>> >
>> > >The commit "drm/i915: Add HLG EOTF" should probably not use a i915
>> > >in prefix as it affects drm core.
>> >
>> > Ok, will update this.
>> >
>> > >
>> > >The blob_ptr should probably be reference counted in
>> > >duplicate_state/destroy_state helper similar to [1].
>> > >I know too little about drm core in order to know if that is
>> > >correct, please feel free to pick/squash if this is correct.
>> >
>> > Sure, I will add these changes to the series.
>> >
>> > >[1] https://github.com/Kwiboo/linux-
>> > >rockchip/commit/5f065ff0bbcca145ee46e9466bb8ca048c7a7b1e
>> > >
>> > >I have tested this patchset on rockchip / dw-hdmi using dw-hdmi [2]
>> > >and Kodi [3] patches with a successful result of enabling HDR mode on the sink.
>> > >There is more work needed to get a full 10-bit pipeline for dw-hdmi
>> > >in order to make full use of HDR, but for the purpose of enabling
>> > >HDR on the sink this patchset seems ready.
>> >
>> > Thanks Jonas for your comments and more importantly for testing and enabling
>with kodi.
>> >
>> > I will rebase the series and send out next version. With the changes
>> > already in place in kodi branch, we should probably be good for merge.
>>
>> Patch 9 is not ready IMO. I don't see any analysis on why it's OK to
>> update infoframes without a full modeset. I would just drop that patch
>> from the initial set and work on it as a followup.

If the timings are not changed, driver goes for fastest and if we don't have
DRM infoframe handled there, we may not be able to send metadata to sink.
Hence its needed here, also it may help later for dynamic metadata cases as well
where we don't need modeset/blanking and just update metadata via infoframe.

>> Also did someone analysze how the new dynamic HDR metadata fits in
>> with the new uapi?

Currently, we have added the metadata as union so we should be able to extend
it later for dynamic metadata. 

>Also I can't see readout + state checker for the HDR infoframe. Am I blind or is it
>missing?

Yeah it's not there, will add the readout and state check.

Thanks Ville for your comments and inputs. Will send out the next version with those fixed.

Regards,
Uma Shankar

>--
>Ville Syrjälä
>Intel

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 5eb4013..8b9c126 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -881,6 +881,8 @@  static void drm_atomic_connector_print_state(struct drm_printer *p,
 
 	drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector->name);
 	drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)");
+	drm_printf(p, "\thdr_metadata_changed=%d\n",
+		   state->hdr_metadata_changed);
 
 	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
 		if (state->writeback_job && state->writeback_job->fb)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index ea797d4..6d0d161 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -673,6 +673,8 @@  static int drm_atomic_connector_set_property(struct drm_connector *connector,
 {
 	struct drm_device *dev = connector->dev;
 	struct drm_mode_config *config = &dev->mode_config;
+	bool replaced = false;
+	int ret;
 
 	if (property == config->prop_crtc_id) {
 		struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val);
@@ -721,6 +723,14 @@  static int drm_atomic_connector_set_property(struct drm_connector *connector,
 		 */
 		if (state->link_status != DRM_LINK_STATUS_GOOD)
 			state->link_status = val;
+	} else if (property == config->hdr_output_metadata_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->hdr_output_metadata_blob_ptr,
+				val,
+				-1, sizeof(struct hdr_output_metadata),
+				&replaced);
+		state->hdr_metadata_changed |= replaced;
+		return ret;
 	} else if (property == config->aspect_ratio_property) {
 		state->picture_aspect_ratio = val;
 	} else if (property == config->content_type_property) {
@@ -807,6 +817,9 @@  static int drm_atomic_connector_set_property(struct drm_connector *connector,
 		*val = state->colorspace;
 	} else if (property == connector->scaling_mode_property) {
 		*val = state->scaling_mode;
+	} else if (property == config->hdr_output_metadata_property) {
+		*val = (state->hdr_output_metadata_blob_ptr) ?
+			state->hdr_output_metadata_blob_ptr->base.id : 0;
 	} else if (property == connector->content_protection_property) {
 		*val = state->content_protection;
 	} else if (property == config->writeback_fb_id_property) {
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 2355124..0bdae90 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1058,6 +1058,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_OUTPUT_METADATA", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.hdr_output_metadata_property = prop;
+
 	return 0;
 }
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 02a1312..5343f60 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -599,6 +599,13 @@  struct drm_connector_state {
 	 * and the connector bpc limitations obtained from edid.
 	 */
 	u8 max_bpc;
+
+	/**
+	 * @metadata_blob_ptr:
+	 * DRM blob property for HDR output metadata
+	 */
+	struct drm_property_blob *hdr_output_metadata_blob_ptr;
+	u8 hdr_metadata_changed : 1;
 };
 
 /**
@@ -1239,6 +1246,10 @@  struct drm_connector {
 	 * &drm_mode_config.connector_free_work.
 	 */
 	struct llist_node free_node;
+
+	/* HDR metdata */
+	struct hdr_output_metadata hdr_output_metadata;
+	struct hdr_sink_metadata hdr_sink_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 7f60e8e..ef2656b 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_output_metadata_property;
+
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
 
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index 927ad64..a065194 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -152,6 +152,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,
+};
+
 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 83cd163..7b6a519 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -630,6 +630,45 @@  struct drm_color_lut {
 	__u16 reserved;
 };
 
+/* HDR Metadata Infoframe as per 861.G spec */
+struct hdr_metadata_infoframe {
+	__u8 eotf;
+	__u8 metadata_type;
+	struct {
+		__u16 x, y;
+		} display_primaries[3];
+	struct {
+		__u16 x, y;
+		} white_point;
+	__u16 max_display_mastering_luminance;
+	__u16 min_display_mastering_luminance;
+	__u16 max_cll;
+	__u16 max_fall;
+};
+
+struct hdr_output_metadata {
+	__u32 metadata_type;
+	union {
+		struct hdr_metadata_infoframe hdmi_metadata_type1;
+	};
+};
+
+/* HDR Metadata as per 861.G spec */
+struct hdr_static_metadata {
+	__u8 eotf;
+	__u8 metadata_type;
+	__u16 max_cll;
+	__u16 max_fall;
+	__u16 min_cll;
+};
+
+struct hdr_sink_metadata {
+	__u32 metadata_type;
+	union {
+		struct hdr_static_metadata hdmi_type1;
+	};
+};
+
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4