[Intel-gfx] [v7 1/9] drm: Add HDR source metadata property
Shankar, Uma
uma.shankar at intel.com
Tue Apr 9 16:17:58 UTC 2019
>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of Jonas
>Karlman
>Sent: Monday, April 8, 2019 3:51 PM
>To: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org; dri-
>devel at lists.freedesktop.org
>Cc: seanpaul at chromium.org; emil.l.velikov at gmail.com; dcastagna at chromium.org;
>Lankhorst, Maarten <maarten.lankhorst at intel.com>; Syrjala, Ville
><ville.syrjala at intel.com>
>Subject: Re: [v7 1/9] drm: Add HDR source metadata property
>
>On 2019-04-02 22:20, 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.
>>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> ---
>> 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 | 10 ++++++++++
>> include/drm/drm_mode_config.h | 6 ++++++
>> include/linux/hdmi.h | 10 ++++++++++
>> include/uapi/drm/drm_mode.h | 22 ++++++++++++++++++++++
>> 7 files changed, 69 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..9e3a39c 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,9 @@ struct drm_connector {
>> * &drm_mode_config.connector_free_work.
>> */
>> struct llist_node free_node;
>> +
>> + /* HDR metdata */
>> + struct hdr_output_metadata hdr_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 09d7296..9d6bdf5 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -629,6 +629,28 @@ struct drm_color_lut {
>> __u16 reserved;
>> };
>>
>> +/* HDR Metadata as per 861.G spec */
>> +struct hdr_static_metadata {
>> + __u8 eotf;
>> + __u8 metadata_type;
>> + struct {
>> + __u16 x, y;
>> + } display_primaries[3];
>> + struct {
>> + __u16 x, y;
>> + } white_point;
>> + __u16 max_cll;
>> + __u16 max_fall;
>> + __u16 min_cll;
>> +};
>
>Does this struct represent a mix of the HDR Static Metadata Data Block (Table 84) and
>Static Metadata Descriptor Type 1 (Table 45) in CTA-861-G [1] ?
>
>display_primaries and white_point is only part of the Static Metadata Descriptor and
>min_cll is only part of the HDR Static Metadata.
>min_cll should be parsed and set in drm_parse_hdr_metadata_block() in next patch or
>be removed from the struct.
>
>max_display_mastering_luminance and min_display_mastering_luminance from
>Static Metadata Descriptor Type 1 is also missing in this struct.
>Why is max_cll and min_cll used for max_display_mastering_luminance and
>min_display_mastering_luminance in infoframe?
>
>"frame->length = sizeof(struct hdr_static_metadata);" also needs to be changed in
>another patch of the patchset.
>The Static Metadata Descriptor Type 1 is 26 bytes and is set to 24 bytes using sizeof
>the above struct.
>
>[1] https://standards.cta.tech/kwspub/published_docs/CTA-861-
>G_FINAL_revised_2017.pdf
>
Thanks Jonas for your feedback. I have created a separate structure for Sink metadata
to avoid these issues and fixed all the inputs you have given. Please review the v8.
Regards,
Uma Shankar
>Regards,
>Jonas
>
>> +
>> +struct hdr_output_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
>
>_______________________________________________
>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