Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

21 participants
3587 discussions

[PATCH] drm/syncobj: use newly allocated stub fences

by David Stevens

From: David Stevens <stevensd(a)chromium.org> Allocate a new private stub fence in drm_syncobj_assign_null_handle, instead of using a static stub fence. When userspace creates a fence with DRM_SYNCOBJ_CREATE_SIGNALED or when userspace signals a fence via DRM_IOCTL_SYNCOBJ_SIGNAL, the timestamp obtained when the fence is exported and queried with SYNC_IOC_FILE_INFO should match when the fence's status was changed from the perspective of userspace, which is during the respective ioctl. When a static stub fence started being used in by these ioctls, this behavior changed. Instead, the timestamp returned by SYNC_IOC_FILE_INFO became the first time anything used the static stub fence, which has no meaning to userspace. Signed-off-by: David Stevens <stevensd(a)chromium.org> --- drivers/dma-buf/dma-fence.c | 33 ++++++++++++++++++++++++++++++++- drivers/gpu/drm/drm_syncobj.c | 28 ++++++++++++++++++++-------- include/linux/dma-fence.h | 1 + 3 files changed, 53 insertions(+), 9 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index d64fc03929be..6081eb962490 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -26,6 +26,11 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_signaled); static DEFINE_SPINLOCK(dma_fence_stub_lock); static struct dma_fence dma_fence_stub; +struct drm_fence_private_stub { + struct dma_fence base; + spinlock_t lock; +}; + /* * fence context counter: each execution context should have its own * fence context, this allows checking if fences belong to the same @@ -123,7 +128,9 @@ static const struct dma_fence_ops dma_fence_stub_ops = { /** * dma_fence_get_stub - return a signaled fence * - * Return a stub fence which is already signaled. + * Return a stub fence which is already signaled. The fence's + * timestamp corresponds to the first time after boot this + * function is called. */ struct dma_fence *dma_fence_get_stub(void) { @@ -141,6 +148,30 @@ struct dma_fence *dma_fence_get_stub(void) } EXPORT_SYMBOL(dma_fence_get_stub); +/** + * dma_fence_allocate_private_stub - return a private, signaled fence + * + * Return a newly allocated and signaled stub fence. + */ +struct dma_fence *dma_fence_allocate_private_stub(void) +{ + struct drm_fence_private_stub *fence; + + fence = kzalloc(sizeof(*fence), GFP_KERNEL); + if (fence == NULL) + return ERR_PTR(-ENOMEM); + + spin_lock_init(&fence->lock); + dma_fence_init(&fence->base, + &dma_fence_stub_ops, + &fence->lock, + 0, 0); + dma_fence_signal(&fence->base); + + return &fence->base; +} +EXPORT_SYMBOL(dma_fence_allocate_private_stub); + /** * dma_fence_context_alloc - allocate an array of fence contexts * @num: amount of contexts to allocate diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 349146049849..c6125e57ae37 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -350,12 +350,15 @@ EXPORT_SYMBOL(drm_syncobj_replace_fence); * * Assign a already signaled stub fence to the sync object. */ -static void drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) +static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) { - struct dma_fence *fence = dma_fence_get_stub(); + struct dma_fence *fence = dma_fence_allocate_private_stub(); + if (IS_ERR(fence)) + return PTR_ERR(fence); - drm_syncobj_replace_fence(syncobj, fence); - dma_fence_put(fence); + drm_syncobj_replace_fence(syncobj, fence); + dma_fence_put(fence); + return 0; } /* 5s default for wait submission */ @@ -469,6 +472,7 @@ EXPORT_SYMBOL(drm_syncobj_free); int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, struct dma_fence *fence) { + int ret; struct drm_syncobj *syncobj; syncobj = kzalloc(sizeof(struct drm_syncobj), GFP_KERNEL); @@ -479,8 +483,13 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, INIT_LIST_HEAD(&syncobj->cb_list); spin_lock_init(&syncobj->lock); - if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) - drm_syncobj_assign_null_handle(syncobj); + if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) { + ret = drm_syncobj_assign_null_handle(syncobj); + if (ret < 0) { + drm_syncobj_put(syncobj); + return ret; + } + } if (fence) drm_syncobj_replace_fence(syncobj, fence); @@ -1322,8 +1331,11 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, if (ret < 0) return ret; - for (i = 0; i < args->count_handles; i++) - drm_syncobj_assign_null_handle(syncobjs[i]); + for (i = 0; i < args->count_handles; i++) { + ret = drm_syncobj_assign_null_handle(syncobjs[i]); + if (ret < 0) + break; + } drm_syncobj_array_free(syncobjs, args->count_handles); diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 9f12efaaa93a..6ffb4b2c6371 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -587,6 +587,7 @@ static inline signed long dma_fence_wait(struct dma_fence *fence, bool intr) } struct dma_fence *dma_fence_get_stub(void); +struct dma_fence *dma_fence_allocate_private_stub(void); u64 dma_fence_context_alloc(unsigned num); #define DMA_FENCE_TRACE(f, fmt, args...) \ -- 2.31.0.208.g409f899ff0-goog

