| Message ID | 1559141030-4386-1-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Fix docbook warnings in hdr metadata helper structures | expand |
On Wed, May 29, 2019 at 08:13:50PM +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. > > 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 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. capabilities > + * 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. > */ > > 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; > + /** @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 May as well fix the spelling of "metadata" while you're here. > * 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 Can you spell this out here so it's unambiguous which DRM you're talking about? With the nits fixed, Reviewed-by: Sean Paul <sean@poorly.run> > * > * 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 >
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 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 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. 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). > > 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. 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. -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 >
>-----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 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
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. */ 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; + /** @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
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(-)