[RFC PATCH 0/3] Propose new struct drm_mem_region
Daniel Vetter
daniel at ffwll.ch
Tue Jul 30 09:38:47 UTC 2019
On Tue, Jul 30, 2019 at 08:45:57AM +0000, Koenig, Christian wrote:
> Yeah, that looks like a good start. Just a couple of random design
> comments/requirements.
>
> First of all please restructure the changes so that you more or less
> have the following:
> 1. Adding of the new structures and functionality without any change to
> existing code.
> 2. Replacing the existing functionality in TTM and all of its drivers.
> 3. Replacing the existing functionality in i915.
>
> This should make it much easier to review the new functionality when it
> is not mixed with existing TTM stuff.
>
>
> Second please completely drop the concept of gpu_offset or start of the
> memory region like here:
> > drm_printf(p, " gpu_offset: 0x%08llX\n", man->region.start);
> At least on AMD hardware we have the following address spaces which are
> sometimes even partially overlapping: VM, MC, SYSTEM, FB, AGP, XGMI, bus
> addresses and physical addresses.
>
> Pushing a concept of a general GPU address space into the memory
> management was a rather bad design mistake in TTM and we should not
> repeat that here.
>
> A region should only consists of a size in bytes and (internal to the
> region manager) allocations in that region.
>
>
> Third please don't use any CPU or architecture specific types in any
> data structures:
> > +struct drm_mem_region {
> > + resource_size_t start; /* within GPU physical address space */
> > + resource_size_t io_start; /* BAR address (CPU accessible) */
> > + resource_size_t size;
>
> I knew that resource_size is mostly 64bit on modern architectures, but
> dGPUs are completely separate to the architecture and we always need
> 64bits here at least for AMD hardware.
>
> So this should either be always uint64_t, or something like
> gpu_resource_size which depends on what the compiled in drivers require
> if we really need that.
>
> And by the way: Please always use bytes for things like sizes and not
> number of pages, cause page size is again CPU/architecture specific and
> GPU drivers don't necessary care about that.
>
>
> And here also a few direct comments on the code:
> > + union {
> > + struct drm_mm *mm;
> > + /* FIXME (for i915): struct drm_buddy_mm *buddy_mm; */
> > + void *priv;
> > + };
> Maybe just always use void *mm here.
>
> > + spinlock_t move_lock;
> > + struct dma_fence *move;
>
> That is TTM specific and I'm not sure if we want it in the common memory
> management handling.
>
> If we want that here we should probably replace the lock with some rcu
> and atomic fence pointer exchange first.
>
> > +/*
> > + * 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.
> 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.
-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
http://blog.ffwll.ch
More information about the amd-gfx
mailing list