5 years, 5 months

[PATCH v3] drm/syncobj: use newly allocated stub fences

by David Stevens

From: David Stevens <stevensd(a)chromium.org> Allocate a new private stub fence in drm_syncobj_assign_null_handle, instead of using a static stub fence. When userspace creates a fence with DRM_SYNCOBJ_CREATE_SIGNALED or when userspace signals a fence via DRM_IOCTL_SYNCOBJ_SIGNAL, the timestamp obtained when the fence is exported and queried with SYNC_IOC_FILE_INFO should match when the fence's status was changed from the perspective of userspace, which is during the respective ioctl. When a static stub fence started being used in by these ioctls, this behavior changed. Instead, the timestamp returned by SYNC_IOC_FILE_INFO became the first time anything used the static stub fence, which has no meaning to userspace. Signed-off-by: David Stevens <stevensd(a)chromium.org> --- v2 -> v3: - reuse the static stub spinlock v1 -> v2: - checkpatch style fixes drivers/dma-buf/dma-fence.c | 27 ++++++++++++++++++++++++++- drivers/gpu/drm/drm_syncobj.c | 25 +++++++++++++++++++------ include/linux/dma-fence.h | 1 + 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index d64fc03929be..ce0f5eff575d 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -123,7 +123,9 @@ static const struct dma_fence_ops dma_fence_stub_ops = { /** * dma_fence_get_stub - return a signaled fence * - * Return a stub fence which is already signaled. + * Return a stub fence which is already signaled. The fence's + * timestamp corresponds to the first time after boot this + * function is called. */ struct dma_fence *dma_fence_get_stub(void) { @@ -141,6 +143,29 @@ struct dma_fence *dma_fence_get_stub(void) } EXPORT_SYMBOL(dma_fence_get_stub); +/** + * dma_fence_allocate_private_stub - return a private, signaled fence + * + * Return a newly allocated and signaled stub fence. + */ +struct dma_fence *dma_fence_allocate_private_stub(void) +{ + struct dma_fence *fence; + + fence = kzalloc(sizeof(*fence), GFP_KERNEL); + if (fence == NULL) + return ERR_PTR(-ENOMEM); + + dma_fence_init(fence, + &dma_fence_stub_ops, + &dma_fence_stub_lock, + 0, 0); + dma_fence_signal(fence); + + return fence; +} +EXPORT_SYMBOL(dma_fence_allocate_private_stub); + /** * dma_fence_context_alloc - allocate an array of fence contexts * @num: amount of contexts to allocate diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 349146049849..a54aa850d143 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -350,12 +350,16 @@ EXPORT_SYMBOL(drm_syncobj_replace_fence); * * Assign a already signaled stub fence to the sync object. */ -static void drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) +static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) { - struct dma_fence *fence = dma_fence_get_stub(); + struct dma_fence *fence = dma_fence_allocate_private_stub(); + + if (IS_ERR(fence)) + return PTR_ERR(fence); drm_syncobj_replace_fence(syncobj, fence); dma_fence_put(fence); + return 0; } /* 5s default for wait submission */ @@ -469,6 +473,7 @@ EXPORT_SYMBOL(drm_syncobj_free); int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, struct dma_fence *fence) { + int ret; struct drm_syncobj *syncobj; syncobj = kzalloc(sizeof(struct drm_syncobj), GFP_KERNEL); @@ -479,8 +484,13 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, INIT_LIST_HEAD(&syncobj->cb_list); spin_lock_init(&syncobj->lock); - if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) - drm_syncobj_assign_null_handle(syncobj); + if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) { + ret = drm_syncobj_assign_null_handle(syncobj); + if (ret < 0) { + drm_syncobj_put(syncobj); + return ret; + } + } if (fence) drm_syncobj_replace_fence(syncobj, fence); @@ -1322,8 +1332,11 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, if (ret < 0) return ret; - for (i = 0; i < args->count_handles; i++) - drm_syncobj_assign_null_handle(syncobjs[i]); + for (i = 0; i < args->count_handles; i++) { + ret = drm_syncobj_assign_null_handle(syncobjs[i]); + if (ret < 0) + break; + } drm_syncobj_array_free(syncobjs, args->count_handles); diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 9f12efaaa93a..6ffb4b2c6371 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -587,6 +587,7 @@ static inline signed long dma_fence_wait(struct dma_fence *fence, bool intr) } struct dma_fence *dma_fence_get_stub(void); +struct dma_fence *dma_fence_allocate_private_stub(void); u64 dma_fence_context_alloc(unsigned num); #define DMA_FENCE_TRACE(f, fmt, args...) \ -- 2.31.0.208.g409f899ff0-goog

