>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of >Sharma, Shashank >Sent: Thursday, December 20, 2018 11:21 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; >dri-devel@xxxxxxxxxxxxxxxxxxxxx >Cc: Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten ><maarten.lankhorst@xxxxxxxxx> >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@xxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx >https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
