Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

24 participants
2727 discussions

[PATCH v2 04/11] drm/panfrost: use scheduler dependency tracking

by Daniel Vetter

Just deletes some code that's now more shared. Note that thanks to the split into drm_sched_job_init/arm we can now easily pull the _init() part from under the submission lock way ahead where we're adding the sync file in-fences as dependencies. v2: Correctly clean up the partially set up job, now that job_init() and job_arm() are apart (Emma). Reviewed-by: Steven Price <steven.price(a)arm.com> (v1) 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_drv.c | 16 ++++++++--- drivers/gpu/drm/panfrost/panfrost_job.c | 37 +++---------------------- drivers/gpu/drm/panfrost/panfrost_job.h | 5 +--- 3 files changed, 17 insertions(+), 41 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 1ffaef5ec5ff..9f53bea07d61 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -218,7 +218,7 @@ panfrost_copy_in_sync(struct drm_device *dev, if (ret) goto fail; - ret = drm_gem_fence_array_add(&job->deps, fence); + ret = drm_sched_job_await_fence(&job->base, fence); if (ret) goto fail; @@ -236,7 +236,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, struct drm_panfrost_submit *args = data; struct drm_syncobj *sync_out = NULL; struct panfrost_job *job; - int ret = 0; + int ret = 0, slot; if (!args->jc) return -EINVAL; @@ -258,14 +258,20 @@ 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; job->flush_id = panfrost_gpu_get_latest_flush_id(pfdev); job->file_priv = file->driver_priv; + slot = panfrost_job_get_slot(job); + + ret = drm_sched_job_init(&job->base, + &job->file_priv->sched_entity[slot], + NULL); + if (ret) + goto fail_job_put; + ret = panfrost_copy_in_sync(dev, file, args, job); if (ret) goto fail_job; @@ -283,6 +289,8 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, drm_syncobj_replace_fence(sync_out, job->render_done_fence); fail_job: + drm_sched_job_cleanup(&job->base); +fail_job_put: panfrost_job_put(job); fail_out_sync: if (sync_out) diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 4bc962763e1f..86c843d8822e 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -102,7 +102,7 @@ static struct dma_fence *panfrost_fence_create(struct panfrost_device *pfdev, in return &fence->base; } -static int panfrost_job_get_slot(struct panfrost_job *job) +int panfrost_job_get_slot(struct panfrost_job *job) { /* JS0: fragment jobs. * JS1: vertex/tiler jobs @@ -242,13 +242,13 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js) static int panfrost_acquire_object_fences(struct drm_gem_object **bos, int bo_count, - struct xarray *deps) + struct drm_sched_job *job) { int i, ret; for (i = 0; i < bo_count; i++) { /* panfrost always uses write mode in its current uapi */ - ret = drm_gem_fence_array_add_implicit(deps, bos[i], true); + ret = drm_sched_job_await_implicit(job, bos[i], true); if (ret) return ret; } @@ -269,31 +269,21 @@ static void panfrost_attach_object_fences(struct drm_gem_object **bos, int panfrost_job_push(struct panfrost_job *job) { struct panfrost_device *pfdev = job->pfdev; - int slot = panfrost_job_get_slot(job); - struct drm_sched_entity *entity = &job->file_priv->sched_entity[slot]; struct ww_acquire_ctx acquire_ctx; int ret = 0; - ret = drm_gem_lock_reservations(job->bos, job->bo_count, &acquire_ctx); if (ret) return ret; mutex_lock(&pfdev->sched_lock); - - ret = drm_sched_job_init(&job->base, entity, NULL); - if (ret) { - mutex_unlock(&pfdev->sched_lock); - goto unlock; - } - drm_sched_job_arm(&job->base); job->render_done_fence = dma_fence_get(&job->base.s_fence->finished); ret = panfrost_acquire_object_fences(job->bos, job->bo_count, - &job->deps); + &job->base); if (ret) { mutex_unlock(&pfdev->sched_lock); goto unlock; @@ -318,15 +308,8 @@ 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; - 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); @@ -365,17 +348,6 @@ static void panfrost_job_free(struct drm_sched_job *sched_job) panfrost_job_put(job); } -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); - - if (!xa_empty(&job->deps)) - return xa_erase(&job->deps, job->last_dep++); - - return NULL; -} - static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job) { struct panfrost_job *job = to_panfrost_job(sched_job); @@ -765,7 +737,6 @@ static void panfrost_reset_work(struct work_struct *work) } static const struct drm_sched_backend_ops panfrost_sched_ops = { - .dependency = panfrost_job_dependency, .run_job = panfrost_job_run, .timedout_job = panfrost_job_timedout, .free_job = panfrost_job_free diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h index 82306a03b57e..77e6d0e6f612 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.h +++ b/drivers/gpu/drm/panfrost/panfrost_job.h @@ -19,10 +19,6 @@ struct panfrost_job { struct panfrost_device *pfdev; struct panfrost_file_priv *file_priv; - /* 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; @@ -42,6 +38,7 @@ int panfrost_job_init(struct panfrost_device *pfdev); void panfrost_job_fini(struct panfrost_device *pfdev); int panfrost_job_open(struct panfrost_file_priv *panfrost_priv); void panfrost_job_close(struct panfrost_file_priv *panfrost_priv); +int panfrost_job_get_slot(struct panfrost_job *job); int panfrost_job_push(struct panfrost_job *job); void panfrost_job_put(struct panfrost_job *job); void panfrost_job_enable_interrupts(struct panfrost_device *pfdev); -- 2.32.0.rc2

3 years, 9 months

Re: [Linaro-mm-sig] [PATCH v7 0/5] drm: address potential UAF bugs with drm_master ptrs

by Daniel Vetter

On Fri, Jul 02, 2021 at 12:53:53AM +0800, Desmond Cheong Zhi Xi wrote: > This patch series addresses potential use-after-free errors when dereferencing pointers to struct drm_master. These were identified after one such bug was caught by Syzbot in drm_getunique(): > https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f8… > > The series is broken up into five patches: > > 1. Move a call to drm_is_current_master() out from a section locked by &dev->mode_config.mutex in drm_mode_getconnector(). This patch does not apply to stable. > > 2. Move a call to _drm_lease_held() out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find(). > > 3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c. > > 4. Serialize drm_file.master by introducing a new lock that's held whenever the value of drm_file.master changes. > > 5. Identify areas in drm_lease.c where pointers to struct drm_master are dereferenced, and ensure that the master pointers are not freed during use. > > Changes in v6 -> v7: > - Patch 2: > Modify code alignment as suggested by the intel-gfx CI. > > Update commit message based on the changes to patch 5. > > - Patch 4: > Add patch 4 to the series. This patch adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI. > > - Patch 5: > Move kerneldoc comment about protecting drm_file.master with drm_device.master_mutex into patch 4. > > Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI. So there's another one now because master->leases is protected by the mode_config.idr_mutex, and that's a bit awkward to untangle. Also I'm really surprised that there was now lockdep through the atomic code anywhere. The reason seems to be that somehow CI reboot first before it managed to run any of the kms_atomic tests, and we can only hit this when we go through the atomic kms ioctl, the legacy kms ioctl don't have that specific issue. Anyway I think this approach doesn't look too workable, and we need something new. But first things first: Are you still on board working on this? You started with a simple patch to fix a UAF bug, now we're deep into reworking tricky locking ... If you feel like you want out I'm totally fine with that. Anyway, I think we need to split drm_device->master_mutex up into two parts: - One part that protects the actual access/changes, which I think for simplicity we'll just leave as the current lock. That lock is a very inner lock, since for the drm_lease.c stuff it has to nest within mode_config.idr_mutex even. - Now the issue with checking master status/leases/whatever as an innermost lock is that you can race, it's a classic time of check vs time of use race: By the time we actually use the thing we validate we'er allowed to use, we might now have access anymore. There's two reasons for that: * DROPMASTER ioctl could remove the master rights, which removes access rights also for all leases * REVOKE_LEASE ioctl can do the same but only for a specific lease This is the thing we're trying to protect against in fbcon code, but that's very spotty protection because all the ioctls by other users aren't actually protected against this. So I think for this we need some kind of big reader lock. Now for the implementation, there's a few things: - I think best option for this big reader lock would be to just use srcu. We only need to flush out all current readers when we drop master or revoke a lease, so synchronize_srcu is perfectly good enough for this purpose. - The fbdev code would switch over to srcu in drm_master_internal_acquire() and drm_master_internal_release(). Ofc within drm_master_internal_acquire we'd still need to check master status with the normal master_mutex. - While we revamp all this we should fix the ioctl checks in drm_ioctl.c. Just noticed that drm_ioctl_permit() could and should be unexported, last user was removed. Within drm_ioctl_kernel we'd then replace the check for drm_is_current_master with the drm_master_internal_acquire/release. - This alone does nothing, we still need to make sure that dropmaster and revoke_lease ioctl flush out all other access before they return to userspace. We can't just call synchronize_srcu because due to the ioctl code in drm_ioctl_kernel we're in that sruc section, we'd need to add a DRM_MASTER_FLUSH ioctl flag which we'd check only when DRM_MASTER is set, and use to call synchronize_srcu. Maybe wrap that in a drm_master_flush or so, or perhaps a drm_master_internal_release_flush. - Also maybe we should drop the _internal_ from that name. Feels a bit wrong when we're also going to use this in the ioctl handler. Thoughts? Totally silly and overkill? Cheers, Daniel > Changes in v5 -> v6: > - Patch 2: > Add patch 2 to the series. This patch moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. > > - Patch 5: > Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter. > > Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov. > > Modify comparison to NULL into "!master", as suggested by the intel-gfx CI. > > Changes in v4 -> v5: > - Patch 1: > Add patch 1 to the series. The changes in patch 1 do not apply to stable because they apply to new changes in the drm-misc-next branch. This patch moves the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. > > Additionally, added a missing semicolon to the patch, caught by the intel-gfx CI. > > - Patch 3: > Move changes to drm_connector.c into patch 1. > > Changes in v3 -> v4: > - Patch 3: > Move the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. As suggested by Daniel Vetter. This avoids a circular lock lock dependency as reported here https://patchwork.freedesktop.org/patch/440406/ > > Additionally, inside drm_is_current_master, instead of grabbing &fpriv->master->dev->master_mutex, we grab &fpriv->minor->dev->master_mutex to avoid dereferencing a null ptr if fpriv->master is not set. > > - Patch 5: > Modify kerneldoc formatting. > > Additionally, add a file_priv->master NULL check inside drm_file_get_master, and handle the NULL result accordingly in drm_lease.c. As suggested by Daniel Vetter. > > Changes in v2 -> v3: > - Patch 3: > Move the definition of drm_is_current_master and the _locked version higher up in drm_auth.c to avoid needing a forward declaration of drm_is_current_master_locked. As suggested by Daniel Vetter. > > - Patch 5: > Instead of leaking drm_device.master_mutex into drm_lease.c to protect drm_master pointers, add a new drm_file_get_master() function that returns drm_file->master while increasing its reference count, to prevent drm_file->master from being freed. As suggested by Daniel Vetter. > > Changes in v1 -> v2: > - Patch 5: > Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov. > > Desmond Cheong Zhi Xi (5): > drm: avoid circular locks in drm_mode_getconnector > drm: separate locks in __drm_mode_object_find > drm: add a locked version of drm_is_current_master > drm: serialize drm_file.master with a master lock > drm: protect drm_master pointers in drm_lease.c > > drivers/gpu/drm/drm_auth.c | 86 +++++++++++++++++++++++-------- > drivers/gpu/drm/drm_connector.c | 5 +- > drivers/gpu/drm/drm_file.c | 1 + > drivers/gpu/drm/drm_lease.c | 81 ++++++++++++++++++++++------- > drivers/gpu/drm/drm_mode_object.c | 10 ++-- > include/drm/drm_auth.h | 1 + > include/drm/drm_file.h | 18 +++++-- > 7 files changed, 153 insertions(+), 49 deletions(-) > > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 9 months

Re: [Linaro-mm-sig] [PATCH V2] treewide: Add missing semicolons to __assign_str uses

by Steven Rostedt

On Wed, 30 Jun 2021 04:28:39 -0700 Joe Perches <joe(a)perches.com> wrote: > On Sat, 2021-06-12 at 08:42 -0700, Joe Perches wrote: > > The __assign_str macro has an unusual ending semicolon but the vast > > majority of uses of the macro already have semicolon termination. > > ping? > I wasn't sure I was the one to take this. I can, as I can run tests on it as well. I have some last minute fixes sent to me on something else, and I can apply this along with them. -- Steve

3 years, 10 months

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

by Daniel Vetter

On Wed, Jun 30, 2021 at 9:18 AM Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> wrote: > > On 30/6/21 12:07 am, Daniel Vetter wrote: > > On Tue, Jun 29, 2021 at 11:37:06AM +0800, Desmond Cheong Zhi Xi wrote: > >> Currently, direct copies of drm_file->master pointers should be > >> protected by drm_device.master_mutex when being dereferenced. This is > >> because drm_file->master is not invariant for the lifetime of > >> drm_file. If drm_file is not the creator of master, then > >> drm_file->is_master is false, and a call to drm_setmaster_ioctl will > >> invoke drm_new_set_master, which then allocates a new master for > >> drm_file and puts the old master. > >> > >> Thus, without holding drm_device.master_mutex, the old value of > >> drm_file->master could be freed while it is being used by another > >> concurrent process. > >> > >> In drm_lease.c, there are multiple instances where drm_file->master is > >> accessed and dereferenced while drm_device.master_mutex is not > >> held. This makes drm_lease.c vulnerable to use-after-free bugs. > >> > >> We address this issue in 3 ways: > >> > >> 1. Clarify in the kerneldoc that drm_file->master is protected by > >> drm_device.master_mutex. > >> > >> 2. Add a new drm_file_get_master() function that calls drm_master_get > >> on drm_file->master while holding on to drm_device.master_mutex. Since > >> drm_master_get increments the reference count of master, this > >> prevents master from being freed until we unreference it with > >> drm_master_put. > >> > >> 3. In each case where drm_file->master is directly accessed and > >> eventually dereferenced in drm_lease.c, we wrap the access in a call > >> to the new drm_file_get_master function, then unreference the master > >> pointer once we are done using it. > >> > >> Reported-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> > > > > Series looks very nice, let's see what intel-gfx-ci says. You should get a > > mail, but results are also here: > > > > https://patchwork.freedesktop.org/series/91969/#rev2 > > > > One tiny comment below. > > > >> --- > >> drivers/gpu/drm/drm_auth.c | 25 ++++++++++++ > >> drivers/gpu/drm/drm_lease.c | 77 +++++++++++++++++++++++++++---------- > >> include/drm/drm_auth.h | 1 + > >> include/drm/drm_file.h | 15 ++++++-- > >> 4 files changed, 95 insertions(+), 23 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > >> index ab1863c5a5a0..c36a0b72be26 100644 > >> --- a/drivers/gpu/drm/drm_auth.c > >> +++ b/drivers/gpu/drm/drm_auth.c > >> @@ -384,6 +384,31 @@ struct drm_master *drm_master_get(struct drm_master *master) > >> } > >> EXPORT_SYMBOL(drm_master_get); > >> > >> +/** > >> + * drm_file_get_master - reference &drm_file.master of @file_priv > >> + * @file_priv: DRM file private > >> + * > >> + * Increments the reference count of @file_priv's &drm_file.master and returns > >> + * the &drm_file.master. If @file_priv has no &drm_file.master, returns NULL. > >> + * > >> + * 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 = NULL; > >> + > >> + mutex_lock(&file_priv->minor->dev->master_mutex); > >> + if (!file_priv->master) > >> + goto unlock; > >> + master = drm_master_get(file_priv->master); > >> + > >> +unlock: > >> + mutex_unlock(&file_priv->minor->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 00fb433bcef1..cdcc87fa9685 100644 > >> --- a/drivers/gpu/drm/drm_lease.c > >> +++ b/drivers/gpu/drm/drm_lease.c > >> @@ -106,10 +106,19 @@ static bool _drm_has_leased(struct drm_master *master, int id) > >> */ > >> bool _drm_lease_held(struct drm_file *file_priv, int id) > >> { > >> - if (!file_priv || !file_priv->master) > >> + bool ret; > >> + struct drm_master *master; > >> + > >> + if (!file_priv) > >> return true; > >> > >> - return _drm_lease_held_master(file_priv->master, id); > >> + master = drm_file_get_master(file_priv); > >> + if (master == NULL) > >> + return true; > >> + ret = _drm_lease_held_master(master, id); > >> + drm_master_put(&master); > >> + > >> + return ret; > >> } > >> > >> /** > >> @@ -128,13 +137,20 @@ bool drm_lease_held(struct drm_file *file_priv, int id) > >> struct drm_master *master; > >> bool ret; > >> > >> - if (!file_priv || !file_priv->master || !file_priv->master->lessor) > >> + if (!file_priv) > >> return true; > >> > >> - master = file_priv->master; > >> + master = drm_file_get_master(file_priv); > >> + if (master == NULL) > >> + return true; > >> + if (!master->lessor) { > >> + drm_master_put(&master); > >> + return true; > >> + } > >> 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; > >> } > >> > >> @@ -154,10 +170,16 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) > >> int count_in, count_out; > >> uint32_t crtcs_out = 0; > >> > >> - if (!file_priv || !file_priv->master || !file_priv->master->lessor) > >> + if (!file_priv) > >> return crtcs_in; > >> > >> - master = file_priv->master; > >> + master = drm_file_get_master(file_priv); > >> + if (master == NULL) > >> + return crtcs_in; > >> + if (!master->lessor) { > >> + drm_master_put(&master); > >> + return crtcs_in; > >> + } > >> dev = master->dev; > >> > >> count_in = count_out = 0; > >> @@ -176,6 +198,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; > >> } > >> > >> @@ -489,7 +512,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; > >> @@ -501,12 +524,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"); > >> @@ -518,12 +535,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) { > >> + 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); > >> > >> @@ -534,14 +561,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"); > >> @@ -577,6 +605,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; > >> > >> @@ -586,6 +615,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; > >> } > >> @@ -608,7 +639,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; > >> > >> @@ -619,6 +650,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); > >> @@ -642,6 +674,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; > >> } > >> @@ -661,7 +694,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; > >> @@ -675,6 +708,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); > >> @@ -702,6 +736,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; > >> } > >> @@ -720,7 +755,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; > >> > >> @@ -730,6 +765,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); > >> @@ -750,6 +786,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 > > > > I found this a bit confusing, since I generally don't think of > > dereferencing the pointer as "taking a copy". That's more for the entire > > datastructure when you have a memcpy() call, or kmemdup() or something > > like that. Also "it is recommended" is a bit weak if you get a > > use-after-free if you dont :-) > > > > So instead "When dererencing this pointer either hold ... or use > > drm_file_get_master() ...." > > > > Cheers, Daniel > > > >> + * 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 > >> > > > > Hi Daniel, > > Thanks for the suggestion, I'll clarify the kerneldoc accordingly. > > Regarding the results from intel-gfx-ci, it seems that the patch is > inverting the lock hierarchy for > &dev->master_mutex --> &dev->mode_config.idr_mutex > > Currently the dmesg warnings share a common call trace: > drm_file_get_master+0x1b/0x70 > _drm_lease_held+0x21/0x70 > __drm_mode_object_find+0xd1/0xe0 > > Looking at the functions that lock &dev->mode_config.idr_mutex, this > should be the only instance of this lock order inversion. > > I'm thinking the call to _drm_lease_held can be moved outside of the > &dev->mode_config.idr_mutex lock in __drm_mode_object. Any thoughts? Uh very annoying. One of the callers of this is the atomic ioctl, where we're calling this while holding drm_modeset_lock. The nesting hierarchy is, from outermost lock to innermost: dev->master_mutex -> dev->mode_config.mutex -> drm_modeset_lock. So I think we'll again have an inversion, just moved it a bit :-( But I'm also not 100% sure, so maybe type it up and see what happens? Otherwise I think we need to figure out a solution for how we can check leases without having to take the dev->master_mutex that serializes a lot more things ... I think the fundamental problem we have here is that dev->master_mutex serves 2 purposes: a) protecting the pointers and just data consistency and b) synchronizing against concurrent master changes where we think that's required. It's the latter (through the fbdev emulation code) that causes all the inversion problems, a) it's could easily nest very deeply in other locks. > diff --git a/drivers/gpu/drm/drm_mode_object.c > b/drivers/gpu/drm/drm_mode_object.c > index b26588b52795..63d35f1f98dd 100644 > --- a/drivers/gpu/drm/drm_mode_object.c > +++ b/drivers/gpu/drm/drm_mode_object.c > @@ -146,16 +146,18 @@ struct drm_mode_object > *__drm_mode_object_find(struct drm_device *dev, > if (obj && obj->id != id) > obj = NULL; > > - if (obj && drm_mode_object_lease_required(obj->type) && > - !_drm_lease_held(file_priv, obj->id)) > - obj = NULL; > - > if (obj && obj->free_cb) { > if (!kref_get_unless_zero(&obj->refcount)) > obj = NULL; > } > mutex_unlock(&dev->mode_config.idr_mutex); > > + if (obj && drm_mode_object_lease_required(obj->type) && > + !_drm_lease_held(file_priv, obj->id)) { > + drm_mode_object_put(obj); > + obj = NULL; > + } Irrespective of all this I think this change here makes sense since in untangels the master stuff from the lookup idr, and that's always good. Maybe do this hunk as a separate patch, and I'll apply that as prep work? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 10 months

[PATCH -next] <linux/dma-resv.h>: correct a function name in kernel-doc

by Randy Dunlap

Fix kernel-doc function name warning: ../include/linux/dma-resv.h:227: warning: expecting prototype for dma_resv_exclusive(). Prototype was for dma_resv_excl_fence() instead Fixes: 6edbd6abb783d ("ma-buf: rename and cleanup dma_resv_get_excl v3") Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org> 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: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org --- include/linux/dma-resv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-next-20210625.orig/include/linux/dma-resv.h +++ linux-next-20210625/include/linux/dma-resv.h @@ -212,7 +212,7 @@ static inline void dma_resv_unlock(struc } /** - * dma_resv_exclusive - return the object's exclusive fence + * dma_resv_excl_fence - return the object's exclusive fence * @obj: the reservation object * * Returns the exclusive fence (if any). Caller must either hold the objects

3 years, 10 months

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

by Daniel Vetter

On Tue, Jun 29, 2021 at 11:37:06AM +0800, Desmond Cheong Zhi Xi wrote: > Currently, direct copies of drm_file->master pointers should be > protected by drm_device.master_mutex when being dereferenced. This is > because drm_file->master is not invariant for the lifetime of > drm_file. If drm_file is not the creator of master, then > drm_file->is_master is false, and a call to drm_setmaster_ioctl will > invoke drm_new_set_master, which then allocates a new master for > drm_file and puts the old master. > > Thus, without holding drm_device.master_mutex, the old value of > drm_file->master could be freed while it is being used by another > concurrent process. > > In drm_lease.c, there are multiple instances where drm_file->master is > accessed and dereferenced while drm_device.master_mutex is not > held. This makes drm_lease.c vulnerable to use-after-free bugs. > > We address this issue in 3 ways: > > 1. Clarify in the kerneldoc that drm_file->master is protected by > drm_device.master_mutex. > > 2. Add a new drm_file_get_master() function that calls drm_master_get > on drm_file->master while holding on to drm_device.master_mutex. Since > drm_master_get increments the reference count of master, this > prevents master from being freed until we unreference it with > drm_master_put. > > 3. In each case where drm_file->master is directly accessed and > eventually dereferenced in drm_lease.c, we wrap the access in a call > to the new drm_file_get_master function, then unreference the master > pointer once we are done using it. > > Reported-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> Series looks very nice, let's see what intel-gfx-ci says. You should get a mail, but results are also here: https://patchwork.freedesktop.org/series/91969/#rev2 One tiny comment below. > --- > drivers/gpu/drm/drm_auth.c | 25 ++++++++++++ > drivers/gpu/drm/drm_lease.c | 77 +++++++++++++++++++++++++++---------- > include/drm/drm_auth.h | 1 + > include/drm/drm_file.h | 15 ++++++-- > 4 files changed, 95 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index ab1863c5a5a0..c36a0b72be26 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -384,6 +384,31 @@ struct drm_master *drm_master_get(struct drm_master *master) > } > EXPORT_SYMBOL(drm_master_get); > > +/** > + * drm_file_get_master - reference &drm_file.master of @file_priv > + * @file_priv: DRM file private > + * > + * Increments the reference count of @file_priv's &drm_file.master and returns > + * the &drm_file.master. If @file_priv has no &drm_file.master, returns NULL. > + * > + * 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 = NULL; > + > + mutex_lock(&file_priv->minor->dev->master_mutex); > + if (!file_priv->master) > + goto unlock; > + master = drm_master_get(file_priv->master); > + > +unlock: > + mutex_unlock(&file_priv->minor->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 00fb433bcef1..cdcc87fa9685 100644 > --- a/drivers/gpu/drm/drm_lease.c > +++ b/drivers/gpu/drm/drm_lease.c > @@ -106,10 +106,19 @@ static bool _drm_has_leased(struct drm_master *master, int id) > */ > bool _drm_lease_held(struct drm_file *file_priv, int id) > { > - if (!file_priv || !file_priv->master) > + bool ret; > + struct drm_master *master; > + > + if (!file_priv) > return true; > > - return _drm_lease_held_master(file_priv->master, id); > + master = drm_file_get_master(file_priv); > + if (master == NULL) > + return true; > + ret = _drm_lease_held_master(master, id); > + drm_master_put(&master); > + > + return ret; > } > > /** > @@ -128,13 +137,20 @@ bool drm_lease_held(struct drm_file *file_priv, int id) > struct drm_master *master; > bool ret; > > - if (!file_priv || !file_priv->master || !file_priv->master->lessor) > + if (!file_priv) > return true; > > - master = file_priv->master; > + master = drm_file_get_master(file_priv); > + if (master == NULL) > + return true; > + if (!master->lessor) { > + drm_master_put(&master); > + return true; > + } > 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; > } > > @@ -154,10 +170,16 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) > int count_in, count_out; > uint32_t crtcs_out = 0; > > - if (!file_priv || !file_priv->master || !file_priv->master->lessor) > + if (!file_priv) > return crtcs_in; > > - master = file_priv->master; > + master = drm_file_get_master(file_priv); > + if (master == NULL) > + return crtcs_in; > + if (!master->lessor) { > + drm_master_put(&master); > + return crtcs_in; > + } > dev = master->dev; > > count_in = count_out = 0; > @@ -176,6 +198,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; > } > > @@ -489,7 +512,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; > @@ -501,12 +524,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"); > @@ -518,12 +535,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) { > + 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); > > @@ -534,14 +561,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"); > @@ -577,6 +605,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; > > @@ -586,6 +615,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; > } > @@ -608,7 +639,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; > > @@ -619,6 +650,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); > @@ -642,6 +674,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; > } > @@ -661,7 +694,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; > @@ -675,6 +708,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); > @@ -702,6 +736,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; > } > @@ -720,7 +755,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; > > @@ -730,6 +765,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); > @@ -750,6 +786,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 I found this a bit confusing, since I generally don't think of dereferencing the pointer as "taking a copy". That's more for the entire datastructure when you have a memcpy() call, or kmemdup() or something like that. Also "it is recommended" is a bit weak if you get a use-after-free if you dont :-) So instead "When dererencing this pointer either hold ... or use drm_file_get_master() ...." Cheers, Daniel > + * 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 10 months

[PATCH 02/11] drm/sched: Add dependency tracking

by Daniel Vetter

Instead of just a callback we can just glue in the gem helpers that panfrost, v3d and lima currently use. There's really not that many ways to skin this cat. On the naming bikeshed: The idea for using _await_ to denote adding dependencies to a job comes from i915, where that's used quite extensively all over the place, in lots of datastructures. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: David Airlie <airlied(a)linux.ie> Cc: Daniel Vetter <daniel(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: Andrey Grodzovsky <andrey.grodzovsky(a)amd.com> Cc: Lee Jones <lee.jones(a)linaro.org> Cc: Nirmoy Das <nirmoy.aiemd(a)gmail.com> Cc: Boris Brezillon <boris.brezillon(a)collabora.com> Cc: Luben Tuikov <luben.tuikov(a)amd.com> Cc: Alex Deucher <alexander.deucher(a)amd.com> Cc: Jack Zhang <Jack.Zhang1(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/gpu/drm/scheduler/sched_entity.c | 18 +++- drivers/gpu/drm/scheduler/sched_main.c | 103 +++++++++++++++++++++++ include/drm/gpu_scheduler.h | 31 ++++++- 3 files changed, 146 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index f7347c284886..b6f72fafd504 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -211,6 +211,19 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f, job->sched->ops->free_job(job); } +static struct dma_fence * +drm_sched_job_dependency(struct drm_sched_job *job, + struct drm_sched_entity *entity) +{ + if (!xa_empty(&job->dependencies)) + return xa_erase(&job->dependencies, job->last_dependency++); + + if (job->sched->ops->dependency) + return job->sched->ops->dependency(job, entity); + + return NULL; +} + /** * drm_sched_entity_kill_jobs - Make sure all remaining jobs are killed * @@ -229,7 +242,7 @@ static void drm_sched_entity_kill_jobs(struct drm_sched_entity *entity) struct drm_sched_fence *s_fence = job->s_fence; /* Wait for all dependencies to avoid data corruptions */ - while ((f = job->sched->ops->dependency(job, entity))) + while ((f = drm_sched_job_dependency(job, entity))) dma_fence_wait(f, false); drm_sched_fence_scheduled(s_fence); @@ -419,7 +432,6 @@ static bool drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity) */ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) { - struct drm_gpu_scheduler *sched = entity->rq->sched; struct drm_sched_job *sched_job; sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue)); @@ -427,7 +439,7 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) return NULL; while ((entity->dependency = - sched->ops->dependency(sched_job, entity))) { + drm_sched_job_dependency(sched_job, entity))) { trace_drm_sched_job_wait_dep(sched_job, entity->dependency); if (drm_sched_entity_add_dependency_cb(entity)) diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 70eefed17e06..370c336d383f 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -603,6 +603,8 @@ int drm_sched_job_init(struct drm_sched_job *job, INIT_LIST_HEAD(&job->list); + xa_init_flags(&job->dependencies, XA_FLAGS_ALLOC); + return 0; } EXPORT_SYMBOL(drm_sched_job_init); @@ -626,6 +628,98 @@ void drm_sched_job_arm(struct drm_sched_job *job) } EXPORT_SYMBOL(drm_sched_job_arm); +/** + * drm_sched_job_await_fence - adds the fence as a job dependency + * @job: scheduler job to add the dependencies to + * @fence: the dma_fence to add to the list of dependencies. + * + * Note that @fence is consumed in both the success and error cases. + * + * Returns: + * 0 on success, or an error on failing to expand the array. + */ +int drm_sched_job_await_fence(struct drm_sched_job *job, + struct dma_fence *fence) +{ + struct dma_fence *entry; + unsigned long index; + u32 id = 0; + int ret; + + if (!fence) + return 0; + + /* Deduplicate if we already depend on a fence from the same context. + * This lets the size of the array of deps scale with the number of + * engines involved, rather than the number of BOs. + */ + xa_for_each(&job->dependencies, index, entry) { + if (entry->context != fence->context) + continue; + + if (dma_fence_is_later(fence, entry)) { + dma_fence_put(entry); + xa_store(&job->dependencies, index, fence, GFP_KERNEL); + } else { + dma_fence_put(fence); + } + return 0; + } + + ret = xa_alloc(&job->dependencies, &id, fence, xa_limit_32b, GFP_KERNEL); + if (ret != 0) + dma_fence_put(fence); + + return ret; +} +EXPORT_SYMBOL(drm_sched_job_await_fence); + +/** + * drm_sched_job_await_implicit - adds implicit dependencies as job dependencies + * @job: scheduler job to add the dependencies to + * @obj: the gem object to add new dependencies from. + * @write: whether the job might write the object (so we need to depend on + * shared fences in the reservation object). + * + * This should be called after drm_gem_lock_reservations() on your array of + * GEM objects used in the job but before updating the reservations with your + * own fences. + * + * Returns: + * 0 on success, or an error on failing to expand the array. + */ +int drm_sched_job_await_implicit(struct drm_sched_job *job, + struct drm_gem_object *obj, + bool write) +{ + int ret; + struct dma_fence **fences; + unsigned int i, fence_count; + + if (!write) { + struct dma_fence *fence = dma_resv_get_excl_unlocked(obj->resv); + + return drm_sched_job_await_fence(job, fence); + } + + ret = dma_resv_get_fences(obj->resv, NULL, &fence_count, &fences); + if (ret || !fence_count) + return ret; + + for (i = 0; i < fence_count; i++) { + ret = drm_sched_job_await_fence(job, fences[i]); + if (ret) + break; + } + + for (; i < fence_count; i++) + dma_fence_put(fences[i]); + kfree(fences); + return ret; +} +EXPORT_SYMBOL(drm_sched_job_await_implicit); + + /** * drm_sched_job_cleanup - clean up scheduler job resources * @@ -633,8 +727,17 @@ EXPORT_SYMBOL(drm_sched_job_arm); */ void drm_sched_job_cleanup(struct drm_sched_job *job) { + struct dma_fence *fence; + unsigned long index; + dma_fence_put(&job->s_fence->finished); job->s_fence = NULL; + + xa_for_each(&job->dependencies, index, fence) { + dma_fence_put(fence); + } + xa_destroy(&job->dependencies); + } EXPORT_SYMBOL(drm_sched_job_cleanup); diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 80438d126c9d..e4d7e1496296 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -27,9 +27,12 @@ #include <drm/spsc_queue.h> #include <linux/dma-fence.h> #include <linux/completion.h> +#include <linux/xarray.h> #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000) +struct drm_gem_object; + struct drm_gpu_scheduler; struct drm_sched_rq; @@ -198,6 +201,16 @@ struct drm_sched_job { enum drm_sched_priority s_priority; struct drm_sched_entity *entity; struct dma_fence_cb cb; + /** + * @dependencies: + * + * Contains the dependencies as struct dma_fence for this job, see + * drm_sched_job_await_fence() and drm_sched_job_await_implicit(). + */ + struct xarray dependencies; + + /** @last_dependency: tracks @dependencies as they signal */ + unsigned long last_dependency; }; static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job, @@ -220,9 +233,14 @@ enum drm_gpu_sched_stat { */ struct drm_sched_backend_ops { /** - * @dependency: Called when the scheduler is considering scheduling - * this job next, to get another struct dma_fence for this job to - * block on. Once it returns NULL, run_job() may be called. + * @dependency: + * + * Called when the scheduler is considering scheduling this job next, to + * get another struct dma_fence for this job to block on. Once it + * returns NULL, run_job() may be called. + * + * If a driver exclusively uses drm_sched_job_await_fence() and + * drm_sched_job_await_implicit() this can be ommitted and left as NULL. */ struct dma_fence *(*dependency)(struct drm_sched_job *sched_job, struct drm_sched_entity *s_entity); @@ -314,6 +332,13 @@ int drm_sched_job_init(struct drm_sched_job *job, struct drm_sched_entity *entity, void *owner); void drm_sched_job_arm(struct drm_sched_job *job); +int drm_sched_job_await_fence(struct drm_sched_job *job, + struct dma_fence *fence); +int drm_sched_job_await_implicit(struct drm_sched_job *job, + struct drm_gem_object *obj, + bool write); + + void drm_sched_entity_modify_sched(struct drm_sched_entity *entity, struct drm_gpu_scheduler **sched_list, unsigned int num_sched_list); -- 2.32.0.rc2

