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.
I'd say lets drop this outright, and handle private data by embedding this
structure in the right place. That's how we handle everything in drm now
as much as possible, and it's so much cleaner. I think ttm still loves
priv pointers a bit too much in some places.
> > + 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.
Yeah not sure we want any of these details in this shared structure
either.
>
> > +/*
> > + * Memory types for drm_mem_region
> > + */
>
> #define DRM_MEM_SWAP ?
>
> 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.
Yeah I think the crux here of having a common drm_mem_region is how do we
name stuff. I think what Brian didn't mention is that the goal here is to
have something we can use for managing using cgroups.
> 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.
I'm not sure how much of all that we really want in a drm_mem_region ...
Otherwise we just reimplement the same midlayer we have already, but with
a drm_ instead of ttm_ prefix. And that seems a bit silly.
-Daniel
> >
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
