[Intel-gfx] [v2 01/14] drm: Add HDR source metadata property
Shankar, Uma
uma.shankar at intel.com
Tue Jan 8 05:29:36 UTC 2019
>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of
>Sharma, Shashank
>Sent: Thursday, December 20, 2018 11:21 PM
>To: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org;
>dri-devel at lists.freedesktop.org
>Cc: Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
><maarten.lankhorst at 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 at 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 at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the Intel-gfx
mailing list