5 years, 5 months

[PATCH v2] drm/syncobj: use newly allocated stub fences

by David Stevens

From: David Stevens <stevensd(a)chromium.org> Allocate a new private stub fence in drm_syncobj_assign_null_handle, instead of using a static stub fence. When userspace creates a fence with DRM_SYNCOBJ_CREATE_SIGNALED or when userspace signals a fence via DRM_IOCTL_SYNCOBJ_SIGNAL, the timestamp obtained when the fence is exported and queried with SYNC_IOC_FILE_INFO should match when the fence's status was changed from the perspective of userspace, which is during the respective ioctl. When a static stub fence started being used in by these ioctls, this behavior changed. Instead, the timestamp returned by SYNC_IOC_FILE_INFO became the first time anything used the static stub fence, which has no meaning to userspace. Signed-off-by: David Stevens <stevensd(a)chromium.org> --- v1 -> v2: - checkpatch style fixes drivers/dma-buf/dma-fence.c | 33 ++++++++++++++++++++++++++++++++- drivers/gpu/drm/drm_syncobj.c | 25 +++++++++++++++++++------ include/linux/dma-fence.h | 1 + 3 files changed, 52 insertions(+), 7 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index d64fc03929be..6081eb962490 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -26,6 +26,11 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_signaled); static DEFINE_SPINLOCK(dma_fence_stub_lock); static struct dma_fence dma_fence_stub; +struct drm_fence_private_stub { + struct dma_fence base; + spinlock_t lock; +}; + /* * fence context counter: each execution context should have its own * fence context, this allows checking if fences belong to the same @@ -123,7 +128,9 @@ static const struct dma_fence_ops dma_fence_stub_ops = { /** * dma_fence_get_stub - return a signaled fence * - * Return a stub fence which is already signaled. + * Return a stub fence which is already signaled. The fence's + * timestamp corresponds to the first time after boot this + * function is called. */ struct dma_fence *dma_fence_get_stub(void) { @@ -141,6 +148,30 @@ struct dma_fence *dma_fence_get_stub(void) } EXPORT_SYMBOL(dma_fence_get_stub); +/** + * dma_fence_allocate_private_stub - return a private, signaled fence + * + * Return a newly allocated and signaled stub fence. + */ +struct dma_fence *dma_fence_allocate_private_stub(void) +{ + struct drm_fence_private_stub *fence; + + fence = kzalloc(sizeof(*fence), GFP_KERNEL); + if (fence == NULL) + return ERR_PTR(-ENOMEM); + + spin_lock_init(&fence->lock); + dma_fence_init(&fence->base, + &dma_fence_stub_ops, + &fence->lock, + 0, 0); + dma_fence_signal(&fence->base); + + return &fence->base; +} +EXPORT_SYMBOL(dma_fence_allocate_private_stub); + /** * dma_fence_context_alloc - allocate an array of fence contexts * @num: amount of contexts to allocate diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 349146049849..a54aa850d143 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -350,12 +350,16 @@ EXPORT_SYMBOL(drm_syncobj_replace_fence); * * Assign a already signaled stub fence to the sync object. */ -static void drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) +static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) { - struct dma_fence *fence = dma_fence_get_stub(); + struct dma_fence *fence = dma_fence_allocate_private_stub(); + + if (IS_ERR(fence)) + return PTR_ERR(fence); drm_syncobj_replace_fence(syncobj, fence); dma_fence_put(fence); + return 0; } /* 5s default for wait submission */ @@ -469,6 +473,7 @@ EXPORT_SYMBOL(drm_syncobj_free); int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, struct dma_fence *fence) { + int ret; struct drm_syncobj *syncobj; syncobj = kzalloc(sizeof(struct drm_syncobj), GFP_KERNEL); @@ -479,8 +484,13 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, INIT_LIST_HEAD(&syncobj->cb_list); spin_lock_init(&syncobj->lock); - if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) - drm_syncobj_assign_null_handle(syncobj); + if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) { + ret = drm_syncobj_assign_null_handle(syncobj); + if (ret < 0) { + drm_syncobj_put(syncobj); + return ret; + } + } if (fence) drm_syncobj_replace_fence(syncobj, fence); @@ -1322,8 +1332,11 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, if (ret < 0) return ret; - for (i = 0; i < args->count_handles; i++) - drm_syncobj_assign_null_handle(syncobjs[i]); + for (i = 0; i < args->count_handles; i++) { + ret = drm_syncobj_assign_null_handle(syncobjs[i]); + if (ret < 0) + break; + } drm_syncobj_array_free(syncobjs, args->count_handles); diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 9f12efaaa93a..6ffb4b2c6371 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -587,6 +587,7 @@ static inline signed long dma_fence_wait(struct dma_fence *fence, bool intr) } struct dma_fence *dma_fence_get_stub(void); +struct dma_fence *dma_fence_allocate_private_stub(void); u64 dma_fence_context_alloc(unsigned num); #define DMA_FENCE_TRACE(f, fmt, args...) \ -- 2.31.0.208.g409f899ff0-goog

