-----Original Message----- From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Daniel Vetter Sent: Monday, June 3, 2019 1:53 PM To: Shankar, Uma uma.shankar@intel.com Cc: Sean Paul sean@poorly.run; linux-fbdev@vger.kernel.org; dcastagna@chromium.org; jonas@kwiboo.se; Maxime Ripard maxime.ripard@bootlin.com; intel-gfx@lists.freedesktop.org; Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com; emil.l.velikov@gmail.com; dri- devel@lists.freedesktop.org; Hans Verkuil hansverk@cisco.com; David Airlie airlied@linux.ie; seanpaul@chromium.org Subject: Re: [PATCH 2/4] drm: Fix docbook warnings in hdr metadata helper structures
On Thu, May 30, 2019 at 01:29:02AM +0530, Uma Shankar wrote:
Fixes the following warnings: ./include/drm/drm_mode_config.h:841: warning: Incorrect use of kernel-doc format: * hdr_output_metadata_property: Connector property containing hdr ./include/drm/drm_mode_config.h:918: warning: Function parameter or member
'hdr_output_metadata_property' not described in 'drm_mode_config'
./include/drm/drm_connector.h:1251: warning: Function parameter or member
'hdr_output_metadata' not described in 'drm_connector'
./include/drm/drm_connector.h:1251: warning: Function parameter or member
'hdr_sink_metadata' not described in 'drm_connector'
Also adds some property documentation for HDR Metadata Connector Property in connector property create function.
v2: Fixed Sean Paul's review comments.
v3: Fixed Daniel Vetter's review comments, added the UAPI structure definition section in kernel docs.
Cc: Shashank Sharma shashank.sharma@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com Cc: "Ville Syrjälä" ville.syrjala@linux.intel.com Cc: Hans Verkuil hansverk@cisco.com Cc: dri-devel@lists.freedesktop.org Cc: linux-fbdev@vger.kernel.org Reviewed-by: Sean Paul sean@poorly.run Signed-off-by: Uma Shankar uma.shankar@intel.com
Documentation/gpu/drm-uapi.rst | 9 +++++++++ drivers/gpu/drm/drm_connector.c | 31 +++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 1 + include/drm/drm_mode_config.h | 4 ++-- include/linux/hdmi.h | 1 + include/uapi/drm/drm_mode.h | 37
++++++++++++++++++++++++++++++++++++-
6 files changed, 80 insertions(+), 3 deletions(-)
diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst index 05874d0..6b39e2c 100644 --- a/Documentation/gpu/drm-uapi.rst +++ b/Documentation/gpu/drm-uapi.rst @@ -329,3 +329,12 @@ DRM_IOCTL_MODESET_CTL mode setting, since on many devices the vertical blank counter is reset to 0 at some point during modeset. Modern drivers should not call this any more since with kernel mode setting it is a no-op.
+Userspace API Structures +========================
+.. kernel-doc:: include/uapi/drm/drm_mode.h
- :doc: overview
+.. kernel-doc:: include/uapi/drm/drm_mode.h
- :internal:
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index c9ac8b9..6a93527 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -956,6 +956,37 @@ int drm_display_info_set_bus_formats(struct
drm_display_info *info,
is no longer protected and userspace should take appropriate action(whatever that might be).
- HDR_OUTPUT_METADATA:
- Connector property to enable userspace to send HDR Metadata to
- driver. This metadata is based on the composition and blending
- policies decided by user, taking into account the hardware and
- sink capabilities. The driver gets this metadata and creates a
- Dynamic Range and Mastering Infoframe (DRM) in case of HDMI,
- SDP packet (Non-audio INFOFRAME SDP v1.3) for DP. This is then
- sent to sink. This notifies the sink of the upcoming frame's Color
- Encoding and Luminance parameters.
- Userspace first need to detect the HDR capabilities of sink by
- reading and parsing the EDID. Details of HDR metadata for HDMI
- are added in CTA 861.G spec. For DP , its defined in VESA DP
- Standard v1.4. It needs to then get the metadata information
- of the video/game/app content which are encoded in HDR (basically
- using HDR transfer functions). With this information it needs to
- decide on a blending policy and compose the relevant
- layers/overlays into a common format. Once this blending is done,
- userspace will be aware of the metadata of the composed frame to
- be send to sink. It then uses this property to communicate this
- metadata to driver which then make a Infoframe packet and sends
- to sink based on the type of encoder connected.
- Userspace will be responsible to do Tone mapping operation in case:
- Some layers are HDR and others are SDR
- HDR layers luminance is not same as sink
- It will even need to do colorspace conversion and get all layers
- to one common colorspace for blending. It can use either GL, Media
- or display engine to get this done based on the capabilties of the
- associated hardware.
I think it'd be good to add 1-2 sentences here about what this looks like for the driver side. E.g. which function drivers need to call to set up hdr, how to get at the metadata and whether there's any helpers for these.
Here I'd point at hdr_output_metadata, hdr_sink_metadata, and drm_add_display_info() for filling in the former.
Sure, will do the same.
I think with that this is a solid doc patch.
- max bpc:
- This range property is used by userspace to limit the bit depth. When
- used the driver would limit the bpc in accordance with the valid range
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 5476561..47e749b 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1244,6 +1244,7 @@ struct drm_connector { */ struct llist_node free_node;
- /** @hdr_sink_metadata: HDR Metadata Information read from sink */ struct hdr_sink_metadata hdr_sink_metadata;
Something I realized while reading the code: This should probably be put into the drm_display_info struct, like all the other things we parse out of the edid. I think that would be a lot more consistent with the code - the driver interface function which calls this code is called drm_add_display_info() after all.
Can you pls change that in a follow-up patch?
Ok, I will do that as a follow-up.
};
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 4f88cc9..759d462 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -837,8 +837,8 @@ struct drm_mode_config { struct drm_property *writeback_out_fence_ptr_property;
/**
* hdr_output_metadata_property: Connector property containing hdr* metatda. This will be provided by userspace compositors based
* @hdr_output_metadata_property: Connector property containing hdr* metatada. This will be provided by userspace compositors based*/ struct drm_property *hdr_output_metadata_property; diff --git
- on HDR content
a/include/linux/hdmi.h b/include/linux/hdmi.h index ee55ba5..55c6db5 100644 --- a/include/linux/hdmi.h +++ b/include/linux/hdmi.h @@ -398,6 +398,7 @@ ssize_t hdmi_vendor_infoframe_pack_only(const struct
hdmi_vendor_infoframe *fram
- @spd: spd infoframe
- @vendor: union of all vendor infoframes
- @audio: audio infoframe
- @drm: Dynamic Range and Mastering infoframe
- This is used by the generic pack function. This works since all infoframes
- have the same header which also indicates which type of infoframe
should be diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 997a7e0..5d3964f 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -33,6 +33,15 @@ extern "C" { #endif
+/**
- DOC: overview
- DRM exposes many UAPI and structure definition to have a
+consistent
- and standardized interface with user.
- Userspace can refer to these structure definitions and UAPI
+formats
- to communicate to driver
- */
#define DRM_CONNECTOR_NAME_LEN 32 #define DRM_DISPLAY_MODE_LEN 32 #define DRM_PROP_NAME_LEN 32 @@ -630,7 +639,26 @@ struct drm_color_lut { __u16 reserved; };
-/* HDR Metadata Infoframe as per 861.G spec */
Keep the spec reference imo, maybe even add a note that this is supposed to perfectly match it.
Ok, will add this.
+/**
- struct hdr_metadata_infoframe - HDR Metadata Infoframe Data.
- @eotf: Electro-Optical Transfer Function (EOTF) used in the stream.
- @metadata_type: Static_Metadata_Descriptor_ID.
- @display_primaries: Color Primaries of the Data.
- @display_primaries.x: X cordinate of color primary.
Would be good to spend a few more words about "in which standard/format" this color number is. I.e. fixed point or whatever, and color space.
Sure, will add more details on these values.
- @display_primaries.y: Y cordinate of color primary.
- @white_point: White Point of Colorspace Data.
- @white_point.x: X cordinate of whitepoint of color primary.
- @white_point.y: Y cordinate of whitepoint of color primary.
- @max_display_mastering_luminance: Max Mastering Display Luminance.
- @min_display_mastering_luminance: Min Mastering Display Luminance.
- @max_cll: Max Content Light Level.
- @max_fall: Max Frame Average Light Level.
btw for long structs I prefer the inline kerneldoc style. This one is just at the edge imo.
Ok, will do that.
- With drm subsystem using struct drm_rect to manage rectangular
- area this
struct &drm_rect to make it a hyperlink. Once we have drm_rect documented :-)
This is not required here, will drop these comments.
- export it to user-space.
- Currently used by drm_mode_atomic blob property FB_DAMAGE_CLIPS.
- */
struct hdr_metadata_infoframe { __u8 eotf; __u8 metadata_type; @@ -646,6 +674,13 @@ struct hdr_metadata_infoframe { __u16 max_fall; };
+/**
- struct hdr_output_metadata - HDR output metadata
- Metadata Information to be passed from userspace
- @metadata_type: Static_Metadata_Descriptor_ID.
- @hdmi_metadata_type1: HDR Metadata Infoframe.
If you want to move the member docs closer to their definition, go with the inline style.
Ok, will update this.
Thanks Daniel for the review comments and suggestions. Will send out the next version soon.
Regards, Uma Shankar
Thanks, Daniel
- */
struct hdr_output_metadata { __u32 metadata_type; union { -- 1.9.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel