| Message ID | 1544560702-16447-2-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add HDR Metadata Parsing and handling in DRM layer | expand |
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. > + 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 ? > + 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(). > }; > > #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 > + > /* 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. > +}; > + > 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 ? - Shashank > + > #define DRM_MODE_PAGE_FLIP_EVENT 0x01 > #define DRM_MODE_PAGE_FLIP_ASYNC 0x02 > #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
>-----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
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); + 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; + 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; }; #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; + /* 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, +}; + 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; +}; + #define DRM_MODE_PAGE_FLIP_EVENT 0x01 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
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(+)