5 years, 5 months

[PATCH] Revert "drm/syncobj: use dma_fence_get_stub"

by David Stevens

From: David Stevens <stevensd(a)chromium.org> This reverts commit 86bbd89d5da66fe760049ad3f04adc407ec0c4d6. Using the singleton stub fence in drm_syncobj_assign_null_handle means that all syncobjs created in an already signaled state or any syncobjs signaled by userspace will reference the singleton fence when exported to a sync_file. If those sync_files are queried with SYNC_IOC_FILE_INFO, then the timestamp_ns value returned will correspond to whenever the singleton stub fence was first initialized. This can break the ability of userspace to use timestamps of these fences, as the singleton stub fence's timestamp bears no relationship to any meaningful event. Signed-off-by: David Stevens <stevensd(a)chromium.org> --- drivers/gpu/drm/drm_syncobj.c | 58 ++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 349146049849..7cc11f1a83f4 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -211,6 +211,21 @@ struct syncobj_wait_entry { static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj, struct syncobj_wait_entry *wait); +struct drm_syncobj_stub_fence { + struct dma_fence base; + spinlock_t lock; +}; + +static const char *drm_syncobj_stub_fence_get_name(struct dma_fence *fence) +{ + return "syncobjstub"; +} + +static const struct dma_fence_ops drm_syncobj_stub_fence_ops = { + .get_driver_name = drm_syncobj_stub_fence_get_name, + .get_timeline_name = drm_syncobj_stub_fence_get_name, +}; + /** * drm_syncobj_find - lookup and reference a sync object. * @file_private: drm file private pointer @@ -344,18 +359,24 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, } EXPORT_SYMBOL(drm_syncobj_replace_fence); -/** - * drm_syncobj_assign_null_handle - assign a stub fence to the sync object - * @syncobj: sync object to assign the fence on - * - * Assign a already signaled stub fence to the sync object. - */ -static void drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) +static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) { - struct dma_fence *fence = dma_fence_get_stub(); + struct drm_syncobj_stub_fence *fence; - drm_syncobj_replace_fence(syncobj, fence); - dma_fence_put(fence); + fence = kzalloc(sizeof(*fence), GFP_KERNEL); + if (fence == NULL) + return -ENOMEM; + + spin_lock_init(&fence->lock); + dma_fence_init(&fence->base, &drm_syncobj_stub_fence_ops, + &fence->lock, 0, 0); + dma_fence_signal(&fence->base); + + drm_syncobj_replace_fence(syncobj, &fence->base); + + dma_fence_put(&fence->base); + + return 0; } /* 5s default for wait submission */ @@ -469,6 +490,7 @@ EXPORT_SYMBOL(drm_syncobj_free); int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, struct dma_fence *fence) { + int ret; struct drm_syncobj *syncobj; syncobj = kzalloc(sizeof(struct drm_syncobj), GFP_KERNEL); @@ -479,8 +501,13 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, INIT_LIST_HEAD(&syncobj->cb_list); spin_lock_init(&syncobj->lock); - if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) - drm_syncobj_assign_null_handle(syncobj); + if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) { + ret = drm_syncobj_assign_null_handle(syncobj); + if (ret < 0) { + drm_syncobj_put(syncobj); + return ret; + } + } if (fence) drm_syncobj_replace_fence(syncobj, fence); @@ -1322,8 +1349,11 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, if (ret < 0) return ret; - for (i = 0; i < args->count_handles; i++) - drm_syncobj_assign_null_handle(syncobjs[i]); + for (i = 0; i < args->count_handles; i++) { + ret = drm_syncobj_assign_null_handle(syncobjs[i]); + if (ret < 0) + break; + } drm_syncobj_array_free(syncobjs, args->count_handles); -- 2.31.0.208.g409f899ff0-goog

