| Message ID | 20210902091126.2312-1-contact@emersion.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/lease: allow empty leases | expand |
On Thu, 02 Sep 2021 09:11:40 +0000 Simon Ser <contact@emersion.fr> wrote: > This can be used to create a separate DRM file description, thus > creating a new GEM handle namespace. See [1]. > > Example usage in wlroots is available at [2]. > > [1]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/110 > [2]: https://github.com/swaywm/wlroots/pull/3158 > Hi Simon, I have a feeling that this is a good idea, but could you explain in this commit message some real use cases where one needs a new GEM handle namespace? Not just "when you share a DRM fd between processes" but *why* you shared a DRM device fd between processes. If I have trouble remembering or figuring that out from those links, then I'm sure others have too. Thanks, pq > Signed-off-by: Simon Ser <contact@emersion.fr> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Daniel Stone <daniels@collabora.com> > Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk> > Cc: Michel Dänzer <michel@daenzer.net> > Cc: Emil Velikov <emil.l.velikov@gmail.com> > Cc: Keith Packard <keithp@keithp.com> > Cc: Boris Brezillon <boris.brezillon@collabora.com> > Cc: Dave Airlie <airlied@redhat.com> > --- > drivers/gpu/drm/drm_lease.c | 39 +++++++++++++++++-------------------- > 1 file changed, 18 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c > index dee4f24a1808..d72c2fac0ff1 100644 > --- a/drivers/gpu/drm/drm_lease.c > +++ b/drivers/gpu/drm/drm_lease.c > @@ -489,12 +489,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return -EOPNOTSUPP; > > - /* need some objects */ > - if (cl->object_count == 0) { > - DRM_DEBUG_LEASE("no objects in lease\n"); > - return -EINVAL; > - } > - > if (cl->flags && (cl->flags & ~(O_CLOEXEC | O_NONBLOCK))) { > DRM_DEBUG_LEASE("invalid flags\n"); > return -EINVAL; > @@ -510,23 +504,26 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > > object_count = cl->object_count; > > - object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), > - array_size(object_count, sizeof(__u32))); > - if (IS_ERR(object_ids)) { > - ret = PTR_ERR(object_ids); > - goto out_lessor; > - } > - > + /* Handle leased objects, if any */ > idr_init(&leases); > + if (object_count != 0) { > + object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), > + array_size(object_count, sizeof(__u32))); > + if (IS_ERR(object_ids)) { > + ret = PTR_ERR(object_ids); > + idr_destroy(&leases); > + goto out_lessor; > + } > > - /* fill and validate the object idr */ > - ret = fill_object_idr(dev, lessor_priv, &leases, > - object_count, object_ids); > - kfree(object_ids); > - if (ret) { > - DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret); > - idr_destroy(&leases); > - goto out_lessor; > + /* fill and validate the object idr */ > + ret = fill_object_idr(dev, lessor_priv, &leases, > + object_count, object_ids); > + kfree(object_ids); > + if (ret) { > + DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret); > + idr_destroy(&leases); > + goto out_lessor; > + } > } > > /* Allocate a file descriptor for the lease */
On Thu, Sep 02, 2021 at 05:28:10PM +0300, Pekka Paalanen wrote: > On Thu, 02 Sep 2021 09:11:40 +0000 > Simon Ser <contact@emersion.fr> wrote: > > > This can be used to create a separate DRM file description, thus > > creating a new GEM handle namespace. See [1]. > > > > Example usage in wlroots is available at [2]. > > > > [1]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/110 > > [2]: https://github.com/swaywm/wlroots/pull/3158 > > > > Hi Simon, > > I have a feeling that this is a good idea, but could you explain in > this commit message some real use cases where one needs a new GEM > handle namespace? Not just "when you share a DRM fd between processes" > but *why* you shared a DRM device fd between processes. > > If I have trouble remembering or figuring that out from those links, > then I'm sure others have too. Also please document the uapi headers and explain the use-case there and why and all that. I'd like that we officiate all uapi we intentionally create in the docs as much as possible. Also igt testcase patch for this too pls. -Daniel > > > Thanks, > pq > > > Signed-off-by: Simon Ser <contact@emersion.fr> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Daniel Stone <daniels@collabora.com> > > Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk> > > Cc: Michel Dänzer <michel@daenzer.net> > > Cc: Emil Velikov <emil.l.velikov@gmail.com> > > Cc: Keith Packard <keithp@keithp.com> > > Cc: Boris Brezillon <boris.brezillon@collabora.com> > > Cc: Dave Airlie <airlied@redhat.com> > > --- > > drivers/gpu/drm/drm_lease.c | 39 +++++++++++++++++-------------------- > > 1 file changed, 18 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c > > index dee4f24a1808..d72c2fac0ff1 100644 > > --- a/drivers/gpu/drm/drm_lease.c > > +++ b/drivers/gpu/drm/drm_lease.c > > @@ -489,12 +489,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > > return -EOPNOTSUPP; > > > > - /* need some objects */ > > - if (cl->object_count == 0) { > > - DRM_DEBUG_LEASE("no objects in lease\n"); > > - return -EINVAL; > > - } > > - > > if (cl->flags && (cl->flags & ~(O_CLOEXEC | O_NONBLOCK))) { > > DRM_DEBUG_LEASE("invalid flags\n"); > > return -EINVAL; > > @@ -510,23 +504,26 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > > > > object_count = cl->object_count; > > > > - object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), > > - array_size(object_count, sizeof(__u32))); > > - if (IS_ERR(object_ids)) { > > - ret = PTR_ERR(object_ids); > > - goto out_lessor; > > - } > > - > > + /* Handle leased objects, if any */ > > idr_init(&leases); > > + if (object_count != 0) { > > + object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), > > + array_size(object_count, sizeof(__u32))); > > + if (IS_ERR(object_ids)) { > > + ret = PTR_ERR(object_ids); > > + idr_destroy(&leases); > > + goto out_lessor; > > + } > > > > - /* fill and validate the object idr */ > > - ret = fill_object_idr(dev, lessor_priv, &leases, > > - object_count, object_ids); > > - kfree(object_ids); > > - if (ret) { > > - DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret); > > - idr_destroy(&leases); > > - goto out_lessor; > > + /* fill and validate the object idr */ > > + ret = fill_object_idr(dev, lessor_priv, &leases, > > + object_count, object_ids); > > + kfree(object_ids); > > + if (ret) { > > + DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret); > > + idr_destroy(&leases); > > + goto out_lessor; > > + } > > } > > > > /* Allocate a file descriptor for the lease */ >
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index dee4f24a1808..d72c2fac0ff1 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -489,12 +489,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP; - /* need some objects */ - if (cl->object_count == 0) { - DRM_DEBUG_LEASE("no objects in lease\n"); - return -EINVAL; - } - if (cl->flags && (cl->flags & ~(O_CLOEXEC | O_NONBLOCK))) { DRM_DEBUG_LEASE("invalid flags\n"); return -EINVAL; @@ -510,23 +504,26 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, object_count = cl->object_count; - object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), - array_size(object_count, sizeof(__u32))); - if (IS_ERR(object_ids)) { - ret = PTR_ERR(object_ids); - goto out_lessor; - } - + /* Handle leased objects, if any */ idr_init(&leases); + if (object_count != 0) { + object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), + array_size(object_count, sizeof(__u32))); + if (IS_ERR(object_ids)) { + ret = PTR_ERR(object_ids); + idr_destroy(&leases); + goto out_lessor; + } - /* fill and validate the object idr */ - ret = fill_object_idr(dev, lessor_priv, &leases, - object_count, object_ids); - kfree(object_ids); - if (ret) { - DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret); - idr_destroy(&leases); - goto out_lessor; + /* fill and validate the object idr */ + ret = fill_object_idr(dev, lessor_priv, &leases, + object_count, object_ids); + kfree(object_ids); + if (ret) { + DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret); + idr_destroy(&leases); + goto out_lessor; + } } /* Allocate a file descriptor for the lease */
This can be used to create a separate DRM file description, thus creating a new GEM handle namespace. See [1]. Example usage in wlroots is available at [2]. [1]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/110 [2]: https://github.com/swaywm/wlroots/pull/3158 Signed-off-by: Simon Ser <contact@emersion.fr> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Daniel Stone <daniels@collabora.com> Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk> Cc: Michel Dänzer <michel@daenzer.net> Cc: Emil Velikov <emil.l.velikov@gmail.com> Cc: Keith Packard <keithp@keithp.com> Cc: Boris Brezillon <boris.brezillon@collabora.com> Cc: Dave Airlie <airlied@redhat.com> --- drivers/gpu/drm/drm_lease.c | 39 +++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 21 deletions(-)