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

From: "Shankar, Uma" <uma.shankar@intel.com>
To: "Sharma, Shashank" <shashank.sharma@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Cc: "Syrjala, Ville" <ville.syrjala@intel.com>,
	"Lankhorst, Maarten" <maarten.lankhorst@intel.com>
Subject: RE: [v2 01/14] drm: Add HDR source metadata property
Date: Tue, 8 Jan 2019 05:29:36 +0000	[thread overview]
Message-ID: <E7C9878FBA1C6D42A1CA3F62AEB6945F81E9B883@BGSMSX104.gar.corp.intel.com> (raw)
In-Reply-To: <cea58d62-4fdc-cddc-b7b8-7247f5e6ebe8@intel.com>

>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of
>Sharma, Shashank
>Sent: Thursday, December 20, 2018 11:21 PM
>To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
>dri-devel@lists.freedesktop.org
>Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
><maarten.lankhorst@intel.com>
>Subject: Re: [v2 01/14] drm: Add HDR source metadata property
>
>Regards
>
>Shashank
>
>
>On 12/12/2018 2:08 AM, Uma Shankar wrote:
>> This patch adds a blob property to get HDR metadata information from
>> userspace. This will be send as part of AVI Infoframe to panel.
>>
>> v2: Rebase and modified the metadata structure elements as per Ville's
>> POC changes.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>   drivers/gpu/drm/drm_connector.c |  6 ++++++
>>   include/drm/drm_connector.h     | 10 ++++++++++
>>   include/drm/drm_mode_config.h   |  6 ++++++
>>   include/linux/hdmi.h            | 10 ++++++++++
>>   include/uapi/drm/drm_mode.h     | 16 ++++++++++++++++
>>   5 files changed, 48 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c
>> b/drivers/gpu/drm/drm_connector.c index da8ae80..361fcda 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1027,6 +1027,12 @@ int
>drm_connector_create_standard_properties(struct drm_device *dev)
>>   		return -ENOMEM;
>>   	dev->mode_config.non_desktop_property = prop;
>>
>> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> +				   "HDR_SOURCE_METADATA", 0);
>I see that the compositor would be using this blob to setup the output HDR curve
>post blending, which would be in most of cases, sink HDR. So we should ideally
>call it HDR_OUTPUT_METADATA or output_hdr_metadata.

Ok Sure, will update the property name.

>> +	if (!prop)
>> +		return -ENOMEM;
>> +	dev->mode_config.hdr_source_metadata_property = prop;
>> +
>>   	return 0;
>>   }
>>
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 9be2181..2ee45dc 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -520,6 +520,13 @@ struct drm_connector_state {
>>   	 * and the connector bpc limitations obtained from edid.
>>   	 */
>>   	u8 max_bpc;
>> +
>> +	/**
>> +	 * @metadata_blob_ptr:
>> +	 * DRM blob property for HDR metadata
>> +	 */
>> +	struct drm_property_blob *hdr_source_metadata_blob_ptr;
>Same goes again here, output_hdr_metadata_blob ?

Ok.

>> +	u8 hdr_metadata_changed : 1;
>>   };
>>
>>   /**
>> @@ -1154,6 +1161,9 @@ struct drm_connector {
>>   	 * &drm_mode_config.connector_free_work.
>>   	 */
>>   	struct llist_node free_node;
>> +
>> +	/* HDR metdata */
>> +	struct hdr_static_metadata hdr_metadata;
>I think while designing this framework, we should leave the scope for dynamic
>metadata, which would be following up soon. So we should have a union inside
>struct hdr_metedata, which will have option for both static and dynamic type of
>metadata. I will add details to the place where you are adding definition of
>hdr_static_metadata().

I feel since we are not yet targeting beyond HDR10, lets not add dynamic metadata
definitions as of now. We will add when the same gets enabled. I hope this is ok.

>>   };
>>
>>   #define obj_to_connector(x) container_of(x, struct drm_connector,
>> base) diff --git a/include/drm/drm_mode_config.h
>> b/include/drm/drm_mode_config.h index 69ccd27..4b3211b 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -836,6 +836,12 @@ struct drm_mode_config {
>>   	 */
>>   	struct drm_property *writeback_out_fence_ptr_property;
>>
>> +	/*
>> +	 * hdr_metadata_property: Connector property containing hdr metatda
>> +	 * This will be provided by userspace compositors based on HDR content
>> +	 */
>> +	struct drm_property *hdr_source_metadata_property;
>Again, source->output

Sure, will change it.

>> +
>>   	/* dumb ioctl parameters */
>>   	uint32_t preferred_depth, prefer_shadow;
>>
>> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index
>> d2bacf5..bed3e08 100644
>> --- a/include/linux/hdmi.h
>> +++ b/include/linux/hdmi.h
>> @@ -137,6 +137,16 @@ enum hdmi_content_type {
>>   	HDMI_CONTENT_TYPE_GAME,
>>   };
>>
>> +enum hdmi_metadata_type {
>> +	HDMI_STATIC_METADATA_TYPE1 = 1,
>> +};
>> +
>> +enum hdmi_eotf {
>> +	HDMI_EOTF_TRADITIONAL_GAMMA_SDR,
>> +	HDMI_EOTF_TRADITIONAL_GAMMA_HDR,
>> +	HDMI_EOTF_SMPTE_ST2084,
>I guess we are not bothering about HLG now, which is fine actually, less
>complicated for now.

Yeah.

>> +};
>> +
>>   struct hdmi_avi_infoframe {
>>   	enum hdmi_infoframe_type type;
>>   	unsigned char version;
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index a439c2e..5012af2 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -630,6 +630,22 @@ struct drm_color_lut {
>>   	__u16 reserved;
>>   };
>>
>> +/* HDR Metadata */
>> +struct hdr_static_metadata {
>> +	uint8_t eotf;
>> +	uint8_t metadata_type;
>> +	struct {
>> +		uint16_t x, y;
>> +		} display_primaries[3];
>> +	struct {
>> +		uint16_t x, y;
>> +		} white_point;
>> +	uint16_t max_mastering_display_luminance;
>> +	uint16_t min_mastering_display_luminance;
>> +	uint16_t max_fall;
>> +	uint16_t max_cll;
>> +};
>How about defining struct hdr_metadata {
>     uint8_t size;
>     union {
>         struct hdr_static_metadata *static;
>         struct hdr_metadata_dynamic *dynamic;
>     } metadata;
>};
>So that its easier when we are extending support for dynamic HDR ?

As mentioned above, lets add them when it gets enabled.

Regards,
Uma Shankar

>- Shashank
>> +
>>   #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>>   #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
>>   #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel