> On 26/04/2023 16:41, Yang, Fei wrote: >>> On 26/04/2023 07:24, fei.y...@intel.com wrote: >>>> From: Fei Yang <fei.y...@intel.com> >>>> >>>> The first three patches in this series are taken from >>>> https://patchwork.freedesktop.org/series/116868/ >>>> These patches are included here because the last patch >>>> has dependency on the pat_index refactor. >>>> >>>> This series is focusing on uAPI changes, >>>> 1. end support for set caching ioctl [PATCH 4/5] >>>> 2. add set_pat extension for gem_create [PATCH 5/5] >>>> >>>> v2: drop one patch that was merged separately >>>> 341ad0e8e254 drm/i915/mtl: Add PTE encode function >>> >>> Are the re-sends for stabilizing the series, or focusing on merge? >> >> v2 was sent just to drop one of patches that has already been merged. >> >>> If the latter then opens are: >>> >>> 1) Link to Mesa MR reviewed and ready to merge. >>> >>> 2) IGT reviewed. >>> >>> 3) I raised an open that get/set_caching should not "lie" but return an >>> error if set pat extension has been used. I don't see a good reason not >>> to do that. >> >> I don't think it's "lying" to the userspace. the comparison is only valid >> for objects created by KMD because only such object uses the pat_index >> from the cachelevel_to_pat table. > > Lets double check my understanding is correct. Userspace sequence of > operations: > 1) > obj = gem_create()+set_pat(PAT_UC) > > 2a) > get_caching(obj) > What gets reported?
I see your point here. nice catch. That should be handled by, if (obj->cachel_level == I915_CACHE_INVAL) /* indicated pat-index is set by userspace */ return -EOPNOTSUPP; Will update the patch. > > 2b) > set_caching(obj, I915_CACHE_LLC) > What is the return code? This will either return -EOPNOTSUPP, or ignored because set_pat extension was called. > > If answer to 2a is I915_CACHING_CACHED and to 2b) success, then please > state a good reason why both shouldn't return an error. > >> >>> + Joonas on this one. >>> >>> 4) Refactoring as done is not very pretty and I proposed an idea for a >>> nicer approach. Feasible or not, open for discussion. >> >> Still digesting your proposal. but not sure how would you define things >> like PAT_UC as that is platform dependent, not a constant. > > "PAT_UC" in my outline was purely a driver specific value, similarly as > I915_CACHE_... are. Okay. Then you were suggesting to add a translation table for pat_index-to-(PAT-UC/WB/WT)? It's going to be a N:1 mapping. > With the whole point that driver can ask if > something is uncached, WT or whatever. Using the platform specific > mapping table which converts platform specific obj->pat_index to driver > representation of caching modes (PAT_UC etc). Are you suggesting completely remove i915_cache_level and use (PAT-UC/WB/WT) instead? Let's say a KMD object wants to be set as WB, how you get the exact pat_index to use? Note that there are multiple PAT indices having caching mod WB, but they are different, e.g. do you want just WB or WB with 1-way-coherency or WB with 2-way coherency? Also, in case a checking of pat_index is needed, do we say WB with 1-way-coherency is equal to WB or not? BTW, isn't PAT-UC/WB/WT the same kind of abstraction as enum i915_cache_level, I'm not sure how that would simplify anything. > Quite possible I missed some detail, but if I haven't then it could be > a neat and lightweight solution. > > Regards, > > Tvrtko
