RE: [PATCH] drm/ttm: Implement strict NUMA pool allocations
>-Original Message-
>From: dri-devel On Behalf Of
>Rajneesh Bhardwaj
>Sent: Friday, March 22, 2024 3:08 AM
>To: amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org
>Cc: felix.kuehl...@amd.com; alexander.deuc...@amd.com;
>christian.koe...@amd.com; Rajneesh Bhardwaj
>; Joe Greathouse
>
>Subject: [PATCH] drm/ttm: Implement strict NUMA pool allocations
>
>This change allows TTM to be flexible to honor NUMA localized
>allocations which can result in significant performance improvement on a
>multi socket NUMA system. On GFXIP 9.4.3 based AMD APUs, we see
>manyfold benefits of this change resulting not only in ~10% performance
>improvement in certain benchmarks but also generating more consistent
>and less sporadic results specially when the NUMA balancing is not
>explecitely disabled. In certain scenarios, workloads show a run-to-run
>variability e.g. HPL would show a ~10x performance drop after running
>back to back 4-5 times and would recover later on a subsequent run. This
>is seen with memory intensive other workloads too. It was seen that when
>CPU caches were dropped e.g. sudo sysctl -w vm.drop_caches=1, the
>variability reduced but the performance was still well below a good run.
>
>Use of __GFP_THISNODE flag ensures that during memory allocation, kernel
>prioritizes memory allocations from the local or closest NUMA node
>thereby reducing memory access latency. When memory is allocated using
>__GFP_THISNODE flag, memory allocations will predominantly be done on
>the local node, consequency, the shrinkers may priotitize reclaiming
>memory from caches assocoated with local node to maintain memory
>locality and minimize latency, thereby provide better shinker targeting.
>
>Reduced memory pressure on remote nodes, can also indirectly influence
>shrinker behavior by potentially reducing the frequency and intensity of
>memory reclamation operation on remote nodes and could provide improved
>overall system performance.
>
>While this change could be more beneficial in general, i.e., without the
>use of a module parameter, but in absence of widespread testing, limit
>it to the AMD GFXIP 9.4.3 based ttm pool initializations only.
>
>
>Cc: Joe Greathouse
>Signed-off-by: Rajneesh Bhardwaj
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 8
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 ++-
> drivers/gpu/drm/ttm/tests/ttm_pool_test.c | 10 +-
> drivers/gpu/drm/ttm/ttm_device.c | 2 +-
> drivers/gpu/drm/ttm/ttm_pool.c| 7 ++-
> include/drm/ttm/ttm_pool.h| 4 +++-
> 7 files changed, 30 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>index 9c62552bec34..96532cfc6230 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>@@ -253,6 +253,7 @@ extern int amdgpu_user_partt_mode;
> extern int amdgpu_agp;
>
> extern int amdgpu_wbrf;
>+extern bool strict_numa_alloc;
>
> #define AMDGPU_VM_MAX_NUM_CTX 4096
> #define AMDGPU_SG_THRESHOLD (256*1024*1024)
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>index 80b9642f2bc4..a183a6b4493d 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>@@ -781,6 +781,14 @@ int queue_preemption_timeout_ms = 9000;
> module_param(queue_preemption_timeout_ms, int, 0644);
> MODULE_PARM_DESC(queue_preemption_timeout_ms, "queue preemption
>timeout in ms (1 = Minimum, 9000 = default)");
>
>+/**
>+ * DOC: strict_numa_alloc(bool)
>+ * Policy to force NUMA allocation requests from the proximity NUMA domain
>only.
>+ */
>+bool strict_numa_alloc;
>+module_param(strict_numa_alloc, bool, 0444);
>+MODULE_PARM_DESC(strict_numa_alloc, "Force NUMA allocation requests
>to be satisfied from the closest node only (false = default)");
>+
> /**
> * DOC: debug_evictions(bool)
> * Enable extra debug messages to help determine the cause of evictions
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>index b0ed10f4de60..a9f78f85e28c 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>@@ -1768,6 +1768,7 @@ static int amdgpu_ttm_reserve_tmr(struct
>amdgpu_device *adev)
>
> static int amdgpu_ttm_pools_init(struct amdgpu_device *adev)
> {
>+ bool policy = true;
> int i;
>
> if (!adev->gmc.is_app_apu || !adev->gmc.num_mem_partitions)
>@@ -1779,11 +1780,15 @@ static int amdgpu_ttm_pools_init(struct
>amdgpu_device *adev)
> if (!adev->mman.ttm_pools)
> return -ENOMEM;
>
>+ /* Policy not only depends on the module param but also on the ASIC
>+ * setting use_strict_numa_alloc as well.
>+ */
> for (i = 0; i < adev->gmc.num_mem_partitions; i++) {
> ttm_pool_init(&adev->mman.ttm_pools[i], ade
RE: [PATCH v5 06/21] drm/i915: Prepare to dynamic dma-buf locking specification
>-Original Message-
>From: Dmitry Osipenko
>Sent: Tuesday, September 13, 2022 3:28 PM
>To: David Airlie ; Gerd Hoffmann ;
>Gurchetan Singh ; Chia-I Wu
>; Daniel Vetter ; Daniel Almeida
>; Gert Wollny ;
>Gustavo Padovan ; Daniel Stone
>; Tomeu Vizoso ;
>Maarten Lankhorst ; Maxime Ripard
>; Thomas Zimmermann ;
>Rob Clark ; Sumit Semwal
>; Christian König ;
>Pan, Xinhui ; Thierry Reding
>; Tomasz Figa ; Marek
>Szyprowski ; Mauro Carvalho Chehab
>; Alex Deucher ; Jani
>Nikula ; Joonas Lahtinen
>; Vivi, Rodrigo ;
>Tvrtko Ursulin ; Thomas Hellström
>; Qiang Yu ; Srinivas
>Kandagatla ; Amol Maheshwari
>; Jason Gunthorpe ; Leon
>Romanovsky ; Gross, Jurgen ; Stefano
>Stabellini ; Oleksandr Tyshchenko
>; Tomi Valkeinen ;
>Russell King ; Lucas Stach ;
>Christian Gmeiner ; Ruhl, Michael J
>
>Cc: dri-de...@lists.freedesktop.org; linux-ker...@vger.kernel.org; Dmitry
>Osipenko ; linux-me...@vger.kernel.org; linaro-mm-
>s...@lists.linaro.org; amd-gfx@lists.freedesktop.org; intel-
>g...@lists.freedesktop.org; ker...@collabora.com; virtualization@lists.linux-
>foundation.org; linux-r...@vger.kernel.org; linux-arm-
>m...@vger.kernel.org
>Subject: [PATCH v5 06/21] drm/i915: Prepare to dynamic dma-buf locking
>specification
>
>Prepare i915 driver to the common dynamic dma-buf locking convention
>by starting to use the unlocked versions of dma-buf API functions
>and handling cases where importer now holds the reservation lock.
>
>Acked-by: Christian König
>Signed-off-by: Dmitry Osipenko
>---
> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 14 ++
> .../gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 16
> 3 files changed, 23 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>index f5062d0c6333..07eee1c09aaf 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>@@ -72,7 +72,7 @@ static int i915_gem_dmabuf_vmap(struct dma_buf
>*dma_buf,
> struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
> void *vaddr;
>
>- vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
>+ vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
> if (IS_ERR(vaddr))
> return PTR_ERR(vaddr);
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>index 85482a04d158..7cab89618bad 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>@@ -290,7 +290,21 @@ void __i915_gem_object_pages_fini(struct
>drm_i915_gem_object *obj)
> __i915_gem_object_free_mmaps(obj);
>
> atomic_set(&obj->mm.pages_pin_count, 0);
>+
>+ /*
>+ * dma_buf_unmap_attachment() requires reservation to be
>+ * locked. The imported GEM shouldn't share reservation lock
>+ * and ttm_bo_cleanup_memtype_use() shouldn't be invoked for
>+ * dma-buf, so it's safe to take the lock.
>+ */
>+ if (obj->base.import_attach)
>+ i915_gem_object_lock(obj, NULL);
>+
> __i915_gem_object_put_pages(obj);
>+
>+ if (obj->base.import_attach)
>+ i915_gem_object_unlock(obj);
>+
> GEM_BUG_ON(i915_gem_object_has_pages(obj));
> }
Hi Dmitry,
I think that this looks correct and reasonable.
Reviewed-by: Michael J. Ruhl
m
>diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>index 51ed824b020c..f2f3cfad807b 100644
>--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>@@ -213,7 +213,7 @@ static int igt_dmabuf_import_same_driver(struct
>drm_i915_private *i915,
> goto out_import;
> }
>
>- st = dma_buf_map_attachment(import_attach,
>DMA_BIDIRECTIONAL);
>+ st = dma_buf_map_attachment_unlocked(import_attach,
>DMA_BIDIRECTIONAL);
> if (IS_ERR(st)) {
> err = PTR_ERR(st);
> goto out_detach;
>@@ -226,7 +226,7 @@ static int igt_dmabuf_import_same_driver(struct
>drm_i915_private *i915,
> timeout = -ETIME;
> }
> err = timeout > 0 ? 0 : timeout;
>- dma_buf_unmap_attachment(import_attach, st,
>DMA_BIDIRECTIONAL);
>+ dma_buf_unmap_attachment_unlocked(import_attach, st,
>DMA_BIDIRECTIONAL);
> out_detach:
> dma_buf_detach(dmabuf, import_attach);
> out_import:
>@@ -296,7 +296,7 @@ static int igt_dmabuf_i
RE: [PATCH v4 06/21] drm/i915: Prepare to dynamic dma-buf locking specification
>-Original Message- >From: Dmitry Osipenko >Sent: Friday, September 2, 2022 6:39 AM >To: Ruhl, Michael J ; Dmitry Osipenko >; Jani Nikula ; >Joonas Lahtinen ; Vivi, Rodrigo >; Tvrtko Ursulin ; >Thomas Hellström ; Christian König >; Chris Wilson >Cc: dri-de...@lists.freedesktop.org; linux-ker...@vger.kernel.org; linux- >me...@vger.kernel.org; linaro-mm-...@lists.linaro.org; amd- >g...@lists.freedesktop.org; intel-...@lists.freedesktop.org; >ker...@collabora.com; virtualizat...@lists.linux-foundation.org; linux- >r...@vger.kernel.org; linux-arm-...@vger.kernel.org; David Airlie >; Gerd Hoffmann ; Gurchetan Singh >; Chia-I Wu ; Daniel >Vetter ; Daniel Almeida ; >Gert Wollny ; Gustavo Padovan >; Daniel Stone ; >Tomeu Vizoso ; Maarten Lankhorst >; Maxime Ripard >; Thomas Zimmermann ; >Rob Clark ; Sumit Semwal >; Pan, Xinhui ; Thierry >Reding ; Tomasz Figa ; >Marek Szyprowski ; Mauro Carvalho Chehab >; Alex Deucher ; >Qiang Yu ; Srinivas Kandagatla >; Amol Maheshwari >; Jason Gunthorpe ; Leon >Romanovsky ; Gross, Jurgen ; Stefano >Stabellini ; Oleksandr Tyshchenko >; Tomi Valkeinen ; >Russell King ; Lucas Stach ; >Christian Gmeiner >Subject: Re: [PATCH v4 06/21] drm/i915: Prepare to dynamic dma-buf locking >specification > >02.09.2022 13:31, Dmitry Osipenko пишет: >> 01.09.2022 17:02, Ruhl, Michael J пишет: >> ... >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> @@ -331,7 +331,19 @@ static void __i915_gem_free_objects(struct >>>> drm_i915_private *i915, >>>>continue; >>>>} >>>> >>>> + /* >>>> + * dma_buf_unmap_attachment() requires reservation to be >>>> + * locked. The imported GEM shouldn't share reservation lock, >>>> + * so it's safe to take the lock. >>>> + */ >>>> + if (obj->base.import_attach) >>>> + i915_gem_object_lock(obj, NULL); >>> >>> There is a lot of stuff going here. Taking the lock may be premature... >>> >>>>__i915_gem_object_pages_fini(obj); >>> >>> The i915_gem_dmabuf.c:i915_gem_object_put_pages_dmabuf is where >>> unmap_attachment is actually called, would it make more sense to make >>> do the locking there? >> >> The __i915_gem_object_put_pages() is invoked with a held reservation >> lock, while freeing object is a special time when we know that GEM is >> unused. >> >> The __i915_gem_free_objects() was taking the lock two weeks ago until >> the change made Chris Wilson [1] reached linux-next. >> >> [1] >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux- >next.git/commit/?id=2826d447fbd60e6a05e53d5f918bceb8c04e315c >> >> I don't think we can take the lock within >> i915_gem_object_put_pages_dmabuf(), it may/should deadlock other code >paths. > >On the other hand, we can check whether the GEM's refcount number is >zero in i915_gem_object_put_pages_dmabuf() and then take the lock if >it's zero. > >Also, seems it should be possible just to bail out from >i915_gem_object_put_pages_dmabuf() if refcount=0. The further >drm_prime_gem_destroy() will take care of unmapping. Perhaps this could >be the best option, I'll give it a test. i915_gem_object_put_pages() is uses the SG, and the usage for drm_prim_gem_destroy() from __i915_gem_free_objects() doesn't use the SG because it has been "freed" already, I am not sure if that would work... Hmm.. with that in mind, maybe moving the base.import_attach check to __i915_gem_object_put_pages with your attach check? atomic_set(&obj->mm.pages_pin_count, 0); if (obj->base.import) i915_gem_object_lock(obj, NULL); __i915_gem_object_put_pages(obj); if (obj->base.import) i915_gem_object_unlock(obj, NULL); GEM_BUG_ON(i915_gem_object_has_pages(obj)); Pretty much one step up from the dmabuf interface, but we are guaranteed to not have any pinned pages? The other caller of __i915_gem_object_pages_fini is the i915_ttm move_notify which should not conflict (export side, not import side). Since it appears that not locking during the clean up is desirable, trying to make sure take the lock is taken at the last moment might be the right path? M
RE: [PATCH v4 06/21] drm/i915: Prepare to dynamic dma-buf locking specification
>-Original Message-
>From: Dmitry Osipenko
>Sent: Wednesday, August 31, 2022 11:38 AM
>To: David Airlie ; Gerd Hoffmann ;
>Gurchetan Singh ; Chia-I Wu
>; Daniel Vetter ; Daniel Almeida
>; Gert Wollny ;
>Gustavo Padovan ; Daniel Stone
>; Tomeu Vizoso ;
>Maarten Lankhorst ; Maxime Ripard
>; Thomas Zimmermann ;
>Rob Clark ; Sumit Semwal
>; Christian König ;
>Pan, Xinhui ; Thierry Reding
>; Tomasz Figa ; Marek
>Szyprowski ; Mauro Carvalho Chehab
>; Alex Deucher ; Jani
>Nikula ; Joonas Lahtinen
>; Vivi, Rodrigo ;
>Tvrtko Ursulin ; Thomas Hellström
>; Qiang Yu ; Srinivas
>Kandagatla ; Amol Maheshwari
>; Jason Gunthorpe ; Leon
>Romanovsky ; Gross, Jurgen ; Stefano
>Stabellini ; Oleksandr Tyshchenko
>; Tomi Valkeinen ;
>Russell King ; Lucas Stach ;
>Christian Gmeiner
>Cc: dri-de...@lists.freedesktop.org; linux-ker...@vger.kernel.org; Dmitry
>Osipenko ; linux-me...@vger.kernel.org; linaro-mm-
>s...@lists.linaro.org; amd-gfx@lists.freedesktop.org; intel-
>g...@lists.freedesktop.org; ker...@collabora.com; virtualization@lists.linux-
>foundation.org; linux-r...@vger.kernel.org; linux-arm-
>m...@vger.kernel.org
>Subject: [PATCH v4 06/21] drm/i915: Prepare to dynamic dma-buf locking
>specification
>
>Prepare i915 driver to the common dynamic dma-buf locking convention
>by starting to use the unlocked versions of dma-buf API functions
>and handling cases where importer now holds the reservation lock.
>
>Signed-off-by: Dmitry Osipenko
>---
> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 12
> .../gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 16
> 3 files changed, 21 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>index f5062d0c6333..07eee1c09aaf 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>@@ -72,7 +72,7 @@ static int i915_gem_dmabuf_vmap(struct dma_buf
>*dma_buf,
> struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
> void *vaddr;
>
>- vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
>+ vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
> if (IS_ERR(vaddr))
> return PTR_ERR(vaddr);
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>index 389e9f157ca5..7e2a9b02526c 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>@@ -331,7 +331,19 @@ static void __i915_gem_free_objects(struct
>drm_i915_private *i915,
> continue;
> }
>
>+ /*
>+ * dma_buf_unmap_attachment() requires reservation to be
>+ * locked. The imported GEM shouldn't share reservation lock,
>+ * so it's safe to take the lock.
>+ */
>+ if (obj->base.import_attach)
>+ i915_gem_object_lock(obj, NULL);
There is a lot of stuff going here. Taking the lock may be premature...
> __i915_gem_object_pages_fini(obj);
The i915_gem_dmabuf.c:i915_gem_object_put_pages_dmabuf is where
unmap_attachment is actually called, would it make more sense to make
do the locking there?
Mike
>+
>+ if (obj->base.import_attach)
>+ i915_gem_object_unlock(obj);
>+
> __i915_gem_free_object(obj);
>
> /* But keep the pointer alive for RCU-protected lookups */
>diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>index 62c61af77a42..9e3ed634aa0e 100644
>--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>@@ -213,7 +213,7 @@ static int igt_dmabuf_import_same_driver(struct
>drm_i915_private *i915,
> goto out_import;
> }
>
>- st = dma_buf_map_attachment(import_attach,
>DMA_BIDIRECTIONAL);
>+ st = dma_buf_map_attachment_unlocked(import_attach,
>DMA_BIDIRECTIONAL);
> if (IS_ERR(st)) {
> err = PTR_ERR(st);
> goto out_detach;
>@@ -226,7 +226,7 @@ static int igt_dmabuf_import_same_driver(struct
>drm_i915_private *i915,
> timeout = -ETIME;
> }
> err = timeout > 0 ? 0 : timeout;
>- dma_buf_unmap_attachment(import_attach, st,
>DMA_BIDIRECTIONAL);
>+ dma_buf_unmap_attachment_unlocked(import_attach, st,
>DMA_BIDIRECTIONAL);
> out_detach:
> dma_buf_detach(dmabuf, import_attach);
> out_import:
>@@ -296,7 +296,7 @@ static int igt_dmabuf_import(void *arg)
> goto out_obj;
> }
>
>- err = dma_buf_vmap(dmabuf, &map);
>+ err = dma_buf_vmap_unlocked(dmabuf, &map);
> dma_map = err ? NULL : map.vaddr;
> if (!dma_map) {
> pr_err("dma_buf_vmap failed\n");
>@@ -337,7 +337,7 @@ static int igt_dmabuf
RE: [PATCH 6/6] drm/ttm: stop allocating a dummy resource for pipelined gutting
>-Original Message-
>From: dri-devel On Behalf Of
>Christian König
>Sent: Thursday, July 7, 2022 6:25 AM
>To: intel-...@lists.freedesktop.org; dri-de...@lists.freedesktop.org;
>nouv...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
>Cc: Christian König
>Subject: [PATCH 6/6] drm/ttm: stop allocating a dummy resource for pipelined
>gutting
>
>That should not be necessary any more when drivers should at least be
>able to handle a move without a resource.
>
>Signed-off-by: Christian König
Reviewed-by: Michael J. Ruhl
M
>---
> drivers/gpu/drm/ttm/ttm_bo_util.c | 15 ++-
> 1 file changed, 2 insertions(+), 13 deletions(-)
>
>diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
>b/drivers/gpu/drm/ttm/ttm_bo_util.c
>index 1530982338e9..1e76149c62ff 100644
>--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
>+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
>@@ -603,16 +603,10 @@ EXPORT_SYMBOL(ttm_bo_move_sync_cleanup);
> */
> int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
> {
>- static const struct ttm_place sys_mem = { .mem_type =
>TTM_PL_SYSTEM };
> struct ttm_buffer_object *ghost;
>- struct ttm_resource *sys_res;
> struct ttm_tt *ttm;
> int ret;
>
>- ret = ttm_resource_alloc(bo, &sys_mem, &sys_res);
>- if (ret)
>- return ret;
>-
> /* If already idle, no need for ghost object dance. */
> ret = ttm_bo_wait(bo, false, true);
> if (ret != -EBUSY) {
>@@ -620,14 +614,13 @@ int ttm_bo_pipeline_gutting(struct
>ttm_buffer_object *bo)
> /* See comment below about clearing. */
> ret = ttm_tt_create(bo, true);
> if (ret)
>- goto error_free_sys_mem;
>+ return ret;
> } else {
> ttm_tt_unpopulate(bo->bdev, bo->ttm);
> if (bo->type == ttm_bo_type_device)
> ttm_tt_mark_for_clear(bo->ttm);
> }
> ttm_resource_free(bo, &bo->resource);
>- ttm_bo_assign_mem(bo, sys_res);
> return 0;
> }
>
>@@ -644,7 +637,7 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object
>*bo)
> ret = ttm_tt_create(bo, true);
> swap(bo->ttm, ttm);
> if (ret)
>- goto error_free_sys_mem;
>+ return ret;
>
> ret = ttm_buffer_object_transfer(bo, &ghost);
> if (ret)
>@@ -658,13 +651,9 @@ int ttm_bo_pipeline_gutting(struct
>ttm_buffer_object *bo)
> dma_resv_unlock(&ghost->base._resv);
> ttm_bo_put(ghost);
> bo->ttm = ttm;
>- ttm_bo_assign_mem(bo, sys_res);
> return 0;
>
> error_destroy_tt:
> ttm_tt_destroy(bo->bdev, ttm);
>-
>-error_free_sys_mem:
>- ttm_resource_free(bo, &sys_res);
> return ret;
> }
>--
>2.25.1
RE: [PATCH 5/6] drm/ttm: stop allocating dummy resources during BO creation
>-Original Message-
>From: dri-devel On Behalf Of
>Christian König
>Sent: Thursday, July 7, 2022 6:25 AM
>To: intel-...@lists.freedesktop.org; dri-de...@lists.freedesktop.org;
>nouv...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
>Cc: Christian König
>Subject: [PATCH 5/6] drm/ttm: stop allocating dummy resources during BO
>creation
>
>That should not be necessary any more when drivers should at least be
>able to handle the move without a resource.
>
>Signed-off-by: Christian König
Reviewed-by: Michael J. Ruhl
M
>---
> drivers/gpu/drm/ttm/ttm_bo.c | 7 ---
> 1 file changed, 7 deletions(-)
>
>diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>index a2f49bdda8a1..f491be751a2f 100644
>--- a/drivers/gpu/drm/ttm/ttm_bo.c
>+++ b/drivers/gpu/drm/ttm/ttm_bo.c
>@@ -960,7 +960,6 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
>struct ttm_buffer_object *bo,
>struct sg_table *sg, struct dma_resv *resv,
>void (*destroy) (struct ttm_buffer_object *))
> {
>- static const struct ttm_place sys_mem = { .mem_type =
>TTM_PL_SYSTEM };
> int ret;
>
> kref_init(&bo->kref);
>@@ -978,12 +977,6 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
>struct ttm_buffer_object *bo,
> bo->base.resv = &bo->base._resv;
> atomic_inc(&ttm_glob.bo_count);
>
>- ret = ttm_resource_alloc(bo, &sys_mem, &bo->resource);
>- if (unlikely(ret)) {
>- ttm_bo_put(bo);
>- return ret;
>- }
>-
> /*
>* For ttm_bo_type_device buffers, allocate
>* address space from the device.
>--
>2.25.1
RE: [Intel-gfx] [PATCH 4/6] drm/ttm: audit bo->resource usage v2
>-Original Message-
>From: Intel-gfx On Behalf Of
>Christian König
>Sent: Thursday, July 7, 2022 6:25 AM
>To: intel-...@lists.freedesktop.org; dri-de...@lists.freedesktop.org;
>nouv...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
>Cc: Christian König
>Subject: [Intel-gfx] [PATCH 4/6] drm/ttm: audit bo->resource usage v2
>
>Allow BOs to exist without backing store.
>
>v2: handle ttm_bo_move_memcpy as well.
>
>Signed-off-by: Christian König
Reviewed-by: Michael J. Ruhl
M
>---
> drivers/gpu/drm/ttm/ttm_bo.c | 16
> drivers/gpu/drm/ttm/ttm_bo_util.c | 7 +--
> 2 files changed, 13 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>index 984535d6a2d0..a2f49bdda8a1 100644
>--- a/drivers/gpu/drm/ttm/ttm_bo.c
>+++ b/drivers/gpu/drm/ttm/ttm_bo.c
>@@ -117,12 +117,13 @@ static int ttm_bo_handle_move_mem(struct
>ttm_buffer_object *bo,
> struct ttm_operation_ctx *ctx,
> struct ttm_place *hop)
> {
>- struct ttm_resource_manager *old_man, *new_man;
> struct ttm_device *bdev = bo->bdev;
>+ bool old_use_tt, new_use_tt;
> int ret;
>
>- old_man = ttm_manager_type(bdev, bo->resource->mem_type);
>- new_man = ttm_manager_type(bdev, mem->mem_type);
>+ old_use_tt = bo->resource &&
>+ ttm_manager_type(bdev, bo->resource->mem_type)-
>>use_tt;
>+ new_use_tt = ttm_manager_type(bdev, mem->mem_type)->use_tt;
>
> ttm_bo_unmap_virtual(bo);
>
>@@ -130,11 +131,11 @@ static int ttm_bo_handle_move_mem(struct
>ttm_buffer_object *bo,
>* Create and bind a ttm if required.
>*/
>
>- if (new_man->use_tt) {
>+ if (new_use_tt) {
> /* Zero init the new TTM structure if the old location should
>* have used one as well.
>*/
>- ret = ttm_tt_create(bo, old_man->use_tt);
>+ ret = ttm_tt_create(bo, old_use_tt);
> if (ret)
> goto out_err;
>
>@@ -160,8 +161,7 @@ static int ttm_bo_handle_move_mem(struct
>ttm_buffer_object *bo,
> return 0;
>
> out_err:
>- new_man = ttm_manager_type(bdev, bo->resource->mem_type);
>- if (!new_man->use_tt)
>+ if (!old_use_tt)
> ttm_bo_tt_destroy(bo);
>
> return ret;
>@@ -904,7 +904,7 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
> /*
>* Check whether we need to move buffer.
>*/
>- if (!ttm_resource_compat(bo->resource, placement)) {
>+ if (!bo->resource || !ttm_resource_compat(bo->resource,
>placement)) {
> ret = ttm_bo_move_buffer(bo, placement, ctx);
> if (ret)
> return ret;
>diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
>b/drivers/gpu/drm/ttm/ttm_bo_util.c
>index 1cbfb00c1d65..1530982338e9 100644
>--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
>+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
>@@ -137,8 +137,7 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object
>*bo,
> ttm_manager_type(bo->bdev, dst_mem->mem_type);
> struct ttm_tt *ttm = bo->ttm;
> struct ttm_resource *src_mem = bo->resource;
>- struct ttm_resource_manager *src_man =
>- ttm_manager_type(bdev, src_mem->mem_type);
>+ struct ttm_resource_manager *src_man;
> union {
> struct ttm_kmap_iter_tt tt;
> struct ttm_kmap_iter_linear_io io;
>@@ -147,6 +146,10 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object
>*bo,
> bool clear;
> int ret = 0;
>
>+ if (!src_mem)
>+ return 0;
>+
>+ src_man = ttm_manager_type(bdev, src_mem->mem_type);
> if (ttm && ((ttm->page_flags & TTM_TT_FLAG_SWAPPED) ||
> dst_man->use_tt)) {
> ret = ttm_tt_populate(bdev, ttm, ctx);
>--
>2.25.1
RE: [Intel-gfx] [PATCH 3/6] drm/nouveau: audit bo->resource usage
>-Original Message-
>From: Intel-gfx On Behalf Of
>Christian König
>Sent: Thursday, July 7, 2022 6:25 AM
>To: intel-...@lists.freedesktop.org; dri-de...@lists.freedesktop.org;
>nouv...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
>Cc: Christian König
>Subject: [Intel-gfx] [PATCH 3/6] drm/nouveau: audit bo->resource usage
>
>Make sure we can at least move and release BOs without backing store.
>
>Signed-off-by: Christian König
Reviewed-by: Michael J. Ruhl
M
>---
> drivers/gpu/drm/nouveau/nouveau_bo.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
>b/drivers/gpu/drm/nouveau/nouveau_bo.c
>index 92cd19021084..f83fb43b2e44 100644
>--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
>+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
>@@ -1006,7 +1006,8 @@ nouveau_bo_move(struct ttm_buffer_object *bo,
>bool evict,
> }
>
> /* Fake bo copy. */
>- if (old_reg->mem_type == TTM_PL_SYSTEM && !bo->ttm) {
>+ if (!old_reg || (old_reg->mem_type == TTM_PL_SYSTEM &&
>+ !bo->ttm)) {
> ttm_bo_move_null(bo, new_reg);
> goto out;
> }
>--
>2.25.1
RE: [Intel-gfx] [PATCH 2/6] drm/amdgpu: audit bo->resource usage
>-Original Message-
>From: Intel-gfx On Behalf Of
>Christian König
>Sent: Thursday, July 7, 2022 6:25 AM
>To: intel-...@lists.freedesktop.org; dri-de...@lists.freedesktop.org;
>nouv...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
>Cc: Christian König
>Subject: [Intel-gfx] [PATCH 2/6] drm/amdgpu: audit bo->resource usage
>
>Make sure we can at least move and release BOs without backing store.
>
>Signed-off-by: Christian König
Reviewed-by: Michael J. Ruhl
M
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>index d9cfe259f2a9..677d1dfab37f 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>@@ -1305,7 +1305,7 @@ 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->resource->mem_type != TTM_PL_VRAM ||
>+ if (!bo->resource || bo->resource->mem_type != TTM_PL_VRAM ||
> !(abo->flags &
>AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE) ||
> adev->in_suspend || adev->shutdown)
> return;
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>index be6f76a30ac6..3bddf266e8b5 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>@@ -471,7 +471,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object
>*bo, bool evict,
>
> adev = amdgpu_ttm_adev(bo->bdev);
>
>- if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) {
>+ if (!old_mem || (old_mem->mem_type == TTM_PL_SYSTEM &&
>+ bo->ttm == NULL)) {
> ttm_bo_move_null(bo, new_mem);
> goto out;
> }
>--
>2.25.1
RE: [PATCH 1/6] drm/ttm: rename and cleanup ttm_bo_init_reserved
>-Original Message-
>From: dri-devel On Behalf Of
>Christian König
>Sent: Thursday, July 7, 2022 6:25 AM
>To: intel-...@lists.freedesktop.org; dri-de...@lists.freedesktop.org;
>nouv...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
>Cc: Christian König
>Subject: [PATCH 1/6] drm/ttm: rename and cleanup ttm_bo_init_reserved
>
>Rename ttm_bo_init_reserved to ttm_bo_init_validate since that better
>matches what the function is actually doing.
>
>Remove the unused size parameter, move the function's kerneldoc to the
>implementation and cleanup the whole error handling.
>
>Signed-off-by: Christian König
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +-
> drivers/gpu/drm/drm_gem_vram_helper.c | 6 +-
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c| 5 +-
> drivers/gpu/drm/nouveau/nouveau_bo.c | 6 +-
> drivers/gpu/drm/qxl/qxl_object.c | 2 +-
> drivers/gpu/drm/radeon/radeon_object.c | 6 +-
> drivers/gpu/drm/ttm/ttm_bo.c | 147 +++--
> drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 12 +-
> include/drm/ttm/ttm_bo_api.h | 93 ++---
> 9 files changed, 129 insertions(+), 150 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>index 2c82b1d5a0d7..d9cfe259f2a9 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>@@ -591,7 +591,7 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
> if (!bp->destroy)
> bp->destroy = &amdgpu_bo_destroy;
>
>- r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, bp-
>>type,
>+ r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, bp->type,
>&bo->placement, page_align, &ctx, NULL,
>bp->resv, bp->destroy);
> if (unlikely(r != 0))
>diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>b/drivers/gpu/drm/drm_gem_vram_helper.c
>index d607043716d3..125160b534be 100644
>--- a/drivers/gpu/drm/drm_gem_vram_helper.c
>+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>@@ -226,9 +226,9 @@ struct drm_gem_vram_object
>*drm_gem_vram_create(struct drm_device *dev,
>* A failing ttm_bo_init will call ttm_buffer_object_destroy
>* to release gbo->bo.base and kfree gbo.
>*/
>- ret = ttm_bo_init(bdev, &gbo->bo, size, ttm_bo_type_device,
>-&gbo->placement, pg_align, false, NULL, NULL,
>-ttm_buffer_object_destroy);
>+ ret = ttm_bo_init_validate(bdev, &gbo->bo, ttm_bo_type_device,
>+ &gbo->placement, pg_align, false, NULL,
>NULL,
>+ ttm_buffer_object_destroy);
> if (ret)
> return ERR_PTR(ret);
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>index 4c25d9b2f138..70e2ed4e99df 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>@@ -1229,9 +1229,8 @@ int __i915_gem_ttm_object_init(struct
>intel_memory_region *mem,
>* Similarly, in delayed_destroy, we can't call ttm_bo_put()
>* until successful initialization.
>*/
>- ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj),
>size,
>- bo_type, &i915_sys_placement,
>- page_size >> PAGE_SHIFT,
>+ ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj),
>bo_type,
>+ &i915_sys_placement, page_size >>
>PAGE_SHIFT,
> &ctx, NULL, NULL, i915_ttm_bo_destroy);
> if (ret)
> return i915_ttm_err_to_gem(ret);
>diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
>b/drivers/gpu/drm/nouveau/nouveau_bo.c
>index 05076e530e7d..92cd19021084 100644
>--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
>+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
>@@ -307,9 +307,9 @@ nouveau_bo_init(struct nouveau_bo *nvbo, u64 size,
>int align, u32 domain,
> nouveau_bo_placement_set(nvbo, domain, 0);
> INIT_LIST_HEAD(&nvbo->io_reserve_lru);
>
>- ret = ttm_bo_init(nvbo->bo.bdev, &nvbo->bo, size, type,
>-&nvbo->placement, align >> PAGE_SHIFT, false, sg,
>-robj, nouveau_bo_del_ttm);
>+ ret = ttm_bo_init_validate(nvbo->bo.bdev, &nvbo->bo, type,
>+ &nvbo->placement, align >> PAGE_SHIFT,
>false,
>+ sg, robj, nouveau_bo_del_ttm);
> if (ret) {
> /* ttm will call nouveau_bo_del_ttm if it fails.. */
> return ret;
>diff --git a/drivers/gpu/drm/qxl/qxl_object.c
>b/drivers/gpu/drm/qxl/qxl_object.c
>index b42a657e4c2f..695d9308d1f0 100644
>--- a/drivers/gpu/drm/qxl/qxl_object.c
>+++ b/drivers/gpu/drm/qxl/qxl_object.c
>@@ -141,7 +141,7 @@ int qxl_bo_create(struct qxl_device *qdev, unsigned
>long size,
>
RE: [PATCH] drm/nouveau: Fix out-of-bounds access when deferencing MMU type
>-Original Message- >From: Ben Skeggs >Sent: Wednesday, November 11, 2020 9:39 PM >To: Ruhl, Michael J >Cc: Thomas Zimmermann ; bske...@redhat.com; >airl...@linux.ie; dan...@ffwll.ch; christian.koe...@amd.com; amd- >g...@lists.freedesktop.org; nouv...@lists.freedesktop.org; dri- >de...@lists.freedesktop.org; virtualizat...@lists.linux-foundation.org; Roland >Scheidegger ; Jason Gunthorpe ; >Huang Rui ; VMware Graphics maintai...@vmware.com>; Gerd Hoffmann ; spice- >de...@lists.freedesktop.org; Alex Deucher ; >Dave Airlie ; Likun Gao ; Felix >Kuehling ; Hawking Zhang > >Subject: Re: [PATCH] drm/nouveau: Fix out-of-bounds access when >deferencing MMU type > >On Thu, 12 Nov 2020 at 02:27, Ruhl, Michael J >wrote: >> >> >-Original Message- >> >From: Thomas Zimmermann >> >Sent: Wednesday, November 11, 2020 7:08 AM >> >To: Ruhl, Michael J ; bske...@redhat.com; >> >airl...@linux.ie; dan...@ffwll.ch; christian.koe...@amd.com >> >Cc: nouv...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; >> >Maarten Lankhorst ; Maxime Ripard >> >; Dave Airlie ; Gerd Hoffmann >> >; Alex Deucher ; >> >VMware Graphics ; Roland >> >Scheidegger ; Huang Rui ; >> >Felix Kuehling ; Hawking Zhang >> >; Jason Gunthorpe ; Likun >Gao >> >; virtualizat...@lists.linux-foundation.org; spice- >> >de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org >> >Subject: Re: [PATCH] drm/nouveau: Fix out-of-bounds access when >> >deferencing MMU type >> > >> >Hi >> > >> >Am 10.11.20 um 16:27 schrieb Ruhl, Michael J: >> >> >> >> >> >>> -Original Message- >> >>> From: Thomas Zimmermann >> >>> Sent: Tuesday, November 10, 2020 8:37 AM >> >>> To: bske...@redhat.com; airl...@linux.ie; dan...@ffwll.ch; Ruhl, >Michael J >> >>> ; christian.koe...@amd.com >> >>> Cc: nouv...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; >> >Thomas >> >>> Zimmermann ; Maarten Lankhorst >> >>> ; Maxime Ripard >> >>> ; Dave Airlie ; Gerd >Hoffmann >> >>> ; Alex Deucher ; >> >>> VMware Graphics ; Roland >> >>> Scheidegger ; Huang Rui >; >> >>> Felix Kuehling ; Hawking Zhang >> >>> ; Jason Gunthorpe ; Likun >> >Gao >> >>> ; virtualizat...@lists.linux-foundation.org; >spice- >> >>> de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org >> >>> Subject: [PATCH] drm/nouveau: Fix out-of-bounds access when >> >deferencing >> >>> MMU type >> >>> >> >>> The value of struct drm_device.ttm.type_vram can become -1 for >> >unknown >> >>> types of memory (see nouveau_ttm_init()). This leads to an out-of- >bounds >> >>> error when accessing struct nvif_mmu.type[]: >> >> >> >> Would this make more sense to just set the type_vram = 0 instead of -1? >> > >> >From what I understand, these indices refer to an internal type of MMU, >> >rsp the MMU's capabilities. However, my hardware (pre-NV50) does not >> >have an MMU at all. >> >> Yeah, and upon further review I see that my comment was completely >wrong >> (value vs. index). >> >> A better suggestion would have been, create an entry in the array that >means, >> "unsupported type" with a value of 0, but... >> >> >I agree that it would be nice to have a cleaner design that incorporates >> >this case, but resolving that would apparently require more than a bugfix. >> >> I agree. The -1 index is a special case for the platform path >> (platform != NV_DEVICE_INFO_V0_SOC). This is a fix for the issue, but not >> a complete solution. >> >> If you need it: >> Reviewed-by: Michael J. Ruhl >I've put an alternate fix for this here[1], and will get it into >drm-fixes later today. > >Ben. > >[1] >https://github.com/skeggsb/nouveau/commit/4590f7120c2f1f4aea9d8b93a2d >ae43b312d35ad This makes a lot of sense. I spent some time trying to reconcile the platform info that was not being used in this case, but didn't see the solution like this. This is pretty clean. If you would like: Reviewed-by: Michael J. Ruhl For this solution as well. Mike >> >> Thanks, >> Mike >> >> >Best regards >> >Thomas >> > >> >> >> >> Mike >> >> >> >>> >> &g
RE: [PATCH] drm/nouveau: Fix out-of-bounds access when deferencing MMU type
>-Original Message- >From: Thomas Zimmermann >Sent: Wednesday, November 11, 2020 7:08 AM >To: Ruhl, Michael J ; bske...@redhat.com; >airl...@linux.ie; dan...@ffwll.ch; christian.koe...@amd.com >Cc: nouv...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; >Maarten Lankhorst ; Maxime Ripard >; Dave Airlie ; Gerd Hoffmann >; Alex Deucher ; >VMware Graphics ; Roland >Scheidegger ; Huang Rui ; >Felix Kuehling ; Hawking Zhang >; Jason Gunthorpe ; Likun Gao >; virtualizat...@lists.linux-foundation.org; spice- >de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org >Subject: Re: [PATCH] drm/nouveau: Fix out-of-bounds access when >deferencing MMU type > >Hi > >Am 10.11.20 um 16:27 schrieb Ruhl, Michael J: >> >> >>> -Original Message- >>> From: Thomas Zimmermann >>> Sent: Tuesday, November 10, 2020 8:37 AM >>> To: bske...@redhat.com; airl...@linux.ie; dan...@ffwll.ch; Ruhl, Michael J >>> ; christian.koe...@amd.com >>> Cc: nouv...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; >Thomas >>> Zimmermann ; Maarten Lankhorst >>> ; Maxime Ripard >>> ; Dave Airlie ; Gerd Hoffmann >>> ; Alex Deucher ; >>> VMware Graphics ; Roland >>> Scheidegger ; Huang Rui ; >>> Felix Kuehling ; Hawking Zhang >>> ; Jason Gunthorpe ; Likun >Gao >>> ; virtualizat...@lists.linux-foundation.org; spice- >>> de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org >>> Subject: [PATCH] drm/nouveau: Fix out-of-bounds access when >deferencing >>> MMU type >>> >>> The value of struct drm_device.ttm.type_vram can become -1 for >unknown >>> types of memory (see nouveau_ttm_init()). This leads to an out-of-bounds >>> error when accessing struct nvif_mmu.type[]: >> >> Would this make more sense to just set the type_vram = 0 instead of -1? > >From what I understand, these indices refer to an internal type of MMU, >rsp the MMU's capabilities. However, my hardware (pre-NV50) does not >have an MMU at all. Yeah, and upon further review I see that my comment was completely wrong (value vs. index). A better suggestion would have been, create an entry in the array that means, "unsupported type" with a value of 0, but... >I agree that it would be nice to have a cleaner design that incorporates >this case, but resolving that would apparently require more than a bugfix. I agree. The -1 index is a special case for the platform path (platform != NV_DEVICE_INFO_V0_SOC). This is a fix for the issue, but not a complete solution. If you need it: Reviewed-by: Michael J. Ruhl Thanks, Mike >Best regards >Thomas > >> >> Mike >> >>> >>> [ 18.304116] >>> >=== >>> === >>> [ 18.311649] BUG: KASAN: slab-out-of-bounds in >>> nouveau_ttm_io_mem_reserve+0x17a/0x7e0 [nouveau] >>> [ 18.320415] Read of size 1 at addr 88810ffac1fe by task systemd- >>> udevd/342 >>> [ 18.327681] >>> [ 18.329208] CPU: 1 PID: 342 Comm: systemd-udevd Tainted: GE >>> 5.10.0-rc2-1-default+ #581 >>> [ 18.338681] Hardware name: Dell Inc. OptiPlex 9020/0N4YC8, BIOS A24 >>> 10/24/2018 >>> [ 18.346032] Call Trace: >>> [ 18.348536] dump_stack+0xae/0xe5 >>> [ 18.351919] print_address_description.constprop.0+0x17/0xf0 >>> [ 18.357787] ? nouveau_ttm_io_mem_reserve+0x17a/0x7e0 [nouveau] >>> [ 18.363818] __kasan_report.cold+0x20/0x38 >>> [ 18.368099] ? nouveau_ttm_io_mem_reserve+0x17a/0x7e0 [nouveau] >>> [ 18.374133] kasan_report+0x3a/0x50 >>> [ 18.377789] nouveau_ttm_io_mem_reserve+0x17a/0x7e0 [nouveau] >>> <...> >>> [ 18.767690] Allocated by task 342: >>> [ 18.773087] kasan_save_stack+0x1b/0x40 >>> [ 18.778890] __kasan_kmalloc.constprop.0+0xbf/0xd0 >>> [ 18.785646] __kmalloc_track_caller+0x1be/0x390 >>> [ 18.792165] kstrdup_const+0x46/0x70 >>> [ 18.797686] kobject_set_name_vargs+0x2f/0xb0 >>> [ 18.803992] kobject_init_and_add+0x9d/0xf0 >>> [ 18.810117] ttm_mem_global_init+0x12c/0x210 [ttm] >>> [ 18.816853] ttm_bo_global_init+0x4a/0x160 [ttm] >>> [ 18.823420] ttm_bo_device_init+0x39/0x220 [ttm] >>> [ 18.830046] nouveau_ttm_init+0x2c3/0x830 [nouveau] >>> [ 18.836929] nouveau_drm_device_init+0x1b4/0x3f0 [nouveau] >>> <...> >>> [ 19.105336] >>> >===
RE: [PATCH] drm/nouveau: Fix out-of-bounds access when deferencing MMU type
>-Original Message-
>From: Thomas Zimmermann
>Sent: Tuesday, November 10, 2020 8:37 AM
>To: bske...@redhat.com; airl...@linux.ie; dan...@ffwll.ch; Ruhl, Michael J
>; christian.koe...@amd.com
>Cc: nouv...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; Thomas
>Zimmermann ; Maarten Lankhorst
>; Maxime Ripard
>; Dave Airlie ; Gerd Hoffmann
>; Alex Deucher ;
>VMware Graphics ; Roland
>Scheidegger ; Huang Rui ;
>Felix Kuehling ; Hawking Zhang
>; Jason Gunthorpe ; Likun Gao
>; virtualizat...@lists.linux-foundation.org; spice-
>de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
>Subject: [PATCH] drm/nouveau: Fix out-of-bounds access when deferencing
>MMU type
>
>The value of struct drm_device.ttm.type_vram can become -1 for unknown
>types of memory (see nouveau_ttm_init()). This leads to an out-of-bounds
>error when accessing struct nvif_mmu.type[]:
Would this make more sense to just set the type_vram = 0 instead of -1?
Mike
>
> [ 18.304116]
>===
>===
> [ 18.311649] BUG: KASAN: slab-out-of-bounds in
>nouveau_ttm_io_mem_reserve+0x17a/0x7e0 [nouveau]
> [ 18.320415] Read of size 1 at addr 88810ffac1fe by task systemd-
>udevd/342
> [ 18.327681]
> [ 18.329208] CPU: 1 PID: 342 Comm: systemd-udevd Tainted: GE
>5.10.0-rc2-1-default+ #581
> [ 18.338681] Hardware name: Dell Inc. OptiPlex 9020/0N4YC8, BIOS A24
>10/24/2018
> [ 18.346032] Call Trace:
> [ 18.348536] dump_stack+0xae/0xe5
> [ 18.351919] print_address_description.constprop.0+0x17/0xf0
> [ 18.357787] ? nouveau_ttm_io_mem_reserve+0x17a/0x7e0 [nouveau]
> [ 18.363818] __kasan_report.cold+0x20/0x38
> [ 18.368099] ? nouveau_ttm_io_mem_reserve+0x17a/0x7e0 [nouveau]
> [ 18.374133] kasan_report+0x3a/0x50
> [ 18.377789] nouveau_ttm_io_mem_reserve+0x17a/0x7e0 [nouveau]
> <...>
> [ 18.767690] Allocated by task 342:
> [ 18.773087] kasan_save_stack+0x1b/0x40
> [ 18.778890] __kasan_kmalloc.constprop.0+0xbf/0xd0
> [ 18.785646] __kmalloc_track_caller+0x1be/0x390
> [ 18.792165] kstrdup_const+0x46/0x70
> [ 18.797686] kobject_set_name_vargs+0x2f/0xb0
> [ 18.803992] kobject_init_and_add+0x9d/0xf0
> [ 18.810117] ttm_mem_global_init+0x12c/0x210 [ttm]
> [ 18.816853] ttm_bo_global_init+0x4a/0x160 [ttm]
> [ 18.823420] ttm_bo_device_init+0x39/0x220 [ttm]
> [ 18.830046] nouveau_ttm_init+0x2c3/0x830 [nouveau]
> [ 18.836929] nouveau_drm_device_init+0x1b4/0x3f0 [nouveau]
> <...>
> [ 19.105336]
>===
>===
>
>Fix this error, by not using type_vram as an index if it's negative.
>Assume default values instead.
>
>The error was seen on Nvidia G72 hardware.
>
>Signed-off-by: Thomas Zimmermann
>Fixes: 1cf65c45183a ("drm/ttm: add caching state to ttm_bus_placement")
>Cc: Christian König
>Cc: Michael J. Ruhl
>Cc: Maarten Lankhorst
>Cc: Maxime Ripard
>Cc: Thomas Zimmermann
>Cc: David Airlie
>Cc: Daniel Vetter
>Cc: Ben Skeggs
>Cc: Dave Airlie
>Cc: Gerd Hoffmann
>Cc: Alex Deucher
>Cc: "Christian König"
>Cc: VMware Graphics
>Cc: Roland Scheidegger
>Cc: Huang Rui
>Cc: Felix Kuehling
>Cc: Hawking Zhang
>Cc: Jason Gunthorpe
>Cc: Likun Gao
>Cc: dri-de...@lists.freedesktop.org
>Cc: nouv...@lists.freedesktop.org
>Cc: virtualizat...@lists.linux-foundation.org
>Cc: spice-de...@lists.freedesktop.org
>Cc: amd-gfx@lists.freedesktop.org
>---
> drivers/gpu/drm/nouveau/nouveau_bo.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
>b/drivers/gpu/drm/nouveau/nouveau_bo.c
>index 8133377d865d..fe15299d417e 100644
>--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
>+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
>@@ -1142,9 +1142,12 @@ nouveau_ttm_io_mem_reserve(struct
>ttm_bo_device *bdev, struct ttm_resource *reg)
> struct nvkm_device *device = nvxx_device(&drm->client.device);
> struct nouveau_mem *mem = nouveau_mem(reg);
> struct nvif_mmu *mmu = &drm->client.mmu;
>- const u8 type = mmu->type[drm->ttm.type_vram].type;
>+ u8 type = 0;
> int ret;
>
>+ if (drm->ttm.type_vram >= 0)
>+ type = mmu->type[drm->ttm.type_vram].type;
>+
> mutex_lock(&drm->ttm.io_reserve_mutex);
> retry:
> switch (reg->mem_type) {
>--
>2.29.2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/dp_mst: Don't return error code when crtc is null
>-Original Message- >From: dri-devel On Behalf Of >Bhawanpreet Lakha >Sent: Friday, August 14, 2020 1:02 PM >To: mikita.lip...@amd.com; nicholas.kazlaus...@amd.com; >alexander.deuc...@amd.com >Cc: Bhawanpreet Lakha ; dri- >de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org >Subject: [PATCH] drm/dp_mst: Don't return error code when crtc is null > >[Why] >In certain cases the crtc can be NULL and returning -EINVAL causes >atomic check to fail when it shouln't. This leads to valid >configurations failing because atomic check fails. So is this a bug fix or an exception case, or an expected possibility? >From my reading of the function comments, it is not clear that >pos->port->connector might be NULL for some reason. A better explanation of why this would occur would make this a much more useful commit message. My reading is that you ran into this issue an are masking it with this fix. Rather than this is a real possibility and this is the correct fix. Mike >[How] >Don't early return if crtc is null > >Signed-off-by: Bhawanpreet Lakha >--- > drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c >b/drivers/gpu/drm/drm_dp_mst_topology.c >index 70c4b7afed12..bc90a1485699 100644 >--- a/drivers/gpu/drm/drm_dp_mst_topology.c >+++ b/drivers/gpu/drm/drm_dp_mst_topology.c >@@ -5037,8 +5037,8 @@ int drm_dp_mst_add_affected_dsc_crtcs(struct >drm_atomic_state *state, struct drm > > crtc = conn_state->crtc; > >- if (WARN_ON(!crtc)) >- return -EINVAL; >+ if (!crtc) >+ continue; > > if (!drm_dp_mst_dsc_aux_for_port(pos->port)) > continue; >-- >2.17.1 > >___ >dri-devel mailing list >dri-de...@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where appropriate
>-Original Message-
>From: dri-devel On Behalf Of
>Piotr Stankiewicz
>Sent: Tuesday, June 2, 2020 5:21 AM
>To: Alex Deucher ; Christian König
>; David Zhou ; David
>Airlie ; Daniel Vetter
>Cc: Stankiewicz, Piotr ; dri-
>de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; linux-
>ker...@vger.kernel.org
>Subject: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where
>appropriate
>
>Seeing as there is shorthand available to use when asking for any type
>of interrupt, or any type of message signalled interrupt, leverage it.
>
>Signed-off-by: Piotr Stankiewicz
>Reviewed-by: Andy Shevchenko
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>index 5ed4227f304b..6dbe173a9fd4 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>@@ -251,11 +251,11 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
> int nvec = pci_msix_vec_count(adev->pdev);
> unsigned int flags;
>
>- if (nvec <= 0) {
>+ if (nvec > 0)
>+ flags = PCI_IRQ_MSI_TYPES;
>+ else
> flags = PCI_IRQ_MSI;
>- } else {
>- flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
>- }
Minor nit:
Is it really necessary to set do this check? Can flags just
be set?
I.e.:
flags = PCI_IRQ_MSI_TYPES;
pci_alloc_irq_vector() tries stuff in order. If MSIX is not available,
it will try MSI.
M
>+
> /* we only need one vector */
> nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, flags);
> if (nvec > 0) {
>--
>2.17.2
>
>___
>dri-devel mailing list
>dri-de...@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH v3] drm/amdgpu: off by one in amdgpu_device_attr_create_groups() error handling
>-Original Message-
>From: Dan Carpenter
>Sent: Wednesday, May 20, 2020 11:26 AM
>To: Alex Deucher ; Kevin Wang
>; Ruhl, Michael J
>Cc: Christian König ; David Airlie
>; Daniel Vetter ; Evan Quan
>; Rui Huang ; Kenneth Feng
>; Yintian Tao ; Hawking Zhang
>; amd-gfx@lists.freedesktop.org; dri-
>de...@lists.freedesktop.org; linux-ker...@vger.kernel.org; kernel-
>janit...@vger.kernel.org
>Subject: [PATCH v3] drm/amdgpu: off by one in
>amdgpu_device_attr_create_groups() error handling
>
>This loop in the error handling code should start a "i - 1" and end at
>"i == 0". Currently it starts a "i" and ends at "i == 1". The result
>is that it removes one attribute that wasn't created yet, and leaks the
>zeroeth attribute.
>
>Fixes: 4e01847c38f7 ("drm/amdgpu: optimize amdgpu device attribute code")
>Signed-off-by: Dan Carpenter
>---
>v2: style change
>v3: Fix embarrassing typo in the subject
😊
Acked-by: Michael J. Ruhl
m
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>index b75362bf0742..e809534fabd4 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>@@ -1942,9 +1942,8 @@ static int amdgpu_device_attr_create_groups(struct
>amdgpu_device *adev,
> return 0;
>
> failed:
>- for (; i > 0; i--) {
>+ while (i--)
> amdgpu_device_attr_remove(adev, &attrs[i]);
>- }
>
> return ret;
> }
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH v2] drm/amdgpu: off by on in amdgpu_device_attr_create_groups() error handling
"off by on"
or
"off by one"
?
M
>-Original Message-
>From: dri-devel On Behalf Of Dan
>Carpenter
>Sent: Wednesday, May 20, 2020 9:08 AM
>To: Alex Deucher ; Kevin Wang
>
>Cc: David Airlie ; kernel-janit...@vger.kernel.org; linux-
>ker...@vger.kernel.org; amd-gfx@lists.freedesktop.org; Hawking Zhang
>; Rui Huang ; dri-
>de...@lists.freedesktop.org; Evan Quan ; Kenneth
>Feng ; Christian König
>; Yintian Tao
>Subject: [PATCH v2] drm/amdgpu: off by on in
>amdgpu_device_attr_create_groups() error handling
>
>This loop in the error handling code should start a "i - 1" and end at
>"i == 0". Currently it starts a "i" and ends at "i == 1". The result
>is that it removes one attribute that wasn't created yet, and leaks the
>zeroeth attribute.
>
>Fixes: 4e01847c38f7 ("drm/amdgpu: optimize amdgpu device attribute code")
>Signed-off-by: Dan Carpenter
>---
>v2: style change
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>index b75362bf0742..e809534fabd4 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>@@ -1942,9 +1942,8 @@ static int amdgpu_device_attr_create_groups(struct
>amdgpu_device *adev,
> return 0;
>
> failed:
>- for (; i > 0; i--) {
>+ while (i--)
> amdgpu_device_attr_remove(adev, &attrs[i]);
>- }
>
> return ret;
> }
>___
>dri-devel mailing list
>dri-de...@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH v2] drm/amd/amdgpu: cleanup coding style a bit
>-Original Message-
>From: dri-devel On Behalf Of
>Bernard Zhao
>Sent: Thursday, May 7, 2020 5:13 AM
>To: Alex Deucher ; Christian König
>; David (ChunMing) Zhou
>; David Airlie ; Daniel Vetter
>; Tom St Denis ; Sam Ravnborg
>; Ori Messinger ; Bernard
>Zhao ; amd-gfx@lists.freedesktop.org; dri-
>de...@lists.freedesktop.org; linux-ker...@vger.kernel.org
>Cc: opensource.ker...@vivo.com
>Subject: [PATCH v2] drm/amd/amdgpu: cleanup coding style a bit
>
>There is DEVICE_ATTR mechanism in separate attribute define.
>So this change is to use attr array, also use
>sysfs_create_files in init function & sysfs_remove_files in
>fini function.
>This maybe make the code a bit readable.
>
>Signed-off-by: Bernard Zhao
>
>Changes since V1:
>*Use DEVICE_ATTR mechanism
>
>Link for V1:
>*https://lore.kernel.org/patchwork/patch/1228076/
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 43 ++-
>-
> 1 file changed, 13 insertions(+), 30 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>index 82a3299e53c0..57bbc70662ff 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>@@ -148,6 +148,15 @@ static DEVICE_ATTR(mem_info_vis_vram_used,
>S_IRUGO,
> static DEVICE_ATTR(mem_info_vram_vendor, S_IRUGO,
> amdgpu_mem_info_vram_vendor, NULL);
>
>+static struct attribute *amdgpu_vram_mgr_attributes[] = {
>+ &dev_attr_mem_info_vram_total.attr,
>+ &dev_attr_mem_info_vis_vram_total.attr,
>+ &dev_attr_mem_info_vram_used.attr,
>+ &dev_attr_mem_info_vis_vram_used.attr,
>+ &dev_attr_mem_info_vram_vendor.attr,
>+ NULL
>+};
>+
> /**
> * amdgpu_vram_mgr_init - init VRAM manager and DRM MM
> *
>@@ -172,31 +181,9 @@ static int amdgpu_vram_mgr_init(struct
>ttm_mem_type_manager *man,
> man->priv = mgr;
>
> /* Add the two VRAM-related sysfs files */
>- ret = device_create_file(adev->dev,
>&dev_attr_mem_info_vram_total);
>- if (ret) {
>- DRM_ERROR("Failed to create device file
>mem_info_vram_total\n");
>- return ret;
>- }
>- ret = device_create_file(adev->dev,
>&dev_attr_mem_info_vis_vram_total);
>- if (ret) {
>- DRM_ERROR("Failed to create device file
>mem_info_vis_vram_total\n");
>- return ret;
>- }
>- ret = device_create_file(adev->dev,
>&dev_attr_mem_info_vram_used);
>- if (ret) {
>- DRM_ERROR("Failed to create device file
>mem_info_vram_used\n");
>- return ret;
>- }
>- ret = device_create_file(adev->dev,
>&dev_attr_mem_info_vis_vram_used);
>- if (ret) {
>- DRM_ERROR("Failed to create device file
>mem_info_vis_vram_used\n");
>- return ret;
>- }
>- ret = device_create_file(adev->dev,
>&dev_attr_mem_info_vram_vendor);
>- if (ret) {
>- DRM_ERROR("Failed to create device file
>mem_info_vram_vendor\n");
>- return ret;
>- }
>+ ret = sysfs_create_files(&adev->dev->kobj,
>amdgpu_vram_mgr_attributes);
>+ if (ret)
>+ DRM_ERROR("Failed to register sysfs\n");
This looks good to me.
I think that there is a new error macro (drm_err?) that you might
want to use instead of DRM_ERROR().
Otherwise:
Acked-by: Michael J. Ruhl
m
>
> return 0;
> }
>@@ -219,11 +206,7 @@ static int amdgpu_vram_mgr_fini(struct
>ttm_mem_type_manager *man)
> spin_unlock(&mgr->lock);
> kfree(mgr);
> man->priv = NULL;
>- device_remove_file(adev->dev, &dev_attr_mem_info_vram_total);
>- device_remove_file(adev->dev,
>&dev_attr_mem_info_vis_vram_total);
>- device_remove_file(adev->dev, &dev_attr_mem_info_vram_used);
>- device_remove_file(adev->dev,
>&dev_attr_mem_info_vis_vram_used);
>- device_remove_file(adev->dev,
>&dev_attr_mem_info_vram_vendor);
>+ sysfs_remove_files(&adev->dev->kobj,
>amdgpu_vram_mgr_attributes);
> return 0;
> }
>
>--
>2.26.2
>
>___
>dri-devel mailing list
>dri-de...@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] amdgpu: fixes memleak issue when init failed
>-Original Message-
>From: dri-devel On Behalf Of
>Bernard Zhao
>Sent: Tuesday, April 21, 2020 7:17 AM
>To: Alex Deucher ; Christian König
>; David (ChunMing) Zhou
>; David Airlie ; Daniel Vetter
>; Tom St Denis ; Ori Messinger
>; Sam Ravnborg ; Bernard
>Zhao ; amd-gfx@lists.freedesktop.org; dri-
>de...@lists.freedesktop.org; linux-ker...@vger.kernel.org
>Cc: opensource.ker...@vivo.com
>Subject: [PATCH] amdgpu: fixes memleak issue when init failed
>
>VRAM manager and DRM MM when init failed, there is no operaction
>to free kzalloc memory & remove device file.
>This will lead to memleak & cause stability issue.
>
>Signed-off-by: Bernard Zhao
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 24
>
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>index 82a3299e53c0..4c5fb153e6b4 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>@@ -175,30 +175,44 @@ static int amdgpu_vram_mgr_init(struct
>ttm_mem_type_manager *man,
> ret = device_create_file(adev->dev,
>&dev_attr_mem_info_vram_total);
> if (ret) {
> DRM_ERROR("Failed to create device file
>mem_info_vram_total\n");
>- return ret;
>+ goto VRAM_TOTAL_FAIL;
> }
> ret = device_create_file(adev->dev,
>&dev_attr_mem_info_vis_vram_total);
Have you looked at the DEVICE_ATTR mechanism?
It is set up to add device files. You won't get the granularity of each file,
but it has a lot more automatic-ness to setting this stuff up.
Mike
> if (ret) {
> DRM_ERROR("Failed to create device file
>mem_info_vis_vram_total\n");
>- return ret;
>+ goto VIS_VRAM_TOTA_FAIL;
> }
> ret = device_create_file(adev->dev,
>&dev_attr_mem_info_vram_used);
> if (ret) {
> DRM_ERROR("Failed to create device file
>mem_info_vram_used\n");
>- return ret;
>+ goto VRAM_USED_FAIL;
> }
> ret = device_create_file(adev->dev,
>&dev_attr_mem_info_vis_vram_used);
> if (ret) {
> DRM_ERROR("Failed to create device file
>mem_info_vis_vram_used\n");
>- return ret;
>+ goto VIS_VRAM_USED_FAIL;
> }
> ret = device_create_file(adev->dev,
>&dev_attr_mem_info_vram_vendor);
> if (ret) {
> DRM_ERROR("Failed to create device file
>mem_info_vram_vendor\n");
>- return ret;
>+ goto VRAM_VERDOR_FAIL;
> }
>
> return 0;
>+
>+VRAM_VERDOR_FAIL:
>+ device_remove_file(adev->dev,
>&dev_attr_mem_info_vis_vram_used);
>+VIS_VRAM_USED_FAIL:
>+ device_remove_file(adev->dev, &dev_attr_mem_info_vram_used);
>+RVAM_USED_FAIL:
>+ device_remove_file(adev->dev,
>&dev_attr_mem_info_vis_vram_total);
>+VIS_VRAM_TOTA_FAIL:
>+ device_remove_file(adev->dev, &dev_attr_mem_info_vram_total);
>+VRAM_TOTAL_FAIL:
>+ kfree(mgr);
>+ man->priv = NULL;
>+
>+ return ret;
> }
>
> /**
>--
>2.26.2
>
>___
>dri-devel mailing list
>dri-de...@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH 1/6] dma-buf: add peer2peer flag
>-Original Message-
>From: dri-devel On Behalf Of
>Daniel Vetter
>Sent: Wednesday, April 1, 2020 7:35 AM
>To: Christian König
>Cc: amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org
>Subject: Re: [PATCH 1/6] dma-buf: add peer2peer flag
>
>On Mon, Mar 30, 2020 at 03:55:31PM +0200, Christian König wrote:
>> Add a peer2peer flag noting that the importer can deal with device
>> resources which are not backed by pages.
>>
>> Signed-off-by: Christian König
>> ---
>> drivers/dma-buf/dma-buf.c | 2 ++
>> include/linux/dma-buf.h | 10 ++
>> 2 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index ccc9eda1bc28..570c923023e6 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -690,6 +690,8 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf,
>struct device *dev,
>>
>> attach->dev = dev;
>> attach->dmabuf = dmabuf;
>> +if (importer_ops)
>> +attach->peer2peer = importer_ops->allow_peer2peer;
>
>So an idea that crossed my mind to validate this, since we need quite some
>bad amounts of bad luck if someone accidentally introduces and access to
>struct_page in sg lists in some slowpath.
>
>On map_sg, if ->peer2peer is set, we could mangle the struct_page
>pointers, e.g. swap high bits for low bits (so that NULL stays NULL). On
>unmap_sg we obviously need to undo that, in case the exporter needs those
>pointers for its own book-keeping for some reason. I was also pondering
>just setting them all to NULL, but that might break some exporters. With
>the pointer mangling trick (especially if we flip high for low bits on 64
>where this should result in invalid addresses in almost all cases) we
>should be able to catch buggy p2p importers quite quickly.
The scatter list usage of the struct page pointer has other information in the
lower bits for keeping track of linking and other stuff. Swizzling the page
pointers will probably make the scatter list unusable.
Mike
>Thoughts? Maybe add as a follow-up patch for testing?
>-Daniel
>> attach->importer_ops = importer_ops;
>> attach->importer_priv = importer_priv;
>>
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index 1ade486fc2bb..82e0a4a64601 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -334,6 +334,14 @@ struct dma_buf {
>> * Attachment operations implemented by the importer.
>> */
>> struct dma_buf_attach_ops {
>> +/**
>> + * @allow_peer2peer:
>> + *
>> + * If this is set to true the importer must be able to handle peer
>> + * resources without struct pages.
>> + */
>> +bool allow_peer2peer;
>> +
>> /**
>> * @move_notify
>> *
>> @@ -362,6 +370,7 @@ struct dma_buf_attach_ops {
>> * @node: list of dma_buf_attachment, protected by dma_resv lock of the
>dmabuf.
>> * @sgt: cached mapping.
>> * @dir: direction of cached mapping.
>> + * @peer2peer: true if the importer can handle peer resources without
>pages.
>> * @priv: exporter specific attachment data.
>> * @importer_ops: importer operations for this attachment, if provided
>> * dma_buf_map/unmap_attachment() must be called with the dma_resv
>lock held.
>> @@ -382,6 +391,7 @@ struct dma_buf_attachment {
>> struct list_head node;
>> struct sg_table *sgt;
>> enum dma_data_direction dir;
>> +bool peer2peer;
>> const struct dma_buf_attach_ops *importer_ops;
>> void *importer_priv;
>> void *priv;
>> --
>> 2.17.1
>>
>
>--
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
>___
>dri-devel mailing list
>dri-de...@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
