Re: [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
On 5/12/21 3:05 PM, Christian König wrote: Am 12.05.21 um 15:02 schrieb Thomas Hellström: On Wed, 2021-05-12 at 09:09 +0200, Christian König wrote: Am 12.05.21 um 09:05 schrieb Thomas Hellström: On Wed, 2021-05-12 at 08:57 +0200, Christian König wrote: Am 11.05.21 um 16:28 schrieb Thomas Hellström: On 5/11/21 4:09 PM, Christian König wrote: Am 11.05.21 um 16:06 schrieb Thomas Hellström (Intel): On 5/11/21 3:58 PM, Christian König wrote: Am 11.05.21 um 15:25 schrieb Thomas Hellström: Most logical place to introduce TTM buffer objects is as an i915 gem object backend. We need to add some ops to account for added functionality like delayed delete and LRU list manipulation. Initially we support only LMEM and SYSTEM memory, but SYSTEM (which in this case means evicted LMEM objects) is not visible to i915 GEM yet. The plan is to move the i915 gem system region over to the TTM system memory type in upcoming patches. We set up GPU bindings directly both from LMEM and from the system region, as there is no need to use the legacy TTM_TT memory type. We reserve that for future porting of GGTT bindings to TTM. There are some changes to TTM to allow for purging system memory buffer objects and to refuse swapping of some objects: Unfortunately i915 gem still relies heavily on short-term object pinning, and we've chosen to keep short-term-pinned buffer objects on the TTM LRU lists for now, meaning that we need some sort of mechanism to tell TTM they are not swappable. A longer term goal is to get rid of the short- term pinning. Well just use the eviction_valuable interface for this. Yes, we do that for vram/lmem eviction, but we have nothing similar for system swapping. Do I understand you correctly that you want me to add a call to eviction_valuable() also for that instead of swap_possible()? You should already have that. eviction_valuable is called in both cases. Hmm. I can only see it called from ttm_mem_evict_first() which is not in the swapping path? Or do I miss something? Mhm, looks like my recollection was wrong. We should probably move the call into the ttm_bo_evict_swapout_allowable() function. Yes, I think we also need a convention whether it's called dma_resv locked or not, since the helper accesses bo->mem, which should really only be done under reservation. At the same point, there is value in calling this function while holding the LRU lock. You actually need to call it while holding the lock because eviction otherwise ends up in an endless loop. Trying to fix that for years, but so far no luck with that. Also, I wonder whether implementations of this callback might encounter unexpected data when called from the swapout path, because at least the helper assumes it not in system memory, since it is accessing bo- mem.start. So unless we use a separate callback for swapout, there's some auditing to be done. Please audit the existing callbacks and move the callback into the function after doing that. Thanks, Christian. Would it be OK if I also move the kref_get_unless_zero() to before ttm_bo_evict_swapout_allowable() to make the code less sensitive to surprises? No, because then you need a kref_put while holding the spinlock which is not allowed. Christian. Ugh. yes, you're right. /Thomas
Re: [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
Am 12.05.21 um 15:02 schrieb Thomas Hellström: On Wed, 2021-05-12 at 09:09 +0200, Christian König wrote: Am 12.05.21 um 09:05 schrieb Thomas Hellström: On Wed, 2021-05-12 at 08:57 +0200, Christian König wrote: Am 11.05.21 um 16:28 schrieb Thomas Hellström: On 5/11/21 4:09 PM, Christian König wrote: Am 11.05.21 um 16:06 schrieb Thomas Hellström (Intel): On 5/11/21 3:58 PM, Christian König wrote: Am 11.05.21 um 15:25 schrieb Thomas Hellström: Most logical place to introduce TTM buffer objects is as an i915 gem object backend. We need to add some ops to account for added functionality like delayed delete and LRU list manipulation. Initially we support only LMEM and SYSTEM memory, but SYSTEM (which in this case means evicted LMEM objects) is not visible to i915 GEM yet. The plan is to move the i915 gem system region over to the TTM system memory type in upcoming patches. We set up GPU bindings directly both from LMEM and from the system region, as there is no need to use the legacy TTM_TT memory type. We reserve that for future porting of GGTT bindings to TTM. There are some changes to TTM to allow for purging system memory buffer objects and to refuse swapping of some objects: Unfortunately i915 gem still relies heavily on short-term object pinning, and we've chosen to keep short-term-pinned buffer objects on the TTM LRU lists for now, meaning that we need some sort of mechanism to tell TTM they are not swappable. A longer term goal is to get rid of the short- term pinning. Well just use the eviction_valuable interface for this. Yes, we do that for vram/lmem eviction, but we have nothing similar for system swapping. Do I understand you correctly that you want me to add a call to eviction_valuable() also for that instead of swap_possible()? You should already have that. eviction_valuable is called in both cases. Hmm. I can only see it called from ttm_mem_evict_first() which is not in the swapping path? Or do I miss something? Mhm, looks like my recollection was wrong. We should probably move the call into the ttm_bo_evict_swapout_allowable() function. Yes, I think we also need a convention whether it's called dma_resv locked or not, since the helper accesses bo->mem, which should really only be done under reservation. At the same point, there is value in calling this function while holding the LRU lock. You actually need to call it while holding the lock because eviction otherwise ends up in an endless loop. Trying to fix that for years, but so far no luck with that. Also, I wonder whether implementations of this callback might encounter unexpected data when called from the swapout path, because at least the helper assumes it not in system memory, since it is accessing bo- mem.start. So unless we use a separate callback for swapout, there's some auditing to be done. Please audit the existing callbacks and move the callback into the function after doing that. Thanks, Christian. Would it be OK if I also move the kref_get_unless_zero() to before ttm_bo_evict_swapout_allowable() to make the code less sensitive to surprises? No, because then you need a kref_put while holding the spinlock which is not allowed. Christian. /Thomas Pls let me know what you think. Thanks, Thomas Christian. Thanks, Thomas
Re: [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
On Wed, 2021-05-12 at 09:09 +0200, Christian König wrote: > Am 12.05.21 um 09:05 schrieb Thomas Hellström: > > On Wed, 2021-05-12 at 08:57 +0200, Christian König wrote: > > > Am 11.05.21 um 16:28 schrieb Thomas Hellström: > > > > On 5/11/21 4:09 PM, Christian König wrote: > > > > > > > > > > Am 11.05.21 um 16:06 schrieb Thomas Hellström (Intel): > > > > > > On 5/11/21 3:58 PM, Christian König wrote: > > > > > > > Am 11.05.21 um 15:25 schrieb Thomas Hellström: > > > > > > > > Most logical place to introduce TTM buffer objects is > > > > > > > > as an > > > > > > > > i915 > > > > > > > > gem object backend. We need to add some ops to account > > > > > > > > for > > > > > > > > added > > > > > > > > functionality like delayed delete and LRU list > > > > > > > > manipulation. > > > > > > > > > > > > > > > > Initially we support only LMEM and SYSTEM memory, but > > > > > > > > SYSTEM > > > > > > > > (which in this case means evicted LMEM objects) is not > > > > > > > > visible to i915 GEM yet. The plan is to move the i915 > > > > > > > > gem > > > > > > > > system > > > > > > > > region > > > > > > > > over to the TTM system memory type in upcoming patches. > > > > > > > > > > > > > > > > We set up GPU bindings directly both from LMEM and from > > > > > > > > the > > > > > > > > system > > > > > > > > region, > > > > > > > > as there is no need to use the legacy TTM_TT memory > > > > > > > > type. > > > > > > > > We reserve > > > > > > > > that for future porting of GGTT bindings to TTM. > > > > > > > > > > > > > > > > There are some changes to TTM to allow for purging > > > > > > > > system > > > > > > > > memory > > > > > > > > buffer > > > > > > > > objects and to refuse swapping of some objects: > > > > > > > > Unfortunately i915 > > > > > > > > gem > > > > > > > > still relies heavily on short-term object pinning, and > > > > > > > > we've > > > > > > > > chosen to > > > > > > > > keep short-term-pinned buffer objects on the TTM LRU > > > > > > > > lists > > > > > > > > for now, > > > > > > > > meaning that we need some sort of mechanism to tell TTM > > > > > > > > they are not > > > > > > > > swappable. A longer term goal is to get rid of the > > > > > > > > short- > > > > > > > > term > > > > > > > > pinning. > > > > > > > Well just use the eviction_valuable interface for this. > > > > > > Yes, we do that for vram/lmem eviction, but we have nothing > > > > > > similar > > > > > > for system swapping. Do I understand you correctly that you > > > > > > want me > > > > > > to add a call to eviction_valuable() also for that instead > > > > > > of > > > > > > swap_possible()? > > > > > You should already have that. eviction_valuable is called in > > > > > both > > > > > cases. > > > > > > > > > Hmm. I can only see it called from ttm_mem_evict_first() which > > > > is > > > > not > > > > in the swapping path? Or do I miss something? > > > Mhm, looks like my recollection was wrong. We should probably > > > move > > > the > > > call into the ttm_bo_evict_swapout_allowable() function. > > Yes, I think we also need a convention whether it's called dma_resv > > locked or not, since the helper accesses bo->mem, which should > > really > > only be done under reservation. At the same point, there is value > > in > > calling this function while holding the LRU lock. > > You actually need to call it while holding the lock because eviction > otherwise ends up in an endless loop. > > Trying to fix that for years, but so far no luck with that. > > > Also, I wonder whether implementations of this callback might > > encounter > > unexpected data when called from the swapout path, because at least > > the > > helper assumes it not in system memory, since it is accessing bo- > > > mem.start. > > So unless we use a separate callback for swapout, there's some > > auditing > > to be done. > > Please audit the existing callbacks and move the callback into the > function after doing that. > > Thanks, > Christian. Would it be OK if I also move the kref_get_unless_zero() to before ttm_bo_evict_swapout_allowable() to make the code less sensitive to surprises? /Thomas > > > > > Pls let me know what you think. > > Thanks, > > Thomas > > > > > > > > > Christian. > > > > > > > Thanks, > > > > > > > > Thomas > > > > > > > > > > > > > > >
Re: [Intel-gfx] [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
Hi, Matthew,
Thanks for reviewing!
On 5/12/21 1:45 PM, Matthew Auld wrote:
On Tue, 11 May 2021 at 14:26, Thomas Hellström
wrote:
Most logical place to introduce TTM buffer objects is as an i915
gem object backend. We need to add some ops to account for added
functionality like delayed delete and LRU list manipulation.
Initially we support only LMEM and SYSTEM memory, but SYSTEM
(which in this case means evicted LMEM objects) is not
visible to i915 GEM yet. The plan is to move the i915 gem system region
over to the TTM system memory type in upcoming patches.
We set up GPU bindings directly both from LMEM and from the system region,
as there is no need to use the legacy TTM_TT memory type. We reserve
that for future porting of GGTT bindings to TTM.
There are some changes to TTM to allow for purging system memory buffer
objects and to refuse swapping of some objects: Unfortunately i915 gem
still relies heavily on short-term object pinning, and we've chosen to
keep short-term-pinned buffer objects on the TTM LRU lists for now,
meaning that we need some sort of mechanism to tell TTM they are not
swappable. A longer term goal is to get rid of the short-term pinning.
Remove the old lmem backend.
Cc: Christian König
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 83 ---
drivers/gpu/drm/i915/gem/i915_gem_lmem.h | 5 -
drivers/gpu/drm/i915/gem/i915_gem_object.c| 126 +++--
drivers/gpu/drm/i915/gem/i915_gem_object.h| 9 +
.../gpu/drm/i915/gem/i915_gem_object_types.h | 18 +
drivers/gpu/drm/i915/gem/i915_gem_region.c| 6 +-
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 534 ++
drivers/gpu/drm/i915/gem/i915_gem_ttm.h | 48 ++
drivers/gpu/drm/i915/gt/intel_region_lmem.c | 3 +-
drivers/gpu/drm/i915/i915_gem.c | 5 +-
drivers/gpu/drm/i915/intel_memory_region.c| 1 -
drivers/gpu/drm/i915/intel_memory_region.h| 1 -
drivers/gpu/drm/i915/intel_region_ttm.c | 5 +-
drivers/gpu/drm/i915/intel_region_ttm.h | 7 +-
drivers/gpu/drm/ttm/ttm_bo.c | 12 +
include/drm/ttm/ttm_device.h | 9 +
17 files changed, 733 insertions(+), 140 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm.c
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 958ccc1edfed..ef0d884a9e2d 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -155,6 +155,7 @@ gem-y += \
gem/i915_gem_stolen.o \
gem/i915_gem_throttle.o \
gem/i915_gem_tiling.o \
+ gem/i915_gem_ttm.o \
gem/i915_gem_ttm_bo_util.o \
gem/i915_gem_userptr.o \
gem/i915_gem_wait.o \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
index f42803ea48f2..2b8cd15de1d9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
@@ -4,73 +4,10 @@
*/
#include "intel_memory_region.h"
-#include "intel_region_ttm.h"
#include "gem/i915_gem_region.h"
#include "gem/i915_gem_lmem.h"
#include "i915_drv.h"
-static void lmem_put_pages(struct drm_i915_gem_object *obj,
- struct sg_table *pages)
-{
- intel_region_ttm_node_free(obj->mm.region, obj->mm.st_mm_node);
- obj->mm.dirty = false;
- sg_free_table(pages);
- kfree(pages);
-}
-
-static int lmem_get_pages(struct drm_i915_gem_object *obj)
-{
- unsigned int flags;
- struct sg_table *pages;
-
- flags = I915_ALLOC_MIN_PAGE_SIZE;
- if (obj->flags & I915_BO_ALLOC_CONTIGUOUS)
- flags |= I915_ALLOC_CONTIGUOUS;
-
- obj->mm.st_mm_node = intel_region_ttm_node_alloc(obj->mm.region,
-obj->base.size,
-flags);
- if (IS_ERR(obj->mm.st_mm_node))
- return PTR_ERR(obj->mm.st_mm_node);
-
- /* Range manager is always contigous */
- if (obj->mm.region->is_range_manager)
- obj->flags |= I915_BO_ALLOC_CONTIGUOUS;
- pages = intel_region_ttm_node_to_st(obj->mm.region, obj->mm.st_mm_node);
- if (IS_ERR(pages))
- return PTR_ERR(pages);
-
- __i915_gem_object_set_pages(obj, pages,
- i915_sg_dma_page_sizes(pages->sgl));
-
- if (obj->flags & I915_BO_ALLOC_CPU_CLEAR) {
- void __iomem *vaddr =
- i915_gem_object_lmem_io_map(obj, 0, obj->base.size);
Where did the object clearing go? I'm not seeing it in the new code.
It's in the move callback with TTM. If the object had not been
previously initialized,
the copying is skipped, and a fill is done instead.
+
+static struct sg_table *
Re: [Intel-gfx] [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
On Tue, 11 May 2021 at 14:26, Thomas Hellström
wrote:
>
> Most logical place to introduce TTM buffer objects is as an i915
> gem object backend. We need to add some ops to account for added
> functionality like delayed delete and LRU list manipulation.
>
> Initially we support only LMEM and SYSTEM memory, but SYSTEM
> (which in this case means evicted LMEM objects) is not
> visible to i915 GEM yet. The plan is to move the i915 gem system region
> over to the TTM system memory type in upcoming patches.
>
> We set up GPU bindings directly both from LMEM and from the system region,
> as there is no need to use the legacy TTM_TT memory type. We reserve
> that for future porting of GGTT bindings to TTM.
>
> There are some changes to TTM to allow for purging system memory buffer
> objects and to refuse swapping of some objects: Unfortunately i915 gem
> still relies heavily on short-term object pinning, and we've chosen to
> keep short-term-pinned buffer objects on the TTM LRU lists for now,
> meaning that we need some sort of mechanism to tell TTM they are not
> swappable. A longer term goal is to get rid of the short-term pinning.
>
> Remove the old lmem backend.
>
> Cc: Christian König
> Signed-off-by: Thomas Hellström
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 83 ---
> drivers/gpu/drm/i915/gem/i915_gem_lmem.h | 5 -
> drivers/gpu/drm/i915/gem/i915_gem_object.c| 126 +++--
> drivers/gpu/drm/i915/gem/i915_gem_object.h| 9 +
> .../gpu/drm/i915/gem/i915_gem_object_types.h | 18 +
> drivers/gpu/drm/i915/gem/i915_gem_region.c| 6 +-
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 534 ++
> drivers/gpu/drm/i915/gem/i915_gem_ttm.h | 48 ++
> drivers/gpu/drm/i915/gt/intel_region_lmem.c | 3 +-
> drivers/gpu/drm/i915/i915_gem.c | 5 +-
> drivers/gpu/drm/i915/intel_memory_region.c| 1 -
> drivers/gpu/drm/i915/intel_memory_region.h| 1 -
> drivers/gpu/drm/i915/intel_region_ttm.c | 5 +-
> drivers/gpu/drm/i915/intel_region_ttm.h | 7 +-
> drivers/gpu/drm/ttm/ttm_bo.c | 12 +
> include/drm/ttm/ttm_device.h | 9 +
> 17 files changed, 733 insertions(+), 140 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 958ccc1edfed..ef0d884a9e2d 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -155,6 +155,7 @@ gem-y += \
> gem/i915_gem_stolen.o \
> gem/i915_gem_throttle.o \
> gem/i915_gem_tiling.o \
> + gem/i915_gem_ttm.o \
> gem/i915_gem_ttm_bo_util.o \
> gem/i915_gem_userptr.o \
> gem/i915_gem_wait.o \
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
> b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
> index f42803ea48f2..2b8cd15de1d9 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
> @@ -4,73 +4,10 @@
> */
>
> #include "intel_memory_region.h"
> -#include "intel_region_ttm.h"
> #include "gem/i915_gem_region.h"
> #include "gem/i915_gem_lmem.h"
> #include "i915_drv.h"
>
> -static void lmem_put_pages(struct drm_i915_gem_object *obj,
> - struct sg_table *pages)
> -{
> - intel_region_ttm_node_free(obj->mm.region, obj->mm.st_mm_node);
> - obj->mm.dirty = false;
> - sg_free_table(pages);
> - kfree(pages);
> -}
> -
> -static int lmem_get_pages(struct drm_i915_gem_object *obj)
> -{
> - unsigned int flags;
> - struct sg_table *pages;
> -
> - flags = I915_ALLOC_MIN_PAGE_SIZE;
> - if (obj->flags & I915_BO_ALLOC_CONTIGUOUS)
> - flags |= I915_ALLOC_CONTIGUOUS;
> -
> - obj->mm.st_mm_node = intel_region_ttm_node_alloc(obj->mm.region,
> -obj->base.size,
> -flags);
> - if (IS_ERR(obj->mm.st_mm_node))
> - return PTR_ERR(obj->mm.st_mm_node);
> -
> - /* Range manager is always contigous */
> - if (obj->mm.region->is_range_manager)
> - obj->flags |= I915_BO_ALLOC_CONTIGUOUS;
> - pages = intel_region_ttm_node_to_st(obj->mm.region,
> obj->mm.st_mm_node);
> - if (IS_ERR(pages))
> - return PTR_ERR(pages);
> -
> - __i915_gem_object_set_pages(obj, pages,
> - i915_sg_dma_page_sizes(pages->sgl));
> -
> - if (obj->flags & I915_BO_ALLOC_CPU_CLEAR) {
> - void __iomem *vaddr =
> - i915_gem_object_lmem_io_map(obj, 0, obj->base.size);
Where did the object clearing go? I'm not seeing it in the new code.
> +
> +static struct sg_table *
> +i915_ttm_resource_get_st(struct
Re: [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
Am 12.05.21 um 09:05 schrieb Thomas Hellström: On Wed, 2021-05-12 at 08:57 +0200, Christian König wrote: Am 11.05.21 um 16:28 schrieb Thomas Hellström: On 5/11/21 4:09 PM, Christian König wrote: Am 11.05.21 um 16:06 schrieb Thomas Hellström (Intel): On 5/11/21 3:58 PM, Christian König wrote: Am 11.05.21 um 15:25 schrieb Thomas Hellström: Most logical place to introduce TTM buffer objects is as an i915 gem object backend. We need to add some ops to account for added functionality like delayed delete and LRU list manipulation. Initially we support only LMEM and SYSTEM memory, but SYSTEM (which in this case means evicted LMEM objects) is not visible to i915 GEM yet. The plan is to move the i915 gem system region over to the TTM system memory type in upcoming patches. We set up GPU bindings directly both from LMEM and from the system region, as there is no need to use the legacy TTM_TT memory type. We reserve that for future porting of GGTT bindings to TTM. There are some changes to TTM to allow for purging system memory buffer objects and to refuse swapping of some objects: Unfortunately i915 gem still relies heavily on short-term object pinning, and we've chosen to keep short-term-pinned buffer objects on the TTM LRU lists for now, meaning that we need some sort of mechanism to tell TTM they are not swappable. A longer term goal is to get rid of the short- term pinning. Well just use the eviction_valuable interface for this. Yes, we do that for vram/lmem eviction, but we have nothing similar for system swapping. Do I understand you correctly that you want me to add a call to eviction_valuable() also for that instead of swap_possible()? You should already have that. eviction_valuable is called in both cases. Hmm. I can only see it called from ttm_mem_evict_first() which is not in the swapping path? Or do I miss something? Mhm, looks like my recollection was wrong. We should probably move the call into the ttm_bo_evict_swapout_allowable() function. Yes, I think we also need a convention whether it's called dma_resv locked or not, since the helper accesses bo->mem, which should really only be done under reservation. At the same point, there is value in calling this function while holding the LRU lock. You actually need to call it while holding the lock because eviction otherwise ends up in an endless loop. Trying to fix that for years, but so far no luck with that. Also, I wonder whether implementations of this callback might encounter unexpected data when called from the swapout path, because at least the helper assumes it not in system memory, since it is accessing bo- mem.start. So unless we use a separate callback for swapout, there's some auditing to be done. Please audit the existing callbacks and move the callback into the function after doing that. Thanks, Christian. Pls let me know what you think. Thanks, Thomas Christian. Thanks, Thomas
Re: [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
On Wed, 2021-05-12 at 08:57 +0200, Christian König wrote: > Am 11.05.21 um 16:28 schrieb Thomas Hellström: > > > > On 5/11/21 4:09 PM, Christian König wrote: > > > > > > > > > Am 11.05.21 um 16:06 schrieb Thomas Hellström (Intel): > > > > > > > > On 5/11/21 3:58 PM, Christian König wrote: > > > > > Am 11.05.21 um 15:25 schrieb Thomas Hellström: > > > > > > Most logical place to introduce TTM buffer objects is as an > > > > > > i915 > > > > > > gem object backend. We need to add some ops to account for > > > > > > added > > > > > > functionality like delayed delete and LRU list > > > > > > manipulation. > > > > > > > > > > > > Initially we support only LMEM and SYSTEM memory, but > > > > > > SYSTEM > > > > > > (which in this case means evicted LMEM objects) is not > > > > > > visible to i915 GEM yet. The plan is to move the i915 gem > > > > > > system > > > > > > region > > > > > > over to the TTM system memory type in upcoming patches. > > > > > > > > > > > > We set up GPU bindings directly both from LMEM and from the > > > > > > system > > > > > > region, > > > > > > as there is no need to use the legacy TTM_TT memory type. > > > > > > We reserve > > > > > > that for future porting of GGTT bindings to TTM. > > > > > > > > > > > > There are some changes to TTM to allow for purging system > > > > > > memory > > > > > > buffer > > > > > > objects and to refuse swapping of some objects: > > > > > > Unfortunately i915 > > > > > > gem > > > > > > still relies heavily on short-term object pinning, and > > > > > > we've > > > > > > chosen to > > > > > > keep short-term-pinned buffer objects on the TTM LRU lists > > > > > > for now, > > > > > > meaning that we need some sort of mechanism to tell TTM > > > > > > they are not > > > > > > swappable. A longer term goal is to get rid of the short- > > > > > > term > > > > > > pinning. > > > > > > > > > > Well just use the eviction_valuable interface for this. > > > > > > > > Yes, we do that for vram/lmem eviction, but we have nothing > > > > similar > > > > for system swapping. Do I understand you correctly that you > > > > want me > > > > to add a call to eviction_valuable() also for that instead of > > > > swap_possible()? > > > > > > You should already have that. eviction_valuable is called in both > > > cases. > > > > > Hmm. I can only see it called from ttm_mem_evict_first() which is > > not > > in the swapping path? Or do I miss something? > > Mhm, looks like my recollection was wrong. We should probably move > the > call into the ttm_bo_evict_swapout_allowable() function. Yes, I think we also need a convention whether it's called dma_resv locked or not, since the helper accesses bo->mem, which should really only be done under reservation. At the same point, there is value in calling this function while holding the LRU lock. Also, I wonder whether implementations of this callback might encounter unexpected data when called from the swapout path, because at least the helper assumes it not in system memory, since it is accessing bo- >mem.start. So unless we use a separate callback for swapout, there's some auditing to be done. Pls let me know what you think. Thanks, Thomas > > Christian. > > > > > Thanks, > > > > Thomas > > > > > > >
Re: [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
Am 11.05.21 um 16:28 schrieb Thomas Hellström: On 5/11/21 4:09 PM, Christian König wrote: Am 11.05.21 um 16:06 schrieb Thomas Hellström (Intel): On 5/11/21 3:58 PM, Christian König wrote: Am 11.05.21 um 15:25 schrieb Thomas Hellström: Most logical place to introduce TTM buffer objects is as an i915 gem object backend. We need to add some ops to account for added functionality like delayed delete and LRU list manipulation. Initially we support only LMEM and SYSTEM memory, but SYSTEM (which in this case means evicted LMEM objects) is not visible to i915 GEM yet. The plan is to move the i915 gem system region over to the TTM system memory type in upcoming patches. We set up GPU bindings directly both from LMEM and from the system region, as there is no need to use the legacy TTM_TT memory type. We reserve that for future porting of GGTT bindings to TTM. There are some changes to TTM to allow for purging system memory buffer objects and to refuse swapping of some objects: Unfortunately i915 gem still relies heavily on short-term object pinning, and we've chosen to keep short-term-pinned buffer objects on the TTM LRU lists for now, meaning that we need some sort of mechanism to tell TTM they are not swappable. A longer term goal is to get rid of the short-term pinning. Well just use the eviction_valuable interface for this. Yes, we do that for vram/lmem eviction, but we have nothing similar for system swapping. Do I understand you correctly that you want me to add a call to eviction_valuable() also for that instead of swap_possible()? You should already have that. eviction_valuable is called in both cases. Hmm. I can only see it called from ttm_mem_evict_first() which is not in the swapping path? Or do I miss something? Mhm, looks like my recollection was wrong. We should probably move the call into the ttm_bo_evict_swapout_allowable() function. Christian. Thanks, Thomas
Re: [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
On 5/11/21 4:09 PM, Christian König wrote: Am 11.05.21 um 16:06 schrieb Thomas Hellström (Intel): On 5/11/21 3:58 PM, Christian König wrote: Am 11.05.21 um 15:25 schrieb Thomas Hellström: Most logical place to introduce TTM buffer objects is as an i915 gem object backend. We need to add some ops to account for added functionality like delayed delete and LRU list manipulation. Initially we support only LMEM and SYSTEM memory, but SYSTEM (which in this case means evicted LMEM objects) is not visible to i915 GEM yet. The plan is to move the i915 gem system region over to the TTM system memory type in upcoming patches. We set up GPU bindings directly both from LMEM and from the system region, as there is no need to use the legacy TTM_TT memory type. We reserve that for future porting of GGTT bindings to TTM. There are some changes to TTM to allow for purging system memory buffer objects and to refuse swapping of some objects: Unfortunately i915 gem still relies heavily on short-term object pinning, and we've chosen to keep short-term-pinned buffer objects on the TTM LRU lists for now, meaning that we need some sort of mechanism to tell TTM they are not swappable. A longer term goal is to get rid of the short-term pinning. Well just use the eviction_valuable interface for this. Yes, we do that for vram/lmem eviction, but we have nothing similar for system swapping. Do I understand you correctly that you want me to add a call to eviction_valuable() also for that instead of swap_possible()? You should already have that. eviction_valuable is called in both cases. Hmm. I can only see it called from ttm_mem_evict_first() which is not in the swapping path? Or do I miss something? Thanks, Thomas
Re: [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
Am 11.05.21 um 16:06 schrieb Thomas Hellström (Intel): On 5/11/21 3:58 PM, Christian König wrote: Am 11.05.21 um 15:25 schrieb Thomas Hellström: Most logical place to introduce TTM buffer objects is as an i915 gem object backend. We need to add some ops to account for added functionality like delayed delete and LRU list manipulation. Initially we support only LMEM and SYSTEM memory, but SYSTEM (which in this case means evicted LMEM objects) is not visible to i915 GEM yet. The plan is to move the i915 gem system region over to the TTM system memory type in upcoming patches. We set up GPU bindings directly both from LMEM and from the system region, as there is no need to use the legacy TTM_TT memory type. We reserve that for future porting of GGTT bindings to TTM. There are some changes to TTM to allow for purging system memory buffer objects and to refuse swapping of some objects: Unfortunately i915 gem still relies heavily on short-term object pinning, and we've chosen to keep short-term-pinned buffer objects on the TTM LRU lists for now, meaning that we need some sort of mechanism to tell TTM they are not swappable. A longer term goal is to get rid of the short-term pinning. Well just use the eviction_valuable interface for this. Yes, we do that for vram/lmem eviction, but we have nothing similar for system swapping. Do I understand you correctly that you want me to add a call to eviction_valuable() also for that instead of swap_possible()? You should already have that. eviction_valuable is called in both cases. In general please make separate patches for the TTM changes and for the i915 changes using them for easier review. I'll respin with a split. Do you want me to do the same also for the other two patches that minmally touch TTM? Yes, that makes it much easier to review the general usefulness of interface changes. Thanks, Christian. Thanks, Thomas
Re: [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
On 5/11/21 3:58 PM, Christian König wrote: Am 11.05.21 um 15:25 schrieb Thomas Hellström: Most logical place to introduce TTM buffer objects is as an i915 gem object backend. We need to add some ops to account for added functionality like delayed delete and LRU list manipulation. Initially we support only LMEM and SYSTEM memory, but SYSTEM (which in this case means evicted LMEM objects) is not visible to i915 GEM yet. The plan is to move the i915 gem system region over to the TTM system memory type in upcoming patches. We set up GPU bindings directly both from LMEM and from the system region, as there is no need to use the legacy TTM_TT memory type. We reserve that for future porting of GGTT bindings to TTM. There are some changes to TTM to allow for purging system memory buffer objects and to refuse swapping of some objects: Unfortunately i915 gem still relies heavily on short-term object pinning, and we've chosen to keep short-term-pinned buffer objects on the TTM LRU lists for now, meaning that we need some sort of mechanism to tell TTM they are not swappable. A longer term goal is to get rid of the short-term pinning. Well just use the eviction_valuable interface for this. Yes, we do that for vram/lmem eviction, but we have nothing similar for system swapping. Do I understand you correctly that you want me to add a call to eviction_valuable() also for that instead of swap_possible()? In general please make separate patches for the TTM changes and for the i915 changes using them for easier review. I'll respin with a split. Do you want me to do the same also for the other two patches that minmally touch TTM? Thanks, Thomas
Re: [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
Am 11.05.21 um 15:25 schrieb Thomas Hellström:
Most logical place to introduce TTM buffer objects is as an i915
gem object backend. We need to add some ops to account for added
functionality like delayed delete and LRU list manipulation.
Initially we support only LMEM and SYSTEM memory, but SYSTEM
(which in this case means evicted LMEM objects) is not
visible to i915 GEM yet. The plan is to move the i915 gem system region
over to the TTM system memory type in upcoming patches.
We set up GPU bindings directly both from LMEM and from the system region,
as there is no need to use the legacy TTM_TT memory type. We reserve
that for future porting of GGTT bindings to TTM.
There are some changes to TTM to allow for purging system memory buffer
objects and to refuse swapping of some objects: Unfortunately i915 gem
still relies heavily on short-term object pinning, and we've chosen to
keep short-term-pinned buffer objects on the TTM LRU lists for now,
meaning that we need some sort of mechanism to tell TTM they are not
swappable. A longer term goal is to get rid of the short-term pinning.
Well just use the eviction_valuable interface for this.
In general please make separate patches for the TTM changes and for the
i915 changes using them for easier review.
Christian.
Remove the old lmem backend.
Cc: Christian König
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 83 ---
drivers/gpu/drm/i915/gem/i915_gem_lmem.h | 5 -
drivers/gpu/drm/i915/gem/i915_gem_object.c| 126 +++--
drivers/gpu/drm/i915/gem/i915_gem_object.h| 9 +
.../gpu/drm/i915/gem/i915_gem_object_types.h | 18 +
drivers/gpu/drm/i915/gem/i915_gem_region.c| 6 +-
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 534 ++
drivers/gpu/drm/i915/gem/i915_gem_ttm.h | 48 ++
drivers/gpu/drm/i915/gt/intel_region_lmem.c | 3 +-
drivers/gpu/drm/i915/i915_gem.c | 5 +-
drivers/gpu/drm/i915/intel_memory_region.c| 1 -
drivers/gpu/drm/i915/intel_memory_region.h| 1 -
drivers/gpu/drm/i915/intel_region_ttm.c | 5 +-
drivers/gpu/drm/i915/intel_region_ttm.h | 7 +-
drivers/gpu/drm/ttm/ttm_bo.c | 12 +
include/drm/ttm/ttm_device.h | 9 +
17 files changed, 733 insertions(+), 140 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm.c
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 958ccc1edfed..ef0d884a9e2d 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -155,6 +155,7 @@ gem-y += \
gem/i915_gem_stolen.o \
gem/i915_gem_throttle.o \
gem/i915_gem_tiling.o \
+ gem/i915_gem_ttm.o \
gem/i915_gem_ttm_bo_util.o \
gem/i915_gem_userptr.o \
gem/i915_gem_wait.o \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
index f42803ea48f2..2b8cd15de1d9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
@@ -4,73 +4,10 @@
*/
#include "intel_memory_region.h"
-#include "intel_region_ttm.h"
#include "gem/i915_gem_region.h"
#include "gem/i915_gem_lmem.h"
#include "i915_drv.h"
-static void lmem_put_pages(struct drm_i915_gem_object *obj,
- struct sg_table *pages)
-{
- intel_region_ttm_node_free(obj->mm.region, obj->mm.st_mm_node);
- obj->mm.dirty = false;
- sg_free_table(pages);
- kfree(pages);
-}
-
-static int lmem_get_pages(struct drm_i915_gem_object *obj)
-{
- unsigned int flags;
- struct sg_table *pages;
-
- flags = I915_ALLOC_MIN_PAGE_SIZE;
- if (obj->flags & I915_BO_ALLOC_CONTIGUOUS)
- flags |= I915_ALLOC_CONTIGUOUS;
-
- obj->mm.st_mm_node = intel_region_ttm_node_alloc(obj->mm.region,
-obj->base.size,
-flags);
- if (IS_ERR(obj->mm.st_mm_node))
- return PTR_ERR(obj->mm.st_mm_node);
-
- /* Range manager is always contigous */
- if (obj->mm.region->is_range_manager)
- obj->flags |= I915_BO_ALLOC_CONTIGUOUS;
- pages = intel_region_ttm_node_to_st(obj->mm.region, obj->mm.st_mm_node);
- if (IS_ERR(pages))
- return PTR_ERR(pages);
-
- __i915_gem_object_set_pages(obj, pages,
- i915_sg_dma_page_sizes(pages->sgl));
-
- if (obj->flags & I915_BO_ALLOC_CPU_CLEAR) {
- void __iomem *vaddr =
- i915_gem_object_lmem_io_map(obj, 0, obj->base.size);
-
- if (!vaddr) {
- struct sg_table *pages =
-
[PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend
Most logical place to introduce TTM buffer objects is as an i915
gem object backend. We need to add some ops to account for added
functionality like delayed delete and LRU list manipulation.
Initially we support only LMEM and SYSTEM memory, but SYSTEM
(which in this case means evicted LMEM objects) is not
visible to i915 GEM yet. The plan is to move the i915 gem system region
over to the TTM system memory type in upcoming patches.
We set up GPU bindings directly both from LMEM and from the system region,
as there is no need to use the legacy TTM_TT memory type. We reserve
that for future porting of GGTT bindings to TTM.
There are some changes to TTM to allow for purging system memory buffer
objects and to refuse swapping of some objects: Unfortunately i915 gem
still relies heavily on short-term object pinning, and we've chosen to
keep short-term-pinned buffer objects on the TTM LRU lists for now,
meaning that we need some sort of mechanism to tell TTM they are not
swappable. A longer term goal is to get rid of the short-term pinning.
Remove the old lmem backend.
Cc: Christian König
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 83 ---
drivers/gpu/drm/i915/gem/i915_gem_lmem.h | 5 -
drivers/gpu/drm/i915/gem/i915_gem_object.c| 126 +++--
drivers/gpu/drm/i915/gem/i915_gem_object.h| 9 +
.../gpu/drm/i915/gem/i915_gem_object_types.h | 18 +
drivers/gpu/drm/i915/gem/i915_gem_region.c| 6 +-
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 534 ++
drivers/gpu/drm/i915/gem/i915_gem_ttm.h | 48 ++
drivers/gpu/drm/i915/gt/intel_region_lmem.c | 3 +-
drivers/gpu/drm/i915/i915_gem.c | 5 +-
drivers/gpu/drm/i915/intel_memory_region.c| 1 -
drivers/gpu/drm/i915/intel_memory_region.h| 1 -
drivers/gpu/drm/i915/intel_region_ttm.c | 5 +-
drivers/gpu/drm/i915/intel_region_ttm.h | 7 +-
drivers/gpu/drm/ttm/ttm_bo.c | 12 +
include/drm/ttm/ttm_device.h | 9 +
17 files changed, 733 insertions(+), 140 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm.c
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 958ccc1edfed..ef0d884a9e2d 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -155,6 +155,7 @@ gem-y += \
gem/i915_gem_stolen.o \
gem/i915_gem_throttle.o \
gem/i915_gem_tiling.o \
+ gem/i915_gem_ttm.o \
gem/i915_gem_ttm_bo_util.o \
gem/i915_gem_userptr.o \
gem/i915_gem_wait.o \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
index f42803ea48f2..2b8cd15de1d9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
@@ -4,73 +4,10 @@
*/
#include "intel_memory_region.h"
-#include "intel_region_ttm.h"
#include "gem/i915_gem_region.h"
#include "gem/i915_gem_lmem.h"
#include "i915_drv.h"
-static void lmem_put_pages(struct drm_i915_gem_object *obj,
- struct sg_table *pages)
-{
- intel_region_ttm_node_free(obj->mm.region, obj->mm.st_mm_node);
- obj->mm.dirty = false;
- sg_free_table(pages);
- kfree(pages);
-}
-
-static int lmem_get_pages(struct drm_i915_gem_object *obj)
-{
- unsigned int flags;
- struct sg_table *pages;
-
- flags = I915_ALLOC_MIN_PAGE_SIZE;
- if (obj->flags & I915_BO_ALLOC_CONTIGUOUS)
- flags |= I915_ALLOC_CONTIGUOUS;
-
- obj->mm.st_mm_node = intel_region_ttm_node_alloc(obj->mm.region,
-obj->base.size,
-flags);
- if (IS_ERR(obj->mm.st_mm_node))
- return PTR_ERR(obj->mm.st_mm_node);
-
- /* Range manager is always contigous */
- if (obj->mm.region->is_range_manager)
- obj->flags |= I915_BO_ALLOC_CONTIGUOUS;
- pages = intel_region_ttm_node_to_st(obj->mm.region, obj->mm.st_mm_node);
- if (IS_ERR(pages))
- return PTR_ERR(pages);
-
- __i915_gem_object_set_pages(obj, pages,
- i915_sg_dma_page_sizes(pages->sgl));
-
- if (obj->flags & I915_BO_ALLOC_CPU_CLEAR) {
- void __iomem *vaddr =
- i915_gem_object_lmem_io_map(obj, 0, obj->base.size);
-
- if (!vaddr) {
- struct sg_table *pages =
- __i915_gem_object_unset_pages(obj);
-
- if (!IS_ERR_OR_NULL(pages))
- lmem_put_pages(obj, pages);
- }
-
- memset_io(vaddr, 0, obj->base.size);
- io_mapping_unmap(vaddr);
- }
-
- return 0;
-}
-
