>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Jonas >Karlman >Sent: Monday, April 8, 2019 3:51 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri- >devel@xxxxxxxxxxxxxxxxxxxxx >Cc: seanpaul@xxxxxxxxxxxx; emil.l.velikov@xxxxxxxxx; dcastagna@xxxxxxxxxxxx; >Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx>; Syrjala, Ville ><ville.syrjala@xxxxxxxxx> >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@xxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx >https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
