[v10,01/12] drm: Add HDR source metadata property

Commit Message

Shankar, Uma May 14, 2019, 5:36 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.

v8: Addressed Ville's review comments. Moved sink metadata structure
out of uapi headers as suggested by Jonas Karlman.

v9: Rebase and addressed Jonas Karlman review comments.

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     |  7 +++++++
 include/linux/hdmi.h              | 26 ++++++++++++++++++++++++++
 include/uapi/drm/drm_mode.h       | 23 +++++++++++++++++++++++
 7 files changed, 88 insertions(+)

Comments

Ville Syrjala May 15, 2019, 7:10 p.m. UTC | #1

On Tue, May 14, 2019 at 11:06:23PM +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.
> 
> v8: Addressed Ville's review comments. Moved sink metadata structure
> out of uapi headers as suggested by Jonas Karlman.
> 
> v9: Rebase and addressed Jonas Karlman review comments.
> 
> 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     |  7 +++++++
>  include/linux/hdmi.h              | 26 ++++++++++++++++++++++++++
>  include/uapi/drm/drm_mode.h       | 23 +++++++++++++++++++++++
>  7 files changed, 88 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f4924cb..0d27195 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -925,6 +925,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 4131e66..4aa82c91 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -676,6 +676,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, file_priv, val);
> @@ -726,6 +728,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,
> +				val,
> +				-1, sizeof(struct hdr_output_metadata),

Those function arguments are nonsesne for a blob that isn't an array.

