Vegard Nossum
2023-Dec-25 06:52 UTC
[PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings
As of commit b77fdd6a48e6 ("scripts/kernel-doc: restore warning for Excess struct/union"), we see the following warnings when running 'make htmldocs': ./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member 'DRM_NOUVEAU_VM_BIND_OP_MAP' description in 'drm_nouveau_vm_bind_op' ./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member 'DRM_NOUVEAU_VM_BIND_OP_UNMAP' description in 'drm_nouveau_vm_bind_op' ./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member 'DRM_NOUVEAU_VM_BIND_SPARSE' description in 'drm_nouveau_vm_bind_op' ./include/uapi/drm/nouveau_drm.h:336: warning: Excess struct member 'DRM_NOUVEAU_VM_BIND_RUN_ASYNC' description in 'drm_nouveau_vm_bind' The problem is that these values are #define constants, but had kerneldoc comments attached to them as if they were actual struct members. There are a number of ways we could fix this, but I chose to draw inspiration from include/uapi/drm/i915_drm.h, which pulls them into the corresponding kerneldoc comment for the struct member that they are intended to be used with. To keep the diff readable, there are a number of things I _didn't_ do in this patch, but which we should also consider: - This is pretty good documentation, but it ends up in gpu/driver-uapi, which is part of subsystem-apis/ when it really ought to display under userspace-api/ (the "Linux kernel user-space API guide" book of the documentation). - More generally, we might want a warning if include/uapi/ files are kerneldoc'd outside userspace-api/. - I'd consider it cleaner if the #defines appeared between the kerneldoc for the member and the member itself (which is something other DRM- related UAPI docs do). - The %IDENTIFIER kerneldoc syntax is intended for "constants", and is more appropriate in this context than ``IDENTIFIER`` or &IDENTIFIER. The DRM docs aren't very consistent on this. Cc: Randy Dunlap <rdunlap at infradead.org> Cc: Jonathan Corbet <corbet at lwn.net> Signed-off-by: Vegard Nossum <vegard.nossum at oracle.com> --- include/uapi/drm/nouveau_drm.h | 56 ++++++++++++++++------------------ 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/include/uapi/drm/nouveau_drm.h b/include/uapi/drm/nouveau_drm.h index 0bade1592f34..c95ef8a4d94a 100644 --- a/include/uapi/drm/nouveau_drm.h +++ b/include/uapi/drm/nouveau_drm.h @@ -238,34 +238,32 @@ struct drm_nouveau_vm_init { struct drm_nouveau_vm_bind_op { /** * @op: the operation type + * + * Supported values: + * + * %DRM_NOUVEAU_VM_BIND_OP_MAP - Map a GEM object to the GPU's VA + * space. Optionally, the &DRM_NOUVEAU_VM_BIND_SPARSE flag can be + * passed to instruct the kernel to create sparse mappings for the + * given range. + * + * %DRM_NOUVEAU_VM_BIND_OP_UNMAP - Unmap an existing mapping in the + * GPU's VA space. If the region the mapping is located in is a + * sparse region, new sparse mappings are created where the unmapped + * (memory backed) mapping was mapped previously. To remove a sparse + * region the &DRM_NOUVEAU_VM_BIND_SPARSE must be set. */ __u32 op; -/** - * @DRM_NOUVEAU_VM_BIND_OP_MAP: - * - * Map a GEM object to the GPU's VA space. Optionally, the - * &DRM_NOUVEAU_VM_BIND_SPARSE flag can be passed to instruct the kernel to - * create sparse mappings for the given range. - */ #define DRM_NOUVEAU_VM_BIND_OP_MAP 0x0 -/** - * @DRM_NOUVEAU_VM_BIND_OP_UNMAP: - * - * Unmap an existing mapping in the GPU's VA space. If the region the mapping - * is located in is a sparse region, new sparse mappings are created where the - * unmapped (memory backed) mapping was mapped previously. To remove a sparse - * region the &DRM_NOUVEAU_VM_BIND_SPARSE must be set. - */ #define DRM_NOUVEAU_VM_BIND_OP_UNMAP 0x1 /** * @flags: the flags for a &drm_nouveau_vm_bind_op + * + * Supported values: + * + * %DRM_NOUVEAU_VM_BIND_SPARSE - Indicates that an allocated VA + * space region should be sparse. */ __u32 flags; -/** - * @DRM_NOUVEAU_VM_BIND_SPARSE: - * - * Indicates that an allocated VA space region should be sparse. - */ #define DRM_NOUVEAU_VM_BIND_SPARSE (1 << 8) /** * @handle: the handle of the DRM GEM object to map @@ -301,17 +299,17 @@ struct drm_nouveau_vm_bind { __u32 op_count; /** * @flags: the flags for a &drm_nouveau_vm_bind ioctl + * + * Supported values: + * + * %DRM_NOUVEAU_VM_BIND_RUN_ASYNC - Indicates that the given VM_BIND + * operation should be executed asynchronously by the kernel. + * + * If this flag is not supplied the kernel executes the associated + * operations synchronously and doesn't accept any &drm_nouveau_sync + * objects. */ __u32 flags; -/** - * @DRM_NOUVEAU_VM_BIND_RUN_ASYNC: - * - * Indicates that the given VM_BIND operation should be executed asynchronously - * by the kernel. - * - * If this flag is not supplied the kernel executes the associated operations - * synchronously and doesn't accept any &drm_nouveau_sync objects. - */ #define DRM_NOUVEAU_VM_BIND_RUN_ASYNC 0x1 /** * @wait_count: the number of wait &drm_nouveau_syncs -- 2.34.1
Randy Dunlap
2023-Dec-25 07:40 UTC
[PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings
On 12/24/23 22:51, Vegard Nossum wrote:> As of commit b77fdd6a48e6 ("scripts/kernel-doc: restore warning for > Excess struct/union"), we see the following warnings when running 'make > htmldocs': > > ./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member 'DRM_NOUVEAU_VM_BIND_OP_MAP' description in 'drm_nouveau_vm_bind_op' > ./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member 'DRM_NOUVEAU_VM_BIND_OP_UNMAP' description in 'drm_nouveau_vm_bind_op' > ./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member 'DRM_NOUVEAU_VM_BIND_SPARSE' description in 'drm_nouveau_vm_bind_op' > ./include/uapi/drm/nouveau_drm.h:336: warning: Excess struct member 'DRM_NOUVEAU_VM_BIND_RUN_ASYNC' description in 'drm_nouveau_vm_bind' > > The problem is that these values are #define constants, but had kerneldoc > comments attached to them as if they were actual struct members. > > There are a number of ways we could fix this, but I chose to draw > inspiration from include/uapi/drm/i915_drm.h, which pulls them into the > corresponding kerneldoc comment for the struct member that they are > intended to be used with. > > To keep the diff readable, there are a number of things I _didn't_ do in > this patch, but which we should also consider: > > - This is pretty good documentation, but it ends up in gpu/driver-uapi, > which is part of subsystem-apis/ when it really ought to display under > userspace-api/ (the "Linux kernel user-space API guide" book of the > documentation). > > - More generally, we might want a warning if include/uapi/ files are > kerneldoc'd outside userspace-api/. > > - I'd consider it cleaner if the #defines appeared between the kerneldoc > for the member and the member itself (which is something other DRM- > related UAPI docs do). > > - The %IDENTIFIER kerneldoc syntax is intended for "constants", and is > more appropriate in this context than ``IDENTIFIER`` or &IDENTIFIER. > The DRM docs aren't very consistent on this. > > Cc: Randy Dunlap <rdunlap at infradead.org> > Cc: Jonathan Corbet <corbet at lwn.net> > Signed-off-by: Vegard Nossum <vegard.nossum at oracle.com>This all looks good to me. Thanks for your help. Reviewed-by: Randy Dunlap <rdunlap at infradead.org> Tested-by: Randy Dunlap <rdunlap at infradead.org> I do see one thing that I don't like in the generated html output. It's not a problem with this patch. The #defines for DRM_NOUVEAU_VM_BIND_OP_MAP etc. have a ';' at the end of each line: struct drm_nouveau_vm_bind_op { __u32 op; #define DRM_NOUVEAU_VM_BIND_OP_MAP 0x0; #define DRM_NOUVEAU_VM_BIND_OP_UNMAP 0x1; __u32 flags; #define DRM_NOUVEAU_VM_BIND_SPARSE (1 << 8); __u32 handle; __u32 pad; __u64 addr; __u64 bo_offset; __u64 range; }; so something else to look into one of these days....> --- > include/uapi/drm/nouveau_drm.h | 56 ++++++++++++++++------------------ > 1 file changed, 27 insertions(+), 29 deletions(-) > > diff --git a/include/uapi/drm/nouveau_drm.h b/include/uapi/drm/nouveau_drm.h > index 0bade1592f34..c95ef8a4d94a 100644-- #Randy https://people.kernel.org/tglx/notes-about-netiquette https://subspace.kernel.org/etiquette.html
Danilo Krummrich
2024-Jan-08 19:54 UTC
[PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings
On 12/25/23 07:51, Vegard Nossum wrote:> As of commit b77fdd6a48e6 ("scripts/kernel-doc: restore warning for > Excess struct/union"), we see the following warnings when running 'make > htmldocs': > > ./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member 'DRM_NOUVEAU_VM_BIND_OP_MAP' description in 'drm_nouveau_vm_bind_op' > ./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member 'DRM_NOUVEAU_VM_BIND_OP_UNMAP' description in 'drm_nouveau_vm_bind_op' > ./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member 'DRM_NOUVEAU_VM_BIND_SPARSE' description in 'drm_nouveau_vm_bind_op' > ./include/uapi/drm/nouveau_drm.h:336: warning: Excess struct member 'DRM_NOUVEAU_VM_BIND_RUN_ASYNC' description in 'drm_nouveau_vm_bind' > > The problem is that these values are #define constants, but had kerneldoc > comments attached to them as if they were actual struct members. > > There are a number of ways we could fix this, but I chose to draw > inspiration from include/uapi/drm/i915_drm.h, which pulls them into the > corresponding kerneldoc comment for the struct member that they are > intended to be used with. > > To keep the diff readable, there are a number of things I _didn't_ do in > this patch, but which we should also consider: > > - This is pretty good documentation, but it ends up in gpu/driver-uapi, > which is part of subsystem-apis/ when it really ought to display under > userspace-api/ (the "Linux kernel user-space API guide" book of the > documentation).I agree, it indeed looks like this would make sense, same goes for gpu/drm-uapi.rst. @Jani, Sima: Was this intentional? Or can we change it?> > - More generally, we might want a warning if include/uapi/ files are > kerneldoc'd outside userspace-api/. > > - I'd consider it cleaner if the #defines appeared between the kerneldoc > for the member and the member itself (which is something other DRM- > related UAPI docs do). > > - The %IDENTIFIER kerneldoc syntax is intended for "constants", and is > more appropriate in this context than ``IDENTIFIER`` or &IDENTIFIER. > The DRM docs aren't very consistent on this. > > Cc: Randy Dunlap <rdunlap at infradead.org> > Cc: Jonathan Corbet <corbet at lwn.net> > Signed-off-by: Vegard Nossum <vegard.nossum at oracle.com>Applied to drm-misc-next, thanks!> --- > include/uapi/drm/nouveau_drm.h | 56 ++++++++++++++++------------------ > 1 file changed, 27 insertions(+), 29 deletions(-) > > diff --git a/include/uapi/drm/nouveau_drm.h b/include/uapi/drm/nouveau_drm.h > index 0bade1592f34..c95ef8a4d94a 100644 > --- a/include/uapi/drm/nouveau_drm.h > +++ b/include/uapi/drm/nouveau_drm.h > @@ -238,34 +238,32 @@ struct drm_nouveau_vm_init { > struct drm_nouveau_vm_bind_op { > /** > * @op: the operation type > + * > + * Supported values: > + * > + * %DRM_NOUVEAU_VM_BIND_OP_MAP - Map a GEM object to the GPU's VA > + * space. Optionally, the &DRM_NOUVEAU_VM_BIND_SPARSE flag can be > + * passed to instruct the kernel to create sparse mappings for the > + * given range. > + * > + * %DRM_NOUVEAU_VM_BIND_OP_UNMAP - Unmap an existing mapping in the > + * GPU's VA space. If the region the mapping is located in is a > + * sparse region, new sparse mappings are created where the unmapped > + * (memory backed) mapping was mapped previously. To remove a sparse > + * region the &DRM_NOUVEAU_VM_BIND_SPARSE must be set. > */ > __u32 op; > -/** > - * @DRM_NOUVEAU_VM_BIND_OP_MAP: > - * > - * Map a GEM object to the GPU's VA space. Optionally, the > - * &DRM_NOUVEAU_VM_BIND_SPARSE flag can be passed to instruct the kernel to > - * create sparse mappings for the given range. > - */ > #define DRM_NOUVEAU_VM_BIND_OP_MAP 0x0 > -/** > - * @DRM_NOUVEAU_VM_BIND_OP_UNMAP: > - * > - * Unmap an existing mapping in the GPU's VA space. If the region the mapping > - * is located in is a sparse region, new sparse mappings are created where the > - * unmapped (memory backed) mapping was mapped previously. To remove a sparse > - * region the &DRM_NOUVEAU_VM_BIND_SPARSE must be set. > - */ > #define DRM_NOUVEAU_VM_BIND_OP_UNMAP 0x1 > /** > * @flags: the flags for a &drm_nouveau_vm_bind_op > + * > + * Supported values: > + * > + * %DRM_NOUVEAU_VM_BIND_SPARSE - Indicates that an allocated VA > + * space region should be sparse. > */ > __u32 flags; > -/** > - * @DRM_NOUVEAU_VM_BIND_SPARSE: > - * > - * Indicates that an allocated VA space region should be sparse. > - */ > #define DRM_NOUVEAU_VM_BIND_SPARSE (1 << 8) > /** > * @handle: the handle of the DRM GEM object to map > @@ -301,17 +299,17 @@ struct drm_nouveau_vm_bind { > __u32 op_count; > /** > * @flags: the flags for a &drm_nouveau_vm_bind ioctl > + * > + * Supported values: > + * > + * %DRM_NOUVEAU_VM_BIND_RUN_ASYNC - Indicates that the given VM_BIND > + * operation should be executed asynchronously by the kernel. > + * > + * If this flag is not supplied the kernel executes the associated > + * operations synchronously and doesn't accept any &drm_nouveau_sync > + * objects. > */ > __u32 flags; > -/** > - * @DRM_NOUVEAU_VM_BIND_RUN_ASYNC: > - * > - * Indicates that the given VM_BIND operation should be executed asynchronously > - * by the kernel. > - * > - * If this flag is not supplied the kernel executes the associated operations > - * synchronously and doesn't accept any &drm_nouveau_sync objects. > - */ > #define DRM_NOUVEAU_VM_BIND_RUN_ASYNC 0x1 > /** > * @wait_count: the number of wait &drm_nouveau_syncs
Possibly Parallel Threads
- [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings
- [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings
- [PATCH drm-next 05/14] drm/nouveau: new VM_BIND uapi interfaces
- [PATCH drm-next 05/14] drm/nouveau: new VM_BIND uapi interfaces
- [PATCH drm-misc-next v8 03/12] drm/nouveau: new VM_BIND uapi interfaces