Re: [RFC PATCH 0/3] Propose new struct drm_mem_region

["Koenig, Christian" <Christian.Koenig-5C7GfCeVMHo@public.gmane.org>]

Am 30.07.19 um 11:38 schrieb Daniel Vetter:
> 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.

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.

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

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx