Linaro-mm-sig June 2021

linaro-mm-sig@lists.linaro.org

20 participants
50 discussions

Re: [Linaro-mm-sig] [PATCH v3 1/2] drm: add a locked version of drm_is_current_master

by Daniel Vetter

On Sun, Jun 20, 2021 at 07:03:26PM +0800, Desmond Cheong Zhi Xi wrote: > While checking the master status of the DRM file in > drm_is_current_master(), the device's master mutex should be > held. Without the mutex, the pointer fpriv->master may be freed > concurrently by another process calling drm_setmaster_ioctl(). This > could lead to use-after-free errors when the pointer is subsequently > dereferenced in drm_lease_owner(). > > The callers of drm_is_current_master() from drm_auth.c hold the > device's master mutex, but external callers do not. Hence, we implement > drm_is_current_master_locked() to be used within drm_auth.c, and > modify drm_is_current_master() to grab the device's master mutex > before checking the master status. > > Reported-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> > Reviewed-by: Emil Velikov <emil.l.velikov(a)gmail.com> Merged to drm-misc-fixes, thanks for your patch. -Daniel > --- > drivers/gpu/drm/drm_auth.c | 51 ++++++++++++++++++++++++-------------- > 1 file changed, 32 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index 232abbba3686..86d4b72e95cb 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -61,6 +61,35 @@ > * trusted clients. > */ > > +static bool drm_is_current_master_locked(struct drm_file *fpriv) > +{ > + lockdep_assert_held_once(&fpriv->master->dev->master_mutex); > + > + return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; > +} > + > +/** > + * drm_is_current_master - checks whether @priv is the current master > + * @fpriv: DRM file private > + * > + * Checks whether @fpriv is current master on its device. This decides whether a > + * client is allowed to run DRM_MASTER IOCTLs. > + * > + * Most of the modern IOCTL which require DRM_MASTER are for kernel modesetting > + * - the current master is assumed to own the non-shareable display hardware. > + */ > +bool drm_is_current_master(struct drm_file *fpriv) > +{ > + bool ret; > + > + mutex_lock(&fpriv->master->dev->master_mutex); > + ret = drm_is_current_master_locked(fpriv); > + mutex_unlock(&fpriv->master->dev->master_mutex); > + > + return ret; > +} > +EXPORT_SYMBOL(drm_is_current_master); > + > int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv) > { > struct drm_auth *auth = data; > @@ -223,7 +252,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, > if (ret) > goto out_unlock; > > - if (drm_is_current_master(file_priv)) > + if (drm_is_current_master_locked(file_priv)) > goto out_unlock; > > if (dev->master) { > @@ -272,7 +301,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, > if (ret) > goto out_unlock; > > - if (!drm_is_current_master(file_priv)) { > + if (!drm_is_current_master_locked(file_priv)) { > ret = -EINVAL; > goto out_unlock; > } > @@ -321,7 +350,7 @@ void drm_master_release(struct drm_file *file_priv) > if (file_priv->magic) > idr_remove(&file_priv->master->magic_map, file_priv->magic); > > - if (!drm_is_current_master(file_priv)) > + if (!drm_is_current_master_locked(file_priv)) > goto out; > > drm_legacy_lock_master_cleanup(dev, master); > @@ -342,22 +371,6 @@ void drm_master_release(struct drm_file *file_priv) > mutex_unlock(&dev->master_mutex); > } > > -/** > - * drm_is_current_master - checks whether @priv is the current master > - * @fpriv: DRM file private > - * > - * Checks whether @fpriv is current master on its device. This decides whether a > - * client is allowed to run DRM_MASTER IOCTLs. > - * > - * Most of the modern IOCTL which require DRM_MASTER are for kernel modesetting > - * - the current master is assumed to own the non-shareable display hardware. > - */ > -bool drm_is_current_master(struct drm_file *fpriv) > -{ > - return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; > -} > -EXPORT_SYMBOL(drm_is_current_master); > - > /** > * drm_master_get - reference a master pointer > * @master: &struct drm_master > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH v3 2/2] drm: protect drm_master pointers in drm_lease.c

by Daniel Vetter

