-----Original Message----- From: Daniel Vetter [mailto:daniel@ffwll.ch] Sent: Wednesday, May 29, 2019 8:31 PM To: Shankar, Uma uma.shankar@intel.com Cc: intel-gfx intel-gfx@lists.freedesktop.org; dri-devel <dri- devel@lists.freedesktop.org>; Sharma, Shashank shashank.sharma@intel.com; Ville Syrjälä ville.syrjala@linux.intel.com; Maarten Lankhorst maarten.lankhorst@linux.intel.com; Maxime Ripard maxime.ripard@bootlin.com; Sean Paul sean@poorly.run; David Airlie airlied@linux.ie; Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com; Hans Verkuil hansverk@cisco.com; Linux Fbdev development list <linux- fbdev@vger.kernel.org> Subject: Re: [PATCH] drm: Fix docbook warnings in hdr metadata helper structures
On Wed, May 29, 2019 at 4:16 PM Uma Shankar uma.shankar@intel.com 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.
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 Signed-off-by: Uma Shankar uma.shankar@intel.com
drivers/gpu/drm/drm_connector.c | 8 ++++++++ include/drm/drm_connector.h | 3 ++- include/drm/drm_mode_config.h | 2 +- include/linux/hdmi.h | 1 + 4 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index c9ac8b9..702307c 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1003,6 +1003,14 @@ int drm_display_info_set_bus_formats(struct
drm_display_info *info,
can also expose this property to external outputs, in which case theymust support "None", which should be the default (since external screenshave a built-in scaler).
- 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 capabilties.
The driver gets this metadata and creates a Dynamic Range and Mastering
Infoframe (DRM) which is then sent to sink. This notifies the sink of*/
the upcoming frame's Color Encoding and Luminance parameters.Assuming I'm applying this correctly your adding this to the "lcd panel properties" section. That doesn't make sense to me. I think we already have a section for hdmi properties somewhere, would fit better there.
This is generic (applies for HDMI as well as DP). I will move this above and near to General properties like EDID.
This should also contain a bit more about how this is supposed to work, how it's set up from a driver pov (sprinkle links all over it) and how userspace it supposed to use it.
OK, will add elaborate this adding these details as well.
I think since this is a using a rather complicated struct I think we need to fully document that structure too. Atm uapi/drm_mode.h isn't pulled into anywhere, so we need to fix that (a new chapter titled "Userspace API Structures" in drm-uapi.rst would be good, cross-links will work).
Ok, will add this new section and link the HDR structure definitions to this.
int drm_connector_create_standard_properties(struct drm_device *dev) diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index f8f4003..f226ef0 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1244,8 +1244,9 @@ struct drm_connector { */ struct llist_node free_node;
/* HDR metdata */
/** @hdr_output_metadata: HDR Metadata to be sent to sink */ struct hdr_output_metadata hdr_output_metadata;Uh, is this even used? It would be a bug if so, since the state userspace can set must be stored in drm_connector_state, not in drm_connector. Only read-only stuff can be in there.
Yeah, this is not required. We have the metadata handled as part of drm_connector_state. Will drop this from here. Thanks for spotting this.
Please don't just blindly type docs, try to make sure that what you're documenting actually makes sense. Also, should have been a clear sign that you've forgotten to document one of the properties in the enumeration above.
Ok Sure, will try to be careful with respect to the sections where things get placed. Thanks for all your inputs and feedback. Will send out the changes soon.
Regards, Uma Shankar
-Daniel
/** @hdr_sink_metadata: HDR Metadata Information read from- sink */ struct hdr_sink_metadata hdr_sink_metadata; };
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 4f88cc9..0b180e0 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -837,7 +837,7 @@ struct drm_mode_config { struct drm_property *writeback_out_fence_ptr_property;
/**
* hdr_output_metadata_property: Connector property containing hdr
* @hdr_output_metadata_property: Connector property- containing hdr * metatda. This will be provided by userspace compositors based * on HDR content */
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index ee55ba5..ea5858e 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: DRM 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
1.9.1
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch