[PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE

[PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE

[Simon Ser]

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.
+ *
+ * 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

[PATCH 2/2] drm: document DRM_IOCTL_GEM_CLOSE

[Simon Ser]

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

Re: [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE

[Daniel Vetter]

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

Re: [PATCH 2/2] drm: document DRM_IOCTL_GEM_CLOSE

[Daniel Vetter]

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

Re: [PATCH 2/2] drm: document DRM_IOCTL_GEM_CLOSE

[Pekka Paalanen]

[-- 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 --]

Re: [PATCH 2/2] drm: document DRM_IOCTL_GEM_CLOSE

[Simon Ser]

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.

Re: [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE

[Dave Stevenson]

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
>
>

Re: [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE

[Simon Ser]

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.

Re: [PATCH 1/2] drm: document DRM_IOCTL_PRIME_HANDLE_TO_FD and PRIME_FD_TO_HANDLE

[Dave Stevenson]

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