[Intel-gfx] [PATCH v7 3/3] drm/i915/ttm: Use TTM for system memory

[Thomas Hellström]

On 6/22/21 12:55 PM, Matthew Auld wrote:
> On Tue, 22 Jun 2021 at 10:34, Thomas Hellström
> <thomas.hellstrom at linux.intel.com> wrote:
>> For discrete, use TTM for both cached and WC system memory. That means
>> we currently rely on the TTM memory accounting / shrinker. For cached
>> system memory we should consider remaining shmem-backed, which can be
>> implemented from our ttm_tt_populate callback. We can then also reuse our
>> own very elaborate shrinker for that memory.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>> Reviewed-by: Matthew Auld <matthew.auld at intel.com>
>> ---
>> v2:
>> - Fix IS_ERR_OR_NULL() check to IS_ERR() (Reported by Matthew Auld)
>> v3:
>> - Commit message typo fix
>> v6:
>> - Fix TODO:s for supporting system memory with TTM.
>> - Update the object GEM region after a TTM move if compatible.
>> - Add a couple of warnings for shmem on DGFX.
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_shmem.c  |  3 ++
>>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c    | 51 +++++++++++++++++-----
>>   drivers/gpu/drm/i915/i915_drv.h            |  3 --
>>   drivers/gpu/drm/i915/intel_memory_region.c |  7 ++-
>>   drivers/gpu/drm/i915/intel_memory_region.h |  8 ++++
>>   5 files changed, 58 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> index 7aa1c95c7b7d..3648ae1d6628 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> @@ -284,6 +284,7 @@ __i915_gem_object_release_shmem(struct drm_i915_gem_object *obj,
>>                                  bool needs_clflush)
>>   {
>>          GEM_BUG_ON(obj->mm.madv == __I915_MADV_PURGED);
>> +       GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
>>
>>          if (obj->mm.madv == I915_MADV_DONTNEED)
>>                  obj->mm.dirty = false;
>> @@ -302,6 +303,7 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_
>>          struct pagevec pvec;
>>          struct page *page;
>>
>> +       GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
>>          __i915_gem_object_release_shmem(obj, pages, true);
>>
>>          i915_gem_gtt_finish_pages(obj, pages);
>> @@ -560,6 +562,7 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
>>          resource_size_t offset;
>>          int err;
>>
>> +       GEM_WARN_ON(IS_DGFX(dev_priv));
>>          obj = i915_gem_object_create_shmem(dev_priv, round_up(size, PAGE_SIZE));
>>          if (IS_ERR(obj))
>>                  return obj;
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index 966b292d07da..07097f150065 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -286,6 +286,25 @@ static void i915_ttm_adjust_gem_after_move(struct drm_i915_gem_object *obj)
>>   {
>>          struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
>>          unsigned int cache_level;
>> +       unsigned int i;
>> +
>> +       /*
>> +        * If object was moved to an allowable region, update the object
>> +        * region to consider it migrated. Note that if it's currently not
>> +        * in an allowable region, it's evicted and we don't update the
>> +        * object region.
>> +        */
>> +       if (intel_region_to_ttm_type(obj->mm.region) != bo->resource->mem_type) {
>> +               for (i = 0; i < obj->mm.n_placements; ++i) {
>> +                       struct intel_memory_region *mr = obj->mm.placements[i];
>> +
>> +                       if (intel_region_to_ttm_type(mr) == bo->resource->mem_type &&
>> +                           mr != obj->mm.region) {
>> +                               intel_memory_region_put(obj->mm.region);
>> +                               obj->mm.region = intel_memory_region_get(mr);
> break;?
>
> i915_gem_object_{init, release}_memory_region?
>
> There is also the region_link stuff, but I guess we can nuke that?

Ah, yes, I'll fix that up. I think we will actually need that for 
suspend/resume, as the TTM LRU lists aren't sufficient...

Thanks for reviewing!

/Thomas