[PATCH] drm: document expectations for GETFB2 handles

[PATCH] drm: document expectations for GETFB2 handles

[Simon Ser]

There are two important details missing from the docs:

- If the memory object backing the FB already has a GEM handle,
  it's not re-used, a new one is generated.
- Aliased planes will return the same GEM handle.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
---
 include/uapi/drm/drm.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 642808520d92..4cb956a52aee 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -1104,8 +1104,13 @@ extern "C" {
  * struct as the output.
  *
  * If the client is DRM master or has &CAP_SYS_ADMIN, &drm_mode_fb_cmd2.handles
- * will be filled with GEM buffer handles. Planes are valid until one has a
- * zero handle -- this can be used to compute the number of planes.
+ * will be filled with GEM buffer handles. Fresh new GEM handles are always
+ * returned, even if another GEM handle referring to the same memory object
+ * already exists on the DRM file description. The caller is responsible for
+ * removing the new handles, e.g. via the &DRM_IOCTL_GEM_CLOSE IOCTL. The same
+ * new handle will be returned for multiple planes in case they use the same
+ * memory object. Planes are valid until one has a zero handle -- this can be
+ * used to compute the number of planes.
  *
  * Otherwise, &drm_mode_fb_cmd2.handles will be zeroed and planes are valid
  * until one has a zero &drm_mode_fb_cmd2.pitches.
-- 
2.39.1

Re: [PATCH] drm: document expectations for GETFB2 handles

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2125 bytes --]

On Wed, 15 Feb 2023 12:42:00 +0000
Simon Ser <contact@emersion.fr> wrote:

> There are two important details missing from the docs:
> 
> - If the memory object backing the FB already has a GEM handle,
>   it's not re-used, a new one is generated.
> - Aliased planes will return the same GEM handle.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> ---
>  include/uapi/drm/drm.h | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 642808520d92..4cb956a52aee 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -1104,8 +1104,13 @@ extern "C" {
>   * struct as the output.
>   *
>   * If the client is DRM master or has &CAP_SYS_ADMIN, &drm_mode_fb_cmd2.handles
> - * will be filled with GEM buffer handles. Planes are valid until one has a
> - * zero handle -- this can be used to compute the number of planes.
> + * will be filled with GEM buffer handles. Fresh new GEM handles are always
> + * returned, even if another GEM handle referring to the same memory object
> + * already exists on the DRM file description. The caller is responsible for
> + * removing the new handles, e.g. via the &DRM_IOCTL_GEM_CLOSE IOCTL. The same
> + * new handle will be returned for multiple planes in case they use the same
> + * memory object. Planes are valid until one has a zero handle -- this can be
> + * used to compute the number of planes.
>   *
>   * Otherwise, &drm_mode_fb_cmd2.handles will be zeroed and planes are valid
>   * until one has a zero &drm_mode_fb_cmd2.pitches.

It is well-written, clear, and a surprise to me.

Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>

I didn't know it was at all possible to have different GEM handles
pointing to the same object. DMABUF import is guaranteed to return the
existing GEM handle, right? Why is GETFB2 different? Why does it not
have the same problem as what forced DMABUF import to return existing
handles?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: document expectations for GETFB2 handles

[Simon Ser]

On Wednesday, February 15th, 2023 at 14:41, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> I didn't know it was at all possible to have different GEM handles
> pointing to the same object. DMABUF import is guaranteed to return the
> existing GEM handle, right? Why is GETFB2 different? Why does it not
> have the same problem as what forced DMABUF import to return existing
> handles?

drm_gem_prime_fd_to_handle() explicitly checks whether the memory object
already has a GEM handle via drm_prime_lookup_buf_handle(). OTOH,
drm_mode_getfb() and drm_mode_getfb2_ioctl() just unconditionally call
drm_gem_handle_create().

Yes, it's a rather inconsistent detail. A detail which becomes very
important when ref'counting and trying not to leak GEM handles from
user-space. Fortunately GETFB/GETFB2 usage is pretty seldom.

Re: [PATCH] drm: document expectations for GETFB2 handles

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2204 bytes --]

On Wed, 15 Feb 2023 17:03:54 +0000
Simon Ser <contact@emersion.fr> wrote:

> On Wednesday, February 15th, 2023 at 14:41, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
> > I didn't know it was at all possible to have different GEM handles
> > pointing to the same object. DMABUF import is guaranteed to return the
> > existing GEM handle, right? Why is GETFB2 different? Why does it not
> > have the same problem as what forced DMABUF import to return existing
> > handles?  
> 
> drm_gem_prime_fd_to_handle() explicitly checks whether the memory object
> already has a GEM handle via drm_prime_lookup_buf_handle(). OTOH,
> drm_mode_getfb() and drm_mode_getfb2_ioctl() just unconditionally call
> drm_gem_handle_create().
> 
> Yes, it's a rather inconsistent detail. A detail which becomes very
> important when ref'counting and trying not to leak GEM handles from
> user-space. Fortunately GETFB/GETFB2 usage is pretty seldom.

I see that GETFB was introduced in

commit f453ba0460742ad027ae0c4c7d61e62817b3e7ef
Author: Dave Airlie <airlied@redhat.com>
Date:   Fri Nov 7 14:05:41 2008 -0800

    DRM: add mode setting support

and I guess GETFB2 just replicated the behaviour for no reason. Was
that it, Daniel S.? Are there other pitfalls that should be documented?