5 years, 5 months

Re: [Linaro-mm-sig] [PATCH] drm/amdgpu: Fix a potential sdma invalid access

by Christian König

Hi Qu, Am 02.04.21 um 05:18 schrieb Qu Huang: > Before dma_resv_lock(bo->base.resv, NULL) in amdgpu_bo_release_notify(), > the bo->base.resv lock may be held by ttm_mem_evict_first(), That can't happen since when bo_release_notify is called the BO has not more references and is therefore deleted. And we never evict a deleted BO, we just wait for it to become idle. Regards, Christian. > and the VRAM mem will be evicted, mem region was replaced > by Gtt mem region. amdgpu_bo_release_notify() will then > hold the bo->base.resv lock, and SDMA will get an invalid > address in amdgpu_fill_buffer(), resulting in a VMFAULT > or memory corruption. > > To avoid it, we have to hold bo->base.resv lock first, and > check whether the mem.mem_type is TTM_PL_VRAM. > > Signed-off-by: Qu Huang <jinsdb(a)126.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > index 4b29b82..8018574 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > @@ -1300,12 +1300,16 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object *bo) > if (bo->base.resv == &bo->base._resv) > amdgpu_amdkfd_remove_fence_on_pt_pd_bos(abo); > > - if (bo->mem.mem_type != TTM_PL_VRAM || !bo->mem.mm_node || > - !(abo->flags & AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE)) > + if (!(abo->flags & AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE)) > return; > > dma_resv_lock(bo->base.resv, NULL); > > + if (bo->mem.mem_type != TTM_PL_VRAM || !bo->mem.mm_node) { > + dma_resv_unlock(bo->base.resv); > + return; > + } > + > r = amdgpu_fill_buffer(abo, AMDGPU_POISON, bo->base.resv, &fence); > if (!WARN_ON(r)) { > amdgpu_bo_fence(abo, fence, false); > -- > 1.8.3.1 >

5 years, 5 months

Re: [Linaro-mm-sig] [PATCH v2 2/4] drm/msm: Avoid mutex in shrinker_count()

by Rob Clark

