This series adds DRM UAPI header structure documentation to kernel docs. Fixes issues with existing structure documentation in drm uapi header.
This also fixes warnings in HDR doc and addresses suggestions from Daniel Vetter.
Also dropped static function documentation in hdmi core.
Uma Shankar (4): drm: Drop a redundant unused variable drm: Fix docbook warnings in hdr metadata helper structures drm: Fixed doc warnings in drm uapi header video/hdmi: Dropped static functions from kernel doc
Documentation/gpu/drm-uapi.rst | 9 +++++++ drivers/gpu/drm/drm_connector.c | 31 ++++++++++++++++++++++ drivers/video/hdmi.c | 32 +++++++++++----------- include/drm/drm_connector.h | 3 +-- include/drm/drm_mode_config.h | 4 +-- include/linux/hdmi.h | 1 + include/uapi/drm/drm_mode.h | 59 ++++++++++++++++++++++++++++++++++++++++- 7 files changed, 118 insertions(+), 21 deletions(-)
Drop a redundant and unused variable "hdr_output_metadata" from drm_connector.
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Uma Shankar uma.shankar@intel.com --- include/drm/drm_connector.h | 2 -- 1 file changed, 2 deletions(-)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index f8f4003..5476561 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1244,8 +1244,6 @@ struct drm_connector { */ struct llist_node free_node;
- /* HDR metdata */ - struct hdr_output_metadata hdr_output_metadata; struct hdr_sink_metadata hdr_sink_metadata; };
On Thu, May 30, 2019 at 01:29:01AM +0530, Uma Shankar wrote:
Drop a redundant and unused variable "hdr_output_metadata" from drm_connector.
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Uma Shankar uma.shankar@intel.com
For next time around: Please add Fixes: lines to indicate which already merged commit you're improving.
Patch merged, thanks. -Daniel
include/drm/drm_connector.h | 2 -- 1 file changed, 2 deletions(-)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index f8f4003..5476561 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1244,8 +1244,6 @@ struct drm_connector { */ struct llist_node free_node;
- /* HDR metdata */
- struct hdr_output_metadata hdr_output_metadata; struct hdr_sink_metadata hdr_sink_metadata;
};
-- 1.9.1
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Monday, June 3, 2019 1:42 PM To: Shankar, Uma uma.shankar@intel.com Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; maarten.lankhorst@linux.intel.com; ville.syrjala@linux.intel.com; Sharma, Shashank shashank.sharma@intel.com; emil.l.velikov@gmail.com; brian.starkey@arm.com; dcastagna@chromium.org; seanpaul@chromium.org; Roper, Matthew D matthew.d.roper@intel.com; jonas@kwiboo.se; daniel@ffwll.ch Subject: Re: [PATCH 1/4] drm: Drop a redundant unused variable
On Thu, May 30, 2019 at 01:29:01AM +0530, Uma Shankar wrote:
Drop a redundant and unused variable "hdr_output_metadata" from drm_connector.
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Uma Shankar uma.shankar@intel.com
For next time around: Please add Fixes: lines to indicate which already merged commit you're improving.
Thanks Daniel. Sure, will take care of that in future.
Regards, Uma Shankar
Patch merged, thanks. -Daniel
include/drm/drm_connector.h | 2 -- 1 file changed, 2 deletions(-)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index f8f4003..5476561 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1244,8 +1244,6 @@ struct drm_connector { */ struct llist_node free_node;
- /* HDR metdata */
- struct hdr_output_metadata hdr_output_metadata; struct hdr_sink_metadata hdr_sink_metadata; };
-- 1.9.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
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. + * * 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; };
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 */ +/** + * 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. + * @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. + * + * With drm subsystem using struct drm_rect to manage rectangular area this + * 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. + */ struct hdr_output_metadata { __u32 metadata_type; union {
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.
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?
};
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;
- on HDR content
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.
+/**
- 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.
- @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.
- 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 :-)
- 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.
Thanks, Daniel
- */
struct hdr_output_metadata { __u32 metadata_type; union { -- 1.9.1
-----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
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.
v4: Fixed Daniel Vetter's review comments.
v5: Added structure member references as per Daniel's suggestion.
Cc: Shashank Sharma shashank.sharma@intel.com Cc: Ville Syrjä 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ä" 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 --- drivers/gpu/drm/drm_connector.c | 39 ++++++++++++++++++++++ include/drm/drm_connector.h | 1 + include/drm/drm_mode_config.h | 4 +-- include/linux/hdmi.h | 12 +++++++ include/uapi/drm/drm_mode.h | 74 ++++++++++++++++++++++++++++++++++++++++- 5 files changed, 127 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index c9ac8b9..d371f5c 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -956,6 +956,45 @@ 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. + * + * Driver expects metadata to be put in &hdr_output_metadata structure + * from userspace. This is received as blob and stored in + * &drm_connector_state.hdr_output_metadata. It parses EDID and saves the + * sink metadata in &hdr_sink_metadata, as &drm_connector.hdr_sink_metadata. + * Driver uses &drm_hdmi_infoframe_set_hdr_metadata helper to set the HDR + * metadata, &hdmi_drm_infoframe_pack to pack the infoframe as per spec, + * in case of HDMI encoder. + * * 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; };
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..9918a6c 100644 --- a/include/linux/hdmi.h +++ b/include/linux/hdmi.h @@ -367,8 +367,19 @@ struct hdr_static_metadata { __u16 min_cll; };
+/** + * struct hdr_sink_metadata - HDR sink metadata + * + * Metadata Information read from Sink's EDID + */ struct hdr_sink_metadata { + /** + * @metadata_type: Static_Metadata_Descriptor_ID. + */ __u32 metadata_type; + /** + * @hdmi_type1: HDR Metadata Infoframe. + */ union { struct hdr_static_metadata hdmi_type1; }; @@ -398,6 +409,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 19b5cf3..5ab331e 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,24 +639,87 @@ struct drm_color_lut { __u16 reserved; };
-/* HDR Metadata Infoframe as per 861.G spec */ +/** + * struct hdr_metadata_infoframe - HDR Metadata Infoframe Data. + * + * HDR Metadata Infoframe as per CTA 861.G spec. This is expected + * to match exactly with the spec. + * + * Userspace is expected to pass the metadata information as per + * the format described in this structure. + */ struct hdr_metadata_infoframe { + /** + * @eotf: Electro-Optical Transfer Function (EOTF) + * used in the stream. + */ __u8 eotf; + /** + * @metadata_type: Static_Metadata_Descriptor_ID. + */ __u8 metadata_type; + /** + * @display_primaries: Color Primaries of the Data. + * These are coded as unsigned 16-bit values in units of + * 0.00002, where 0x0000 represents zero and 0xC350 + * represents 1.0000. + * @display_primaries.x: X cordinate of color primary. + * @display_primaries.y: Y cordinate of color primary. + */ struct { __u16 x, y; } display_primaries[3]; + /** + * @white_point: White Point of Colorspace Data. + * These are coded as unsigned 16-bit values in units of + * 0.00002, where 0x0000 represents zero and 0xC350 + * represents 1.0000. + * @white_point.x: X cordinate of whitepoint of color primary. + * @white_point.y: Y cordinate of whitepoint of color primary. + */ struct { __u16 x, y; } white_point; + /** + * @max_display_mastering_luminance: Max Mastering Display Luminance. + * This value is coded as an unsigned 16-bit value in units of 1 cd/m2, + * where 0x0001 represents 1 cd/m2 and 0xFFFF represents 65535 cd/m2. + */ __u16 max_display_mastering_luminance; + /** + * @min_display_mastering_luminance: Min Mastering Display Luminance. + * This value is coded as an unsigned 16-bit value in units of + * 0.0001 cd/m2, where 0x0001 represents 0.0001 cd/m2 and 0xFFFF + * represents 6.5535 cd/m2. + */ __u16 min_display_mastering_luminance; + /** + * @max_cll: Max Content Light Level. + * This value is coded as an unsigned 16-bit value in units of 1 cd/m2, + * where 0x0001 represents 1 cd/m2 and 0xFFFF represents 65535 cd/m2. + */ __u16 max_cll; + /** + * @max_fall: Max Frame Average Light Level. + * This value is coded as an unsigned 16-bit value in units of 1 cd/m2, + * where 0x0001 represents 1 cd/m2 and 0xFFFF represents 65535 cd/m2. + */ __u16 max_fall; };
+/** + * struct hdr_output_metadata - HDR output metadata + * + * Metadata Information to be passed from userspace + */ struct hdr_output_metadata { + /** + * @metadata_type: Static_Metadata_Descriptor_ID. + */ __u32 metadata_type; + /** + * @hdmi_metadata_type1: HDR Metadata Infoframe. + */ union { struct hdr_metadata_infoframe hdmi_metadata_type1; };
Fixed doc warnings in drm uapi header. All the UAPI structures are now documented in kernel doc.
Signed-off-by: Uma Shankar uma.shankar@intel.com --- include/uapi/drm/drm_mode.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 5d3964f..02b2a2b 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -861,6 +861,10 @@ struct drm_format_modifier { };
/** + * struct drm_mode_create_blob - Create New block property + * @data: Pointer to data to copy. + * @length: Length of data to copy. + * @blob_id: new property ID. * Create a new 'blob' data property, copying length bytes from data pointer, * and returning new blob ID. */ @@ -874,6 +878,8 @@ struct drm_mode_create_blob { };
/** + * struct drm_mode_destroy_blob - Destroy user blob + * @blob_id: blob_id to destroy * Destroy a user-created blob property. */ struct drm_mode_destroy_blob { @@ -881,6 +887,12 @@ struct drm_mode_destroy_blob { };
/** + * struct drm_mode_create_lease - Create lease + * @object_ids: Pointer to array of object ids. + * @object_count: Number of object ids. + * @flags: flags for new FD. + * @lessee_id: unique identifier for lessee. + * @fd: file descriptor to new drm_master file. * Lease mode resources, creating another drm_master. */ struct drm_mode_create_lease { @@ -898,6 +910,10 @@ struct drm_mode_create_lease { };
/** + * struct drm_mode_list_lessees - List lessees + * @count_lessees: Number of lessees. + * @pad: pad. + * @lessees_ptr: Pointer to lessess. * List lesses from a drm_master */ struct drm_mode_list_lessees { @@ -918,6 +934,10 @@ struct drm_mode_list_lessees { };
/** + * struct drm_mode_get_lease - Get Lease + * @count_objects: Number of leased objects. + * @pad: pad. + * @objects_ptr: Pointer to objects. * Get leased objects */ struct drm_mode_get_lease { @@ -938,6 +958,8 @@ struct drm_mode_get_lease { };
/** + * struct drm_mode_revoke_lease - Revoke lease + * @lessee_id: Unique ID of lessee. * Revoke lease */ struct drm_mode_revoke_lease {
On Thu, May 30, 2019 at 01:29:03AM +0530, Uma Shankar wrote:
Fixed doc warnings in drm uapi header. All the UAPI structures are now documented in kernel doc.
Signed-off-by: Uma Shankar uma.shankar@intel.com
Applied, thanks for the patch.
Long-term there's obviously a lot more to do here, but this at least gets us started.
Btw I think it'd be good to split out the "add new uapi ioctl structures section" part from the previous patch, and merge that separately.
Thanks, Daniel
include/uapi/drm/drm_mode.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 5d3964f..02b2a2b 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -861,6 +861,10 @@ struct drm_format_modifier { };
/**
- struct drm_mode_create_blob - Create New block property
- @data: Pointer to data to copy.
- @length: Length of data to copy.
*/
- @blob_id: new property ID.
- Create a new 'blob' data property, copying length bytes from data pointer,
- and returning new blob ID.
@@ -874,6 +878,8 @@ struct drm_mode_create_blob { };
/**
- struct drm_mode_destroy_blob - Destroy user blob
*/
- @blob_id: blob_id to destroy
- Destroy a user-created blob property.
struct drm_mode_destroy_blob { @@ -881,6 +887,12 @@ struct drm_mode_destroy_blob { };
/**
- struct drm_mode_create_lease - Create lease
- @object_ids: Pointer to array of object ids.
- @object_count: Number of object ids.
- @flags: flags for new FD.
- @lessee_id: unique identifier for lessee.
*/
- @fd: file descriptor to new drm_master file.
- Lease mode resources, creating another drm_master.
struct drm_mode_create_lease { @@ -898,6 +910,10 @@ struct drm_mode_create_lease { };
/**
- struct drm_mode_list_lessees - List lessees
- @count_lessees: Number of lessees.
- @pad: pad.
*/
- @lessees_ptr: Pointer to lessess.
- List lesses from a drm_master
struct drm_mode_list_lessees { @@ -918,6 +934,10 @@ struct drm_mode_list_lessees { };
/**
- struct drm_mode_get_lease - Get Lease
- @count_objects: Number of leased objects.
- @pad: pad.
*/
- @objects_ptr: Pointer to objects.
- Get leased objects
struct drm_mode_get_lease { @@ -938,6 +958,8 @@ struct drm_mode_get_lease { };
/**
- struct drm_mode_revoke_lease - Revoke lease
*/
- @lessee_id: Unique ID of lessee.
- Revoke lease
struct drm_mode_revoke_lease {
1.9.1
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Monday, June 3, 2019 1:56 PM To: Shankar, Uma uma.shankar@intel.com Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; maarten.lankhorst@linux.intel.com; ville.syrjala@linux.intel.com; Sharma, Shashank shashank.sharma@intel.com; emil.l.velikov@gmail.com; brian.starkey@arm.com; dcastagna@chromium.org; seanpaul@chromium.org; Roper, Matthew D matthew.d.roper@intel.com; jonas@kwiboo.se; daniel@ffwll.ch Subject: Re: [PATCH 3/4] drm: Fixed doc warnings in drm uapi header
On Thu, May 30, 2019 at 01:29:03AM +0530, Uma Shankar wrote:
Fixed doc warnings in drm uapi header. All the UAPI structures are now documented in kernel doc.
Signed-off-by: Uma Shankar uma.shankar@intel.com
Applied, thanks for the patch.
Long-term there's obviously a lot more to do here, but this at least gets us started.
Btw I think it'd be good to split out the "add new uapi ioctl structures section" part from the previous patch, and merge that separately.
Ok, will do the same.
Regards, Uma Shankar
Thanks, Daniel
include/uapi/drm/drm_mode.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 5d3964f..02b2a2b 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -861,6 +861,10 @@ struct drm_format_modifier { };
/**
- struct drm_mode_create_blob - Create New block property
- @data: Pointer to data to copy.
- @length: Length of data to copy.
*/
- @blob_id: new property ID.
- Create a new 'blob' data property, copying length bytes from data pointer,
- and returning new blob ID.
@@ -874,6 +878,8 @@ struct drm_mode_create_blob { };
/**
- struct drm_mode_destroy_blob - Destroy user blob
*/
- @blob_id: blob_id to destroy
- Destroy a user-created blob property.
struct drm_mode_destroy_blob { @@ -881,6 +887,12 @@ struct drm_mode_destroy_blob { };
/**
- struct drm_mode_create_lease - Create lease
- @object_ids: Pointer to array of object ids.
- @object_count: Number of object ids.
- @flags: flags for new FD.
- @lessee_id: unique identifier for lessee.
*/
- @fd: file descriptor to new drm_master file.
- Lease mode resources, creating another drm_master.
struct drm_mode_create_lease { @@ -898,6 +910,10 @@ struct drm_mode_create_lease { };
/**
- struct drm_mode_list_lessees - List lessees
- @count_lessees: Number of lessees.
- @pad: pad.
*/
- @lessees_ptr: Pointer to lessess.
- List lesses from a drm_master
struct drm_mode_list_lessees { @@ -918,6 +934,10 @@ struct drm_mode_list_lessees { };
/**
- struct drm_mode_get_lease - Get Lease
- @count_objects: Number of leased objects.
- @pad: pad.
*/
- @objects_ptr: Pointer to objects.
- Get leased objects
struct drm_mode_get_lease { @@ -938,6 +958,8 @@ struct drm_mode_get_lease { };
/**
- struct drm_mode_revoke_lease - Revoke lease
*/
- @lessee_id: Unique ID of lessee.
- Revoke lease
struct drm_mode_revoke_lease {
1.9.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Dropped static functions from kernel documentation.
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Uma Shankar uma.shankar@intel.com --- drivers/video/hdmi.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index b99ba01..72c654b 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -1191,11 +1191,11 @@ static const char *hdmi_nups_get_name(enum hdmi_nups nups) return "Invalid"; }
-/** +/* * hdmi_avi_infoframe_log() - log info of HDMI AVI infoframe - * @level: logging level - * @dev: device - * @frame: HDMI AVI infoframe + * level: logging level + * dev: device + * frame: HDMI AVI infoframe */ static void hdmi_avi_infoframe_log(const char *level, struct device *dev, @@ -1268,11 +1268,11 @@ static const char *hdmi_spd_sdi_get_name(enum hdmi_spd_sdi sdi) return "Reserved"; }
-/** +/* * hdmi_spd_infoframe_log() - log info of HDMI SPD infoframe - * @level: logging level - * @dev: device - * @frame: HDMI SPD infoframe + * level: logging level + * dev: device + * frame: HDMI SPD infoframe */ static void hdmi_spd_infoframe_log(const char *level, struct device *dev, @@ -1404,11 +1404,11 @@ static void hdmi_spd_infoframe_log(const char *level, return "Reserved"; }
-/** +/* * hdmi_audio_infoframe_log() - log info of HDMI AUDIO infoframe - * @level: logging level - * @dev: device - * @frame: HDMI AUDIO infoframe + * level: logging level + * dev: device + * frame: HDMI AUDIO infoframe */ static void hdmi_audio_infoframe_log(const char *level, struct device *dev, @@ -1437,11 +1437,11 @@ static void hdmi_audio_infoframe_log(const char *level, frame->downmix_inhibit ? "Yes" : "No"); }
-/** +/* * hdmi_drm_infoframe_log() - log info of HDMI DRM infoframe - * @level: logging level - * @dev: device - * @frame: HDMI DRM infoframe + * level: logging level + * dev: device + * frame: HDMI DRM infoframe */ static void hdmi_drm_infoframe_log(const char *level, struct device *dev,
On Thu, May 30, 2019 at 01:29:04AM +0530, Uma Shankar wrote:
Dropped static functions from kernel documentation.
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Uma Shankar uma.shankar@intel.com
drivers/video/hdmi.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index b99ba01..72c654b 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -1191,11 +1191,11 @@ static const char *hdmi_nups_get_name(enum hdmi_nups nups) return "Invalid"; }
-/** +/*
- hdmi_avi_infoframe_log() - log info of HDMI AVI infoframe
- @level: logging level
- @dev: device
- @frame: HDMI AVI infoframe
- level: logging level
- dev: device
*/
- frame: HDMI AVI infoframe
static void hdmi_avi_infoframe_log(const char *level, struct device *dev, @@ -1268,11 +1268,11 @@ static const char *hdmi_spd_sdi_get_name(enum hdmi_spd_sdi sdi) return "Reserved"; }
-/** +/*
- hdmi_spd_infoframe_log() - log info of HDMI SPD infoframe
- @level: logging level
- @dev: device
- @frame: HDMI SPD infoframe
- level: logging level
- dev: device
*/
- frame: HDMI SPD infoframe
For internal functions I think there's not really any value in documenting this. The variable names are obvious enough. Imo better to ditch this outright. -Daniel
static void hdmi_spd_infoframe_log(const char *level, struct device *dev, @@ -1404,11 +1404,11 @@ static void hdmi_spd_infoframe_log(const char *level, return "Reserved"; }
-/** +/*
- hdmi_audio_infoframe_log() - log info of HDMI AUDIO infoframe
- @level: logging level
- @dev: device
- @frame: HDMI AUDIO infoframe
- level: logging level
- dev: device
*/
- frame: HDMI AUDIO infoframe
static void hdmi_audio_infoframe_log(const char *level, struct device *dev, @@ -1437,11 +1437,11 @@ static void hdmi_audio_infoframe_log(const char *level, frame->downmix_inhibit ? "Yes" : "No"); }
-/** +/*
- hdmi_drm_infoframe_log() - log info of HDMI DRM infoframe
- @level: logging level
- @dev: device
- @frame: HDMI DRM infoframe
- level: logging level
- dev: device
*/
- frame: HDMI DRM infoframe
static void hdmi_drm_infoframe_log(const char *level, struct device *dev, -- 1.9.1
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Monday, June 3, 2019 1:55 PM To: Shankar, Uma uma.shankar@intel.com Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; maarten.lankhorst@linux.intel.com; ville.syrjala@linux.intel.com; Sharma, Shashank shashank.sharma@intel.com; emil.l.velikov@gmail.com; brian.starkey@arm.com; dcastagna@chromium.org; seanpaul@chromium.org; Roper, Matthew D matthew.d.roper@intel.com; jonas@kwiboo.se; daniel@ffwll.ch Subject: Re: [PATCH 4/4] video/hdmi: Dropped static functions from kernel doc
On Thu, May 30, 2019 at 01:29:04AM +0530, Uma Shankar wrote:
Dropped static functions from kernel documentation.
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Uma Shankar uma.shankar@intel.com
drivers/video/hdmi.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index b99ba01..72c654b 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -1191,11 +1191,11 @@ static const char *hdmi_nups_get_name(enum
hdmi_nups nups)
return "Invalid"; }
-/** +/*
- hdmi_avi_infoframe_log() - log info of HDMI AVI infoframe
- @level: logging level
- @dev: device
- @frame: HDMI AVI infoframe
- level: logging level
- dev: device
*/
- frame: HDMI AVI infoframe
static void hdmi_avi_infoframe_log(const char *level, struct device *dev, @@ -1268,11 +1268,11 @@ static const char *hdmi_spd_sdi_get_name(enum
hdmi_spd_sdi sdi)
return "Reserved"; }
-/** +/*
- hdmi_spd_infoframe_log() - log info of HDMI SPD infoframe
- @level: logging level
- @dev: device
- @frame: HDMI SPD infoframe
- level: logging level
- dev: device
*/
- frame: HDMI SPD infoframe
For internal functions I think there's not really any value in documenting this. The variable names are obvious enough. Imo better to ditch this outright.
Ok, will drop the comments from all these static functions.
Regards, Uma Shankar
-Daniel
static void hdmi_spd_infoframe_log(const char *level, struct device *dev, @@ -1404,11 +1404,11 @@ static void hdmi_spd_infoframe_log(const char *level, return "Reserved"; }
-/** +/*
- hdmi_audio_infoframe_log() - log info of HDMI AUDIO infoframe
- @level: logging level
- @dev: device
- @frame: HDMI AUDIO infoframe
- level: logging level
- dev: device
*/
- frame: HDMI AUDIO infoframe
static void hdmi_audio_infoframe_log(const char *level, struct device *dev, @@ -1437,11 +1437,11 @@ static void hdmi_audio_infoframe_log(const char
*level,
frame->downmix_inhibit ? "Yes" : "No"); }-/** +/*
- hdmi_drm_infoframe_log() - log info of HDMI DRM infoframe
- @level: logging level
- @dev: device
- @frame: HDMI DRM infoframe
- level: logging level
- dev: device
*/
- frame: HDMI DRM infoframe
static void hdmi_drm_infoframe_log(const char *level, struct device *dev, -- 1.9.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Shankar, Uma -
Uma Shankar