So it was long before dmabuf was a thing I believe, meaning that any
dmabuf import problems could not have been known.

Btw. does this also mean that if you use GETFB2 to get handle A, you
export that as dmabuf and import in the same open device instance, you
again get handle A?

IOW, you should *never* ever export a dmabuf of what you got with
GETFB2. If one did, one might import it oneself via GBM, breaking all
reference counting. But you also cannot "just leak" the handle A,
because if GBM happens to run on a different DRM device opened
instance, GBM would get a different handle to own.

That's... err. How is a compositor supposed to do transition animation
from an old FB to its own thing? I guess mmap + glTexImage2D equivalent
to make a copy of the old FB so one can use it as a texture?

Maybe something about this would be good to spell out in the doc?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: document expectations for GETFB2 handles

[Simon Ser]

On Thursday, February 16th, 2023 at 10:11, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> Btw. does this also mean that if you use GETFB2 to get handle A, you
> export that as dmabuf and import in the same open device instance, you
> again get handle A?

I haven't tested it, but I believe that is correct.

> IOW, you should never ever export a dmabuf of what you got with
> GETFB2. If one did, one might import it oneself via GBM, breaking all
> reference counting. But you also cannot "just leak" the handle A,
> because if GBM happens to run on a different DRM device opened
> instance, GBM would get a different handle to own.
> 
> That's... err. How is a compositor supposed to do transition animation
> from an old FB to its own thing? I guess mmap + glTexImage2D equivalent
> to make a copy of the old FB so one can use it as a texture?

I think the compositor can export the handle as DMA-BUF, then close the
handle immediately. Then go about its business as usual.

Re: [PATCH] drm: document expectations for GETFB2 handles

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

On Thu, 16 Feb 2023 09:25:38 +0000
Simon Ser <contact@emersion.fr> wrote:

> On Thursday, February 16th, 2023 at 10:11, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
> > Btw. does this also mean that if you use GETFB2 to get handle A, you
> > export that as dmabuf and import in the same open device instance, you
> > again get handle A?  
> 
> I haven't tested it, but I believe that is correct.
> 
> > IOW, you should never ever export a dmabuf of what you got with
> > GETFB2. If one did, one might import it oneself via GBM, breaking all
> > reference counting. But you also cannot "just leak" the handle A,
> > because if GBM happens to run on a different DRM device opened
> > instance, GBM would get a different handle to own.
> > 
> > That's... err. How is a compositor supposed to do transition animation
> > from an old FB to its own thing? I guess mmap + glTexImage2D equivalent
> > to make a copy of the old FB so one can use it as a texture?  
> 
> I think the compositor can export the handle as DMA-BUF, then close the
> handle immediately. Then go about its business as usual.

Ah! Of course, I didn't think of that.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: document expectations for GETFB2 handles

[Daniel Stone]

Hi,

On Thu, 16 Feb 2023 at 09:25, Simon Ser <contact@emersion.fr> wrote:
> On Thursday, February 16th, 2023 at 10:11, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > Btw. does this also mean that if you use GETFB2 to get handle A, you
> > export that as dmabuf and import in the same open device instance, you
> > again get handle A?
>
> I haven't tested it, but I believe that is correct.
>
> > IOW, you should never ever export a dmabuf of what you got with
> > GETFB2. If one did, one might import it oneself via GBM, breaking all
> > reference counting. But you also cannot "just leak" the handle A,
> > because if GBM happens to run on a different DRM device opened
> > instance, GBM would get a different handle to own.
> >
> > That's... err. How is a compositor supposed to do transition animation
> > from an old FB to its own thing? I guess mmap + glTexImage2D equivalent
> > to make a copy of the old FB so one can use it as a texture?
>
> I think the compositor can export the handle as DMA-BUF, then close the
> handle immediately. Then go about its business as usual.

Yeah, I think either of those two are the most sensible. We did go
back and forth over the semantics a few times - part over mail and
part over IRC - and the eventual conclusion was to match GetFB to make
it easier for users to transition, but to de-duplicate the handles
_within_ the call for consistency with the rest of everything else.
It's not the most pleasant compromise, but eh.

Cheers,
Daniel

Re: [PATCH] drm: document expectations for GETFB2 handles

[Daniel Vetter]

On Thu, 16 Feb 2023 at 10:25, Simon Ser <contact@emersion.fr> wrote:
>
> On Thursday, February 16th, 2023 at 10:11, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> > Btw. does this also mean that if you use GETFB2 to get handle A, you
> > export that as dmabuf and import in the same open device instance, you
> > again get handle A?
>
> I haven't tested it, but I believe that is correct.

Yup. If we haven't, we should probably document this in the FD2HANDLE ioctl.
-Daniel

> > IOW, you should never ever export a dmabuf of what you got with
> > GETFB2. If one did, one might import it oneself via GBM, breaking all
> > reference counting. But you also cannot "just leak" the handle A,
> > because if GBM happens to run on a different DRM device opened
> > instance, GBM would get a different handle to own.
> >
> > That's... err. How is a compositor supposed to do transition animation
> > from an old FB to its own thing? I guess mmap + glTexImage2D equivalent
> > to make a copy of the old FB so one can use it as a texture?
>
> I think the compositor can export the handle as DMA-BUF, then close the
> handle immediately. Then go about its business as usual.



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch