| Message ID | 20230215124152.101548-1-contact@emersion.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: document expectations for GETFB2 handles | expand |
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
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.
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
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.
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
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
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.
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.
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(-)