* [PATCH 2/2] drm: document DRM_IOCTL_GEM_CLOSE
2023-02-16 13:09 [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE Simon Ser
@ 2023-02-16 13:09 ` Simon Ser
2023-02-16 19:48 ` Daniel Vetter
2023-02-16 19:46 ` [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE Daniel Vetter
2023-02-20 15:49 ` Dave Stevenson
2 siblings, 1 reply; 9+ messages in thread
From: Simon Ser @ 2023-02-16 13:09 UTC (permalink / raw)
To: dri-devel; +Cc: Pekka Paalanen
This is a bit tricky, because of the ref'counting considerations.
See also [1] for more discussion about this topic. Since this is
kernel docs, I've decided to elaborate a bit less on the user-space
details.
[1]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/110
Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Daniel Stone <daniel@fooishbar.org>
---
include/uapi/drm/drm.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 54b2313c8332..4829f1fa9570 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -972,6 +972,19 @@ extern "C" {
#define DRM_IOCTL_GET_STATS DRM_IOR( 0x06, struct drm_stats)
#define DRM_IOCTL_SET_VERSION DRM_IOWR(0x07, struct drm_set_version)
#define DRM_IOCTL_MODESET_CTL DRM_IOW(0x08, struct drm_modeset_ctl)
+/**
+ * DRM_IOCTL_GEM_CLOSE - Close a GEM handle.
+ *
+ * GEM handles are not reference-counted by the kernel. User-space is
+ * responsible for managing their lifetime. For example, if user-space imports
+ * the same memory object twice on the same DRM file description, the same GEM
+ * handle is returned by both imports, and user-space needs to ensure
+ * &DRM_IOCTL_GEM_CLOSE is performed once only. The same situation can happen
+ * when a memory object is allocated, then exported and imported again on the
+ * same DRM file description. The &DRM_IOCTL_MODE_GETFB2 IOCTL is an exception
+ * and always returns fresh new GEM handles even if an existing GEM handle
+ * already refers to the same memory object before the IOCTL is performed.
+ */
#define DRM_IOCTL_GEM_CLOSE DRM_IOW (0x09, struct drm_gem_close)
#define DRM_IOCTL_GEM_FLINK DRM_IOWR(0x0a, struct drm_gem_flink)
#define DRM_IOCTL_GEM_OPEN DRM_IOWR(0x0b, struct drm_gem_open)
--
2.39.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/2] drm: document DRM_IOCTL_GEM_CLOSE
2023-02-16 13:09 ` [PATCH 2/2] drm: document DRM_IOCTL_GEM_CLOSE Simon Ser
@ 2023-02-16 19:48 ` Daniel Vetter
2023-02-17 9:28 ` Pekka Paalanen
2023-02-17 9:33 ` Simon Ser
0 siblings, 2 replies; 9+ messages in thread
From: Daniel Vetter @ 2023-02-16 19:48 UTC (permalink / raw)
To: Simon Ser; +Cc: Pekka Paalanen, dri-devel
On Thu, Feb 16, 2023 at 01:09:45PM +0000, Simon Ser wrote:
> This is a bit tricky, because of the ref'counting considerations.
> See also [1] for more discussion about this topic. Since this is
> kernel docs, I've decided to elaborate a bit less on the user-space
> details.
>
> [1]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/110
>
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Daniel Stone <daniel@fooishbar.org>
> ---
> include/uapi/drm/drm.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 54b2313c8332..4829f1fa9570 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -972,6 +972,19 @@ extern "C" {
> #define DRM_IOCTL_GET_STATS DRM_IOR( 0x06, struct drm_stats)
> #define DRM_IOCTL_SET_VERSION DRM_IOWR(0x07, struct drm_set_version)
> #define DRM_IOCTL_MODESET_CTL DRM_IOW(0x08, struct drm_modeset_ctl)
> +/**
> + * DRM_IOCTL_GEM_CLOSE - Close a GEM handle.
> + *
> + * GEM handles are not reference-counted by the kernel. User-space is
> + * responsible for managing their lifetime. For example, if user-space imports
> + * the same memory object twice on the same DRM file description, the same GEM
> + * handle is returned by both imports, and user-space needs to ensure
> + * &DRM_IOCTL_GEM_CLOSE is performed once only. The same situation can happen
> + * when a memory object is allocated, then exported and imported again on the
> + * same DRM file description. The &DRM_IOCTL_MODE_GETFB2 IOCTL is an exception
> + * and always returns fresh new GEM handles even if an existing GEM handle
> + * already refers to the same memory object before the IOCTL is performed.
I'd duplicate the relevant parts into each ioctl doc too, just to increase
the chances people notice these caveats. But that's stuf for these other
patches. for this one:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
btw do the links from on ioctl text to the other work?
-Daniel
> + */
> #define DRM_IOCTL_GEM_CLOSE DRM_IOW (0x09, struct drm_gem_close)
> #define DRM_IOCTL_GEM_FLINK DRM_IOWR(0x0a, struct drm_gem_flink)
> #define DRM_IOCTL_GEM_OPEN DRM_IOWR(0x0b, struct drm_gem_open)
> --
> 2.39.1
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/2] drm: document DRM_IOCTL_GEM_CLOSE
2023-02-16 19:48 ` Daniel Vetter
@ 2023-02-17 9:28 ` Pekka Paalanen
2023-02-17 9:33 ` Simon Ser
1 sibling, 0 replies; 9+ messages in thread
From: Pekka Paalanen @ 2023-02-17 9:28 UTC (permalink / raw)
To: Simon Ser; +Cc: dri-devel
[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]
On Thu, 16 Feb 2023 20:48:05 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Feb 16, 2023 at 01:09:45PM +0000, Simon Ser wrote:
> > This is a bit tricky, because of the ref'counting considerations.
> > See also [1] for more discussion about this topic. Since this is
> > kernel docs, I've decided to elaborate a bit less on the user-space
> > details.
> >
> > [1]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/110
> >
> > Signed-off-by: Simon Ser <contact@emersion.fr>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Pekka Paalanen <ppaalanen@gmail.com>
> > Cc: Daniel Stone <daniel@fooishbar.org>
> > ---
> > include/uapi/drm/drm.h | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 54b2313c8332..4829f1fa9570 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -972,6 +972,19 @@ extern "C" {
> > #define DRM_IOCTL_GET_STATS DRM_IOR( 0x06, struct drm_stats)
> > #define DRM_IOCTL_SET_VERSION DRM_IOWR(0x07, struct drm_set_version)
> > #define DRM_IOCTL_MODESET_CTL DRM_IOW(0x08, struct drm_modeset_ctl)
> > +/**
> > + * DRM_IOCTL_GEM_CLOSE - Close a GEM handle.
> > + *
> > + * GEM handles are not reference-counted by the kernel. User-space is
> > + * responsible for managing their lifetime. For example, if user-space imports
> > + * the same memory object twice on the same DRM file description, the same GEM
> > + * handle is returned by both imports, and user-space needs to ensure
> > + * &DRM_IOCTL_GEM_CLOSE is performed once only. The same situation can happen
> > + * when a memory object is allocated, then exported and imported again on the
> > + * same DRM file description. The &DRM_IOCTL_MODE_GETFB2 IOCTL is an exception
> > + * and always returns fresh new GEM handles even if an existing GEM handle
> > + * already refers to the same memory object before the IOCTL is performed.
>
> I'd duplicate the relevant parts into each ioctl doc too, just to increase
> the chances people notice these caveats. But that's stuf for these other
> patches. for this one:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> btw do the links from on ioctl text to the other work?
> -Daniel
This one
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
I agree with Daniel's comments to both patches, and nothing else came
to my mind.
Thanks,
pq
>
> > + */
> > #define DRM_IOCTL_GEM_CLOSE DRM_IOW (0x09, struct drm_gem_close)
> > #define DRM_IOCTL_GEM_FLINK DRM_IOWR(0x0a, struct drm_gem_flink)
> > #define DRM_IOCTL_GEM_OPEN DRM_IOWR(0x0b, struct drm_gem_open)
> > --
> > 2.39.1
> >
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/2] drm: document DRM_IOCTL_GEM_CLOSE
2023-02-16 19:48 ` Daniel Vetter
2023-02-17 9:28 ` Pekka Paalanen
@ 2023-02-17 9:33 ` Simon Ser
1 sibling, 0 replies; 9+ messages in thread
From: Simon Ser @ 2023-02-17 9:33 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Pekka Paalanen, dri-devel
On Thursday, February 16th, 2023 at 20:48, Daniel Vetter <daniel@ffwll.ch> wrote:
> I'd duplicate the relevant parts into each ioctl doc too, just to increase
> the chances people notice these caveats.
Hm, I'd rather not duplicate, this makes it very easy for each version
to go out-of-sync with the rest. I will add links.
> But that's stuf for these other
> patches. for this one:
>
> Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks!
> btw do the links from on ioctl text to the other work?
Yes, they work fine.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE
2023-02-16 13:09 [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE Simon Ser
2023-02-16 13:09 ` [PATCH 2/2] drm: document DRM_IOCTL_GEM_CLOSE Simon Ser
@ 2023-02-16 19:46 ` Daniel Vetter
2023-02-20 15:49 ` Dave Stevenson
2 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2023-02-16 19:46 UTC (permalink / raw)
To: Simon Ser; +Cc: Pekka Paalanen, dri-devel
On Thu, Feb 16, 2023 at 01:09:41PM +0000, Simon Ser wrote:
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Daniel Stone <daniel@fooishbar.org>
> ---
> include/uapi/drm/drm.h | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 4cb956a52aee..54b2313c8332 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -1012,7 +1012,24 @@ extern "C" {
> #define DRM_IOCTL_UNLOCK DRM_IOW( 0x2b, struct drm_lock)
> #define DRM_IOCTL_FINISH DRM_IOW( 0x2c, struct drm_lock)
>
> +/**
> + * DRM_IOCTL_PRIME_HANDLE_TO_FD - Convert a GEM handle to a DMA-BUF FD.
> + *
> + * User-space sets &drm_prime_handle.handle with the GEM handle to export and
> + * &drm_prime_handle.flags, and gets back a DMA-BUF file descriptor in
> + * &drm_prime_handle.fd.
I think it'd be good to mention here that export could fail for any number
of driver specific reason, e.g. because the driver doesn't support dma-buf
export at all, or it's not supported for this specific gem handle because
it's a special one. And therefore generic clients need to be able to cope
with some kind of fallback path ideally.
Also I think it'd be good to mention that export support is announced with
the DRM_PRIME_CAP_EXPORT flag in the DRM_CAP_PRIME GETCAP ioctl
capability.
> + */
> #define DRM_IOCTL_PRIME_HANDLE_TO_FD DRM_IOWR(0x2d, struct drm_prime_handle)
> +/**
> + * DRM_IOCTL_PRIME_FD_TO_HANDLE - Convert a DMA-BUF FD to a GEM handle.
> + *
> + * User-space sets &drm_prime_handle.fd with a DMA-BUF file descriptor to
> + * import, and gets back a GEM handle in &drm_prime_handle.handle.
> + * &drm_prime_handle.flags is unused.
Same here, some mention that import can fail for any reason (many drivers
do not support importing to anything else than the exporting device). Also
DRM_PRIME_CAP_IMPORT for querying.
> + *
> + * If an existing GEM handle refers to the memory object backing the DMA-BUF,
> + * that GEM handle is returned.
I think we need to state clearly that this means importers who get unknown
buffers must have a userspace lookup structure to catch these handle
deduplications and refcount objects properly.
With these additions: Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> + */
> #define DRM_IOCTL_PRIME_FD_TO_HANDLE DRM_IOWR(0x2e, struct drm_prime_handle)
>
> #define DRM_IOCTL_AGP_ACQUIRE DRM_IO( 0x30)
> --
> 2.39.1
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE
2023-02-16 13:09 [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE Simon Ser
2023-02-16 13:09 ` [PATCH 2/2] drm: document DRM_IOCTL_GEM_CLOSE Simon Ser
2023-02-16 19:46 ` [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE Daniel Vetter
@ 2023-02-20 15:49 ` Dave Stevenson
2023-02-20 15:57 ` Simon Ser
2 siblings, 1 reply; 9+ messages in thread
From: Dave Stevenson @ 2023-02-20 15:49 UTC (permalink / raw)
To: Simon Ser; +Cc: Pekka Paalanen, dri-devel
Hi Simon
On Thu, 16 Feb 2023 at 13:09, Simon Ser <contact@emersion.fr> wrote:
>
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Daniel Stone <daniel@fooishbar.org>
> ---
> include/uapi/drm/drm.h | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 4cb956a52aee..54b2313c8332 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -1012,7 +1012,24 @@ extern "C" {
> #define DRM_IOCTL_UNLOCK DRM_IOW( 0x2b, struct drm_lock)
> #define DRM_IOCTL_FINISH DRM_IOW( 0x2c, struct drm_lock)
>
> +/**
> + * DRM_IOCTL_PRIME_HANDLE_TO_FD - Convert a GEM handle to a DMA-BUF FD.
> + *
> + * User-space sets &drm_prime_handle.handle with the GEM handle to export and
> + * &drm_prime_handle.flags, and gets back a DMA-BUF file descriptor in
> + * &drm_prime_handle.fd.
> + */
> #define DRM_IOCTL_PRIME_HANDLE_TO_FD DRM_IOWR(0x2d, struct drm_prime_handle)
> +/**
> + * DRM_IOCTL_PRIME_FD_TO_HANDLE - Convert a DMA-BUF FD to a GEM handle.
> + *
> + * User-space sets &drm_prime_handle.fd with a DMA-BUF file descriptor to
> + * import, and gets back a GEM handle in &drm_prime_handle.handle.
> + * &drm_prime_handle.flags is unused.
Is it worth explicitly stating that the handle would be released via
DRM_IOCTL_GEM_CLOSE? I've had userspace developers query how to
release imported handles in the past.
Dave
> + *
> + * If an existing GEM handle refers to the memory object backing the DMA-BUF,
> + * that GEM handle is returned.
> + */
> #define DRM_IOCTL_PRIME_FD_TO_HANDLE DRM_IOWR(0x2e, struct drm_prime_handle)
>
> #define DRM_IOCTL_AGP_ACQUIRE DRM_IO( 0x30)
> --
> 2.39.1
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE
2023-02-20 15:49 ` Dave Stevenson
@ 2023-02-20 15:57 ` Simon Ser
2023-02-20 16:12 ` Dave Stevenson
0 siblings, 1 reply; 9+ messages in thread
From: Simon Ser @ 2023-02-20 15:57 UTC (permalink / raw)
To: Dave Stevenson; +Cc: Pekka Paalanen, dri-devel
On Monday, February 20th, 2023 at 16:49, Dave Stevenson <dave.stevenson@raspberrypi.com> wrote:
> > + * User-space sets &drm_prime_handle.fd with a DMA-BUF file descriptor to
> > + * import, and gets back a GEM handle in &drm_prime_handle.handle.
> > + * &drm_prime_handle.flags is unused.
>
> Is it worth explicitly stating that the handle would be released via
> DRM_IOCTL_GEM_CLOSE? I've had userspace developers query how to
> release imported handles in the past.
v2 spells this out I think.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE
2023-02-20 15:57 ` Simon Ser
@ 2023-02-20 16:12 ` Dave Stevenson
0 siblings, 0 replies; 9+ messages in thread
From: Dave Stevenson @ 2023-02-20 16:12 UTC (permalink / raw)
To: Simon Ser; +Cc: Pekka Paalanen, dri-devel
On Mon, 20 Feb 2023 at 15:57, Simon Ser <contact@emersion.fr> wrote:
>
> On Monday, February 20th, 2023 at 16:49, Dave Stevenson <dave.stevenson@raspberrypi.com> wrote:
>
> > > + * User-space sets &drm_prime_handle.fd with a DMA-BUF file descriptor to
> > > + * import, and gets back a GEM handle in &drm_prime_handle.handle.
> > > + * &drm_prime_handle.flags is unused.
> >
> > Is it worth explicitly stating that the handle would be released via
> > DRM_IOCTL_GEM_CLOSE? I've had userspace developers query how to
> > release imported handles in the past.
>
> v2 spells this out I think.
It does - thanks.
I was reading back through my emails from Friday and the weekend, and
hadn't noticed v2 :-/ Sorry for the noise.
Dave
^ permalink raw reply [flat|nested] 9+ messages in thread