On Wed, Jun 23, 2021 at 5:49 AM Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> wrote: > > On 21/6/21 10:47 pm, Daniel Vetter wrote: > > On Sun, Jun 20, 2021 at 07:03:27PM +0800, Desmond Cheong Zhi Xi wrote: > >> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > >> index 86d4b72e95cb..0c64a77c67a6 100644 > >> --- a/drivers/gpu/drm/drm_auth.c > >> +++ b/drivers/gpu/drm/drm_auth.c > >> @@ -384,6 +384,28 @@ struct drm_master *drm_master_get(struct drm_master *master) > >> } > >> EXPORT_SYMBOL(drm_master_get); > >> > >> +/** > >> + * drm_file_get_master - reference @file_priv->master > >> + * @file_priv: DRM file private > >> + * > >> + * Increments the reference count of @file_priv->master and returns > > > > Does this format correctly? I'd go with "&drm_file.master of @file_priv". > > > > Got it. "file_priv->master" was bolded, but no link to drm_file.master > was generated. I'll update this. > > >> + * @file_priv->master. > >> + * > >> + * Master pointers returned from this function should be unreferenced using > >> + * drm_master_put(). > >> + */ > >> +struct drm_master *drm_file_get_master(struct drm_file *file_priv) > >> +{ > >> + struct drm_master *master; > >> + > >> + mutex_lock(&file_priv->master->dev->master_mutex); > >> + master = drm_master_get(file_priv->master); > >> + mutex_unlock(&file_priv->master->dev->master_mutex); > >> + > >> + return master; > >> +} > >> +EXPORT_SYMBOL(drm_file_get_master); > >> + > >> static void drm_master_destroy(struct kref *kref) > >> { > >> struct drm_master *master = container_of(kref, struct drm_master, refcount); > >> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c > >> index da4f085fc09e..65eab82f8acc 100644 > >> --- a/drivers/gpu/drm/drm_lease.c > >> +++ b/drivers/gpu/drm/drm_lease.c > >> @@ -107,10 +107,17 @@ static bool _drm_has_leased(struct drm_master *master, int id) > >> */ > >> bool _drm_lease_held(struct drm_file *file_priv, int id) > >> { > >> + bool ret; > >> + struct drm_master *master; > >> + > >> if (!file_priv || !file_priv->master) > > > > So here we still have a ->master access outside of the locked code > > section. I think the best fix for that would be to move the NULL check > > into drm_file_get_master (where we grab the lock already anyway), and > > update the kerneldoc to state that it might return NULL. > > > > Same with all the checks for ->master below. > > > > Moving the check into drm_file_get_master sounds good. Grabbing the lock > before performing the NULL check poses a little chicken-and-egg problem > though. > > It's true that without the lock, even if file_priv->master passes the > NULL check, it could be freed in the time between the check and grabbing > the lock. > > However, based on the original code, it seems there's the possibility > that file_priv->master might be NULL. In this case, grabbing the lock > results in a null ptr dereference because we get the mutex via > &file_priv->master->dev->master_mutex. > > By this reasoning, I think the safer method is still to perform the NULL > check before grabbing the lock. file_priv->dev->master_mutex should also work and avoid the trouble. Please also cc intel-gfx list, there's a CI system there to test your patches. Since patch 1 of this series had pretty bad deadlock that I didn't see would be good to make sure we get more test coverage on these. Thanks, Daniel > > >> return true; > >> > >> - return _drm_lease_held_master(file_priv->master, id); > >> + master = drm_file_get_master(file_priv); > >> + ret = _drm_lease_held_master(master, id); > >> + drm_master_put(&master); > >> + > >> + return ret; > >> } > >> > >> /** > >> @@ -132,10 +139,11 @@ bool drm_lease_held(struct drm_file *file_priv, int id) > >> if (!file_priv || !file_priv->master || !file_priv->master->lessor) > >> return true; > > > > master->lessor dereferenced outside the lock or without holding a > > reference. > > > >> > >> - master = file_priv->master; > >> + master = drm_file_get_master(file_priv); > >> mutex_lock(&master->dev->mode_config.idr_mutex); > >> ret = _drm_lease_held_master(master, id); > >> mutex_unlock(&master->dev->mode_config.idr_mutex); > >> + drm_master_put(&master); > >> return ret; > >> } > >> > >> @@ -158,7 +166,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) > >> if (!file_priv || !file_priv->master || !file_priv->master->lessor) > >> return crtcs_in; > > > > Same here. > > > >> > >> - master = file_priv->master; > >> + master = drm_file_get_master(file_priv); > >> dev = master->dev; > >> > >> count_in = count_out = 0; > >> @@ -177,6 +185,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) > >> count_in++; > >> } > >> mutex_unlock(&master->dev->mode_config.idr_mutex); > >> + drm_master_put(&master); > >> return crtcs_out; > >> } > >> > >> @@ -490,7 +499,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > >> size_t object_count; > >> int ret = 0; > >> struct idr leases; > >> - struct drm_master *lessor = lessor_priv->master; > >> + struct drm_master *lessor; > >> struct drm_master *lessee = NULL; > >> struct file *lessee_file = NULL; > >> struct file *lessor_file = lessor_priv->filp; > >> @@ -502,12 +511,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) > >> return -EOPNOTSUPP; > >> > >> - /* Do not allow sub-leases */ > >> - if (lessor->lessor) { > >> - DRM_DEBUG_LEASE("recursive leasing not allowed\n"); > >> - return -EINVAL; > >> - } > >> - > >> /* need some objects */ > >> if (cl->object_count == 0) { > >> DRM_DEBUG_LEASE("no objects in lease\n"); > >> @@ -519,12 +522,22 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > >> return -EINVAL; > >> } > >> > >> + lessor = drm_file_get_master(lessor_priv); > >> + /* Do not allow sub-leases */ > >> + if (lessor->lessor) { > > > > Here we check after grabbing the reference, so looks correct. > > > >> + DRM_DEBUG_LEASE("recursive leasing not allowed\n"); > >> + ret = -EINVAL; > >> + goto out_lessor; > >> + } > >> + > >> 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)) > >> - return PTR_ERR(object_ids); > >> + if (IS_ERR(object_ids)) { > >> + ret = PTR_ERR(object_ids); > >> + goto out_lessor; > >> + } > >> > >> idr_init(&leases); > >> > >> @@ -535,14 +548,15 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > >> if (ret) { > >> DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret); > >> idr_destroy(&leases); > >> - return ret; > >> + goto out_lessor; > >> } > >> > >> /* Allocate a file descriptor for the lease */ > >> fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK)); > >> if (fd < 0) { > >> idr_destroy(&leases); > >> - return fd; > >> + ret = fd; > >> + goto out_lessor; > >> } > >> > >> DRM_DEBUG_LEASE("Creating lease\n"); > >> @@ -578,6 +592,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > >> /* Hook up the fd */ > >> fd_install(fd, lessee_file); > >> > >> + drm_master_put(&lessor); > >> DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n"); > >> return 0; > >> > >> @@ -587,6 +602,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > >> out_leases: > >> put_unused_fd(fd); > >> > >> +out_lessor: > >> + drm_master_put(&lessor); > >> DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret); > >> return ret; > >> } > >> @@ -609,7 +626,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, > >> struct drm_mode_list_lessees *arg = data; > >> __u32 __user *lessee_ids = (__u32 __user *) (uintptr_t) (arg->lessees_ptr); > >> __u32 count_lessees = arg->count_lessees; > >> - struct drm_master *lessor = lessor_priv->master, *lessee; > >> + struct drm_master *lessor, *lessee; > >> int count; > >> int ret = 0; > >> > >> @@ -620,6 +637,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, > >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) > >> return -EOPNOTSUPP; > >> > >> + lessor = drm_file_get_master(lessor_priv); > >> DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id); > >> > >> mutex_lock(&dev->mode_config.idr_mutex); > >> @@ -643,6 +661,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, > >> arg->count_lessees = count; > >> > >> mutex_unlock(&dev->mode_config.idr_mutex); > >> + drm_master_put(&lessor); > >> > >> return ret; > >> } > >> @@ -662,7 +681,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, > >> struct drm_mode_get_lease *arg = data; > >> __u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr); > >> __u32 count_objects = arg->count_objects; > >> - struct drm_master *lessee = lessee_priv->master; > >> + struct drm_master *lessee; > >> struct idr *object_idr; > >> int count; > >> void *entry; > >> @@ -676,6 +695,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, > >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) > >> return -EOPNOTSUPP; > >> > >> + lessee = drm_file_get_master(lessee_priv); > >> DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id); > >> > >> mutex_lock(&dev->mode_config.idr_mutex); > >> @@ -703,6 +723,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, > >> arg->count_objects = count; > >> > >> mutex_unlock(&dev->mode_config.idr_mutex); > >> + drm_master_put(&lessee); > >> > >> return ret; > >> } > >> @@ -721,7 +742,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, > >> void *data, struct drm_file *lessor_priv) > >> { > >> struct drm_mode_revoke_lease *arg = data; > >> - struct drm_master *lessor = lessor_priv->master; > >> + struct drm_master *lessor; > >> struct drm_master *lessee; > >> int ret = 0; > >> > >> @@ -731,6 +752,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, > >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) > >> return -EOPNOTSUPP; > >> > >> + lessor = drm_file_get_master(lessor_priv); > >> mutex_lock(&dev->mode_config.idr_mutex); > >> > >> lessee = _drm_find_lessee(lessor, arg->lessee_id); > >> @@ -751,6 +773,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, > >> > >> fail: > >> mutex_unlock(&dev->mode_config.idr_mutex); > >> + drm_master_put(&lessor); > >> > >> return ret; > >> } > >> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h > >> index 6bf8b2b78991..f99d3417f304 100644 > >> --- a/include/drm/drm_auth.h > >> +++ b/include/drm/drm_auth.h > >> @@ -107,6 +107,7 @@ struct drm_master { > >> }; > >> > >> struct drm_master *drm_master_get(struct drm_master *master); > >> +struct drm_master *drm_file_get_master(struct drm_file *file_priv); > >> void drm_master_put(struct drm_master **master); > >> bool drm_is_current_master(struct drm_file *fpriv); > >> > >> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > >> index b81b3bfb08c8..e9931fca4ab7 100644 > >> --- a/include/drm/drm_file.h > >> +++ b/include/drm/drm_file.h > >> @@ -226,9 +226,18 @@ struct drm_file { > >> /** > >> * @master: > >> * > >> - * Master this node is currently associated with. Only relevant if > >> - * drm_is_primary_client() returns true. Note that this only > >> - * matches &drm_device.master if the master is the currently active one. > >> + * Master this node is currently associated with. Protected by struct > >> + * &drm_device.master_mutex. > >> + * > >> + * Only relevant if drm_is_primary_client() returns true. Note that > >> + * this only matches &drm_device.master if the master is the currently > >> + * active one. > >> + * > >> + * When obtaining a copy of this pointer, it is recommended to either > >> + * hold struct &drm_device.master_mutex for the duration of the > >> + * pointer's use, or to use drm_file_get_master() if struct > >> + * &drm_device.master_mutex is not currently held and there is no other > >> + * need to hold it. This prevents @master from being freed during use. > >> * > >> * See also @authentication and @is_master and the :ref:`section on > >> * primary nodes and authentication <drm_primary_node>`. > >> -- > >> 2.25.1 > >> > > > > Thanks for the feedback, Daniel. I'll send out an updated patch to > address these issues. > > Best wishes, > Desmond -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 6 months

