> On 20/04/2023 00:00, fei.yang@xxxxxxxxx wrote: >> From: Fei Yang <fei.yang@xxxxxxxxx> >> >> 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. >> >> 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. >> >> Cc: Chris Wilson <chris.p.wilson@xxxxxxxxxxxxxxx> >> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx> >> Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx> >> Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx> > > I think have some ideas no how to perhaps make this simpler, please bear > with me. > > In my mind get/set caching ioctls need to be failing once explicit pat > index has been set by userspace. Or at least not return false information. By design we are ending the support for set caching ioctl. The patch is included in this series, "drm/i915/mtl: end support for set caching ioctl" + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) + return -EOPNOTSUPP; + > And I don't like i915_gem_object_has_cache_level and > i915_gem_get_pat_index as a refactoring step. > > It also seems that the driver has a need to query the caching mode set > regardless of the route (of setting). Only for the objects created by the KMD. For UMD created objects with PAT index set KMD should never touch the setting. > So how about this. > > Three callers which query the caching mode: use_cpu_reloc, vm_fault_gtt, > gpu_write_needs_clflush. > > We convert them to be like: > > i915_gem_object_has_caching_mode(obj, PAT_UC / PAT_WT / ...); PAT_UC/WT/WB are platform dependent (https://gfxspecs.intel.com/Predator/Home/Index/45101), performing this check you would have to do something like, if (MTL) ... else if (PVC) ... else if (GEN12) ... else ... > Then apart from the per platform tables for mapping between cache level > to pat index, you add tables which map pat index to caching modes > (PAT_UC, etc, naming TBD, just enums or bitmasks also TBD, I haven't > looked at the bspec to see how exactly it works). > > You would use that table in the i915_gem_object_has_caching_mode helper, > called from the above three functions instead of obj->cache_level direct > comparison. > > I am assuming at least for instance cache_level != I915_CACHE_NONE would > be equivalent to i915_gem_object_has_caching_mode(obj, PAT_UC), etc. So far kernel only needs 4 cache levels defined in enum i915_cache_level, kernel doesn't need to understand all PAT indices. By desgin if the userspace is setting PAT index directly, kernel only needs to pass the setting to PTE. For objects created by kernel (including objects created by userspace without specifying pat index), there are only 4 options (defined in the cachelevel_to_pat). For objects created by userspace with PAT index set (GEM_CREATE + set_pat extension), kernel should not touch the setting, just pass it to the PAT index bits in PTE. That's why I was only checking cache_level. Handling PAT index is much more complicated because of its platform dependent nature and even the number of PAT indices varies from platform to platform. Fortunately kernel doesn't need to understand that. -Fei > Same mapping table could also be used in debugfs (i915_cache_level_str) > to universally describe any obj->pat_index, with no need to have > anything platform dependend there. > > In set caching set you always set obj->pat_index and so low level code > can always just use that. > > Unless I am missing something (possible) I think like that we end up > with no i915_gem_get_pat_index sprinkled around and also no confusing > i915_gem_object_has_cache_level. > > Obj->pat_index would be a single point of truth, while obj->cache_level > is just a legacy field for get/set_caching ioctl - not used in the > internal driver flows. > > We would need an additional field for storing the boolean of whether > userspace had overriden the PAT. > > Regards, > > Tvrtko