3 years, 10 months

[PATCH 09/11] drm/gem: Delete gem array fencing helpers

by Daniel Vetter

Integrated into the scheduler now and all users converted over. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: David Airlie <airlied(a)linux.ie> Cc: Daniel Vetter <daniel(a)ffwll.ch> 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/drm_gem.c | 96 --------------------------------------- include/drm/drm_gem.h | 5 -- 2 files changed, 101 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 68deb1de8235..24d49a2636e0 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1294,99 +1294,3 @@ drm_gem_unlock_reservations(struct drm_gem_object **objs, int count, ww_acquire_fini(acquire_ctx); } EXPORT_SYMBOL(drm_gem_unlock_reservations); - -/** - * drm_gem_fence_array_add - Adds the fence to an array of fences to be - * waited on, deduplicating fences from the same context. - * - * @fence_array: array of dma_fence * for the job to block on. - * @fence: the dma_fence to add to the list of dependencies. - * - * This functions consumes the reference for @fence both on success and error - * cases. - * - * Returns: - * 0 on success, or an error on failing to expand the array. - */ -int drm_gem_fence_array_add(struct xarray *fence_array, - struct dma_fence *fence) -{ - struct dma_fence *entry; - unsigned long index; - u32 id = 0; - int ret; - - if (!fence) - return 0; - - /* Deduplicate if we already depend on a fence from the same context. - * This lets the size of the array of deps scale with the number of - * engines involved, rather than the number of BOs. - */ - xa_for_each(fence_array, index, entry) { - if (entry->context != fence->context) - continue; - - if (dma_fence_is_later(fence, entry)) { - dma_fence_put(entry); - xa_store(fence_array, index, fence, GFP_KERNEL); - } else { - dma_fence_put(fence); - } - return 0; - } - - ret = xa_alloc(fence_array, &id, fence, xa_limit_32b, GFP_KERNEL); - if (ret != 0) - dma_fence_put(fence); - - return ret; -} -EXPORT_SYMBOL(drm_gem_fence_array_add); - -/** - * drm_gem_fence_array_add_implicit - Adds the implicit dependencies tracked - * in the GEM object's reservation object to an array of dma_fences for use in - * scheduling a rendering job. - * - * This should be called after drm_gem_lock_reservations() on your array of - * GEM objects used in the job but before updating the reservations with your - * own fences. - * - * @fence_array: array of dma_fence * for the job to block on. - * @obj: the gem object to add new dependencies from. - * @write: whether the job might write the object (so we need to depend on - * shared fences in the reservation object). - */ -int drm_gem_fence_array_add_implicit(struct xarray *fence_array, - struct drm_gem_object *obj, - bool write) -{ - int ret; - struct dma_fence **fences; - unsigned int i, fence_count; - - if (!write) { - struct dma_fence *fence = - dma_resv_get_excl_unlocked(obj->resv); - - return drm_gem_fence_array_add(fence_array, fence); - } - - ret = dma_resv_get_fences(obj->resv, NULL, - &fence_count, &fences); - if (ret || !fence_count) - return ret; - - for (i = 0; i < fence_count; i++) { - ret = drm_gem_fence_array_add(fence_array, fences[i]); - if (ret) - break; - } - - for (; i < fence_count; i++) - dma_fence_put(fences[i]); - kfree(fences); - return ret; -} -EXPORT_SYMBOL(drm_gem_fence_array_add_implicit); diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 240049566592..6d5e33b89074 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -409,11 +409,6 @@ int drm_gem_lock_reservations(struct drm_gem_object **objs, int count, struct ww_acquire_ctx *acquire_ctx); void drm_gem_unlock_reservations(struct drm_gem_object **objs, int count, struct ww_acquire_ctx *acquire_ctx); -int drm_gem_fence_array_add(struct xarray *fence_array, - struct dma_fence *fence); -int drm_gem_fence_array_add_implicit(struct xarray *fence_array, - struct drm_gem_object *obj, - bool write); int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, u32 handle, u64 *offset); -- 2.32.0.rc2

