- Linaro-mm-sig - lists.linaro.org

Re: [Linaro-mm-sig] [PATCH 1/2] drm: avoid races with modesetting rights

by Daniel Vetter

On Fri, Aug 13, 2021 at 04:54:49PM +0800, Desmond Cheong Zhi Xi wrote: > In drm_client_modeset.c and drm_fb_helper.c, > drm_master_internal_{acquire,release} are used to avoid races with DRM > userspace. These functions hold onto drm_device.master_mutex while > committing, and bail if there's already a master. > > However, ioctls can still race between themselves. A > time-of-check-to-time-of-use error can occur if an ioctl that changes > the modeset has its rights revoked after it validates its permissions, > but before it completes. > > There are three ioctls that can affect modesetting permissions: > > - DROP_MASTER ioctl removes rights for a master and its leases > > - REVOKE_LEASE ioctl revokes rights for a specific lease > > - SET_MASTER ioctl sets the device master if the master role hasn't > been acquired yet > > All these races can be avoided by introducing an SRCU that acts as a > barrier for ioctls that can change modesetting permissions. Processes > that perform modesetting should hold a read lock on the new > drm_device.master_barrier_srcu, and ioctls that change these > permissions should call synchronize_srcu before returning. > > This ensures that any process that might have seen old permissions are > flushed out before DROP_MASTER/REVOKE_LEASE/SET_MASTER ioctls return > to userspace. > > Reported-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> This looks pretty solid, but I think there's one gap where we can still race. Scenario. Process A has a drm fd with master rights and two threads: - thread 1 does a long-running display operation (like a modeset or whatever) - thread 2 does a drop-master Then we start a new process B, which acquires master in drm_open (there is no other one left). This is like setmaster ioctl, but your DRM_MASTER_FLUSH bit doesn't work there. The other thing is that for modeset stuff (which this all is) srcu is probably massive overkill, and a simple rwsem should be good enough too. Maybe even better, since the rwsem guarantees that no new reader can start once you try to acquire the write side. Finally, and this is a bit a bikeshed: I don't like much how DRM_MASTER_FLUSH leaks the need of these very few places into the very core drm_ioctl function. One idea I had was to use task_work in a special function, roughly void master_flush() { down_write(master_rwsem); up_write(master_rwms); } void drm_master_flush() { init_task_work(fpriv->master_flush_work, master_flush) task_work_add(fpriv->master_flush_work); /* if task_work_add fails we're exiting, at which point the lack * of master flush doesn't matter); } And maybe put a comment above the function explaining why and how this works. We could even do a drm_master_unlock_and_flush helper, since that's really what everyone wants, and it would make it very clear which master state changes need this flush. Instead of setting a flag bit in an ioctl table very far away ... Thoughts? -Daniel > --- > drivers/gpu/drm/drm_auth.c | 17 ++++++++++++++--- > drivers/gpu/drm/drm_client_modeset.c | 10 ++++++---- > drivers/gpu/drm/drm_drv.c | 2 ++ > drivers/gpu/drm/drm_fb_helper.c | 20 ++++++++++++-------- > drivers/gpu/drm/drm_internal.h | 5 +++-- > drivers/gpu/drm/drm_ioctl.c | 25 +++++++++++++++++++++---- > include/drm/drm_device.h | 11 +++++++++++ > include/drm/drm_ioctl.h | 7 +++++++ > 8 files changed, 76 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index 60a6b21474b1..004506608e76 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -29,6 +29,7 @@ > */ > > #include <linux/slab.h> > +#include <linux/srcu.h> > > #include <drm/drm_auth.h> > #include <drm/drm_drv.h> > @@ -448,21 +449,31 @@ void drm_master_put(struct drm_master **master) > EXPORT_SYMBOL(drm_master_put); > > /* Used by drm_client and drm_fb_helper */ > -bool drm_master_internal_acquire(struct drm_device *dev) > +bool drm_master_internal_acquire(struct drm_device *dev, int *idx) > { > + *idx = srcu_read_lock(&dev->master_barrier_srcu); > + > mutex_lock(&dev->master_mutex); > if (dev->master) { > mutex_unlock(&dev->master_mutex); > + srcu_read_unlock(&dev->master_barrier_srcu, *idx); > return false; > } > + mutex_unlock(&dev->master_mutex); > > return true; > } > EXPORT_SYMBOL(drm_master_internal_acquire); > > /* Used by drm_client and drm_fb_helper */ > -void drm_master_internal_release(struct drm_device *dev) > +void drm_master_internal_release(struct drm_device *dev, int idx) > { > - mutex_unlock(&dev->master_mutex); > + srcu_read_unlock(&dev->master_barrier_srcu, idx); > } > EXPORT_SYMBOL(drm_master_internal_release); > + > +/* Used by drm_ioctl */ > +void drm_master_flush(struct drm_device *dev) > +{ > + synchronize_srcu(&dev->master_barrier_srcu); > +} > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index ced09c7c06f9..9885f36f71b7 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -1165,13 +1165,14 @@ int drm_client_modeset_commit(struct drm_client_dev *client) > { > struct drm_device *dev = client->dev; > int ret; > + int idx; > > - if (!drm_master_internal_acquire(dev)) > + if (!drm_master_internal_acquire(dev, &idx)) > return -EBUSY; > > ret = drm_client_modeset_commit_locked(client); > > - drm_master_internal_release(dev); > + drm_master_internal_release(dev, idx); > > return ret; > } > @@ -1215,8 +1216,9 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode) > { > struct drm_device *dev = client->dev; > int ret = 0; > + int idx; > > - if (!drm_master_internal_acquire(dev)) > + if (!drm_master_internal_acquire(dev, &idx)) > return -EBUSY; > > mutex_lock(&client->modeset_mutex); > @@ -1226,7 +1228,7 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode) > drm_client_modeset_dpms_legacy(client, mode); > mutex_unlock(&client->modeset_mutex); > > - drm_master_internal_release(dev); > + drm_master_internal_release(dev, idx); > > return ret; > } > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 7a5097467ba5..c313f0674db3 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -574,6 +574,7 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) > mutex_destroy(&dev->clientlist_mutex); > mutex_destroy(&dev->filelist_mutex); > mutex_destroy(&dev->struct_mutex); > + cleanup_srcu_struct(&dev->master_barrier_srcu); > drm_legacy_destroy_members(dev); > } > > @@ -612,6 +613,7 @@ static int drm_dev_init(struct drm_device *dev, > mutex_init(&dev->filelist_mutex); > mutex_init(&dev->clientlist_mutex); > mutex_init(&dev->master_mutex); > + init_srcu_struct(&dev->master_barrier_srcu); > > ret = drmm_add_action(dev, drm_dev_init_release, NULL); > if (ret) > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 3ab078321045..0d594bc15f18 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1116,13 +1116,14 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > struct drm_fb_helper *fb_helper = info->par; > struct drm_device *dev = fb_helper->dev; > int ret; > + int idx; > > if (oops_in_progress) > return -EBUSY; > > mutex_lock(&fb_helper->lock); > > - if (!drm_master_internal_acquire(dev)) { > + if (!drm_master_internal_acquire(dev, &idx)) { > ret = -EBUSY; > goto unlock; > } > @@ -1136,7 +1137,7 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > ret = setcmap_legacy(cmap, info); > mutex_unlock(&fb_helper->client.modeset_mutex); > > - drm_master_internal_release(dev); > + drm_master_internal_release(dev, idx); > unlock: > mutex_unlock(&fb_helper->lock); > > @@ -1160,9 +1161,10 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, > struct drm_device *dev = fb_helper->dev; > struct drm_crtc *crtc; > int ret = 0; > + int idx; > > mutex_lock(&fb_helper->lock); > - if (!drm_master_internal_acquire(dev)) { > + if (!drm_master_internal_acquire(dev, &idx)) { > ret = -EBUSY; > goto unlock; > } > @@ -1204,7 +1206,7 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, > ret = -ENOTTY; > } > > - drm_master_internal_release(dev); > + drm_master_internal_release(dev, idx); > unlock: > mutex_unlock(&fb_helper->lock); > return ret; > @@ -1474,12 +1476,13 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, > struct drm_fb_helper *fb_helper = info->par; > struct drm_device *dev = fb_helper->dev; > int ret; > + int idx; > > if (oops_in_progress) > return -EBUSY; > > mutex_lock(&fb_helper->lock); > - if (!drm_master_internal_acquire(dev)) { > + if (!drm_master_internal_acquire(dev, &idx)) { > ret = -EBUSY; > goto unlock; > } > @@ -1489,7 +1492,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, > else > ret = pan_display_legacy(var, info); > > - drm_master_internal_release(dev); > + drm_master_internal_release(dev, idx); > unlock: > mutex_unlock(&fb_helper->lock); > > @@ -1948,6 +1951,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); > int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > { > int err = 0; > + int idx; > > if (!drm_fbdev_emulation || !fb_helper) > return 0; > @@ -1959,13 +1963,13 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > return err; > } > > - if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { > + if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev, &idx)) { > fb_helper->delayed_hotplug = true; > mutex_unlock(&fb_helper->lock); > return err; > } > > - drm_master_internal_release(fb_helper->dev); > + drm_master_internal_release(fb_helper->dev, idx); > > drm_dbg_kms(fb_helper->dev, "\n"); > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > index 17f3548c8ed2..578fd2769913 100644 > --- a/drivers/gpu/drm/drm_internal.h > +++ b/drivers/gpu/drm/drm_internal.h > @@ -142,8 +142,9 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_priv); > int drm_master_open(struct drm_file *file_priv); > void drm_master_release(struct drm_file *file_priv); > -bool drm_master_internal_acquire(struct drm_device *dev); > -void drm_master_internal_release(struct drm_device *dev); > +bool drm_master_internal_acquire(struct drm_device *dev, int *idx); > +void drm_master_internal_release(struct drm_device *dev, int idx); > +void drm_master_flush(struct drm_device *dev); > > /* drm_sysfs.c */ > extern struct class *drm_class; > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index be4a52dc4d6f..eb4ec3fab7d1 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -600,8 +600,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_SET_SAREA_CTX, drm_legacy_setsareactx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), > DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_GET_SAREA_CTX, drm_legacy_getsareactx, DRM_AUTH), > > - DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, 0), > - DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, 0), > + DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, > + DRM_MASTER_FLUSH), > + DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, > + DRM_MASTER_FLUSH), > > DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_ADD_CTX, drm_legacy_addctx, DRM_AUTH|DRM_ROOT_ONLY), > DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_RM_CTX, drm_legacy_rmctx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), > @@ -722,7 +724,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER), > - DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER), > + DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, > + DRM_MASTER | DRM_MASTER_FLUSH), > }; > > #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) > @@ -781,13 +784,17 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata, > struct drm_file *file_priv = file->private_data; > struct drm_device *dev = file_priv->minor->dev; > int retcode; > + int idx; > > if (drm_dev_is_unplugged(dev)) > return -ENODEV; > > + if (unlikely(flags & DRM_MASTER)) > + idx = srcu_read_lock(&dev->master_barrier_srcu); > + > retcode = drm_ioctl_permit(flags, file_priv); > if (unlikely(retcode)) > - return retcode; > + goto release_master; > > /* Enforce sane locking for modern driver ioctls. */ > if (likely(!drm_core_check_feature(dev, DRIVER_LEGACY)) || > @@ -798,6 +805,16 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata, > retcode = func(dev, kdata, file_priv); > mutex_unlock(&drm_global_mutex); > } > + > +release_master: > + if (unlikely(flags & DRM_MASTER)) > + srcu_read_unlock(&dev->master_barrier_srcu, idx); > + /* After flushing, processes are guaranteed to see the new master/lease > + * permissions, and any process which might have seen the old > + * permissions is guaranteed to have finished. > + */ > + if (unlikely(flags & DRM_MASTER_FLUSH)) > + drm_master_flush(dev); > return retcode; > } > EXPORT_SYMBOL(drm_ioctl_kernel); > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > index 604b1d1b2d72..0ac5fdb375f8 100644 > --- a/include/drm/drm_device.h > +++ b/include/drm/drm_device.h > @@ -111,6 +111,17 @@ struct drm_device { > */ > struct drm_master *master; > > + /** > + * @master_barrier_srcu: > + * > + * Used to synchronize modesetting rights between multiple users. Users > + * that can change the modeset or display state must hold an > + * srcu_read_lock() on @master_barrier_srcu, and ioctls that can change > + * modesetting rights should call synchronize_srcu() before returning > + * to userspace. > + */ > + struct srcu_struct master_barrier_srcu; > + > /** > * @driver_features: per-device driver features > * > diff --git a/include/drm/drm_ioctl.h b/include/drm/drm_ioctl.h > index afb27cb6a7bd..13a68cdcea36 100644 > --- a/include/drm/drm_ioctl.h > +++ b/include/drm/drm_ioctl.h > @@ -130,6 +130,13 @@ enum drm_ioctl_flags { > * not set DRM_AUTH because they do not require authentication. > */ > DRM_RENDER_ALLOW = BIT(5), > + /** > + * @DRM_MASTER_FLUSH: > + * > + * This must be set for any ioctl which can change the modesetting > + * permissions for DRM users. > + */ > + DRM_MASTER_FLUSH = BIT(6), > }; > > /** > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 9 months