[PATCH 06/15] drm/panfrost: Fix implicit sync

by Daniel Vetter

Currently this has no practial relevance I think because there's not many who can pull off a setup with panfrost and another gpu in the same system. But the rules are that if you're setting an exclusive fence, indicating a gpu write access in the implicit fencing system, then you need to wait for all fences, not just the previous exclusive fence. panfrost against itself has no problem, because it always sets the exclusive fence (but that's probably something that will need to be fixed for vulkan and/or multi-engine gpus, or you'll suffer badly). Also no problem with that against display. With the prep work done to switch over to the dependency helpers this is now a oneliner. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Rob Herring <robh(a)kernel.org> Cc: Tomeu Vizoso <tomeu.vizoso(a)collabora.com> Cc: Steven Price <steven.price(a)arm.com> Cc: Alyssa Rosenzweig <alyssa.rosenzweig(a)collabora.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/gpu/drm/panfrost/panfrost_job.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 71cd43fa1b36..ef004d587dc4 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -203,9 +203,8 @@ static int panfrost_acquire_object_fences(struct drm_gem_object **bos, int i, ret; for (i = 0; i < bo_count; i++) { - struct dma_fence *fence = dma_resv_get_excl_unlocked(bos[i]->resv); - - ret = drm_gem_fence_array_add(deps, fence); + /* panfrost always uses write mode in its current uapi */ + ret = drm_gem_fence_array_add_implicit(deps, bos[i], true); if (ret) return ret; } -- 2.32.0.rc2

3 years, 6 months

[PATCH 05/15] drm/panfrost: Use xarray and helpers for depedency tracking

by Daniel Vetter

More consistency and prep work for the next patch. Aside: I wonder whether we shouldn't just move this entire xarray business into the scheduler so that not everyone has to reinvent the same wheels. Cc'ing some scheduler people for this too. v2: Correctly handle sched_lock since Lucas pointed out it's needed. v3: Rebase, dma_resv_get_excl_unlocked got renamed v4: Don't leak job references on failure (Steven). Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: Luben Tuikov <luben.tuikov(a)amd.com> Cc: Alex Deucher <alexander.deucher(a)amd.com> Cc: Lee Jones <lee.jones(a)linaro.org> Cc: Steven Price <steven.price(a)arm.com> Cc: Rob Herring <robh(a)kernel.org> Cc: Tomeu Vizoso <tomeu.vizoso(a)collabora.com> Cc: Alyssa Rosenzweig <alyssa.rosenzweig(a)collabora.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/panfrost/panfrost_drv.c | 41 +++++++--------- drivers/gpu/drm/panfrost/panfrost_job.c | 65 +++++++++++-------------- drivers/gpu/drm/panfrost/panfrost_job.h | 8 ++- 3 files changed, 49 insertions(+), 65 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 075ec0ef746c..3ee828f1e7a5 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -138,12 +138,6 @@ panfrost_lookup_bos(struct drm_device *dev, if (!job->bo_count) return 0; - job->implicit_fences = kvmalloc_array(job->bo_count, - sizeof(struct dma_fence *), - GFP_KERNEL | __GFP_ZERO); - if (!job->implicit_fences) - return -ENOMEM; - ret = drm_gem_objects_lookup(file_priv, (void __user *)(uintptr_t)args->bo_handles, job->bo_count, &job->bos); @@ -174,7 +168,7 @@ panfrost_lookup_bos(struct drm_device *dev, } /** - * panfrost_copy_in_sync() - Sets up job->in_fences[] with the sync objects + * panfrost_copy_in_sync() - Sets up job->deps with the sync objects * referenced by the job. * @dev: DRM device * @file_priv: DRM file for this fd @@ -194,22 +188,14 @@ panfrost_copy_in_sync(struct drm_device *dev, { u32 *handles; int ret = 0; - int i; + int i, in_fence_count; - job->in_fence_count = args->in_sync_count; + in_fence_count = args->in_sync_count; - if (!job->in_fence_count) + if (!in_fence_count) return 0; - job->in_fences = kvmalloc_array(job->in_fence_count, - sizeof(struct dma_fence *), - GFP_KERNEL | __GFP_ZERO); - if (!job->in_fences) { - DRM_DEBUG("Failed to allocate job in fences\n"); - return -ENOMEM; - } - - handles = kvmalloc_array(job->in_fence_count, sizeof(u32), GFP_KERNEL); + handles = kvmalloc_array(in_fence_count, sizeof(u32), GFP_KERNEL); if (!handles) { ret = -ENOMEM; DRM_DEBUG("Failed to allocate incoming syncobj handles\n"); @@ -218,16 +204,23 @@ panfrost_copy_in_sync(struct drm_device *dev, if (copy_from_user(handles, (void __user *)(uintptr_t)args->in_syncs, - job->in_fence_count * sizeof(u32))) { + in_fence_count * sizeof(u32))) { ret = -EFAULT; DRM_DEBUG("Failed to copy in syncobj handles\n"); goto fail; } - for (i = 0; i < job->in_fence_count; i++) { + for (i = 0; i < in_fence_count; i++) { + struct dma_fence *fence; + ret = drm_syncobj_find_fence(file_priv, handles[i], 0, 0, - &job->in_fences[i]); - if (ret == -EINVAL) + &fence); + if (ret) + goto fail; + + ret = drm_gem_fence_array_add(&job->deps, fence); + + if (ret) goto fail; } @@ -265,6 +258,8 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, kref_init(&job->refcount); + xa_init_flags(&job->deps, XA_FLAGS_ALLOC); + job->pfdev = pfdev; job->jc = args->jc; job->requirements = args->requirements; diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 38f8580c19f1..71cd43fa1b36 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -196,14 +196,21 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js) job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START); } -static void panfrost_acquire_object_fences(struct drm_gem_object **bos, - int bo_count, - struct dma_fence **implicit_fences) +static int panfrost_acquire_object_fences(struct drm_gem_object **bos, + int bo_count, + struct xarray *deps) { - int i; + int i, ret; - for (i = 0; i < bo_count; i++) - implicit_fences[i] = dma_resv_get_excl_unlocked(bos[i]->resv); + for (i = 0; i < bo_count; i++) { + struct dma_fence *fence = dma_resv_get_excl_unlocked(bos[i]->resv); + + ret = drm_gem_fence_array_add(deps, fence); + if (ret) + return ret; + } + + return 0; } static void panfrost_attach_object_fences(struct drm_gem_object **bos, @@ -240,10 +247,14 @@ int panfrost_job_push(struct panfrost_job *job) job->render_done_fence = dma_fence_get(&job->base.s_fence->finished); - kref_get(&job->refcount); /* put by scheduler job completion */ + ret = panfrost_acquire_object_fences(job->bos, job->bo_count, + &job->deps); + if (ret) { + mutex_unlock(&pfdev->sched_lock); + goto unlock; + } - panfrost_acquire_object_fences(job->bos, job->bo_count, - job->implicit_fences); + kref_get(&job->refcount); /* put by scheduler job completion */ drm_sched_entity_push_job(&job->base, entity); @@ -262,18 +273,15 @@ static void panfrost_job_cleanup(struct kref *ref) { struct panfrost_job *job = container_of(ref, struct panfrost_job, refcount); + struct dma_fence *fence; + unsigned long index; unsigned int i; - if (job->in_fences) { - for (i = 0; i < job->in_fence_count; i++) - dma_fence_put(job->in_fences[i]); - kvfree(job->in_fences); - } - if (job->implicit_fences) { - for (i = 0; i < job->bo_count; i++) - dma_fence_put(job->implicit_fences[i]); - kvfree(job->implicit_fences); + xa_for_each(&job->deps, index, fence) { + dma_fence_put(fence); } + xa_destroy(&job->deps); + dma_fence_put(job->done_fence); dma_fence_put(job->render_done_fence); @@ -316,26 +324,9 @@ static struct dma_fence *panfrost_job_dependency(struct drm_sched_job *sched_job struct drm_sched_entity *s_entity) { struct panfrost_job *job = to_panfrost_job(sched_job); - struct dma_fence *fence; - unsigned int i; - - /* Explicit fences */ - for (i = 0; i < job->in_fence_count; i++) { - if (job->in_fences[i]) { - fence = job->in_fences[i]; - job->in_fences[i] = NULL; - return fence; - } - } - /* Implicit fences, max. one per BO */ - for (i = 0; i < job->bo_count; i++) { - if (job->implicit_fences[i]) { - fence = job->implicit_fences[i]; - job->implicit_fences[i] = NULL; - return fence; - } - } + if (!xa_empty(&job->deps)) + return xa_erase(&job->deps, job->last_dep++); return NULL; } diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h index bbd3ba97ff67..82306a03b57e 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.h +++ b/drivers/gpu/drm/panfrost/panfrost_job.h @@ -19,9 +19,9 @@ struct panfrost_job { struct panfrost_device *pfdev; struct panfrost_file_priv *file_priv; - /* Optional fences userspace can pass in for the job to depend on. */ - struct dma_fence **in_fences; - u32 in_fence_count; + /* Contains both explicit and implicit fences */ + struct xarray deps; + unsigned long last_dep; /* Fence to be signaled by IRQ handler when the job is complete. */ struct dma_fence *done_fence; @@ -30,8 +30,6 @@ struct panfrost_job { __u32 requirements; __u32 flush_id; - /* Exclusive fences we have taken from the BOs to wait for */ - struct dma_fence **implicit_fences; struct panfrost_gem_mapping **mappings; struct drm_gem_object **bos; u32 bo_count; -- 2.32.0.rc2

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Christian König

Am 22.06.21 um 17:40 schrieb Oded Gabbay: > On Tue, Jun 22, 2021 at 6:31 PM Christian König > <christian.koenig(a)amd.com> wrote: >> >> >> Am 22.06.21 um 17:28 schrieb Jason Gunthorpe: >>> On Tue, Jun 22, 2021 at 05:24:08PM +0200, Christian König wrote: >>> >>>>>> I will take two GAUDI devices and use one as an exporter and one as an >>>>>> importer. I want to see that the solution works end-to-end, with real >>>>>> device DMA from importer to exporter. >>>>> I can tell you it doesn't. Stuffing physical addresses directly into >>>>> the sg list doesn't involve any of the IOMMU code so any configuration >>>>> that requires IOMMU page table setup will not work. >>>> Sure it does. See amdgpu_vram_mgr_alloc_sgt: >>>> >>>> amdgpu_res_first(res, offset, length, &cursor); >>> ^^^^^^^^^^ >>> >>> I'm not talking about the AMD driver, I'm talking about this patch. >>> >>> + bar_address = hdev->dram_pci_bar_start + >>> + (pages[cur_page] - prop->dram_base_address); >>> + sg_dma_address(sg) = bar_address; >> Yeah, that is indeed not working. >> >> Oded you need to use dma_map_resource() for this. >> >> Christian. > Yes, of course. > But will it be enough ? > Jason said that supporting IOMMU isn't nice when we don't have struct pages. > I fail to understand the connection, I need to dig into this. Question is what you want to do with this? A struct page is always needed if you want to do stuff like HMM with it, if you only want P2P between device I actually recommend to avoid it. Christian. > > Oded > >> >> >>> Jason

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Tue, Jun 22, 2021 at 06:24:28PM +0300, Oded Gabbay wrote: > On Tue, Jun 22, 2021 at 6:11 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > On Tue, Jun 22, 2021 at 04:12:26PM +0300, Oded Gabbay wrote: > > > > > > 1) Setting sg_page to NULL > > > > 2) 'mapping' pages for P2P DMA without going through the iommu > > > > 3) Allowing P2P DMA without using the p2p dma API to validate that it > > > > can work at all in the first place. > > > > > > > > All of these result in functional bugs in certain system > > > > configurations. > > > > > > > > Jason > > > > > > Hi Jason, > > > Thanks for the feedback. > > > Regarding point 1, why is that a problem if we disable the option to > > > mmap the dma-buf from user-space ? > > > > Userspace has nothing to do with needing struct pages or not > > > > Point 1 and 2 mostly go together, you supporting the iommu is not nice > > if you dont have struct pages. > > > > You should study Logan's patches I pointed you at as they are solving > > exactly this problem. > Yes, I do need to study them. I agree with you here. It appears I > have a hole in my understanding. I'm missing the connection between > iommu support (which I must have of course) and struct pages. Chistian explained what the AMD driver is doing by calling dma_map_resource(). Which is a hacky and slow way of achieving what Logan's series is doing. > > No, the design of the dmabuf requires the exporter to do the dma maps > > and so it is only the exporter that is wrong to omit all the iommu and > > p2p logic. > > > > RDMA is OK today only because nobody has implemented dma buf support > > in rxe/si - mainly because the only implementations of exporters don't > > Can you please educate me, what is rxe/si ? Sorry, rxe/siw - these are the all-software implementations of RDMA and they require the struct page to do a SW memory copy. They can't implement dmabuf without it. > ok... > so how come that patch-set was merged into 5.12 if it's buggy ? We only implemented true dma devices for RDMA DMABUF support, so it is isn't buggy right now. > Yes, that's what I expect to see. But I want to see it with my own > eyes and then figure out how to solve this. It might be tricky to test because you have to ensure the iommu is turned on and has a non-idenity page table. Basically if it doesn't trigger a IOMMU failure then the IOMMU isn't setup properly. Jason

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Tue, Jun 22, 2021 at 04:12:26PM +0300, Oded Gabbay wrote: > > 1) Setting sg_page to NULL > > 2) 'mapping' pages for P2P DMA without going through the iommu > > 3) Allowing P2P DMA without using the p2p dma API to validate that it > > can work at all in the first place. > > > > All of these result in functional bugs in certain system > > configurations. > > > > Jason > > Hi Jason, > Thanks for the feedback. > Regarding point 1, why is that a problem if we disable the option to > mmap the dma-buf from user-space ? Userspace has nothing to do with needing struct pages or not Point 1 and 2 mostly go together, you supporting the iommu is not nice if you dont have struct pages. You should study Logan's patches I pointed you at as they are solving exactly this problem. > In addition, I didn't see any problem with sg_page being NULL in the > RDMA p2p dma-buf code. Did I miss something here ? No, the design of the dmabuf requires the exporter to do the dma maps and so it is only the exporter that is wrong to omit all the iommu and p2p logic. RDMA is OK today only because nobody has implemented dma buf support in rxe/si - mainly because the only implementations of exporters don't set the struct page and are thus buggy. > I will take two GAUDI devices and use one as an exporter and one as an > importer. I want to see that the solution works end-to-end, with real > device DMA from importer to exporter. I can tell you it doesn't. Stuffing physical addresses directly into the sg list doesn't involve any of the IOMMU code so any configuration that requires IOMMU page table setup will not work. Jason

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Tue, Jun 22, 2021 at 03:04:30PM +0300, Oded Gabbay wrote: > On Tue, Jun 22, 2021 at 3:01 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > On Tue, Jun 22, 2021 at 11:42:27AM +0300, Oded Gabbay wrote: > > > On Tue, Jun 22, 2021 at 9:37 AM Christian König > > > <ckoenig.leichtzumerken(a)gmail.com> wrote: > > > > > > > > Am 22.06.21 um 01:29 schrieb Jason Gunthorpe: > > > > > On Mon, Jun 21, 2021 at 10:24:16PM +0300, Oded Gabbay wrote: > > > > > > > > > >> Another thing I want to emphasize is that we are doing p2p only > > > > >> through the export/import of the FD. We do *not* allow the user to > > > > >> mmap the dma-buf as we do not support direct IO. So there is no access > > > > >> to these pages through the userspace. > > > > > Arguably mmaping the memory is a better choice, and is the direction > > > > > that Logan's series goes in. Here the use of DMABUF was specifically > > > > > designed to allow hitless revokation of the memory, which this isn't > > > > > even using. > > > > > > > > The major problem with this approach is that DMA-buf is also used for > > > > memory which isn't CPU accessible. > > > > That isn't an issue here because the memory is only intended to be > > used with P2P transfers so it must be CPU accessible. > > > > > > That was one of the reasons we didn't even considered using the mapping > > > > memory approach for GPUs. > > > > Well, now we have DEVICE_PRIVATE memory that can meet this need > > too.. Just nobody has wired it up to hmm_range_fault() > > > > > > > So you are taking the hit of very limited hardware support and reduced > > > > > performance just to squeeze into DMABUF.. > > > > > > Thanks Jason for the clarification, but I honestly prefer to use > > > DMA-BUF at the moment. > > > It gives us just what we need (even more than what we need as you > > > pointed out), it is *already* integrated and tested in the RDMA > > > subsystem, and I'm feeling comfortable using it as I'm somewhat > > > familiar with it from my AMD days. > > > > You still have the issue that this patch is doing all of this P2P > > stuff wrong - following the already NAK'd AMD approach. > > Could you please point me exactly to the lines of code that are wrong > in your opinion ? 1) Setting sg_page to NULL 2) 'mapping' pages for P2P DMA without going through the iommu 3) Allowing P2P DMA without using the p2p dma API to validate that it can work at all in the first place. All of these result in functional bugs in certain system configurations. Jason

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Mon, Jun 21, 2021 at 10:24:16PM +0300, Oded Gabbay wrote: > Another thing I want to emphasize is that we are doing p2p only > through the export/import of the FD. We do *not* allow the user to > mmap the dma-buf as we do not support direct IO. So there is no access > to these pages through the userspace. Arguably mmaping the memory is a better choice, and is the direction that Logan's series goes in. Here the use of DMABUF was specifically designed to allow hitless revokation of the memory, which this isn't even using. So you are taking the hit of very limited hardware support and reduced performance just to squeeze into DMABUF.. Jason

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Mon, Jun 21, 2021 at 07:26:14PM +0300, Oded Gabbay wrote: > On Mon, Jun 21, 2021 at 5:12 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > On Mon, Jun 21, 2021 at 03:02:10PM +0200, Greg KH wrote: > > > On Mon, Jun 21, 2021 at 02:28:48PM +0200, Daniel Vetter wrote: > > > > > > Also I'm wondering which is the other driver that we share buffers > > > > with. The gaudi stuff doesn't have real struct pages as backing > > > > storage, it only fills out the dma_addr_t. That tends to blow up with > > > > other drivers, and the only place where this is guaranteed to work is > > > > if you have a dynamic importer which sets the allow_peer2peer flag. > > > > Adding maintainers from other subsystems who might want to chime in > > > > here. So even aside of the big question as-is this is broken. > > > > > > From what I can tell this driver is sending the buffers to other > > > instances of the same hardware, > > > > A dmabuf is consumed by something else in the kernel calling > > dma_buf_map_attachment() on the FD. > > > > What is the other side of this? I don't see any > > dma_buf_map_attachment() calls in drivers/misc, or added in this patch > > set. > > This patch-set is only to enable the support for the exporter side. > The "other side" is any generic RDMA networking device that will want > to perform p2p communication over PCIe with our GAUDI accelerator. > An example is indeed the mlnx5 card which has already integrated > support for being an "importer". It raises the question of how you are testing this if you aren't using it with the only intree driver: mlx5. Jason

3 years, 6 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig June 2021