> +				&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) {
> @@ -814,6 +824,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 ?
> +			state->hdr_output_metadata->base.id : 0;
>  	} else if (property == config->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 11fcd25..c9ac8b9 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1051,6 +1051,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 e257b87..54daa54 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -603,6 +603,13 @@ struct drm_connector_state {
>  	 * and the connector bpc limitations obtained from edid.
>  	 */
>  	u8 max_bpc;
> +
> +	/**
> +	 * @hdr_output_metadata:
> +	 * DRM blob property for HDR output metadata
> +	 */
> +	struct drm_property_blob *hdr_output_metadata;
> +	u8 hdr_metadata_changed : 1;

I think the changed flag is now unused. Should be dropped if so.

>  };
>  
>  /**
> @@ -1237,6 +1244,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 5764ee3..58278cc 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -842,6 +842,13 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *content_protection_property;
>  
> +	/**
> +	 * hdr_output_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..6780476 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;
> @@ -320,6 +330,22 @@ struct hdmi_vendor_infoframe {
>  	unsigned int s3d_ext_data;
>  };
>  
> +/* 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;
> +	};
> +};
> +
>  int hdmi_vendor_infoframe_init(struct hdmi_vendor_infoframe *frame);
>  ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
>  				   void *buffer, size_t size);
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 83cd163..997a7e0 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -630,6 +630,29 @@ 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;
> +	};
> +};
> +
>  #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

Jonas Karlman May 15, 2019, 7:33 p.m. UTC | #2

On 2019-05-15 21:10, Ville Syrjälä wrote:
> On Tue, May 14, 2019 at 11:06:23PM +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.
>>
>> v8: Addressed Ville's review comments. Moved sink metadata structure
>> out of uapi headers as suggested by Jonas Karlman.
>>
>> v9: Rebase and addressed Jonas Karlman review comments.
>>
>> 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     |  7 +++++++
>>  include/linux/hdmi.h              | 26 ++++++++++++++++++++++++++
>>  include/uapi/drm/drm_mode.h       | 23 +++++++++++++++++++++++
>>  7 files changed, 88 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index f4924cb..0d27195 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -925,6 +925,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 4131e66..4aa82c91 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -676,6 +676,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, file_priv, val);
>> @@ -726,6 +728,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,
>> +				val,
>> +				-1, sizeof(struct hdr_output_metadata),
> Those function arguments are nonsesne for a blob that isn't an array.
>
>> +				&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) {
>> @@ -814,6 +824,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 ?
>> +			state->hdr_output_metadata->base.id : 0;
>>  	} else if (property == config->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 11fcd25..c9ac8b9 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1051,6 +1051,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 e257b87..54daa54 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -603,6 +603,13 @@ struct drm_connector_state {
>>  	 * and the connector bpc limitations obtained from edid.
>>  	 */
>>  	u8 max_bpc;
>> +
>> +	/**
>> +	 * @hdr_output_metadata:
>> +	 * DRM blob property for HDR output metadata
>> +	 */
>> +	struct drm_property_blob *hdr_output_metadata;
>> +	u8 hdr_metadata_changed : 1;
> I think the changed flag is now unused. Should be dropped if so.

I am using this changed flag in my dw-hdmi HDR patch [1] to force set mode_changed.

if (new_state->hdr_metadata_changed)
    crtc_state->mode_changed = true;

Maybe this is bad practice and should be handled in some other way?

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

Regards,
Jonas
>
>>  };
>>  
>>  /**
>> @@ -1237,6 +1244,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 5764ee3..58278cc 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -842,6 +842,13 @@ struct drm_mode_config {
>>  	 */
>>  	struct drm_property *content_protection_property;
>>  
>> +	/**
>> +	 * hdr_output_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..6780476 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;
>> @@ -320,6 +330,22 @@ struct hdmi_vendor_infoframe {
>>  	unsigned int s3d_ext_data;
>>  };
>>  
>> +/* 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;
>> +	};
>> +};
>> +
>>  int hdmi_vendor_infoframe_init(struct hdmi_vendor_infoframe *frame);
>>  ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
>>  				   void *buffer, size_t size);
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 83cd163..997a7e0 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -630,6 +630,29 @@ 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;
>> +	};
>> +};
>> +
>>  #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

Ville Syrjala May 15, 2019, 7:45 p.m. UTC | #3

On Wed, May 15, 2019 at 07:33:10PM +0000, Jonas Karlman wrote:
> On 2019-05-15 21:10, Ville Syrjälä wrote:
> > On Tue, May 14, 2019 at 11:06:23PM +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.
> >>
> >> v8: Addressed Ville's review comments. Moved sink metadata structure
> >> out of uapi headers as suggested by Jonas Karlman.
> >>
> >> v9: Rebase and addressed Jonas Karlman review comments.
> >>
> >> 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     |  7 +++++++
> >>  include/linux/hdmi.h              | 26 ++++++++++++++++++++++++++
> >>  include/uapi/drm/drm_mode.h       | 23 +++++++++++++++++++++++
> >>  7 files changed, 88 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index f4924cb..0d27195 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -925,6 +925,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 4131e66..4aa82c91 100644
> >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> @@ -676,6 +676,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, file_priv, val);
> >> @@ -726,6 +728,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,
> >> +				val,
> >> +				-1, sizeof(struct hdr_output_metadata),
> > Those function arguments are nonsesne for a blob that isn't an array.
> >
> >> +				&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) {
> >> @@ -814,6 +824,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 ?
> >> +			state->hdr_output_metadata->base.id : 0;
> >>  	} else if (property == config->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 11fcd25..c9ac8b9 100644
> >> --- a/drivers/gpu/drm/drm_connector.c
> >> +++ b/drivers/gpu/drm/drm_connector.c
> >> @@ -1051,6 +1051,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 e257b87..54daa54 100644
> >> --- a/include/drm/drm_connector.h
> >> +++ b/include/drm/drm_connector.h
> >> @@ -603,6 +603,13 @@ struct drm_connector_state {
> >>  	 * and the connector bpc limitations obtained from edid.
> >>  	 */
> >>  	u8 max_bpc;
> >> +
> >> +	/**
> >> +	 * @hdr_output_metadata:
> >> +	 * DRM blob property for HDR output metadata
> >> +	 */
> >> +	struct drm_property_blob *hdr_output_metadata;
> >> +	u8 hdr_metadata_changed : 1;
> > I think the changed flag is now unused. Should be dropped if so.
> 
> I am using this changed flag in my dw-hdmi HDR patch [1] to force set mode_changed.
> 
> if (new_state->hdr_metadata_changed)
>     crtc_state->mode_changed = true;
> 
> Maybe this is bad practice and should be handled in some other way?

Could just check old metadata != new metadata. The i915 patch even
memcmp()s the thing, but I think that's probably overdoing it since
we anyway downgrade the modeset to a fastset whenever possible.

The core doesn't memcmp() either IIRC, so hdr_metadata_changed should
be equal to just comparing the old and new blob pointers.