On Thu, Apr 1, 2021 at 8:34 AM Doug Anderson <dianders(a)chromium.org> wrote: > > Hi, > > On Wed, Mar 31, 2021 at 6:24 PM Rob Clark <robdclark(a)gmail.com> wrote: > > > > @@ -45,6 +30,9 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) > > list_for_each_entry(msm_obj, &priv->inactive_dontneed, mm_list) { > > if (freed >= sc->nr_to_scan) > > break; > > + /* Use trylock, because we cannot block on a obj that > > + * might be trying to acquire mm_lock > > + */ > > nit: I thought the above multi-line commenting style was only for > "net" subsystem? we do use the "net" style a fair bit already.. (OTOH I tend to not really care what checkpatch says) > > if (!msm_gem_trylock(&msm_obj->base)) > > continue; > > if (is_purgeable(msm_obj)) { > > @@ -56,8 +44,11 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) > > > > mutex_unlock(&priv->mm_lock); > > > > - if (freed > 0) > > + if (freed > 0) { > > trace_msm_gem_purge(freed << PAGE_SHIFT); > > + } else { > > + return SHRINK_STOP; > > + } > > It probably doesn't matter, but I wonder if we should still be > returning SHRINK_STOP if we got any trylock failures. It could > possibly be worth returning 0 in that case? On the surface, you'd think that, but there be mm dragons.. we can hit shrinker from the submit path when the obj is locked already and we are trying to allocate backing pages. We don't want to tell vmscan to keep trying, because we'll keep failing to grab that objects lock > > > @@ -75,6 +66,9 @@ vmap_shrink(struct list_head *mm_list) > > unsigned unmapped = 0; > > > > list_for_each_entry(msm_obj, mm_list, mm_list) { > > + /* Use trylock, because we cannot block on a obj that > > + * might be trying to acquire mm_lock > > + */ > > If you end up changing the commenting style above, should also be here. > > At this point this seems fine to land to me. Though I'm not an expert > on every interaction in this code, I've spent enough time starting at > it that I'm comfortable with: > > Reviewed-by: Douglas Anderson <dianders(a)chromium.org> thanks BR, -R

5 years, 5 months

[PATCH v2 0/4] drm/msm: Shrinker (and related) fixes

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> I've been spending some time looking into how things behave under high memory pressure. The first patch is a random cleanup I noticed along the way. The second improves the situation significantly when we are getting shrinker called from many threads in parallel. And the last two are $debugfs/gem fixes I needed so I could monitor the state of GEM objects (ie. how many are active/purgable/purged) while triggering high memory pressure. We could probably go a bit further with dropping the mm_lock in the shrinker->scan() loop, but this is already a pretty big improvement. The next step is probably actually to add support to unpin/evict inactive objects. (We are part way there since we have already de- coupled the iova lifetime from the pages lifetime, but there are a few sharp corners to work through.) Rob Clark (4): drm/msm: Remove unused freed llist node drm/msm: Avoid mutex in shrinker_count() drm/msm: Fix debugfs deadlock drm/msm: Improved debugfs gem stats drivers/gpu/drm/msm/msm_debugfs.c | 14 ++--- drivers/gpu/drm/msm/msm_drv.c | 4 ++ drivers/gpu/drm/msm/msm_drv.h | 15 ++++-- drivers/gpu/drm/msm/msm_fb.c | 3 +- drivers/gpu/drm/msm/msm_gem.c | 65 ++++++++++++++++++----- drivers/gpu/drm/msm/msm_gem.h | 72 +++++++++++++++++++++++--- drivers/gpu/drm/msm/msm_gem_shrinker.c | 28 ++++------ 7 files changed, 150 insertions(+), 51 deletions(-) -- 2.30.2

5 years, 5 months

Re: [Linaro-mm-sig] [PATCH] drm/amd/amdgpu/gfx_v7_0: Trivial typo fixes

by Alex Deucher

Applied. Thanks! Alex On Thu, Mar 25, 2021 at 5:26 AM Nirmoy <nirmodas(a)amd.com> wrote: > > > Reviewed-by: Nirmoy Das<nirmoy.das(a)amd.com> > > On 3/25/21 9:53 AM, Bhaskar Chowdhury wrote: > > s/acccess/access/ > > s/inferface/interface/ > > s/sequnce/sequence/ .....two different places. > > s/retrive/retrieve/ > > s/sheduling/scheduling/ > > s/independant/independent/ > > s/wether/whether/ ......two different places. > > s/emmit/emit/ > > s/synce/sync/ > > > > > > Signed-off-by: Bhaskar Chowdhury <unixbhaskar(a)gmail.com> > > --- > > drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 22 +++++++++++----------- > > 1 file changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c > > index a368724c3dfc..4502b95ddf6b 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c > > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c > > @@ -1877,7 +1877,7 @@ static void gfx_v7_0_init_compute_vmid(struct amdgpu_device *adev) > > mutex_unlock(&adev->srbm_mutex); > > > > /* Initialize all compute VMIDs to have no GDS, GWS, or OA > > - acccess. These should be enabled by FW for target VMIDs. */ > > + access. These should be enabled by FW for target VMIDs. */ > > for (i = adev->vm_manager.first_kfd_vmid; i < AMDGPU_NUM_VMID; i++) { > > WREG32(amdgpu_gds_reg_offset[i].mem_base, 0); > > WREG32(amdgpu_gds_reg_offset[i].mem_size, 0); > > @@ -2058,7 +2058,7 @@ static void gfx_v7_0_constants_init(struct amdgpu_device *adev) > > * @adev: amdgpu_device pointer > > * > > * Set up the number and offset of the CP scratch registers. > > - * NOTE: use of CP scratch registers is a legacy inferface and > > + * NOTE: use of CP scratch registers is a legacy interface and > > * is not used by default on newer asics (r6xx+). On newer asics, > > * memory buffers are used for fences rather than scratch regs. > > */ > > @@ -2172,7 +2172,7 @@ static void gfx_v7_0_ring_emit_vgt_flush(struct amdgpu_ring *ring) > > * @seq: sequence number > > * @flags: fence related flags > > * > > - * Emits a fence sequnce number on the gfx ring and flushes > > + * Emits a fence sequence number on the gfx ring and flushes > > * GPU caches. > > */ > > static void gfx_v7_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64 addr, > > @@ -2215,7 +2215,7 @@ static void gfx_v7_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64 addr, > > * @seq: sequence number > > * @flags: fence related flags > > * > > - * Emits a fence sequnce number on the compute ring and flushes > > + * Emits a fence sequence number on the compute ring and flushes > > * GPU caches. > > */ > > static void gfx_v7_0_ring_emit_fence_compute(struct amdgpu_ring *ring, > > @@ -2245,14 +2245,14 @@ static void gfx_v7_0_ring_emit_fence_compute(struct amdgpu_ring *ring, > > * gfx_v7_0_ring_emit_ib - emit an IB (Indirect Buffer) on the ring > > * > > * @ring: amdgpu_ring structure holding ring information > > - * @job: job to retrive vmid from > > + * @job: job to retrieve vmid from > > * @ib: amdgpu indirect buffer object > > * @flags: options (AMDGPU_HAVE_CTX_SWITCH) > > * > > * Emits an DE (drawing engine) or CE (constant engine) IB > > * on the gfx ring. IBs are usually generated by userspace > > * acceleration drivers and submitted to the kernel for > > - * sheduling on the ring. This function schedules the IB > > + * scheduling on the ring. This function schedules the IB > > * on the gfx ring for execution by the GPU. > > */ > > static void gfx_v7_0_ring_emit_ib_gfx(struct amdgpu_ring *ring, > > @@ -2402,7 +2402,7 @@ static int gfx_v7_0_ring_test_ib(struct amdgpu_ring *ring, long timeout) > > > > /* > > * CP. > > - * On CIK, gfx and compute now have independant command processors. > > + * On CIK, gfx and compute now have independent command processors. > > * > > * GFX > > * Gfx consists of a single ring and can process both gfx jobs and > > @@ -2630,7 +2630,7 @@ static int gfx_v7_0_cp_gfx_resume(struct amdgpu_device *adev) > > ring->wptr = 0; > > WREG32(mmCP_RB0_WPTR, lower_32_bits(ring->wptr)); > > > > - /* set the wb address wether it's enabled or not */ > > + /* set the wb address whether it's enabled or not */ > > rptr_addr = adev->wb.gpu_addr + (ring->rptr_offs * 4); > > WREG32(mmCP_RB0_RPTR_ADDR, lower_32_bits(rptr_addr)); > > WREG32(mmCP_RB0_RPTR_ADDR_HI, upper_32_bits(rptr_addr) & 0xFF); > > @@ -2985,7 +2985,7 @@ static void gfx_v7_0_mqd_init(struct amdgpu_device *adev, > > mqd->cp_hqd_pq_wptr_poll_addr_lo = wb_gpu_addr & 0xfffffffc; > > mqd->cp_hqd_pq_wptr_poll_addr_hi = upper_32_bits(wb_gpu_addr) & 0xffff; > > > > - /* set the wb address wether it's enabled or not */ > > + /* set the wb address whether it's enabled or not */ > > wb_gpu_addr = adev->wb.gpu_addr + (ring->rptr_offs * 4); > > mqd->cp_hqd_pq_rptr_report_addr_lo = wb_gpu_addr & 0xfffffffc; > > mqd->cp_hqd_pq_rptr_report_addr_hi = > > @@ -3198,7 +3198,7 @@ static int gfx_v7_0_cp_resume(struct amdgpu_device *adev) > > /** > > * gfx_v7_0_ring_emit_vm_flush - cik vm flush using the CP > > * > > - * @ring: the ring to emmit the commands to > > + * @ring: the ring to emit the commands to > > * > > * Sync the command pipeline with the PFP. E.g. wait for everything > > * to be completed. > > @@ -3220,7 +3220,7 @@ static void gfx_v7_0_ring_emit_pipeline_sync(struct amdgpu_ring *ring) > > amdgpu_ring_write(ring, 4); /* poll interval */ > > > > if (usepfp) { > > - /* synce CE with ME to prevent CE fetch CEIB before context switch done */ > > + /* sync CE with ME to prevent CE fetch CEIB before context switch done */ > > amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0)); > > amdgpu_ring_write(ring, 0); > > amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0)); > > -- > > 2.30.1 > > > _______________________________________________ > amd-gfx mailing list > amd-gfx(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

5 years, 6 months

Re: [Linaro-mm-sig] WARNING: AMDGPU DRM warning in 5.11.9

by Christian König

Hi, Am 25.03.21 um 09:17 schrieb Oleksandr Natalenko: > Hello. > > On Thu, Mar 25, 2021 at 07:57:33AM +0200, Ilkka Prusi wrote: >> On 24.3.2021 16.16, Chris Rankin wrote: >>> Hi, >>> >>> Theee warnings ares not present in my dmesg log from 5.11.8: >>> >>> [ 43.390159] ------------[ cut here ]------------ >>> [ 43.393574] WARNING: CPU: 2 PID: 1268 at >>> drivers/gpu/drm/ttm/ttm_bo.c:517 ttm_bo_release+0x172/0x282 [ttm] >>> [ 43.401940] Modules linked in: nf_nat_ftp nf_conntrack_ftp cfg80211 >> Changing WARN_ON to WARN_ON_ONCE in drivers/gpu/drm/ttm/ttm_bo.c >> ttm_bo_release() reduces the flood of messages into single splat. >> >> This warning appears to come from 57fcd550eb15bce ("drm/ttm: Warn on pinning >> without holding a reference)" and reverting it might be one choice. >> >> >>> There are others, but I am assuming there is a common cause here. >>> >>> Cheers, >>> Chris >>> >> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c >> index a76eb2c14e8c..50b53355b265 100644 >> --- a/drivers/gpu/drm/ttm/ttm_bo.c >> +++ b/drivers/gpu/drm/ttm/ttm_bo.c >> @@ -514,7 +514,7 @@ static void ttm_bo_release(struct kref *kref) >> * shrinkers, now that they are queued for >> * destruction. >> */ >> - if (WARN_ON(bo->pin_count)) { >> + if (WARN_ON_ONCE(bo->pin_count)) { >> bo->pin_count = 0; >> ttm_bo_del_from_lru(bo); >> ttm_bo_add_mem_to_lru(bo, &bo->mem); >> >> >> >> -- >> - Ilkka >> > WARN_ON_ONCE() will just hide the underlying problem. Do we know why > this happens at all? The patch was incorrectly back ported to 5.11 without also porting the driver changes to not trigger this warning back as well. We are probably going to revert it for 5.11.10. Regards, Christian. > > Same for me, BTW, with v5.11.9: > > ``` > [~]> lspci | grep VGA > 0a:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Lexa PRO [Radeon 540/540X/550/550X / RX 540X/550/550X] (rev c7) > > [ 3676.033140] ------------[ cut here ]------------ > [ 3676.033153] WARNING: CPU: 7 PID: 1318 at drivers/gpu/drm/ttm/ttm_bo.c:517 ttm_bo_release+0x375/0x500 [ttm] > … > [ 3676.033340] Hardware name: ASUS System Product Name/Pro WS X570-ACE, BIOS 3302 03/05/2021 > … > [ 3676.033469] Call Trace: > [ 3676.033473] ttm_bo_move_accel_cleanup+0x1ab/0x3a0 [ttm] > [ 3676.033478] amdgpu_bo_move+0x334/0x860 [amdgpu] > [ 3676.033580] ttm_bo_validate+0x1f1/0x2d0 [ttm] > [ 3676.033585] amdgpu_cs_bo_validate+0x9b/0x1c0 [amdgpu] > [ 3676.033665] amdgpu_cs_list_validate+0x115/0x150 [amdgpu] > [ 3676.033743] amdgpu_cs_ioctl+0x873/0x20a0 [amdgpu] > [ 3676.033960] drm_ioctl_kernel+0xb8/0x140 [drm] > [ 3676.033977] drm_ioctl+0x222/0x3c0 [drm] > [ 3676.034071] amdgpu_drm_ioctl+0x49/0x80 [amdgpu] > [ 3676.034145] __x64_sys_ioctl+0x83/0xb0 > [ 3676.034149] do_syscall_64+0x33/0x40 > … > [ 3676.034171] ---[ end trace 66e9865b027112f3 ]--- > ``` > > Thanks. >

5 years, 6 months

Re: [Linaro-mm-sig] [PATCH] i915_vma: Rename vma_lookup to i915_vma_lookup

by Daniel Vetter

On Tue, Mar 23, 2021 at 01:42:21PM +0000, Liam Howlett wrote: > Use i915 prefix to avoid name collision with future vma_lookup() in mm. > > Signed-off-by: Liam R. Howlett <Liam.Howlett(a)Oracle.com> > Reviewed-by: Matthew Wilcox (Oracle) <willy(a)infradead.org> Applied to i915-gem-next branch for 5.13. We have a bit a branch shuffling going on right now so unusal path, it should show up in linux-next through drm-next hopefully this week still. -Daniel > --- > drivers/gpu/drm/i915/i915_vma.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > index caa9b041616b..ee0028c697f6 100644 > --- a/drivers/gpu/drm/i915/i915_vma.c > +++ b/drivers/gpu/drm/i915/i915_vma.c > @@ -230,7 +230,7 @@ vma_create(struct drm_i915_gem_object *obj, > } > > static struct i915_vma * > -vma_lookup(struct drm_i915_gem_object *obj, > +i915_vma_lookup(struct drm_i915_gem_object *obj, > struct i915_address_space *vm, > const struct i915_ggtt_view *view) > { > @@ -278,7 +278,7 @@ i915_vma_instance(struct drm_i915_gem_object *obj, > GEM_BUG_ON(!atomic_read(&vm->open)); > > spin_lock(&obj->vma.lock); > - vma = vma_lookup(obj, vm, view); > + vma = i915_vma_lookup(obj, vm, view); > spin_unlock(&obj->vma.lock); > > /* vma_create() will resolve the race if another creates the vma */ > -- > 2.30.0 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years, 6 months

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig