[PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings

[PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings

[Vegard Nossum]

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@infradead.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Vegard Nossum <vegard.nossum@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

Re: [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings

[Randy Dunlap]

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@infradead.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>


This all looks good to me. Thanks for your help.

Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Randy Dunlap <rdunlap@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

Re: [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings

[Vegard Nossum]

On 25/12/2023 08:40, Randy Dunlap wrote:
> 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;
> };

Do we actually ever want preprocessor directives to appear inside
definitions in the output? If not, I think this should work:

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 3cdc7dba37e3..61425fc9645e 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1259,6 +1259,8 @@ sub dump_struct($$) {
                 $clause =~ s/\s+$//;
                 $clause =~ s/\s+/ /;
                 next if (!$clause);
+               # skip preprocessor directives
+               next if $clause =~ m/^#/;
                 $level-- if ($clause =~ m/(\})/ && $level > 1);
                 if (!($clause =~ m/^\s*#/)) {
                         $declaration .= "\t" x $level;


Vegard

Re: [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings

[Randy Dunlap]

On 12/25/23 00:30, Vegard Nossum wrote:
> 
> On 25/12/2023 08:40, Randy Dunlap wrote:
>> 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;
>> };
> 
> Do we actually ever want preprocessor directives to appear inside
> definitions in the output? If not, I think this should work:

Not necessarily.

> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> index 3cdc7dba37e3..61425fc9645e 100755
> --- a/scripts/kernel-doc
> +++ b/scripts/kernel-doc
> @@ -1259,6 +1259,8 @@ sub dump_struct($$) {
>                 $clause =~ s/\s+$//;
>                 $clause =~ s/\s+/ /;
>                 next if (!$clause);
> +               # skip preprocessor directives
> +               next if $clause =~ m/^#/;
>                 $level-- if ($clause =~ m/(\})/ && $level > 1);
>                 if (!($clause =~ m/^\s*#/)) {
>                         $declaration .= "\t" x $level;
> 
> 

but that didn't work for me.
I don't have time to look into it any more today.  :)

Thanks.

-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html

Re: [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings

[Randy Dunlap]

Hi Vegard,

On 12/25/23 09:08, Randy Dunlap wrote:
> 
> 
> On 12/25/23 00:30, Vegard Nossum wrote:
>>
>> On 25/12/2023 08:40, Randy Dunlap wrote:
>>> 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;
>>> };
>>
>> Do we actually ever want preprocessor directives to appear inside
>> definitions in the output? If not, I think this should work:
> 
> Not necessarily.
> 
>> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
>> index 3cdc7dba37e3..61425fc9645e 100755
>> --- a/scripts/kernel-doc
>> +++ b/scripts/kernel-doc
>> @@ -1259,6 +1259,8 @@ sub dump_struct($$) {
>>                 $clause =~ s/\s+$//;
>>                 $clause =~ s/\s+/ /;
>>                 next if (!$clause);
>> +               # skip preprocessor directives
>> +               next if $clause =~ m/^#/;
>>                 $level-- if ($clause =~ m/(\})/ && $level > 1);
>>                 if (!($clause =~ m/^\s*#/)) {
>>                         $declaration .= "\t" x $level;
>>
>>
> 
> but that didn't work for me.
> I don't have time to look into it any more today.  :)

I retested this patch. I must have really messed up my testing
in the first round. This now LGTM. Thanks.

Acked-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Randy Dunlap <rdunlap@infradead.org>

-- 
#Randy

Re: [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings

[Randy Dunlap]

On 1/2/24 19:10, Randy Dunlap wrote:
> Hi Vegard,
> 
> On 12/25/23 09:08, Randy Dunlap wrote:
>>
>>
>> On 12/25/23 00:30, Vegard Nossum wrote:
>>>
>>> On 25/12/2023 08:40, Randy Dunlap wrote:
>>>> 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;
>>>> };
>>>
>>> Do we actually ever want preprocessor directives to appear inside
>>> definitions in the output? If not, I think this should work:
>>
>> Not necessarily.
>>
>>> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
>>> index 3cdc7dba37e3..61425fc9645e 100755
>>> --- a/scripts/kernel-doc
>>> +++ b/scripts/kernel-doc
>>> @@ -1259,6 +1259,8 @@ sub dump_struct($$) {
>>>                 $clause =~ s/\s+$//;
>>>                 $clause =~ s/\s+/ /;
>>>                 next if (!$clause);
>>> +               # skip preprocessor directives
>>> +               next if $clause =~ m/^#/;
>>>                 $level-- if ($clause =~ m/(\})/ && $level > 1);
>>>                 if (!($clause =~ m/^\s*#/)) {
>>>                         $declaration .= "\t" x $level;
>>>
>>>
>>
>> but that didn't work for me.
>> I don't have time to look into it any more today.  :)
> 
> I retested this patch. I must have really messed up my testing
> in the first round. This now LGTM. Thanks.
> 
> Acked-by: Randy Dunlap <rdunlap@infradead.org>
> Tested-by: Randy Dunlap <rdunlap@infradead.org>

Vegard, do you plan to submit this as a kernel-doc patch?

Thanks.

-- 
#Randy

Re: [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings

[Danilo Krummrich]

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@infradead.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Signed-off-by: Vegard Nossum <vegard.nossum@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

Re: [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings

[Jani Nikula]

On Mon, 08 Jan 2024, Danilo Krummrich <dakr@redhat.com> wrote:
> 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?

I have no recollection of this, but overall I'd say do what makes sense,
and where things are easiest to find.

BR,
Jani.


>
>> 
>> - 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@infradead.org>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Signed-off-by: Vegard Nossum <vegard.nossum@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
>

-- 
Jani Nikula, Intel