From: Jani Nikula <jani.nikula@linux.intel.com>
To: Danilo Krummrich <dakr@redhat.com>,
Vegard Nossum <vegard.nossum@oracle.com>,
"daniel@ffwll.ch" <daniel@ffwll.ch>
Cc: Jonathan Corbet <corbet@lwn.net>,
nouveau@lists.freedesktop.org,
Randy Dunlap <rdunlap@infradead.org>,
linux-doc@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings
Date: Tue, 09 Jan 2024 12:10:13 +0200 [thread overview]
Message-ID: <87r0iqx1fe.fsf@intel.com> (raw)
In-Reply-To: <724fdfaf-0e38-408b-a2d6-e3f39f28743d@redhat.com>
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
prev parent reply other threads:[~2024-01-09 10:10 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-25 6:51 [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings Vegard Nossum
2023-12-25 7:40 ` Randy Dunlap
2023-12-25 8:30 ` Vegard Nossum
2023-12-25 17:08 ` Randy Dunlap
2024-01-03 3:10 ` Randy Dunlap
2024-01-05 5:31 ` Randy Dunlap
2024-01-08 19:54 ` Danilo Krummrich
2024-01-09 10:10 ` Jani Nikula [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r0iqx1fe.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=corbet@lwn.net \
--cc=dakr@redhat.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-doc@vger.kernel.org \
--cc=nouveau@lists.freedesktop.org \
--cc=rdunlap@infradead.org \
--cc=vegard.nossum@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).