> 
> [1] https://github.com/Kwiboo/linux-rockchip/commit/a9ccea6d3d51001f6b4ab9a1fb600a38e517b9ce
> 
> Regards,
> Jonas
> >
> >>  };
> >>  
> >>  /**
> >> @@ -1237,6 +1244,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 5764ee3..58278cc 100644
> >> --- a/include/drm/drm_mode_config.h
> >> +++ b/include/drm/drm_mode_config.h
> >> @@ -842,6 +842,13 @@ struct drm_mode_config {
> >>  	 */
> >>  	struct drm_property *content_protection_property;
> >>  
> >> +	/**
> >> +	 * hdr_output_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..6780476 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;
> >> @@ -320,6 +330,22 @@ struct hdmi_vendor_infoframe {
> >>  	unsigned int s3d_ext_data;
> >>  };
> >>  
> >> +/* 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;
> >> +	};
> >> +};
> >> +
> >>  int hdmi_vendor_infoframe_init(struct hdmi_vendor_infoframe *frame);
> >>  ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
> >>  				   void *buffer, size_t size);
> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> index 83cd163..997a7e0 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -630,6 +630,29 @@ 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;
> >> +	};
> >> +};
> >> +
> >>  #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

Jonas Karlman May 15, 2019, 7:50 p.m. UTC | #4

On 2019-05-15 21:45, Ville Syrjälä wrote:
> On Wed, May 15, 2019 at 07:33:10PM +0000, Jonas Karlman wrote:
>> On 2019-05-15 21:10, Ville Syrjälä wrote:
>>> On Tue, May 14, 2019 at 11:06:23PM +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.
>>>>
>>>> v8: Addressed Ville's review comments. Moved sink metadata structure
>>>> out of uapi headers as suggested by Jonas Karlman.
>>>>
>>>> v9: Rebase and addressed Jonas Karlman review comments.
>>>>
>>>> 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     |  7 +++++++
>>>>  include/linux/hdmi.h              | 26 ++++++++++++++++++++++++++
>>>>  include/uapi/drm/drm_mode.h       | 23 +++++++++++++++++++++++
>>>>  7 files changed, 88 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>> index f4924cb..0d27195 100644
>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>> @@ -925,6 +925,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 4131e66..4aa82c91 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> @@ -676,6 +676,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, file_priv, val);
>>>> @@ -726,6 +728,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,
>>>> +				val,
>>>> +				-1, sizeof(struct hdr_output_metadata),
>>> Those function arguments are nonsesne for a blob that isn't an array.
>>>
>>>> +				&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) {
>>>> @@ -814,6 +824,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 ?
>>>> +			state->hdr_output_metadata->base.id : 0;
>>>>  	} else if (property == config->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 11fcd25..c9ac8b9 100644
>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>> @@ -1051,6 +1051,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 e257b87..54daa54 100644
>>>> --- a/include/drm/drm_connector.h
>>>> +++ b/include/drm/drm_connector.h
>>>> @@ -603,6 +603,13 @@ struct drm_connector_state {
>>>>  	 * and the connector bpc limitations obtained from edid.
>>>>  	 */
>>>>  	u8 max_bpc;
>>>> +
>>>> +	/**
>>>> +	 * @hdr_output_metadata:
>>>> +	 * DRM blob property for HDR output metadata
>>>> +	 */
>>>> +	struct drm_property_blob *hdr_output_metadata;
>>>> +	u8 hdr_metadata_changed : 1;
>>> I think the changed flag is now unused. Should be dropped if so.
>> I am using this changed flag in my dw-hdmi HDR patch [1] to force set mode_changed.
>>
>> if (new_state->hdr_metadata_changed)
>>     crtc_state->mode_changed = true;
>>
>> Maybe this is bad practice and should be handled in some other way?
> Could just check old metadata != new metadata. The i915 patch even
> memcmp()s the thing, but I think that's probably overdoing it since
> we anyway downgrade the modeset to a fastset whenever possible.
>
> The core doesn't memcmp() either IIRC, so hdr_metadata_changed should
> be equal to just comparing the old and new blob pointers.

Thanks for the pointers, I will probably end up following what the i915 code does once it is fully ready.

Regards,
Jonas

