| Message ID | zvaqgUJLuDcSaX7hbo6wcjWOCFUkiwPThMV1D8tq7dc@cp3-web-020.plabs.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: document drm_mode_get_connector | expand |
On Wed, Nov 18, 2020 at 11:52 AM Simon Ser <contact@emersion.fr> wrote: > > Document how to perform a forced probe, and when should user-space do it. > > Signed-off-by: Simon Ser <contact@emersion.fr> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Pekka Paalanen <ppaalanen@gmail.com> > --- > include/uapi/drm/drm_mode.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 5ad10ab2a577..09647b799f39 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -368,6 +368,19 @@ enum drm_mode_subconnector { > #define DRM_MODE_CONNECTOR_WRITEBACK 18 > #define DRM_MODE_CONNECTOR_SPI 19 > > +/** > + * struct drm_mode_get_connector - get connector metadata > + * > + * If the @count_modes field is set to zero, the kernel will perform a forced > + * probe on the connector to refresh the connector status, modes and EDID. > + * A forced-probe can be slow and the ioctl will block. > + * > + * User-space shouldn't need to force-probe connectors in general: the kernel > + * will automatically take care of probing connectors that don't support > + * hot-plug detection when appropriate. However, user-space may force-probe > + * connectors on user request (e.g. clicking a "Scan connectors" button, or > + * opening a UI to manage screens). > + */ I think this causes warnings, because now we have kerneldoc for this, but not for all the members. Also the member-specific stuff should be documented as inline comment, see https://dri.freedesktop.org/docs/drm/doc-guide/kernel-doc.html#in-line-member-documentation-comments I also noticed that this file has a ton of wrong kerneldoc comments, but they seem to not cause warnings (anything starting with /** is fishy). Can I volunteer you for a bit more here? Thanks, Daniel > struct drm_mode_get_connector { > > __u64 encoders_ptr; > -- > 2.29.2 > >
On Wednesday, November 18, 2020 4:03 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > I think this causes warnings, because now we have kerneldoc for this, > but not for all the members. Also the member-specific stuff should be > documented as inline comment, see > > https://dri.freedesktop.org/docs/drm/doc-guide/kernel-doc.html#in-line-member-documentation-comments Hm, right, will make sure the patch doesn't trigger warnings. I still think the force-probe stuff shouldn't be documented in in-line comments, because I'd never look at the in-line count_modes comment to know whether the ioctl probes or not. Adding short in-line comments sounds fine though. > I also noticed that this file has a ton of wrong kerneldoc comments, > but they seem to not cause warnings (anything starting with /** is > fishy). > > Can I volunteer you for a bit more here? Yeah, I've noticed this as well. Will have a look!
On Wed, Nov 18, 2020 at 4:09 PM Simon Ser <contact@emersion.fr> wrote: > > On Wednesday, November 18, 2020 4:03 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > I think this causes warnings, because now we have kerneldoc for this, > > but not for all the members. Also the member-specific stuff should be > > documented as inline comment, see > > > > https://dri.freedesktop.org/docs/drm/doc-guide/kernel-doc.html#in-line-member-documentation-comments > > Hm, right, will make sure the patch doesn't trigger warnings. > > I still think the force-probe stuff shouldn't be documented in in-line > comments, because I'd never look at the in-line count_modes comment to know > whether the ioctl probes or not. Adding short in-line comments sounds fine > though. Hm yeah, maybe give it a sub-title then like "Probe Modes" with the two options. I think the detail that a small array/count (either 0 or 1 depending whether you want force probing) should be documented in the inline comment though. I think the inline comment for connection should also link to the drm_connector_status enum. Maybe also have that link in the "Probe Modes" section with an explanation that a forced probe can get rid of some the unknown ones. I also kinda wonder where we should up recommendations for autoconfiguration. > > > I also noticed that this file has a ton of wrong kerneldoc comments, > > but they seem to not cause warnings (anything starting with /** is > > fishy). > > > > Can I volunteer you for a bit more here? > > Yeah, I've noticed this as well. Will have a look! Thanks! -Daniel
On Wed, 18 Nov 2020 10:52:12 +0000 Simon Ser <contact@emersion.fr> wrote: > Document how to perform a forced probe, and when should user-space do it. > > Signed-off-by: Simon Ser <contact@emersion.fr> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Pekka Paalanen <ppaalanen@gmail.com> > --- > include/uapi/drm/drm_mode.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 5ad10ab2a577..09647b799f39 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -368,6 +368,19 @@ enum drm_mode_subconnector { > #define DRM_MODE_CONNECTOR_WRITEBACK 18 > #define DRM_MODE_CONNECTOR_SPI 19 > > +/** > + * struct drm_mode_get_connector - get connector metadata > + * > + * If the @count_modes field is set to zero, the kernel will perform a forced > + * probe on the connector to refresh the connector status, modes and EDID. > + * A forced-probe can be slow and the ioctl will block. > + * Hi, as I have no prior knowledge at all about how struct drm_mode_get_connector works, the above paragraph only confuses me. count_modes sounds totally unrelated to probing, and what does that even do in this struct at all, how does one use this thing. I only know the libdrm API for this. So I can't ack this bit. > + * User-space shouldn't need to force-probe connectors in general: the kernel > + * will automatically take care of probing connectors that don't support > + * hot-plug detection when appropriate. However, user-space may force-probe > + * connectors on user request (e.g. clicking a "Scan connectors" button, or > + * opening a UI to manage screens). This paragraph is good and clear to me. > + */ > struct drm_mode_get_connector { > > __u64 encoders_ptr; Thanks, pq
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 5ad10ab2a577..09647b799f39 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -368,6 +368,19 @@ enum drm_mode_subconnector { #define DRM_MODE_CONNECTOR_WRITEBACK 18 #define DRM_MODE_CONNECTOR_SPI 19 +/** + * struct drm_mode_get_connector - get connector metadata + * + * If the @count_modes field is set to zero, the kernel will perform a forced + * probe on the connector to refresh the connector status, modes and EDID. + * A forced-probe can be slow and the ioctl will block. + * + * User-space shouldn't need to force-probe connectors in general: the kernel + * will automatically take care of probing connectors that don't support + * hot-plug detection when appropriate. However, user-space may force-probe + * connectors on user request (e.g. clicking a "Scan connectors" button, or + * opening a UI to manage screens). + */ struct drm_mode_get_connector { __u64 encoders_ptr;
Document how to perform a forced probe, and when should user-space do it. Signed-off-by: Simon Ser <contact@emersion.fr> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Pekka Paalanen <ppaalanen@gmail.com> --- include/uapi/drm/drm_mode.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)