1
0
0 0

[PATCH v5 13/20] 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 09c820045859..37e2e2820f08 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1272,99 +1272,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 35e7f44c2a75..e55a767188af 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -407,11 +407,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

3 years, 9 months

2
1
0 0

[PATCH v5 11/20] 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. v2: Actually fix up cleanup paths by calling drm_sched_job_init, which I wanted to to in the previous round (and did, for all other drivers). Spotted by Lucas. v3: Rebase over renamed functions to add dependencies. 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 | 60 ++++++++++--------- drivers/gpu/drm/etnaviv/etnaviv_sched.c | 63 +------------------- drivers/gpu/drm/etnaviv/etnaviv_sched.h | 3 +- 4 files changed, 37 insertions(+), 94 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..e3d43678eb09 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -188,16 +188,11 @@ 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_add_implicit_dependencies(&submit->sched_job, + &bo->obj->base, + bo->flags & ETNA_SUBMIT_BO_WRITE); + if (ret) + return ret; } return ret; @@ -403,8 +398,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); @@ -529,7 +522,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &submit->cmdbuf, ALIGN(args->stream_size, 8) + 8); if (ret) - goto err_submit_objects; + goto err_submit_put; submit->ctx = file->driver_priv; etnaviv_iommu_context_get(submit->ctx->mmu); @@ -537,51 +530,62 @@ 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_put; + ret = submit_lookup_objects(submit, file, bos, args->nr_bos); if (ret) - goto err_submit_objects; + goto err_submit_job; if ((priv->mmu_global->version != ETNAVIV_IOMMU_V2) && !etnaviv_cmd_validate_one(gpu, stream, args->stream_size / 4, relocs, args->nr_relocs)) { ret = -EINVAL; - goto err_submit_objects; + goto err_submit_job; } 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; + goto err_submit_job; } + + ret = drm_sched_job_add_dependency(&submit->sched_job, + in_fence); + if (ret) + goto err_submit_job; } ret = submit_pin_objects(submit); if (ret) - goto err_submit_objects; + goto err_submit_job; ret = submit_reloc(submit, stream, args->stream_size / 4, relocs, args->nr_relocs); if (ret) - goto err_submit_objects; + goto err_submit_job; ret = submit_perfmon_validate(submit, args->exec_state, pmrs); if (ret) - goto err_submit_objects; + goto err_submit_job; memcpy(submit->cmdbuf.vaddr, stream, args->stream_size); ret = submit_lock_objects(submit, &ticket); if (ret) - goto err_submit_objects; + goto err_submit_job; ret = submit_fence_sync(submit); if (ret) - goto err_submit_objects; + goto err_submit_job; - ret = etnaviv_sched_push_job(&ctx->sched_entity[args->pipe], submit); + ret = etnaviv_sched_push_job(submit); if (ret) - goto err_submit_objects; + goto err_submit_job; submit_attach_object_fences(submit); @@ -595,7 +599,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, sync_file = sync_file_create(submit->out_fence); if (!sync_file) { ret = -ENOMEM; - goto err_submit_objects; + goto err_submit_job; } fd_install(out_fence_fd, sync_file->file); } @@ -603,7 +607,9 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, args->fence_fd = out_fence_fd; args->fence = submit->out_fence_id; -err_submit_objects: +err_submit_job: + drm_sched_job_cleanup(&submit->sched_job); +err_submit_put: etnaviv_submit_put(submit); err_submit_ww_acquire: diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c index 180bb633d5c5..2bbbd6ccc95e 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,29 +88,22 @@ 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; /* * Hold the fence lock across the whole operation to avoid jobs being * pushed out of order with regard to their sched fence seqnos as - * allocated in drm_sched_job_init. + * allocated in drm_sched_job_arm. */ 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

3 years, 9 months

2
1
0 0

[PATCH v5 08/20] drm/lima: use scheduler dependency tracking

by Daniel Vetter