>
>> [1] https://github.com/Kwiboo/linux-rockchip/commit/a9ccea6d3d51001f6b4ab9a1fb600a38e517b9ce
>>
>> Regards,
>> Jonas
>>>>  };
>>>>  
>>>>  /**
>>>> @@ -1237,6 +1244,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 5764ee3..58278cc 100644
>>>> --- a/include/drm/drm_mode_config.h
>>>> +++ b/include/drm/drm_mode_config.h
>>>> @@ -842,6 +842,13 @@ struct drm_mode_config {
>>>>  	 */
>>>>  	struct drm_property *content_protection_property;
>>>>  
>>>> +	/**
>>>> +	 * hdr_output_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..6780476 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;
>>>> @@ -320,6 +330,22 @@ struct hdmi_vendor_infoframe {
>>>>  	unsigned int s3d_ext_data;
>>>>  };
>>>>  
>>>> +/* 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;
>>>> +	};
>>>> +};
>>>> +
>>>>  int hdmi_vendor_infoframe_init(struct hdmi_vendor_infoframe *frame);
>>>>  ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
>>>>  				   void *buffer, size_t size);
>>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>>> index 83cd163..997a7e0 100644
>>>> --- a/include/uapi/drm/drm_mode.h
>>>> +++ b/include/uapi/drm/drm_mode.h
>>>> @@ -630,6 +630,29 @@ 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;
>>>> +	};
>>>> +};
>>>> +
>>>>  #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

Shankar, Uma May 16, 2019, 7:21 a.m. UTC | #5

>-----Original Message-----
>From: Ville Syrjälä [mailto:[e-mail]]
>Sent: Thursday, May 16, 2019 12:40 AM
>To: Shankar, Uma <[e-mail]>
>Cc: [e-mail]; [e-mail];
>[e-mail]; Sharma, Shashank
><[e-mail]>; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; Roper, Matthew D
><[e-mail]>; [e-mail]
>Subject: Re: [v10 01/12] drm: Add HDR source metadata property
>
>On Tue, May 14, 2019 at 11:06:23PM +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.
>>
>> v8: Addressed Ville's review comments. Moved sink metadata structure
>> out of uapi headers as suggested by Jonas Karlman.
>>
>> v9: Rebase and addressed Jonas Karlman review comments.
>>
>> 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     |  7 +++++++
>>  include/linux/hdmi.h              | 26 ++++++++++++++++++++++++++
>>  include/uapi/drm/drm_mode.h       | 23 +++++++++++++++++++++++
>>  7 files changed, 88 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c
>> b/drivers/gpu/drm/drm_atomic.c index f4924cb..0d27195 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -925,6 +925,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 4131e66..4aa82c91 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -676,6 +676,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, file_priv, val); @@
>> -726,6 +728,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,
>> +				val,
>> +				-1, sizeof(struct hdr_output_metadata),
>
>Those function arguments are nonsesne for a blob that isn't an array.

Hmm, just to clarify - Since this is a fixed struct being passed. We should make
ret = drm_atomic_replace_property_blob_from_id(dev,
				&state->hdr_output_metadata,
				val,
				sizeof(struct hdr_output_metadata), -1,
				&replaced);

Basically have check for expected_size rather expected_elem_size.
Hope this is what you meant.

>> +		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) { @@ -814,6
>> +824,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 ?
>> +			state->hdr_output_metadata->base.id : 0;
>>  	} else if (property == config->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 11fcd25..c9ac8b9 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1051,6 +1051,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 e257b87..54daa54 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -603,6 +603,13 @@ struct drm_connector_state {
>>  	 * and the connector bpc limitations obtained from edid.
>>  	 */
>>  	u8 max_bpc;
>> +
>> +	/**
>> +	 * @hdr_output_metadata:
>> +	 * DRM blob property for HDR output metadata
>> +	 */
>> +	struct drm_property_blob *hdr_output_metadata;
>> +	u8 hdr_metadata_changed : 1;
>
>I think the changed flag is now unused. Should be dropped if so.

Ok Sure, will drop this.

>>  };
>>
>>  /**
>> @@ -1237,6 +1244,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 5764ee3..58278cc 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -842,6 +842,13 @@ struct drm_mode_config {
>>  	 */
>>  	struct drm_property *content_protection_property;
>>
>> +	/**
>> +	 * hdr_output_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..6780476 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;
>> @@ -320,6 +330,22 @@ struct hdmi_vendor_infoframe {
>>  	unsigned int s3d_ext_data;
>>  };
>>
>> +/* 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;
>> +	};
>> +};
>> +
>>  int hdmi_vendor_infoframe_init(struct hdmi_vendor_infoframe *frame);
>> ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
>>  				   void *buffer, size_t size);
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 83cd163..997a7e0 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -630,6 +630,29 @@ 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;
>> +	};
>> +};
>> +
>>  #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
>
>--
>Ville Syrjälä
>Intel

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f4924cb..0d27195 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -925,6 +925,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 4131e66..4aa82c91 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -676,6 +676,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, file_priv, val);
@@ -726,6 +728,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,
+				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) {
@@ -814,6 +824,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 ?
+			state->hdr_output_metadata->base.id : 0;
 	} else if (property == config->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 11fcd25..c9ac8b9 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1051,6 +1051,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 e257b87..54daa54 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -603,6 +603,13 @@  struct drm_connector_state {
 	 * and the connector bpc limitations obtained from edid.
 	 */
 	u8 max_bpc;
+
+	/**
+	 * @hdr_output_metadata:
+	 * DRM blob property for HDR output metadata
+	 */
+	struct drm_property_blob *hdr_output_metadata;
+	u8 hdr_metadata_changed : 1;
 };
 
 /**
@@ -1237,6 +1244,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 5764ee3..58278cc 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -842,6 +842,13 @@  struct drm_mode_config {
 	 */
 	struct drm_property *content_protection_property;
 
+	/**
+	 * hdr_output_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..6780476 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;
@@ -320,6 +330,22 @@  struct hdmi_vendor_infoframe {
 	unsigned int s3d_ext_data;
 };
 
+/* 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;
+	};
+};
+
 int hdmi_vendor_infoframe_init(struct hdmi_vendor_infoframe *frame);
 ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
 				   void *buffer, size_t size);
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 83cd163..997a7e0 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -630,6 +630,29 @@  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;
+	};
+};
+
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4