[Intel-gfx] [PATCH] drm: Fix docbook warnings in hdr metadata helper structures

Wed May 29 17:23:01 UTC 2019

>-----Original Message-----
>From: Daniel Vetter [mailto:daniel at ffwll.ch]
>Sent: Wednesday, May 29, 2019 8:31 PM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: intel-gfx <intel-gfx at lists.freedesktop.org>; dri-devel <dri-
>devel at lists.freedesktop.org>; Sharma, Shashank <shashank.sharma at intel.com>;
>Ville Syrjälä <ville.syrjala at linux.intel.com>; Maarten Lankhorst
><maarten.lankhorst at linux.intel.com>; Maxime Ripard
><maxime.ripard at bootlin.com>; Sean Paul <sean at poorly.run>; David Airlie
><airlied at linux.ie>; Bartlomiej Zolnierkiewicz <b.zolnierkie at samsung.com>; Hans
>Verkuil <hansverk at cisco.com>; Linux Fbdev development list <linux-
>fbdev at 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 at 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 at intel.com>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Maxime Ripard <maxime.ripard at bootlin.com>
>> Cc: Sean Paul <sean at poorly.run>
>> Cc: David Airlie <airlied at linux.ie>
>> Cc: Daniel Vetter <daniel at ffwll.ch>
>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie at samsung.com>
>> Cc: "Ville Syrjälä" <ville.syrjala at linux.intel.com>
>> Cc: Hans Verkuil <hansverk at cisco.com>
>> Cc: dri-devel at lists.freedesktop.org
>> Cc: linux-fbdev at vger.kernel.org
>> Signed-off-by: Uma Shankar <uma.shankar at 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 they
>>   *     must support "None", which should be the default (since external screens
>>   *     have 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