Nothing special going on here. Aside reviewing the code, it seems like drm_sched_job_arm() should be moved into lima_sched_context_queue_task and put under some mutex together with drm_sched_push_job(). See the kerneldoc for drm_sched_push_job(). v2: Rebase over renamed functions to add dependencies. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Qiang Yu <yuq825(a)gmail.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: lima(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/gpu/drm/lima/lima_gem.c | 6 ++++-- drivers/gpu/drm/lima/lima_sched.c | 21 --------------------- drivers/gpu/drm/lima/lima_sched.h | 3 --- 3 files changed, 4 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c index c528f40981bb..640acc060467 100644 --- a/drivers/gpu/drm/lima/lima_gem.c +++ b/drivers/gpu/drm/lima/lima_gem.c @@ -267,7 +267,9 @@ static int lima_gem_sync_bo(struct lima_sched_task *task, struct lima_bo *bo, if (explicit) return 0; - return drm_gem_fence_array_add_implicit(&task->deps, &bo->base.base, write); + return drm_sched_job_add_implicit_dependencies(&task->base, + &bo->base.base, + write); } static int lima_gem_add_deps(struct drm_file *file, struct lima_submit *submit) @@ -285,7 +287,7 @@ static int lima_gem_add_deps(struct drm_file *file, struct lima_submit *submit) if (err) return err; - err = drm_gem_fence_array_add(&submit->task->deps, fence); + err = drm_sched_job_add_dependency(&submit->task->base, fence); if (err) { dma_fence_put(fence); return err; diff --git a/drivers/gpu/drm/lima/lima_sched.c b/drivers/gpu/drm/lima/lima_sched.c index e968b5a8f0b0..99d5f6f1a882 100644 --- a/drivers/gpu/drm/lima/lima_sched.c +++ b/drivers/gpu/drm/lima/lima_sched.c @@ -134,24 +134,15 @@ int lima_sched_task_init(struct lima_sched_task *task, task->num_bos = num_bos; task->vm = lima_vm_get(vm); - xa_init_flags(&task->deps, XA_FLAGS_ALLOC); - return 0; } void lima_sched_task_fini(struct lima_sched_task *task) { - struct dma_fence *fence; - unsigned long index; int i; drm_sched_job_cleanup(&task->base); - xa_for_each(&task->deps, index, fence) { - dma_fence_put(fence); - } - xa_destroy(&task->deps); - if (task->bos) { for (i = 0; i < task->num_bos; i++) drm_gem_object_put(&task->bos[i]->base.base); @@ -186,17 +177,6 @@ struct dma_fence *lima_sched_context_queue_task(struct lima_sched_task *task) return fence; } -static struct dma_fence *lima_sched_dependency(struct drm_sched_job *job, - struct drm_sched_entity *entity) -{ - struct lima_sched_task *task = to_lima_task(job); - - if (!xa_empty(&task->deps)) - return xa_erase(&task->deps, task->last_dep++); - - return NULL; -} - static int lima_pm_busy(struct lima_device *ldev) { int ret; @@ -472,7 +452,6 @@ static void lima_sched_free_job(struct drm_sched_job *job) } static const struct drm_sched_backend_ops lima_sched_ops = { - .dependency = lima_sched_dependency, .run_job = lima_sched_run_job, .timedout_job = lima_sched_timedout_job, .free_job = lima_sched_free_job, diff --git a/drivers/gpu/drm/lima/lima_sched.h b/drivers/gpu/drm/lima/lima_sched.h index ac70006b0e26..6a11764d87b3 100644 --- a/drivers/gpu/drm/lima/lima_sched.h +++ b/drivers/gpu/drm/lima/lima_sched.h @@ -23,9 +23,6 @@ struct lima_sched_task { struct lima_vm *vm; void *frame; - struct xarray deps; - unsigned long last_dep; - struct lima_bo **bos; int num_bos; -- 2.32.0

3 years, 9 months

2
1
0 0

[PATCH 1/4] dma-buf: Require VM_PFNMAP vma for mmap

by Daniel Vetter

tldr; DMA buffers aren't normal memory, expecting that you can use them like that (like calling get_user_pages works, or that they're accounting like any other normal memory) cannot be guaranteed. Since some userspace only runs on integrated devices, where all buffers are actually all resident system memory, there's a huge temptation to assume that a struct page is always present and useable like for any more pagecache backed mmap. This has the potential to result in a uapi nightmare. To stop this gap require that DMA buffer mmaps are VM_PFNMAP, which blocks get_user_pages and all the other struct page based infrastructure for everyone. In spirit this is the uapi counterpart to the kernel-internal CONFIG_DMABUF_DEBUG. Motivated by a recent patch which wanted to swich the system dma-buf heap to vm_insert_page instead of vm_insert_pfn. v2: Jason brought up that we also want to guarantee that all ptes have the pte_special flag set, to catch fast get_user_pages (on architectures that support this). Allowing VM_MIXEDMAP (like VM_SPECIAL does) would still allow vm_insert_page, but limiting to VM_PFNMAP will catch that. >From auditing the various functions to insert pfn pte entires (vm_insert_pfn_prot, remap_pfn_range and all it's callers like dma_mmap_wc) it looks like VM_PFNMAP is already required anyway, so this should be the correct flag to check for. v3: Change to WARN_ON_ONCE (Thomas Zimmermann) References: https://lore.kernel.org/lkml/CAKMK7uHi+mG0z0HUmNt13QCCvutuRVjpcR0NjRL12k-Wb… Acked-by: Christian König <christian.koenig(a)amd.com> Acked-by: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Jason Gunthorpe <jgg(a)ziepe.ca> Cc: Suren Baghdasaryan <surenb(a)google.com> Cc: Matthew Wilcox <willy(a)infradead.org> Cc: John Stultz <john.stultz(a)linaro.org> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.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 -- Resending this so I can test the next patches for vgem/shmem in intel-gfx-ci. No immediate plans to merge this patch here since ttm isn't addressed yet (and there we have the hugepte issue, for which I don't think we have a clear consensus yet). -Daniel --- drivers/dma-buf/dma-buf.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 63d32261b63f..d19b1cf6c34f 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -130,6 +130,7 @@ static struct file_system_type dma_buf_fs_type = { static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) { struct dma_buf *dmabuf; + int ret; if (!is_dma_buf_file(file)) return -EINVAL; @@ -145,7 +146,11 @@ static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) dmabuf->size >> PAGE_SHIFT) return -EINVAL; - return dmabuf->ops->mmap(dmabuf, vma); + ret = dmabuf->ops->mmap(dmabuf, vma); + + WARN_ON_ONCE(!(vma->vm_flags & VM_PFNMAP)); + + return ret; } static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) @@ -1260,6 +1265,8 @@ EXPORT_SYMBOL_GPL(dma_buf_end_cpu_access); int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, unsigned long pgoff) { + int ret; + if (WARN_ON(!dmabuf || !vma)) return -EINVAL; @@ -1280,7 +1287,11 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, vma_set_file(vma, dmabuf->file); vma->vm_pgoff = pgoff; - return dmabuf->ops->mmap(dmabuf, vma); + ret = dmabuf->ops->mmap(dmabuf, vma); + + WARN_ON_ONCE(!(vma->vm_flags & VM_PFNMAP)); + + return ret; } EXPORT_SYMBOL_GPL(dma_buf_mmap); -- 2.32.0

3 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH next] udmabuf: fix general protection fault in udmabuf_create

by Gerd Hoffmann

On Wed, Aug 11, 2021 at 08:50:52PM +0300, Pavel Skripkin wrote: > Syzbot reported general protection fault in udmabuf_create. The problem > was in wrong error handling. > > In commit 16c243e99d33 ("udmabuf: Add support for mapping hugepages (v4)") > shmem_read_mapping_page() call was replaced with find_get_page_flags(), > but find_get_page_flags() returns NULL on failure instead PTR_ERR(). > > Wrong error checking was causing GPF in get_page(), since passed page > was equal to NULL. Fix it by changing if (IS_ER(!hpage)) to if (!hpage) > > Reported-by: syzbot+e9cd3122a37c5d6c51e8(a)syzkaller.appspotmail.com > Fixes: 16c243e99d33 ("udmabuf: Add support for mapping hugepages (v4)") > Signed-off-by: Pavel Skripkin <paskripkin(a)gmail.com> Pushed to drm-misc-next. thanks, Gerd

3 years, 9 months

1
0
0 0

[PATCH] dma-buf: heaps: Set allocation limit for system heap

by Hridya Valsaraju

This patch limits the size of total memory that can be requested in a single allocation from the system heap. This would prevent a buggy/malicious client from depleting system memory by requesting for an extremely large allocation which might destabilize the system. The limit is set to half the size of the device's total RAM which is the same as what was set by the deprecated ION system heap. Signed-off-by: Hridya Valsaraju <hridya(a)google.com> --- drivers/dma-buf/heaps/system_heap.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index b7fbce66bcc0..099f5a8304b4 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -371,6 +371,12 @@ static struct dma_buf *system_heap_do_allocate(struct dma_heap *heap, struct page *page, *tmp_page; int i, ret = -ENOMEM; + if (len / PAGE_SIZE > totalram_pages() / 2) { + pr_err("pid %d requested too large an allocation(size %lu) from system heap\n", + current->pid, len); + return ERR_PTR(ret); + } + buffer = kzalloc(sizeof(*buffer), GFP_KERNEL); if (!buffer) return ERR_PTR(-ENOMEM); -- 2.32.0.432.gabb21c7263-goog

3 years, 10 months

4
6
0 0

Re: [Linaro-mm-sig] [syzbot] general protection fault in udmabuf_create

by Dan Carpenter

On Tue, Aug 10, 2021 at 05:10:56PM +0300, Pavel Skripkin wrote: > On 8/10/21 4:47 PM, syzbot wrote: > > Hello, > > > > syzbot found the following issue on: > > > > HEAD commit: 7999516e20bd Add linux-next specific files for 20210806 > > git tree: linux-next > > console output: https://syzkaller.appspot.com/x/log.txt?x=10f15f8e300000 > > kernel config: https://syzkaller.appspot.com/x/.config?x=2f518e910b029c31 > > dashboard link: https://syzkaller.appspot.com/bug?extid=e9cd3122a37c5d6c51e8 > > compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.1 > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1181099a300000 > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11b6fce9300000 > > > > The issue was bisected to: > > > > commit 16c243e99d335e1ef3059871897119affc98b493 > > Author: Vivek Kasireddy <vivek.kasireddy(a)intel.com> > > Date: Wed Jun 9 18:29:15 2021 +0000 > > > > udmabuf: Add support for mapping hugepages (v4) > > > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=12f73dc9300000 > > final oops: https://syzkaller.appspot.com/x/report.txt?x=11f73dc9300000 > > console output: https://syzkaller.appspot.com/x/log.txt?x=16f73dc9300000 > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > > Reported-by: syzbot+e9cd3122a37c5d6c51e8(a)syzkaller.appspotmail.com > > Fixes: 16c243e99d33 ("udmabuf: Add support for mapping hugepages (v4)") > > > > general protection fault, probably for non-canonical address 0xdffffc0000000001: 0000 [#1] PREEMPT SMP KASAN > > KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f] > > CPU: 0 PID: 6603 Comm: syz-executor127 Not tainted 5.14.0-rc4-next-20210806-syzkaller #0 > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > > RIP: 0010:_compound_head include/linux/page-flags.h:187 [inline] > > RIP: 0010:get_page include/linux/mm.h:1203 [inline] > > RIP: 0010:udmabuf_create+0x664/0x16f0 drivers/dma-buf/udmabuf.c:236 > > Code: 03 48 89 84 24 90 00 00 00 e9 38 01 00 00 e8 23 7a f7 fc 4d 89 f4 49 c1 e4 06 4c 03 24 24 49 8d 7c 24 08 48 89 f8 48 c1 e8 03 <42> 80 3c 38 00 0f 85 d3 0d 00 00 4d 8b 6c 24 08 31 ff 4c 89 eb 83 > > RSP: 0018:ffffc90002d7fc70 EFLAGS: 00010202 > > RAX: 0000000000000001 RBX: 0000000000000000 RCX: 0000000000000000 > > RDX: ffff888023f69c80 RSI: ffffffff847e4f3d RDI: 0000000000000008 > > RBP: 0000000000000000 R08: fffffffffffff000 R09: 0000000000000000 > > R10: ffffffff847e50f5 R11: 0000000000000000 R12: 0000000000000000 > > R13: 0000000000000000 R14: 0000000000000000 R15: dffffc0000000000 > > FS: 0000000000935300(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000 > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > CR2: 000000002000020c CR3: 0000000018d16000 CR4: 00000000001506f0 > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > > Call Trace: > > udmabuf_ioctl_create drivers/dma-buf/udmabuf.c:305 [inline] > > The problem is wrong error handling: > > hpage = find_get_page_flags(mapping, pgoff, FGP_ACCESSED); > if (IS_ERR(hpage)) { > ret = PTR_ERR(hpage); > goto err; > } > > find_get_page_flags() return NULL on failure, so this patch should work: > > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index 8df761a10251..c57a609db75b 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -227,8 +227,8 @@ static long udmabuf_create(struct miscdevice *device, > if (!hpage) { > hpage = find_get_page_flags(mapping, pgoff, > FGP_ACCESSED); > - if (IS_ERR(hpage)) { > - ret = PTR_ERR(hpage); > + if (!hpage) { > + ret = -EINVAL; > goto err; > } > } > > I am not sure about ret value in case of failure, so I am looking for any > reviews :) You're right. Smatch is sort of supposed to warn about this but pagecache_get_page() is too complicated. regards, dan carpenter

3 years, 10 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: Fix a few typos in dma-buf documentation

by Randy Dunlap

On 8/9/21 5:22 AM, Gal Pressman wrote: > Fix a few typos in the documentation: > - Remove an extraneous 'or' > - 'unpins' -> 'unpin' > - 'braket' -> 'bracket' > - 'mappinsg' -> 'mappings' > - 'fullfills' -> 'fulfills' > > Signed-off-by: Gal Pressman <galpress(a)amazon.com> Reviewed-by: Randy Dunlap <rdunlap(a)infradead.org> Thanks. > --- > include/linux/dma-buf.h | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index efdc56b9d95f..772403352767 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -54,7 +54,7 @@ struct dma_buf_ops { > * device), and otherwise need to fail the attach operation. > * > * The exporter should also in general check whether the current > - * allocation fullfills the DMA constraints of the new device. If this > + * allocation fulfills the DMA constraints of the new device. If this > * is not the case, and the allocation cannot be moved, it should also > * fail the attach operation. > * > @@ -146,7 +146,7 @@ struct dma_buf_ops { > * > * Returns: > * > - * A &sg_table scatter list of or the backing storage of the DMA buffer, > + * A &sg_table scatter list of the backing storage of the DMA buffer, > * already mapped into the device address space of the &device attached > * with the provided &dma_buf_attachment. The addresses and lengths in > * the scatter list are PAGE_SIZE aligned. > @@ -168,7 +168,7 @@ struct dma_buf_ops { > * > * This is called by dma_buf_unmap_attachment() and should unmap and > * release the &sg_table allocated in @map_dma_buf, and it is mandatory. > - * For static dma_buf handling this might also unpins the backing > + * For static dma_buf handling this might also unpin the backing > * storage if this is the last mapping of the DMA buffer. > */ > void (*unmap_dma_buf)(struct dma_buf_attachment *, > @@ -237,7 +237,7 @@ struct dma_buf_ops { > * This callback is used by the dma_buf_mmap() function > * > * Note that the mapping needs to be incoherent, userspace is expected > - * to braket CPU access using the DMA_BUF_IOCTL_SYNC interface. > + * to bracket CPU access using the DMA_BUF_IOCTL_SYNC interface. > * > * Because dma-buf buffers have invariant size over their lifetime, the > * dma-buf core checks whether a vma is too large and rejects such > @@ -464,7 +464,7 @@ static inline bool dma_buf_is_dynamic(struct dma_buf *dmabuf) > > /** > * dma_buf_attachment_is_dynamic - check if a DMA-buf attachment uses dynamic > - * mappinsg > + * mappings > * @attach: the DMA-buf attachment to check > * > * Returns true if a DMA-buf importer wants to call the map/unmap functions with > -- ~Randy

3 years, 10 months

2
1
0 0

[PATCH v5 04/20] 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. v2/3: Rebased. v4: Repaint this shed. The functions are now called _add_dependency() and _add_implicit_dependency() Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> (v3) Reviewed-by: Steven Price <steven.price(a)arm.com> (v1) Acked-by: Melissa Wen <mwen(a)igalia.com> 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: 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 | 104 +++++++++++++++++++++++ include/drm/gpu_scheduler.h | 33 ++++++- 3 files changed, 149 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 89e3f6eaf519..381fbf462ea7 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 454cb6164bdc..f77456929139 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); @@ -637,6 +639,99 @@ void drm_sched_job_arm(struct drm_sched_job *job) } EXPORT_SYMBOL(drm_sched_job_arm); +/** + * drm_sched_job_add_dependency - 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_add_dependency(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_add_dependency); + +/** + * drm_sched_job_add_implicit_dependencies - 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_add_implicit_dependencies(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_add_dependency(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_add_dependency(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_add_implicit_dependencies); + + /** * drm_sched_job_cleanup - clean up scheduler job resources * @job: scheduler job to clean up @@ -652,6 +747,9 @@ EXPORT_SYMBOL(drm_sched_job_arm); */ void drm_sched_job_cleanup(struct drm_sched_job *job) { + struct dma_fence *fence; + unsigned long index; + if (kref_read(&job->s_fence->finished.refcount)) { /* drm_sched_job_arm() has been called */ dma_fence_put(&job->s_fence->finished); @@ -661,6 +759,12 @@ void drm_sched_job_cleanup(struct drm_sched_job *job) } 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 83afc3aa8e2f..a47946f904b6 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,17 @@ 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_add_dependency() and + * drm_sched_job_add_implicit_dependencies(). + */ + 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 +234,15 @@ 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_add_dependency() and + * drm_sched_job_add_implicit_dependencies() this can be ommitted and + * left as NULL. */ struct dma_fence *(*dependency)(struct drm_sched_job *sched_job, struct drm_sched_entity *s_entity); @@ -349,6 +369,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_add_dependency(struct drm_sched_job *job, + struct dma_fence *fence); +int drm_sched_job_add_implicit_dependencies(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

3 years, 10 months

2
1
0 0

[PATCH v5 07/20] 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). v3: Rebased over renamed functions for adding depdencies Acked-by: Emma Anholt <emma(a)anholt.net> Reviewed-by: Steven Price <steven.price(a)arm.com> (v3) 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 Cc: Emma Anholt <emma(a)anholt.net> --- drivers/gpu/drm/panfrost/panfrost_drv.c | 16 ++++++++--- drivers/gpu/drm/panfrost/panfrost_job.c | 38 ++++--------------------- drivers/gpu/drm/panfrost/panfrost_job.h | 5 +--- 3 files changed, 18 insertions(+), 41 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 1ffaef5ec5ff..16212b6b202e 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_add_dependency(&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..a98f507dc779 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,14 @@ 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_add_implicit_dependencies(job, bos[i], + true); if (ret) return ret; } @@ -269,31 +270,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 +309,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 +349,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 +738,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

3 years, 10 months

1
0
0 0

Re: [Linaro-mm-sig] [RESEND PATCH v2 2/2] drm: add lockdep assert to drm_is_current_master_locked

by Daniel Vetter

On Mon, Aug 02, 2021 at 06:59:57PM +0800, Desmond Cheong Zhi Xi wrote: > In drm_is_current_master_locked, accessing drm_file.master should be > protected by either drm_file.master_lookup_lock or > drm_device.master_mutex. This was previously awkward to assert with > lockdep. > > Following patch ("locking/lockdep: Provide lockdep_assert{,_once}() > helpers"), this assertion is now convenient. So we add in the > assertion and explain this lock design in the kerneldoc. > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> > Acked-by: Boqun Feng <boqun.feng(a)gmail.com> > Acked-by: Waiman Long <longman(a)redhat.com> > Acked-by: Peter Zijlstra (Intel) <peterz(a)infradead.org> Both patches pushed to drm-misc-next, thanks. -Daniel > --- > drivers/gpu/drm/drm_auth.c | 6 +++--- > include/drm/drm_file.h | 4 ++++ > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index 9c24b8cc8e36..6f4d7ff23c80 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -63,9 +63,9 @@ > > static bool drm_is_current_master_locked(struct drm_file *fpriv) > { > - /* Either drm_device.master_mutex or drm_file.master_lookup_lock > - * should be held here. > - */ > + lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) || > + lockdep_is_held(&fpriv->minor->dev->master_mutex)); > + > return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; > } > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > index 726cfe0ff5f5..a3acb7ac3550 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -233,6 +233,10 @@ struct drm_file { > * this only matches &drm_device.master if the master is the currently > * active one. > * > + * To update @master, both &drm_device.master_mutex and > + * @master_lookup_lock need to be held, therefore holding either of > + * them is safe and enough for the read side. > + * > * When dereferencing this pointer, either hold struct > * &drm_device.master_mutex for the duration of the pointer's use, or > * use drm_file_get_master() if struct &drm_device.master_mutex is not > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 10 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] drm/amdgpu: drop redundant null-pointer checks in amdgpu_ttm_tt_populate() and amdgpu_ttm_tt_unpopulate()

by Christian König

Am 04.08.21 um 03:51 schrieb Tuo Li: > The varialbe gtt in the function amdgpu_ttm_tt_populate() and > amdgpu_ttm_tt_unpopulate() is guaranteed to be not NULL in the context. > Thus the null-pointer checks are redundant and can be dropped. > > Reported-by: TOTE Robot <oslab(a)tsinghua.edu.cn> > Signed-off-by: Tuo Li <islituo(a)gmail.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 3a55f08e00e1..719539bd6c44 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1121,7 +1121,7 @@ static int amdgpu_ttm_tt_populate(struct ttm_device *bdev, > struct amdgpu_ttm_tt *gtt = (void *)ttm; > > /* user pages are bound by amdgpu_ttm_tt_pin_userptr() */ > - if (gtt && gtt->userptr) { > + if (gtt->userptr) { > ttm->sg = kzalloc(sizeof(struct sg_table), GFP_KERNEL); > if (!ttm->sg) > return -ENOMEM; > @@ -1146,7 +1146,7 @@ static void amdgpu_ttm_tt_unpopulate(struct ttm_device *bdev, > struct amdgpu_ttm_tt *gtt = (void *)ttm; > struct amdgpu_device *adev; > > - if (gtt && gtt->userptr) { > + if (gtt->userptr) { > amdgpu_ttm_tt_set_user_pages(ttm, NULL); > kfree(ttm->sg); > ttm->sg = NULL;

3 years, 10 months

2
1
0 0

Re: [Linaro-mm-sig] [PATCH] drm/amdgpu: fix possible null-pointer dereference in amdgpu_ttm_tt_unpopulate()

by Christian König

Am 31.07.21 um 10:13 schrieb Tuo Li: > The variable ttm is assigned to the variable gtt, and the variable gtt > is checked in: > if (gtt && gtt->userptr) > > This indicates that both ttm and gtt can be NULL. > If so, a null-pointer dereference will occur: > if (ttm->page_flags & TTM_PAGE_FLAG_SG) > > Also, some null-pointer dereferences will occur in the function > ttm_pool_free() which is called in: > return ttm_pool_free(&adev->mman.bdev.pool, ttm); > > To fix these possible null-pointer dereferences, the function returns > when ttm is NULL. NAK, same as with the other patch. The ttm object is mandatory, asking the driver to destroy a ttm object which doesn't exists makes no sense at all and is a bug in the upper layer. The NULL check is just a leftover from when the gtt and ttm objects where distinct. Please remove that one instead. BTW: Bonus points for changing the (void *) cast into a much cleaner container_of(). Thanks, Christian. > > Reported-by: TOTE Robot <oslab(a)tsinghua.edu.cn> > Signed-off-by: Tuo Li <islituo(a)gmail.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 3a55f08e00e1..0216ca085f11 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1146,7 +1146,10 @@ static void amdgpu_ttm_tt_unpopulate(struct ttm_device *bdev, > struct amdgpu_ttm_tt *gtt = (void *)ttm; > struct amdgpu_device *adev; > > - if (gtt && gtt->userptr) { > + if (ttm == NULL) > + return; > + > + if (gtt->userptr) { > amdgpu_ttm_tt_set_user_pages(ttm, NULL); > kfree(ttm->sg); > ttm->sg = NULL;

3 years, 10 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] drm/amdgpu: fix possible null-pointer dereference in amdgpu_ttm_tt_populate()

by Christian König

Am 31.07.21 um 10:04 schrieb Tuo Li: > The variable ttm is assigned to the variable gtt, and the variable gtt > is checked in: > if (gtt && gtt->userptr) > > This indicates that both ttm and gtt can be NULL. > If so, a null-pointer dereference will occur: > if (ttm->page_flags & TTM_PAGE_FLAG_SG) > > Also, some null-pointer dereferences will occur in the function > ttm_pool_alloc() which is called in: > return ttm_pool_alloc(&adev->mman.bdev.pool, ttm, ctx); > > To fix these possible null-pointer dereferences, the function returns > -EINVAL when ttm is NULL. NAK, the NULL test is just a leftover from when the objects where distinct. Please remove the NULL test instead. Regards, Christian. > > Reported-by: TOTE Robot <oslab(a)tsinghua.edu.cn> > Signed-off-by: Tuo Li <islituo(a)gmail.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 3a55f08e00e1..80440f799c09 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1120,8 +1120,11 @@ static int amdgpu_ttm_tt_populate(struct ttm_device *bdev, > struct amdgpu_device *adev = amdgpu_ttm_adev(bdev); > struct amdgpu_ttm_tt *gtt = (void *)ttm; > > + if (ttm == NULL) > + return -EINVAL; > + > /* user pages are bound by amdgpu_ttm_tt_pin_userptr() */ > - if (gtt && gtt->userptr) { > + if (gtt->userptr) { > ttm->sg = kzalloc(sizeof(struct sg_table), GFP_KERNEL); > if (!ttm->sg) > return -ENOMEM;

3 years, 10 months

1
0
0 0

[PATCH 1/3] dma-buf: nuke seqno-fence

by Christian König

Entirely unused. Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/dma-buf/Makefile | 2 +- drivers/dma-buf/seqno-fence.c | 71 ---------------------- include/linux/seqno-fence.h | 109 ---------------------------------- 3 files changed, 1 insertion(+), 181 deletions(-) delete mode 100644 drivers/dma-buf/seqno-fence.c delete mode 100644 include/linux/seqno-fence.h diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index 40d81f23cacf..1ef021273a06 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \ - dma-resv.o seqno-fence.o + dma-resv.o obj-$(CONFIG_DMABUF_HEAPS) += dma-heap.o obj-$(CONFIG_DMABUF_HEAPS) += heaps/ obj-$(CONFIG_SYNC_FILE) += sync_file.o diff --git a/drivers/dma-buf/seqno-fence.c b/drivers/dma-buf/seqno-fence.c deleted file mode 100644 index bfe14e94c488..000000000000 --- a/drivers/dma-buf/seqno-fence.c +++ /dev/null @@ -1,71 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/* - * seqno-fence, using a dma-buf to synchronize fencing - * - * Copyright (C) 2012 Texas Instruments - * Copyright (C) 2012-2014 Canonical Ltd - * Authors: - * Rob Clark <robdclark(a)gmail.com> - * Maarten Lankhorst <maarten.lankhorst(a)canonical.com> - */ - -#include <linux/slab.h> -#include <linux/export.h> -#include <linux/seqno-fence.h> - -static const char *seqno_fence_get_driver_name(struct dma_fence *fence) -{ - struct seqno_fence *seqno_fence = to_seqno_fence(fence); - - return seqno_fence->ops->get_driver_name(fence); -} - -static const char *seqno_fence_get_timeline_name(struct dma_fence *fence) -{ - struct seqno_fence *seqno_fence = to_seqno_fence(fence); - - return seqno_fence->ops->get_timeline_name(fence); -} - -static bool seqno_enable_signaling(struct dma_fence *fence) -{ - struct seqno_fence *seqno_fence = to_seqno_fence(fence); - - return seqno_fence->ops->enable_signaling(fence); -} - -static bool seqno_signaled(struct dma_fence *fence) -{ - struct seqno_fence *seqno_fence = to_seqno_fence(fence); - - return seqno_fence->ops->signaled && seqno_fence->ops->signaled(fence); -} - -static void seqno_release(struct dma_fence *fence) -{ - struct seqno_fence *f = to_seqno_fence(fence); - - dma_buf_put(f->sync_buf); - if (f->ops->release) - f->ops->release(fence); - else - dma_fence_free(&f->base); -} - -static signed long seqno_wait(struct dma_fence *fence, bool intr, - signed long timeout) -{ - struct seqno_fence *f = to_seqno_fence(fence); - - return f->ops->wait(fence, intr, timeout); -} - -const struct dma_fence_ops seqno_fence_ops = { - .get_driver_name = seqno_fence_get_driver_name, - .get_timeline_name = seqno_fence_get_timeline_name, - .enable_signaling = seqno_enable_signaling, - .signaled = seqno_signaled, - .wait = seqno_wait, - .release = seqno_release, -}; -EXPORT_SYMBOL(seqno_fence_ops); diff --git a/include/linux/seqno-fence.h b/include/linux/seqno-fence.h deleted file mode 100644 index 3cca2b8fac43..000000000000 --- a/include/linux/seqno-fence.h +++ /dev/null @@ -1,109 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* - * seqno-fence, using a dma-buf to synchronize fencing - * - * Copyright (C) 2012 Texas Instruments - * Copyright (C) 2012 Canonical Ltd - * Authors: - * Rob Clark <robdclark(a)gmail.com> - * Maarten Lankhorst <maarten.lankhorst(a)canonical.com> - */ - -#ifndef __LINUX_SEQNO_FENCE_H -#define __LINUX_SEQNO_FENCE_H - -#include <linux/dma-fence.h> -#include <linux/dma-buf.h> - -enum seqno_fence_condition { - SEQNO_FENCE_WAIT_GEQUAL, - SEQNO_FENCE_WAIT_NONZERO -}; - -struct seqno_fence { - struct dma_fence base; - - const struct dma_fence_ops *ops; - struct dma_buf *sync_buf; - uint32_t seqno_ofs; - enum seqno_fence_condition condition; -}; - -extern const struct dma_fence_ops seqno_fence_ops; - -/** - * to_seqno_fence - cast a fence to a seqno_fence - * @fence: fence to cast to a seqno_fence - * - * Returns NULL if the fence is not a seqno_fence, - * or the seqno_fence otherwise. - */ -static inline struct seqno_fence * -to_seqno_fence(struct dma_fence *fence) -{ - if (fence->ops != &seqno_fence_ops) - return NULL; - return container_of(fence, struct seqno_fence, base); -} - -/** - * seqno_fence_init - initialize a seqno fence - * @fence: seqno_fence to initialize - * @lock: pointer to spinlock to use for fence - * @sync_buf: buffer containing the memory location to signal on - * @context: the execution context this fence is a part of - * @seqno_ofs: the offset within @sync_buf - * @seqno: the sequence # to signal on - * @cond: fence wait condition - * @ops: the fence_ops for operations on this seqno fence - * - * This function initializes a struct seqno_fence with passed parameters, - * and takes a reference on sync_buf which is released on fence destruction. - * - * A seqno_fence is a dma_fence which can complete in software when - * enable_signaling is called, but it also completes when - * (s32)((sync_buf)[seqno_ofs] - seqno) >= 0 is true - * - * The seqno_fence will take a refcount on the sync_buf until it's - * destroyed, but actual lifetime of sync_buf may be longer if one of the - * callers take a reference to it. - * - * Certain hardware have instructions to insert this type of wait condition - * in the command stream, so no intervention from software would be needed. - * This type of fence can be destroyed before completed, however a reference - * on the sync_buf dma-buf can be taken. It is encouraged to re-use the same - * dma-buf for sync_buf, since mapping or unmapping the sync_buf to the - * device's vm can be expensive. - * - * It is recommended for creators of seqno_fence to call dma_fence_signal() - * before destruction. This will prevent possible issues from wraparound at - * time of issue vs time of check, since users can check dma_fence_is_signaled() - * before submitting instructions for the hardware to wait on the fence. - * However, when ops.enable_signaling is not called, it doesn't have to be - * done as soon as possible, just before there's any real danger of seqno - * wraparound. - */ -static inline void -seqno_fence_init(struct seqno_fence *fence, spinlock_t *lock, - struct dma_buf *sync_buf, uint32_t context, - uint32_t seqno_ofs, uint32_t seqno, - enum seqno_fence_condition cond, - const struct dma_fence_ops *ops) -{ - BUG_ON(!fence || !sync_buf || !ops); - BUG_ON(!ops->wait || !ops->enable_signaling || - !ops->get_driver_name || !ops->get_timeline_name); - - /* - * ops is used in dma_fence_init for get_driver_name, so needs to be - * initialized first - */ - fence->ops = ops; - dma_fence_init(&fence->base, &seqno_fence_ops, lock, context, seqno); - get_dma_buf(sync_buf); - fence->sync_buf = sync_buf; - fence->seqno_ofs = seqno_ofs; - fence->condition = cond; -} - -#endif /* __LINUX_SEQNO_FENCE_H */ -- 2.25.1

3 years, 10 months

4
14
0 0

[RFC 0/4] dma-fence: Deadline awareness

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> Based on discussion from a previous series[1] to add a "boost" mechanism when, for example, vblank deadlines are missed. Instead of a boost callback, this approach adds a way to set a deadline on the fence, by which the waiter would like to see the fence signalled. I've not yet had a chance to re-work the drm/msm part of this, but wanted to send this out as an RFC in case I don't have a chance to finish the drm/msm part this week. Original description: In some cases, like double-buffered rendering, missing vblanks can trick the GPU into running at a lower frequence, when really we want to be running at a higher frequency to not miss the vblanks in the first place. This is partially inspired by a trick i915 does, but implemented via dma-fence for a couple of reasons: 1) To continue to be able to use the atomic helpers 2) To support cases where display and gpu are different drivers [1] https://patchwork.freedesktop.org/series/90331/ Rob Clark (4): dma-fence: Add deadline awareness drm/vblank: Add helper to get next vblank time drm/atomic-helper: Set fence deadline for vblank drm/scheduler: Add fence deadline support drivers/dma-buf/dma-fence.c | 39 +++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 36 +++++++++++++++++++++++ drivers/gpu/drm/drm_vblank.c | 31 ++++++++++++++++++++ drivers/gpu/drm/scheduler/sched_fence.c | 10 +++++++ drivers/gpu/drm/scheduler/sched_main.c | 3 ++ include/drm/drm_vblank.h | 1 + include/linux/dma-fence.h | 17 +++++++++++ 7 files changed, 137 insertions(+) -- 2.31.1

3 years, 10 months

5
18
0 0

Re: [Linaro-mm-sig] [RFC 0/4] dma-fence: Deadline awareness

by Christian König

Am 27.07.21 um 17:37 schrieb Rob Clark: > On Tue, Jul 27, 2021 at 8:19 AM Michel Dänzer <michel(a)daenzer.net> wrote: >> On 2021-07-27 5:12 p.m., Rob Clark wrote: >>> On Tue, Jul 27, 2021 at 7:50 AM Michel Dänzer <michel(a)daenzer.net> wrote: >>>> On 2021-07-27 1:38 a.m., Rob Clark wrote: >>>>> From: Rob Clark <robdclark(a)chromium.org> >>>>> >>>>> Based on discussion from a previous series[1] to add a "boost" mechanism >>>>> when, for example, vblank deadlines are missed. Instead of a boost >>>>> callback, this approach adds a way to set a deadline on the fence, by >>>>> which the waiter would like to see the fence signalled. >>>>> >>>>> I've not yet had a chance to re-work the drm/msm part of this, but >>>>> wanted to send this out as an RFC in case I don't have a chance to >>>>> finish the drm/msm part this week. >>>>> >>>>> Original description: >>>>> >>>>> In some cases, like double-buffered rendering, missing vblanks can >>>>> trick the GPU into running at a lower frequence, when really we >>>>> want to be running at a higher frequency to not miss the vblanks >>>>> in the first place. >>>>> >>>>> This is partially inspired by a trick i915 does, but implemented >>>>> via dma-fence for a couple of reasons: >>>>> >>>>> 1) To continue to be able to use the atomic helpers >>>>> 2) To support cases where display and gpu are different drivers >>>>> >>>>> [1] https://patchwork.freedesktop.org/series/90331/ >>>> Unfortunately, none of these approaches will have the full intended effect once Wayland compositors start waiting for client buffers to become idle before using them for an output frame (to prevent output frames from getting delayed by client work). See https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1880 (shameless plug :) for a proof of concept of this for mutter. The boost will only affect the compositor's own GPU work, not the client work (which means no effect at all for fullscreen apps where the compositor can scan out the client buffers directly). >>>> >>> I guess you mean "no effect at all *except* for fullscreen..."? >> I meant what I wrote: The compositor will wait for the next buffer to become idle, so there's no boost from this mechanism for the client drawing to that buffer. And since the compositor does no drawing of its own in this case, there's no boost from that either. >> >> >>> I'd perhaps recommend that wayland compositors, in cases where only a >>> single layer is changing, not try to be clever and just push the >>> update down to the kernel. >> Even just for the fullscreen direct scanout case, that would require some kind of atomic KMS API extension to allow queuing multiple page flips for the same CRTC. >> >> For other cases, this would also require a mechanism to cancel a pending atomic commit, for when another surface update comes in before the compositor's deadline, which affects the previously single updating surface as well. >> > Well, in the end, there is more than one compositor out there.. and if > some wayland compositors are going this route, they can also implement > the same mechanism in userspace using the sysfs that devfreq exports. > > But it sounds simpler to me for the compositor to have a sort of "game > mode" for fullscreen games.. I'm less worried about UI interactive > workloads, boosting the GPU freq upon sudden activity after a period > of inactivity seems to work reasonably well there. At least AMD hardware is already capable of flipping frames on GPU events like finishing rendering (or uploading etc). By waiting in userspace on the CPU before send the frame to the hardware you are completely killing of such features. For composing use cases that makes sense, but certainly not for full screen applications as far as I can see. Regards, Christian. > > BR, > -R > >> -- >> Earthling Michel Dänzer | https://redhat.com >> Libre software enthusiast | Mesa and X developer

3 years, 10 months

6
27
0 0

[PATCH v4 03/18] 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. v2/3: Rebased. Reviewed-by: Steven Price <steven.price(a)arm.com> (v1) 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 89e3f6eaf519..381fbf462ea7 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 454cb6164bdc..84c30badb78e 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); @@ -637,6 +639,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 * @job: scheduler job to clean up @@ -652,6 +746,9 @@ EXPORT_SYMBOL(drm_sched_job_arm); */ void drm_sched_job_cleanup(struct drm_sched_job *job) { + struct dma_fence *fence; + unsigned long index; + if (kref_read(&job->s_fence->finished.refcount)) { /* drm_sched_job_arm() has been called */ dma_fence_put(&job->s_fence->finished); @@ -661,6 +758,12 @@ void drm_sched_job_cleanup(struct drm_sched_job *job) } 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 83afc3aa8e2f..74fb321dbc44 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); @@ -349,6 +367,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

3 years, 10 months

3
4
0 0

[PATCH v4 00/13] drm/msm: drm scheduler conversion and cleanups

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> Conversion to gpu_scheduler, and bonus removal of drm_gem_object_put_locked() v2: Fix priority mixup (msm UAPI has lower numeric priority value as higher priority, inverse of drm/scheduler) and add some comments in the UAPI header to clarify. Now that we move active refcnt get into msm_gem_submit, add a patch to mark all bos busy before pinning, to avoid evicting bos used in same batch. Fix bo locking for cmdstream dumping ($debugfs/n/{rd,hangrd}) v3: Add a patch to drop submit bo_list and instead use -EALREADY to detect errors with same obj appearing multiple times in the submit ioctl bos table. Otherwise, with struct_mutex locking dropped, we'd need to move insertion into and removal from bo_list under the obj lock. v4: One last small tweak, drop unused wait_queue_head_t in msm_fence_context Rob Clark (13): drm/msm: Docs and misc cleanup drm/msm: Small submitqueue creation cleanup drm/msm: drop drm_gem_object_put_locked() drm: Drop drm_gem_object_put_locked() drm/msm/submit: Simplify out-fence-fd handling drm/msm: Consolidate submit bo state drm/msm: Track "seqno" fences by idr drm/msm: Return ERR_PTR() from submit_create() drm/msm: Conversion to drm scheduler drm/msm: Drop submit bo_list drm/msm: Drop struct_mutex in submit path drm/msm: Utilize gpu scheduler priorities drm/msm/gem: Mark active before pinning drivers/gpu/drm/drm_gem.c | 22 -- drivers/gpu/drm/msm/Kconfig | 1 + drivers/gpu/drm/msm/adreno/a5xx_debugfs.c | 4 +- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 6 +- drivers/gpu/drm/msm/adreno/a5xx_power.c | 2 +- drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 7 +- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 12 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +- drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 4 +- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 6 +- drivers/gpu/drm/msm/msm_drv.c | 30 +- drivers/gpu/drm/msm/msm_fence.c | 42 --- drivers/gpu/drm/msm/msm_fence.h | 3 - drivers/gpu/drm/msm/msm_gem.c | 94 +----- drivers/gpu/drm/msm/msm_gem.h | 47 +-- drivers/gpu/drm/msm/msm_gem_submit.c | 344 ++++++++++++-------- drivers/gpu/drm/msm/msm_gpu.c | 46 +-- drivers/gpu/drm/msm/msm_gpu.h | 78 ++++- drivers/gpu/drm/msm/msm_rd.c | 6 +- drivers/gpu/drm/msm/msm_ringbuffer.c | 70 +++- drivers/gpu/drm/msm/msm_ringbuffer.h | 12 + drivers/gpu/drm/msm/msm_submitqueue.c | 53 ++- include/drm/drm_gem.h | 2 - include/uapi/drm/msm_drm.h | 14 +- 24 files changed, 516 insertions(+), 391 deletions(-) -- 2.31.1

3 years, 10 months

1
2
0 0

[PATCH v3 00/13] drm/msm: drm scheduler conversion and cleanups

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> Conversion to gpu_scheduler, and bonus removal of drm_gem_object_put_locked() v2: Fix priority mixup (msm UAPI has lower numeric priority value as higher priority, inverse of drm/scheduler) and add some comments in the UAPI header to clarify. Now that we move active refcnt get into msm_gem_submit, add a patch to mark all bos busy before pinning, to avoid evicting bos used in same batch. Fix bo locking for cmdstream dumping ($debugfs/n/{rd,hangrd}) v3: Add a patch to drop submit bo_list and instead use -EALREADY to detect errors with same obj appearing multiple times in the submit ioctl bos table. Otherwise, with struct_mutex locking dropped, we'd need to move insertion into and removal from bo_list under the obj lock. Rob Clark (13): drm/msm: Docs and misc cleanup drm/msm: Small submitqueue creation cleanup drm/msm: drop drm_gem_object_put_locked() drm: Drop drm_gem_object_put_locked() drm/msm/submit: Simplify out-fence-fd handling drm/msm: Consolidate submit bo state drm/msm: Track "seqno" fences by idr drm/msm: Return ERR_PTR() from submit_create() drm/msm: Conversion to drm scheduler drm/msm: Drop submit bo_list drm/msm: Drop struct_mutex in submit path drm/msm: Utilize gpu scheduler priorities drm/msm/gem: Mark active before pinning drivers/gpu/drm/drm_gem.c | 22 -- drivers/gpu/drm/msm/Kconfig | 1 + drivers/gpu/drm/msm/adreno/a5xx_debugfs.c | 4 +- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 6 +- drivers/gpu/drm/msm/adreno/a5xx_power.c | 2 +- drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 7 +- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 12 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +- drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 4 +- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 6 +- drivers/gpu/drm/msm/msm_drv.c | 30 +- drivers/gpu/drm/msm/msm_fence.c | 39 --- drivers/gpu/drm/msm/msm_fence.h | 2 - drivers/gpu/drm/msm/msm_gem.c | 94 +----- drivers/gpu/drm/msm/msm_gem.h | 47 +-- drivers/gpu/drm/msm/msm_gem_submit.c | 344 ++++++++++++-------- drivers/gpu/drm/msm/msm_gpu.c | 46 +-- drivers/gpu/drm/msm/msm_gpu.h | 78 ++++- drivers/gpu/drm/msm/msm_rd.c | 6 +- drivers/gpu/drm/msm/msm_ringbuffer.c | 70 +++- drivers/gpu/drm/msm/msm_ringbuffer.h | 12 + drivers/gpu/drm/msm/msm_submitqueue.c | 53 ++- include/drm/drm_gem.h | 2 - include/uapi/drm/msm_drm.h | 14 +- 24 files changed, 516 insertions(+), 387 deletions(-) -- 2.31.1

3 years, 10 months

1
2
0 0

[PATCH v2 00/12] drm/msm: drm scheduler conversion and cleanups

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> Conversion to gpu_scheduler, and bonus removal of drm_gem_object_put_locked() v2: Fix priority mixup (msm UAPI has lower numeric priority value as higher priority, inverse of drm/scheduler) and add some comments in the UAPI header to clarify. Now that we move active refcnt get into msm_gem_submit, add a patch to mark all bos busy before pinning, to avoid evicting bos used in same batch. Fix bo locking for cmdstream dumping ($debugfs/n/{rd,hangrd}) Rob Clark (12): drm/msm: Docs and misc cleanup drm/msm: Small submitqueue creation cleanup drm/msm: drop drm_gem_object_put_locked() drm: Drop drm_gem_object_put_locked() drm/msm/submit: Simplify out-fence-fd handling drm/msm: Consolidate submit bo state drm/msm: Track "seqno" fences by idr drm/msm: Return ERR_PTR() from submit_create() drm/msm: Conversion to drm scheduler drm/msm: Drop struct_mutex in submit path drm/msm: Utilize gpu scheduler priorities drm/msm/gem: Mark active before pinning drivers/gpu/drm/drm_gem.c | 22 -- drivers/gpu/drm/msm/Kconfig | 1 + drivers/gpu/drm/msm/adreno/a5xx_debugfs.c | 4 +- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 6 +- drivers/gpu/drm/msm/adreno/a5xx_power.c | 2 +- drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 7 +- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 12 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +- drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 4 +- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 6 +- drivers/gpu/drm/msm/msm_drv.c | 30 +- drivers/gpu/drm/msm/msm_fence.c | 39 --- drivers/gpu/drm/msm/msm_fence.h | 2 - drivers/gpu/drm/msm/msm_gem.c | 93 +----- drivers/gpu/drm/msm/msm_gem.h | 39 ++- drivers/gpu/drm/msm/msm_gem_submit.c | 316 ++++++++++++-------- drivers/gpu/drm/msm/msm_gpu.c | 46 +-- drivers/gpu/drm/msm/msm_gpu.h | 78 ++++- drivers/gpu/drm/msm/msm_rd.c | 6 +- drivers/gpu/drm/msm/msm_ringbuffer.c | 70 ++++- drivers/gpu/drm/msm/msm_ringbuffer.h | 12 + drivers/gpu/drm/msm/msm_submitqueue.c | 53 +++- include/drm/drm_gem.h | 2 - include/uapi/drm/msm_drm.h | 14 +- 24 files changed, 503 insertions(+), 363 deletions(-) -- 2.31.1

3 years, 10 months

1
2
0 0

[PATCH 0/2] drm/msm: Reduce fence signal latency

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> A couple tweaks to reduce fence signal latency. Rob Clark (2): drm/msm: Let fences read directly from memptrs drm/msm: Signal fences sooner drivers/gpu/drm/msm/msm_fence.c | 11 +++++-- drivers/gpu/drm/msm/msm_fence.h | 41 +++++++++++++++++++++++--- drivers/gpu/drm/msm/msm_gpu.c | 44 ++++++++++++++++------------ drivers/gpu/drm/msm/msm_ringbuffer.c | 2 +- 4 files changed, 73 insertions(+), 25 deletions(-) -- 2.31.1

3 years, 10 months

1
1
0 0

[PATCH] dma-buf: WARN on dmabuf release with pending attachments

by Charan Teja Reddy

It is expected from the clients to follow the below steps on an imported dmabuf fd: a) dmabuf = dma_buf_get(fd) // Get the dmabuf from fd b) dma_buf_attach(dmabuf); // Clients attach to the dmabuf o Here the kernel does some slab allocations, say for dma_buf_attachment and may be some other slab allocation in the dmabuf->ops->attach(). c) Client may need to do dma_buf_map_attachment(). d) Accordingly dma_buf_unmap_attachment() should be called. e) dma_buf_detach () // Clients detach to the dmabuf. o Here the slab allocations made in b) are freed. f) dma_buf_put(dmabuf) // Can free the dmabuf if it is the last reference. Now say an erroneous client failed at step c) above thus it directly called dma_buf_put(), step f) above. Considering that it may be the last reference to the dmabuf, buffer will be freed with pending attachments left to the dmabuf which can show up as the 'memory leak'. This should at least be reported as the WARN(). Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> --- drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 511fe0d..733c8b1 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -79,6 +79,7 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); + WARN_ON(!list_empty(&dmabuf->attachments)); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation

3 years, 10 months

3
2
0 0

[PATCH] drm/msm: Add fence->wait() op

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> Somehow we had neither ->wait() nor dma_fence_signal() calls, and no one noticed. Oops. Note that this removes the !timeout case, which has not been used in a long time. Signed-off-by: Rob Clark <robdclark(a)chromium.org> --- drivers/gpu/drm/msm/msm_fence.c | 59 +++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c index cd59a5918038..8ee96b90ded6 100644 --- a/drivers/gpu/drm/msm/msm_fence.c +++ b/drivers/gpu/drm/msm/msm_fence.c @@ -38,11 +38,10 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc return (int32_t)(fctx->completed_fence - fence) >= 0; } -/* legacy path for WAIT_FENCE ioctl: */ -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, - ktime_t *timeout, bool interruptible) +static signed long wait_fence(struct msm_fence_context *fctx, uint32_t fence, + signed long remaining_jiffies, bool interruptible) { - int ret; + signed long ret; if (fence > fctx->last_fence) { DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n", @@ -50,33 +49,34 @@ int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, return -EINVAL; } - if (!timeout) { - /* no-wait: */ - ret = fence_completed(fctx, fence) ? 0 : -EBUSY; + if (interruptible) { + ret = wait_event_interruptible_timeout(fctx->event, + fence_completed(fctx, fence), + remaining_jiffies); } else { - unsigned long remaining_jiffies = timeout_to_jiffies(timeout); - - if (interruptible) - ret = wait_event_interruptible_timeout(fctx->event, - fence_completed(fctx, fence), - remaining_jiffies); - else - ret = wait_event_timeout(fctx->event, - fence_completed(fctx, fence), - remaining_jiffies); - - if (ret == 0) { - DBG("timeout waiting for fence: %u (completed: %u)", - fence, fctx->completed_fence); - ret = -ETIMEDOUT; - } else if (ret != -ERESTARTSYS) { - ret = 0; - } + ret = wait_event_timeout(fctx->event, + fence_completed(fctx, fence), + remaining_jiffies); + } + + if (ret == 0) { + DBG("timeout waiting for fence: %u (completed: %u)", + fence, fctx->completed_fence); + ret = -ETIMEDOUT; + } else if (ret != -ERESTARTSYS) { + ret = 0; } return ret; } +/* legacy path for WAIT_FENCE ioctl: */ +int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, + ktime_t *timeout, bool interruptible) +{ + return wait_fence(fctx, fence, timeout_to_jiffies(timeout), interruptible); +} + /* called from workqueue */ void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence) { @@ -114,10 +114,19 @@ static bool msm_fence_signaled(struct dma_fence *fence) return fence_completed(f->fctx, f->base.seqno); } +static signed long msm_fence_wait(struct dma_fence *fence, bool intr, + signed long timeout) +{ + struct msm_fence *f = to_msm_fence(fence); + + return wait_fence(f->fctx, fence->seqno, timeout, intr); +} + static const struct dma_fence_ops msm_fence_ops = { .get_driver_name = msm_fence_get_driver_name, .get_timeline_name = msm_fence_get_timeline_name, .signaled = msm_fence_signaled, + .wait = msm_fence_wait, }; struct dma_fence * -- 2.31.1

3 years, 10 months

4
14
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig