On 7/30/2019 3:45 AM, Daniel Vetter wrote: > On Tue, Jul 30, 2019 at 12:24 PM Koenig, Christian > <Christian.Koenig@xxxxxxx> wrote: >> >> Am 30.07.19 um 11:38 schrieb Daniel Vetter: >>> On Tue, Jul 30, 2019 at 08:45:57AM +0000, Koenig, Christian wrote: Snipped the feedback on struct drm_mem_region. Will be easier to have separate thread. >>>> >>>>> +/* >>>>> + * 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) 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. Whatever we define for these sub-types, does it make sense for SYSTEM and VRAM to each have them defined? 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? >>>> 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(-) >>>>> >> > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
