RE: [PATCH 2/4] drm: Fix docbook warnings in hdr metadata helper structures

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Daniel
>Vetter
>Sent: Monday, June 3, 2019 1:53 PM
>To: Shankar, Uma <uma.shankar@xxxxxxxxx>
>Cc: Sean Paul <sean@xxxxxxxxxx>; linux-fbdev@xxxxxxxxxxxxxxx;
>dcastagna@xxxxxxxxxxxx; jonas@xxxxxxxxx; Maxime Ripard
><maxime.ripard@xxxxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Bartlomiej
>Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>; emil.l.velikov@xxxxxxxxx; dri-
>devel@xxxxxxxxxxxxxxxxxxxxx; Hans Verkuil <hansverk@xxxxxxxxx>; David Airlie
><airlied@xxxxxxxx>; seanpaul@xxxxxxxxxxxx
>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@xxxxxxxxx>
>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> Cc: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
>> Cc: Sean Paul <sean@xxxxxxxxxx>
>> Cc: David Airlie <airlied@xxxxxxxx>
>> Cc: Daniel Vetter <daniel@xxxxxxxx>
>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
>> Cc: "Ville Syrjälä" <ville.syrjala@xxxxxxxxxxxxxxx>
>> Cc: Hans Verkuil <hansverk@xxxxxxxxx>
>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> Cc: linux-fbdev@xxxxxxxxxxxxxxx
>> Reviewed-by: Sean Paul <sean@xxxxxxxxxx>
>> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
>> ---
>>  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
>>  	 * on HDR content
>>  	 */
>>  	struct drm_property *hdr_output_metadata_property; diff --git
>> 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@xxxxxxxxxxxxxxxxxxxxx
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux