> On Sun, May 07, 2023 at 11:39:18PM -0700, Yang, Fei wrote: >>> On Wed, May 03, 2023 at 03:50:59PM -0700, fei.y...@intel.com wrote: >>>> From: Fei Yang <fei.y...@intel.com> >>>> >>>> Currently the KMD is using enum i915_cache_level to set caching policy for >>>> buffer objects. This is flaky because the PAT index which really controls >>>> the caching behavior in PTE has far more levels than what's defined in the >>>> enum. In addition, the PAT index is platform dependent, having to translate >>>> between i915_cache_level and PAT index is not reliable, and makes the code >>>> more complicated. >>>> >>>> From UMD's perspective there is also a necessity to set caching policy for >>>> performance fine tuning. It's much easier for the UMD to directly use PAT >>>> index because the behavior of each PAT index is clearly defined in Bspec. >>>> Having the abstracted i915_cache_level sitting in between would only cause >>>> more ambiguity. >>> >>> It may be worth mentioning here that PAT is expected to work much like >>> MOCS already works today --- the contract on the exact platform-specific >>> meaning of each index is documented in the hardware spec and userspace >>> is expected to select the index that exactly matches the behavior it >>> desires. >> will update.
Please review https://patchwork.freedesktop.org/series/117480/ >>>> >>>> For these reasons this patch replaces i915_cache_level with PAT index. Also >>>> note, the cache_level is not completely removed yet, because the KMD still >>>> has the need of creating buffer objects with simple cache settings such as >>>> cached, uncached, or writethrough. For such simple cases, using cache_level >>>> would help simplify the code. >>> >>> See my comment farther down, but the implementation of >>> i915_gem_object_has_cache_level() seems a bit confusing and you may want >>> to elaborate on it here. >>> >>> Also somewhat confusing from a high-level skim is if/how we're still >>> using obj->cache_coherent once userspace has taken direct control of the >>> cache behavior. Some PAT indices give coherency and others don't (and >>> the combinations will likely get more complicated on future platforms). >>> If obj->cache_coherent is still being considered even once PAT indices >>> are being controlled by userspace, I think we need some explanation of >>> how that works in the commit message (and likely in the kerneldoc for >>> that field too). >> For the objects with pat_index set by user space, coherency is managed by >> user space. Things like obj->cache_coherent and obj->cache_dirty are still >> there for objects created by kernel. > > Right, that's the challenge --- userspace is taking over control of this > stuff, but the fields are still around and still used internally within > the driver. How we reconcile those two things needs to be clearly > explained in the commit message and kerneldoc, otherwise we're going to > wind up with confusing code that's very difficult to maintain down the > road. > > All the cache behavior is complicated enough that it probably wouldn't > be a bad idea to have a dedicated section of the kerneldoc focused on > cache behavior. Updated with kerneldoc and comments. As discussed off-line, I have also squashed the pte_encode patch. >>>> >>>> Cc: Chris Wilson <chris.p.wil...@linux.intel.com> >>>> Cc: Matt Roper <matthew.d.ro...@intel.com> >>>> Signed-off-by: Fei Yang <fei.y...@intel.com> >>>> Reviewed-by: Andi Shyti <andi.sh...@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/display/intel_dpt.c | 12 +-- >>>> drivers/gpu/drm/i915/gem/i915_gem_domain.c | 45 ++++++---- >>>> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 ++- >>>> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 3 +- >>>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 51 +++++++++++- >>>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 4 + >>>> .../gpu/drm/i915/gem/i915_gem_object_types.h | 25 +++++- >>>> drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 4 +- >>>> drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 16 ++-- >>>> .../gpu/drm/i915/gem/selftests/huge_pages.c | 2 +- >>>> .../drm/i915/gem/selftests/i915_gem_migrate.c | 2 +- >>>> .../drm/i915/gem/selftests/i915_gem_mman.c | 2 +- >>>> drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 10 ++- >>>> drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 71 ++++++++-------- >>>> drivers/gpu/drm/i915/gt/gen8_ppgtt.h | 3 +- >>>> drivers/gpu/drm/i915/gt/intel_ggtt.c | 82 +++++++++---------- >>>> drivers/gpu/drm/i915/gt/intel_gtt.h | 20 ++--- >>>> drivers/gpu/drm/i915/gt/intel_migrate.c | 47 ++++++----- >>>> drivers/gpu/drm/i915/gt/intel_migrate.h | 13 ++- >>>> drivers/gpu/drm/i915/gt/intel_ppgtt.c | 6 +- >>>> drivers/gpu/drm/i915/gt/selftest_migrate.c | 47 ++++++----- >>>> drivers/gpu/drm/i915/gt/selftest_reset.c | 8 +- >>>> drivers/gpu/drm/i915/gt/selftest_timeline.c | 2 +- >>>> drivers/gpu/drm/i915/gt/selftest_tlb.c | 4 +- >>>> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 10 ++- >>>> drivers/gpu/drm/i915/i915_debugfs.c | 52 +++++++++--- >>>> drivers/gpu/drm/i915/i915_gem.c | 16 +++- >>>> drivers/gpu/drm/i915/i915_gpu_error.c | 8 +- >>>> drivers/gpu/drm/i915/i915_vma.c | 16 ++-- >>>> drivers/gpu/drm/i915/i915_vma.h | 2 +- >>>> drivers/gpu/drm/i915/i915_vma_types.h | 2 - >>>> drivers/gpu/drm/i915/selftests/i915_gem.c | 5 +- >>>> .../gpu/drm/i915/selftests/i915_gem_evict.c | 4 +- >>>> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 15 ++-- >>>> .../drm/i915/selftests/intel_memory_region.c | 4 +- >>>> drivers/gpu/drm/i915/selftests/mock_gtt.c | 8 +- >>>> 36 files changed, 391 insertions(+), 240 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c >>>> b/drivers/gpu/drm/i915/display/intel_dpt.c >>>> index c5eacfdba1a5..7c5fddb203ba 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_dpt.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_dpt.c >>>> @@ -43,24 +43,24 @@ static void gen8_set_pte(void __iomem *addr, >>>> gen8_pte_t pte) >>>> static void dpt_insert_page(struct i915_address_space *vm, >>>> dma_addr_t addr, >>>> u64 offset, >>>> - enum i915_cache_level level, >>>> + unsigned int pat_index, >>>> u32 flags) >>>> { >>>> struct i915_dpt *dpt = i915_vm_to_dpt(vm); >>>> gen8_pte_t __iomem *base = dpt->iomem; >>>> >>>> gen8_set_pte(base + offset / I915_GTT_PAGE_SIZE, >>>> - vm->pte_encode(addr, level, flags)); >>>> + vm->pte_encode(addr, pat_index, flags)); >>>> } >>>> >>>> static void dpt_insert_entries(struct i915_address_space *vm, >>>> struct i915_vma_resource *vma_res, >>>> - enum i915_cache_level level, >>>> + unsigned int pat_index, >>>> u32 flags) >>>> { >>>> struct i915_dpt *dpt = i915_vm_to_dpt(vm); >>>> gen8_pte_t __iomem *base = dpt->iomem; >>>> - const gen8_pte_t pte_encode = vm->pte_encode(0, level, flags); >>>> + const gen8_pte_t pte_encode = vm->pte_encode(0, pat_index, flags); >>>> struct sgt_iter sgt_iter; >>>> dma_addr_t addr; >>>> int i; >>>> @@ -83,7 +83,7 @@ static void dpt_clear_range(struct i915_address_space >>>> *vm, >>>> static void dpt_bind_vma(struct i915_address_space *vm, >>>> struct i915_vm_pt_stash *stash, >>>> struct i915_vma_resource *vma_res, >>>> - enum i915_cache_level cache_level, >>>> + unsigned int pat_index, >>>> u32 flags) >>>> { >>>> u32 pte_flags; >>>> @@ -98,7 +98,7 @@ static void dpt_bind_vma(struct i915_address_space *vm, >>>> if (vma_res->bi.lmem) >>>> pte_flags |= PTE_LM; >>>> >>>> - vm->insert_entries(vm, vma_res, cache_level, pte_flags); >>>> + vm->insert_entries(vm, vma_res, pat_index, pte_flags); >>>> >>>> vma_res->page_sizes_gtt = I915_GTT_PAGE_SIZE; >>>> >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c >>>> b/drivers/gpu/drm/i915/gem/i915_gem_domain.c >>>> index d2d5a24301b2..ae99b4be5918 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c >>>> @@ -27,8 +27,8 @@ static bool gpu_write_needs_clflush(struct >>>> drm_i915_gem_object *obj) >>>> if (IS_DGFX(i915)) >>>> return false; >>>> >>>> - return !(obj->cache_level == I915_CACHE_NONE || >>>> - obj->cache_level == I915_CACHE_WT); >>>> + return !(i915_gem_object_has_cache_level(obj, I915_CACHE_NONE) || >>>> + i915_gem_object_has_cache_level(obj, I915_CACHE_WT)); >>>> } >>>> >>>> bool i915_gem_cpu_write_needs_clflush(struct drm_i915_gem_object *obj) >>>> @@ -267,7 +267,7 @@ int i915_gem_object_set_cache_level(struct >>>> drm_i915_gem_object *obj, >>>> { >>>> int ret; >>>> >>>> - if (obj->cache_level == cache_level) >>>> + if (i915_gem_object_has_cache_level(obj, cache_level)) >>>> return 0; >>>> >>>> ret = i915_gem_object_wait(obj, >>>> @@ -278,10 +278,8 @@ int i915_gem_object_set_cache_level(struct >>>> drm_i915_gem_object *obj, >>>> return ret; >>>> >>>> /* Always invalidate stale cachelines */ >>>> - if (obj->cache_level != cache_level) { >>>> - i915_gem_object_set_cache_coherency(obj, cache_level); >>>> - obj->cache_dirty = true; >>>> - } >>>> + i915_gem_object_set_cache_coherency(obj, cache_level); >>>> + obj->cache_dirty = true; >>>> >>>> /* The cache-level will be applied when each vma is rebound. */ >>>> return i915_gem_object_unbind(obj, >>>> @@ -306,20 +304,22 @@ int i915_gem_get_caching_ioctl(struct drm_device >>>> *dev, void *data, >>>> goto out; >>>> } >>>> >>>> - switch (obj->cache_level) { >>>> - case I915_CACHE_LLC: >>>> - case I915_CACHE_L3_LLC: >>>> - args->caching = I915_CACHING_CACHED; >>>> - break; >>>> + /* >>>> + * This ioctl should be disabled for the objects with pat_index >>>> + * set by user space. >>>> + */ >>>> + if (obj->pat_set_by_user) { >>>> + err = -EOPNOTSUPP; >>>> + goto out; >>>> + } >>>> >>>> - case I915_CACHE_WT: >>>> + if (i915_gem_object_has_cache_level(obj, I915_CACHE_LLC) || >>>> + i915_gem_object_has_cache_level(obj, I915_CACHE_L3_LLC)) >>>> + args->caching = I915_CACHING_CACHED; >>>> + else if (i915_gem_object_has_cache_level(obj, I915_CACHE_WT)) >>>> args->caching = I915_CACHING_DISPLAY; >>>> - break; >>>> - >>>> - default: >>>> + else >>>> args->caching = I915_CACHING_NONE; >>>> - break; >>>> - } >>>> out: >>>> rcu_read_unlock(); >>>> return err; >>>> @@ -364,6 +364,15 @@ int i915_gem_set_caching_ioctl(struct drm_device >>>> *dev, void *data, >>>> if (!obj) >>>> return -ENOENT; >>>> >>>> + /* >>>> + * This ioctl should be disabled for the objects with pat_index >>>> + * set by user space. >>>> + */ >>>> + if (obj->pat_set_by_user) { >>>> + ret = -EOPNOTSUPP; >>>> + goto out; >>>> + } >>>> + >>>> /* >>>> * The caching mode of proxy object is handled by its generator, and >>>> * not allowed to be changed by userspace. >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >>>> b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >>>> index 3aeede6aee4d..d42915516636 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >>>> @@ -642,7 +642,7 @@ static inline int use_cpu_reloc(const struct >>>> reloc_cache *cache, >>>> >>>> return (cache->has_llc || >>>> obj->cache_dirty || >>>> - obj->cache_level != I915_CACHE_NONE); >>>> + !i915_gem_object_has_cache_level(obj, I915_CACHE_NONE)); >>>> } >>>> >>>> static int eb_reserve_vma(struct i915_execbuffer *eb, >>>> @@ -1323,8 +1323,10 @@ static void *reloc_iomap(struct i915_vma *batch, >>>> offset = cache->node.start; >>>> if (drm_mm_node_allocated(&cache->node)) { >>>> ggtt->vm.insert_page(&ggtt->vm, >>>> - i915_gem_object_get_dma_address(obj, page), >>>> - offset, I915_CACHE_NONE, 0); >>>> + i915_gem_object_get_dma_address(obj, page), >>>> + offset, >>>> + i915_gem_get_pat_index(ggtt->vm.i915, I915_CACHE_NONE), >>>> + 0); >>>> } else { >>>> offset += page << PAGE_SHIFT; >>>> } >>>> @@ -1464,7 +1466,7 @@ eb_relocate_entry(struct i915_execbuffer *eb, >>>> reloc_cache_unmap(&eb->reloc_cache); >>>> mutex_lock(&vma->vm->mutex); >>>> err = i915_vma_bind(target->vma, >>>> - target->vma->obj->cache_level, >>>> + target->vma->obj->pat_index, >>>> PIN_GLOBAL, NULL, NULL); >>>> mutex_unlock(&vma->vm->mutex); >>>> reloc_cache_remap(&eb->reloc_cache, ev->vma->obj); >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c >>>> b/drivers/gpu/drm/i915/gem/i915_gem_mman.c >>>> index 3dbacdf0911a..50c30efa08a3 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c >>>> @@ -383,7 +383,8 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) >>>> } >>>> >>>> /* Access to snoopable pages through the GTT is incoherent. */ >>>> - if (obj->cache_level != I915_CACHE_NONE && !HAS_LLC(i915)) { >>>> + if (!(i915_gem_object_has_cache_level(obj, I915_CACHE_NONE) || >>>> + HAS_LLC(i915))) { >>>> ret = -EFAULT; >>>> goto err_unpin; >>>> } >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> index 8c70a0ec7d2f..46a19b099ec8 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> @@ -54,6 +54,24 @@ unsigned int i915_gem_get_pat_index(struct >>>> drm_i915_private *i915, >>>> return INTEL_INFO(i915)->cachelevel_to_pat[level]; >>>> } >>>> >>>> +bool i915_gem_object_has_cache_level(const struct drm_i915_gem_object >>>> *obj, >>>> + enum i915_cache_level lvl) >>>> +{ >>>> + /* >>>> + * In case the pat_index is set by user space, this kernel mode >>>> + * driver should leave the coherency to be managed by user space, >>>> + * simply return true here. >>>> + */ >>>> + if (obj->pat_set_by_user) >>>> + return true; >>> >>> This function gets called in a few different places; if the question is >>> "does the object have specific cache behavior XX" then universally >>> returning "yes" if the user has taken direct control of PAT seems >>> confusing, especially since we're not looking at what cache level was >>> being queried nor what PAT index was selected by userspace. A return >>> value of 'true' here could be correct in some situations but not others, >>> depending on how the caller intends to use that information. I assume >>> you've gone through all the places this function gets called and ensured >>> that a return value of true results in the expected behavior; you may >>> want to elaborate on that in the commit message. >> There is a patch later in the series disabling {set|get}_caching ioctl for >> objects with pat_index set by userspace. With that the logic here would >> make sure kernel won't touch the cache settings for these objects. There >> is one caller vm_fault_gtt() might be a bit sensitive though, CI doesn't >> report any problem here, but if further changes in this part of the code >> were made, the logic here might need to be refined. > > From a quick skim, it looks like there are callsites at: > - gpu_write_needs_clflush() > - i915_gem_object_set_cache_level() > - i915_gem_get_caching_ioctl() > - use_cpu_reloc() > - vm_fault_gtt() > > For some of them like the ioctl function, we'll presumably just never > get to the point in the function where it's called if userspace has > taken control of PAT indices, but that's the kind of thing that needs to > be clearly explained. These changes are a large change to how the > driver works and people are going to be looking to the commit messages, > existing kerneldoc, etc. to figure out how things should work down the > road. We can't just explain stuff in a mailing list thread and rely on > people remembering the details months or years from now. There may be > new places in the driver where i915_gem_object_has_cache_level() gets > used in the future and the non-obvious "always returns true" behavior > needs to be clearly documented to make sure that the new callers are > able to handle that properly. Added comments for gpu_write_needs_clflush(), use_cpu_reloc() and vm_fault_gtt(). Others have comments explaining when to skip already. > So I'm not saying any of the code is wrong today, I'm just saying that > we need to make sure it's properly documented so that it's maintainable > in the future. > > > Matt