3 years, 10 months

[PATCH 08/11] drm/etnaviv: Use scheduler dependency handling

by Daniel Vetter

We need to pull the drm_sched_job_init much earlier, but that's very minor surgery. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: Russell King <linux+etnaviv(a)armlinux.org.uk> Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: etnaviv(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/gpu/drm/etnaviv/etnaviv_gem.h | 5 +- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 32 +++++----- drivers/gpu/drm/etnaviv/etnaviv_sched.c | 61 +------------------- drivers/gpu/drm/etnaviv/etnaviv_sched.h | 3 +- 4 files changed, 20 insertions(+), 81 deletions(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h index 98e60df882b6..63688e6e4580 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h @@ -80,9 +80,6 @@ struct etnaviv_gem_submit_bo { u64 va; struct etnaviv_gem_object *obj; struct etnaviv_vram_mapping *mapping; - struct dma_fence *excl; - unsigned int nr_shared; - struct dma_fence **shared; }; /* Created per submit-ioctl, to track bo's and cmdstream bufs, etc, @@ -95,7 +92,7 @@ struct etnaviv_gem_submit { struct etnaviv_file_private *ctx; struct etnaviv_gpu *gpu; struct etnaviv_iommu_context *mmu_context, *prev_mmu_context; - struct dma_fence *out_fence, *in_fence; + struct dma_fence *out_fence; int out_fence_id; struct list_head node; /* GPU active submit list */ struct etnaviv_cmdbuf cmdbuf; diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 4dd7d9d541c0..92478a50a580 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -188,16 +188,10 @@ static int submit_fence_sync(struct etnaviv_gem_submit *submit) if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT) continue; - if (bo->flags & ETNA_SUBMIT_BO_WRITE) { - ret = dma_resv_get_fences(robj, &bo->excl, - &bo->nr_shared, - &bo->shared); - if (ret) - return ret; - } else { - bo->excl = dma_resv_get_excl_unlocked(robj); - } - + ret = drm_sched_job_await_implicit(&submit->sched_job, &bo->obj->base, + bo->flags & ETNA_SUBMIT_BO_WRITE); + if (ret) + return ret; } return ret; @@ -403,8 +397,6 @@ static void submit_cleanup(struct kref *kref) wake_up_all(&submit->gpu->fence_event); - if (submit->in_fence) - dma_fence_put(submit->in_fence); if (submit->out_fence) { /* first remove from IDR, so fence can not be found anymore */ mutex_lock(&submit->gpu->fence_lock); @@ -537,6 +529,12 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, submit->exec_state = args->exec_state; submit->flags = args->flags; + ret = drm_sched_job_init(&submit->sched_job, + &ctx->sched_entity[args->pipe], + submit->ctx); + if (ret) + goto err_submit_objects; + ret = submit_lookup_objects(submit, file, bos, args->nr_bos); if (ret) goto err_submit_objects; @@ -549,11 +547,15 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, } if (args->flags & ETNA_SUBMIT_FENCE_FD_IN) { - submit->in_fence = sync_file_get_fence(args->fence_fd); - if (!submit->in_fence) { + struct dma_fence *in_fence = sync_file_get_fence(args->fence_fd); + if (!in_fence) { ret = -EINVAL; goto err_submit_objects; } + + ret = drm_sched_job_await_fence(&submit->sched_job, in_fence); + if (ret) + goto err_submit_objects; } ret = submit_pin_objects(submit); @@ -579,7 +581,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, if (ret) goto err_submit_objects; - ret = etnaviv_sched_push_job(&ctx->sched_entity[args->pipe], submit); + ret = etnaviv_sched_push_job(submit); if (ret) goto err_submit_objects; diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c index 77995f190790..d62053b69953 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c @@ -17,58 +17,6 @@ module_param_named(job_hang_limit, etnaviv_job_hang_limit, int , 0444); static int etnaviv_hw_jobs_limit = 4; module_param_named(hw_job_limit, etnaviv_hw_jobs_limit, int , 0444); -static struct dma_fence * -etnaviv_sched_dependency(struct drm_sched_job *sched_job, - struct drm_sched_entity *entity) -{ - struct etnaviv_gem_submit *submit = to_etnaviv_submit(sched_job); - struct dma_fence *fence; - int i; - - if (unlikely(submit->in_fence)) { - fence = submit->in_fence; - submit->in_fence = NULL; - - if (!dma_fence_is_signaled(fence)) - return fence; - - dma_fence_put(fence); - } - - for (i = 0; i < submit->nr_bos; i++) { - struct etnaviv_gem_submit_bo *bo = &submit->bos[i]; - int j; - - if (bo->excl) { - fence = bo->excl; - bo->excl = NULL; - - if (!dma_fence_is_signaled(fence)) - return fence; - - dma_fence_put(fence); - } - - for (j = 0; j < bo->nr_shared; j++) { - if (!bo->shared[j]) - continue; - - fence = bo->shared[j]; - bo->shared[j] = NULL; - - if (!dma_fence_is_signaled(fence)) - return fence; - - dma_fence_put(fence); - } - kfree(bo->shared); - bo->nr_shared = 0; - bo->shared = NULL; - } - - return NULL; -} - static struct dma_fence *etnaviv_sched_run_job(struct drm_sched_job *sched_job) { struct etnaviv_gem_submit *submit = to_etnaviv_submit(sched_job); @@ -140,14 +88,12 @@ static void etnaviv_sched_free_job(struct drm_sched_job *sched_job) } static const struct drm_sched_backend_ops etnaviv_sched_ops = { - .dependency = etnaviv_sched_dependency, .run_job = etnaviv_sched_run_job, .timedout_job = etnaviv_sched_timedout_job, .free_job = etnaviv_sched_free_job, }; -int etnaviv_sched_push_job(struct drm_sched_entity *sched_entity, - struct etnaviv_gem_submit *submit) +int etnaviv_sched_push_job(struct etnaviv_gem_submit *submit) { int ret = 0; @@ -158,11 +104,6 @@ int etnaviv_sched_push_job(struct drm_sched_entity *sched_entity, */ mutex_lock(&submit->gpu->fence_lock); - ret = drm_sched_job_init(&submit->sched_job, sched_entity, - submit->ctx); - if (ret) - goto out_unlock; - drm_sched_job_arm(&submit->sched_job); submit->out_fence = dma_fence_get(&submit->sched_job.s_fence->finished); diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.h b/drivers/gpu/drm/etnaviv/etnaviv_sched.h index c0a6796e22c9..baebfa069afc 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.h @@ -18,7 +18,6 @@ struct etnaviv_gem_submit *to_etnaviv_submit(struct drm_sched_job *sched_job) int etnaviv_sched_init(struct etnaviv_gpu *gpu); void etnaviv_sched_fini(struct etnaviv_gpu *gpu); -int etnaviv_sched_push_job(struct drm_sched_entity *sched_entity, - struct etnaviv_gem_submit *submit); +int etnaviv_sched_push_job(struct etnaviv_gem_submit *submit); #endif /* __ETNAVIV_SCHED_H__ */ -- 2.32.0.rc2

3 years, 10 months

[PATCH 04/11] drm/panfrost: use scheduler dependency tracking

by Daniel Vetter

Just deletes some code that's now more shared. Note that thanks to the split into drm_sched_job_init/arm we can now easily pull the _init() part from under the submission lock way ahead where we're adding the sync file in-fences as dependencies. 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_drv.c | 14 +++++++--- drivers/gpu/drm/panfrost/panfrost_job.c | 37 +++---------------------- drivers/gpu/drm/panfrost/panfrost_job.h | 5 +--- 3 files changed, 15 insertions(+), 41 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 1ffaef5ec5ff..79904f55c19f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -218,7 +218,7 @@ panfrost_copy_in_sync(struct drm_device *dev, if (ret) goto fail; - ret = drm_gem_fence_array_add(&job->deps, fence); + ret = drm_sched_job_await_fence(&job->base, fence); if (ret) goto fail; @@ -236,7 +236,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, struct drm_panfrost_submit *args = data; struct drm_syncobj *sync_out = NULL; struct panfrost_job *job; - int ret = 0; + int ret = 0, slot; if (!args->jc) return -EINVAL; @@ -258,14 +258,20 @@ 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; job->flush_id = panfrost_gpu_get_latest_flush_id(pfdev); job->file_priv = file->driver_priv; + slot = panfrost_job_get_slot(job); + + ret = drm_sched_job_init(&job->base, + &job->file_priv->sched_entity[slot], + NULL); + if (ret) + goto fail_job; + ret = panfrost_copy_in_sync(dev, file, args, job); if (ret) goto fail_job; diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 2d01a670a4e8..d097e52f8caa 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -109,7 +109,7 @@ static struct dma_fence *panfrost_fence_create(struct panfrost_device *pfdev, in return &fence->base; } -static int panfrost_job_get_slot(struct panfrost_job *job) +int panfrost_job_get_slot(struct panfrost_job *job) { /* JS0: fragment jobs. * JS1: vertex/tiler jobs @@ -198,13 +198,13 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js) static int panfrost_acquire_object_fences(struct drm_gem_object **bos, int bo_count, - struct xarray *deps) + struct drm_sched_job *job) { int i, ret; for (i = 0; i < bo_count; i++) { /* panfrost always uses write mode in its current uapi */ - ret = drm_gem_fence_array_add_implicit(deps, bos[i], true); + ret = drm_sched_job_await_implicit(job, bos[i], true); if (ret) return ret; } @@ -225,31 +225,21 @@ static void panfrost_attach_object_fences(struct drm_gem_object **bos, int panfrost_job_push(struct panfrost_job *job) { struct panfrost_device *pfdev = job->pfdev; - int slot = panfrost_job_get_slot(job); - struct drm_sched_entity *entity = &job->file_priv->sched_entity[slot]; struct ww_acquire_ctx acquire_ctx; int ret = 0; - ret = drm_gem_lock_reservations(job->bos, job->bo_count, &acquire_ctx); if (ret) return ret; mutex_lock(&pfdev->sched_lock); - - ret = drm_sched_job_init(&job->base, entity, NULL); - if (ret) { - mutex_unlock(&pfdev->sched_lock); - goto unlock; - } - drm_sched_job_arm(&job->base); job->render_done_fence = dma_fence_get(&job->base.s_fence->finished); ret = panfrost_acquire_object_fences(job->bos, job->bo_count, - &job->deps); + &job->base); if (ret) { mutex_unlock(&pfdev->sched_lock); goto unlock; @@ -274,15 +264,8 @@ 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; - 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); @@ -321,17 +304,6 @@ static void panfrost_job_free(struct drm_sched_job *sched_job) panfrost_job_put(job); } -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); - - if (!xa_empty(&job->deps)) - return xa_erase(&job->deps, job->last_dep++); - - return NULL; -} - static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job) { struct panfrost_job *job = to_panfrost_job(sched_job); @@ -457,7 +429,6 @@ static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job } static const struct drm_sched_backend_ops panfrost_sched_ops = { - .dependency = panfrost_job_dependency, .run_job = panfrost_job_run, .timedout_job = panfrost_job_timedout, .free_job = panfrost_job_free diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h index 82306a03b57e..77e6d0e6f612 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.h +++ b/drivers/gpu/drm/panfrost/panfrost_job.h @@ -19,10 +19,6 @@ struct panfrost_job { struct panfrost_device *pfdev; struct panfrost_file_priv *file_priv; - /* 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; @@ -42,6 +38,7 @@ int panfrost_job_init(struct panfrost_device *pfdev); void panfrost_job_fini(struct panfrost_device *pfdev); int panfrost_job_open(struct panfrost_file_priv *panfrost_priv); void panfrost_job_close(struct panfrost_file_priv *panfrost_priv); +int panfrost_job_get_slot(struct panfrost_job *job); int panfrost_job_push(struct panfrost_job *job); void panfrost_job_put(struct panfrost_job *job); void panfrost_job_enable_interrupts(struct panfrost_device *pfdev); -- 2.32.0.rc2

3 years, 10 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig