[RFC PATCH 0/3] Propose new struct drm_mem_region

Wed Jul 31 08:05:32 UTC 2019

On Wed, Jul 31, 2019 at 8:54 AM Koenig, Christian
<Christian.Koenig at amd.com> wrote:
>
> 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.

We do pretty much have these three states for i915 gem bo too. Of
course none have a reasonable upper limit since it's all shared
memory. The hard limit would be system memory + swap for 1, and only
system memory for 2 and 3.

> 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.

To avoid lolz against the shrinker I think you also want to limit 2+3.
Afaiui ttm does that with the global limit, to avoid driving the
system against the wall.

> 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.

+1 on domains. Some of these domains might be physically contiguous
regions, but some clearly arent.

> > 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.

Do we have any hw that wants custom versions of 3? The only hw designs
I know of either have one shared translation table (but only one per
device, so having just 1 domain is good enough). Or TT mappings are in
the per-process pagetables, and then you're defacto unlimited (and
again one domain is good enough). So roughly:

- 1&2 global accross all drivers. 1 and 2 are disjoint (i.e. a bo is
only account to one of them, never both).
- 3 would be a subgroup of 2, and per device. A bo in group 3 is also
always in group 2.

For VRAM and VRAM-similar things (like stolen system memory, or if you
have VRAM that's somehow split up like with a dual gpu perhaps) I
agree the driver needs to register that. And we just have some
standard flags indicating that "this is kinda like VRAM".
-Daniel

>
> 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(-)
> >>>>>>
> >>
>

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch