[Intel-gfx] [RFC PATCH 0/3] Propose new struct drm_mem_region

[Koenig, Christian]

Am 31.07.19 um 02:51 schrieb Brian Welty:
[SNIP]
>>>>>> +/*
>>>>>> + * Memory types for drm_mem_region
>>>>>> + */
>>>>> #define DRM_MEM_SWAP    ?
>>>> btw what did you have in mind for this? Since we use shmem we kinda don't
>>>> know whether the BO is actually swapped out or not, at least on the i915
>>>> side. So this would be more NOT_CURRENTLY_PINNED_AND_POSSIBLY_SWAPPED_OUT.
>>> Yeah, the problem is not everybody can use shmem. For some use cases you
>>> have to use memory allocated through dma_alloc_coherent().
>>>
>>> So to be able to swap this out you need a separate domain to copy it
>>> from whatever is backing it currently to shmem.
>>>
>>> So we essentially have:
>>> DRM_MEM_SYS_SWAPABLE
>>> DRM_MEM_SYS_NOT_GPU_MAPPED
>>> DRM_MEM_SYS_GPU_MAPPED
>>>
>>> Or something like that.
>> Yeah i915-gem is similar. We oportunistically keep the pages pinned
>> sometimes even if not currently mapped into the (what ttm calls) TT.
>> So I think these three for system memory make sense for us too. I
>> think that's similar (at least in spirit) to the dma_alloc cache you
>> have going on. Mabye instead of the somewhat cumbersome NOT_GPU_MAPPED
>> we could have something like PINNED or so. Although it's not
>> permanently pinned, so maybe that's confusing too.
>>
> Okay, I see now I was far off the mark with what I thought TTM_PL_SYSTEM
> was.  The discussion helped clear up several bits of confusion on my part.
>  From proposed names, I find MAPPED and PINNED slightly confusing.
> In terms of backing store description, maybe these are a little better:
>    DRM_MEM_SYS_UNTRANSLATED  (TTM_PL_SYSTEM)
>    DRM_MEM_SYS_TRANSLATED    (TTM_PL_TT or i915's SYSTEM)

That's still not correct. Let me describe what each of the tree stands for:

1. The backing store is a shmem file so the individual pages are 
swapable by the core OS.
2. The backing store is allocate GPU accessible but not currently in use 
by the GPU.
3. The backing store is currently in use by the GPU.

For i915 all three of those are basically the same and you only need to 
worry about it much.

But for other drivers that's certainly not true and we need this 
distinction of the backing store of an object.

I'm just not sure how we would handle that for cgroups. From experience 
we certainly want a limit over all 3, but you usually also want to limit 
3 alone.

And you also want to limit the amount of bytes moved between those 
states because each state transition might have a bandwidth cost 
associated with it.

> Are these allowed to be both overlapping? Or non-overlapping (partitioned)?
> Per Christian's point about removing .start, seems it doesn't need to
> matter.

You should probably completely drop the idea of this being regions.

And we should also rename them to something like drm_mem_domains to make 
that clear.

> Whatever we define for these sub-types, does it make sense for SYSTEM and
> VRAM to each have them defined?

No, absolutely not. VRAM as well as other private memory types are 
completely driver specific.

> I'm unclear how DRM_MEM_SWAP (or DRM_MEM_SYS_SWAPABLE) would get
> configured by driver...  this is a fixed size partition of host memory?
> Or it is a kind of dummy memory region just for swap implementation?

#1 and #2 in my example above should probably not be configured by the 
driver itself.

And yes seeing those as special for state handling sounds like the 
correct approach to me.

Regards,
Christian.

>>>>> TTM was clearly missing that resulting in a whole bunch of extra
>>>>> handling and rather complicated handling.
>>>>>
>>>>>> +#define DRM_MEM_SYSTEM     0
>>>>>> +#define DRM_MEM_STOLEN     1
>>>>> I think we need a better naming for that.
>>>>>
>>>>> STOLEN sounds way to much like stolen VRAM for integrated GPUs, but at
>>>>> least for TTM this is the system memory currently GPU accessible.
>>>> Yup this is wrong, for i915 we use this as stolen, for ttm it's the gpu
>>>> translation table window into system memory. Not the same thing at all.
>>> Thought so. The closest I have in mind is GTT, but everything else works
>>> as well.
>> Would your GPU_MAPPED above work for TT? I think we'll also need
>> STOLEN, I'm even hearing noises that there's going to be stolen for
>> discrete vram for us ... Also if we expand I guess we need to teach
>> ttm to cope with more, or maybe treat the DRM one as some kind of
>> sub-flavour.
> Daniel, maybe what i915 calls stolen could just be DRM_MEM_RESERVED or
> DRM_MEM_PRIV.  Or maybe can argue it falls into UNTRANSLATED type that
> I suggested above, I'm not sure.
>
> -Brian
>
>
>> -Daniel
>>
>>> Christian.
>>>
>>>> -Daniel
>>>>
>>>>> Thanks for looking into that,
>>>>> Christian.
>>>>>
>>>>> Am 30.07.19 um 02:32 schrieb Brian Welty:
>>>>>> [ By request, resending to include amd-gfx + intel-gfx.  Since resending,
>>>>>>      I fixed the nit with ordering of header includes that Sam noted. ]
>>>>>>
>>>>>> This RFC series is first implementation of some ideas expressed
>>>>>> earlier on dri-devel [1].
>>>>>>
>>>>>> Some of the goals (open for much debate) are:
>>>>>>      - Create common base structure (subclass) for memory regions (patch #1)
>>>>>>      - Create common memory region types (patch #2)
>>>>>>      - Create common set of memory_region function callbacks (based on
>>>>>>        ttm_mem_type_manager_funcs and intel_memory_regions_ops)
>>>>>>      - Create common helpers that operate on drm_mem_region to be leveraged
>>>>>>        by both TTM drivers and i915, reducing code duplication
>>>>>>      - Above might start with refactoring ttm_bo_manager.c as these are
>>>>>>        helpers for using drm_mm's range allocator and could be made to
>>>>>>        operate on DRM structures instead of TTM ones.
>>>>>>      - Larger goal might be to make LRU management of GEM objects common, and
>>>>>>        migrate those fields into drm_mem_region and drm_gem_object strucures.
>>>>>>
>>>>>> Patches 1-2 implement the proposed struct drm_mem_region and adds
>>>>>> associated common set of definitions for memory region type.
>>>>>>
>>>>>> Patch #3 is update to i915 and is based upon another series which is
>>>>>> in progress to add vram support to i915 [2].
>>>>>>
>>>>>> [1] https://lists.freedesktop.org/archives/dri-devel/2019-June/224501.html
>>>>>> [2] https://lists.freedesktop.org/archives/intel-gfx/2019-June/203649.html
>>>>>>
>>>>>> Brian Welty (3):
>>>>>>      drm: introduce new struct drm_mem_region
>>>>>>      drm: Introduce DRM_MEM defines for specifying type of drm_mem_region
>>>>>>      drm/i915: Update intel_memory_region to use nested drm_mem_region
>>>>>>
>>>>>>     drivers/gpu/drm/i915/gem/i915_gem_object.c    |  2 +-
>>>>>>     drivers/gpu/drm/i915/gem/i915_gem_shmem.c     |  2 +-
>>>>>>     drivers/gpu/drm/i915/i915_gem_gtt.c           | 10 ++---
>>>>>>     drivers/gpu/drm/i915/i915_gpu_error.c         |  2 +-
>>>>>>     drivers/gpu/drm/i915/i915_query.c             |  2 +-
>>>>>>     drivers/gpu/drm/i915/intel_memory_region.c    | 10 +++--
>>>>>>     drivers/gpu/drm/i915/intel_memory_region.h    | 19 +++------
>>>>>>     drivers/gpu/drm/i915/intel_region_lmem.c      | 26 ++++++-------
>>>>>>     .../drm/i915/selftests/intel_memory_region.c  |  8 ++--
>>>>>>     drivers/gpu/drm/ttm/ttm_bo.c                  | 34 +++++++++-------
>>>>>>     drivers/gpu/drm/ttm/ttm_bo_manager.c          | 14 +++----
>>>>>>     drivers/gpu/drm/ttm/ttm_bo_util.c             | 11 +++---
>>>>>>     drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c |  8 ++--
>>>>>>     drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c    |  4 +-
>>>>>>     include/drm/drm_mm.h                          | 39 ++++++++++++++++++-
>>>>>>     include/drm/ttm/ttm_bo_api.h                  |  2 +-
>>>>>>     include/drm/ttm/ttm_bo_driver.h               | 16 ++++----
>>>>>>     include/drm/ttm/ttm_placement.h               |  8 ++--
>>>>>>     18 files changed, 124 insertions(+), 93 deletions(-)
>>>>>>
>>