<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof ContentPasted0 ContentPasted1 ContentPasted2 ContentPasted3 ContentPasted4"> > On 27/04/2023 17:07, Yang, Fei wrote: <div class="ContentPasted0">>>> On 26/04/2023 16:41, Yang, Fei wrote:</div> <div class="ContentPasted0">>>>>> On 26/04/2023 07:24, fei.yang@intel.com wrote:</div> <div class="ContentPasted0">>>>>>> From: Fei Yang <fei.yang@intel.com></div> <div class="ContentPasted0">>>>>>></div> <div class="ContentPasted0">>>>>>> The first three patches in this series are taken from</div> <div class="ContentPasted0">>>>>>> https://patchwork.freedesktop.org/series/116868/</div> <div class="ContentPasted0">>>>>>> These patches are included here because the last patch</div> <div class="ContentPasted0">>>>>>> has dependency on the pat_index refactor.</div> <div class="ContentPasted0">>>>>>></div> <div class="ContentPasted0">>>>>>> This series is focusing on uAPI changes,</div> <div class="ContentPasted0">>>>>>> 1. end support for set caching ioctl [PATCH 4/5]</div> <div class="ContentPasted0">>>>>>> 2. add set_pat extension for gem_create [PATCH 5/5]</div> <div class="ContentPasted0">>>>>>></div> <div class="ContentPasted0">>>>>>> v2: drop one patch that was merged separately</div> <div class="ContentPasted0">>>>>>> 341ad0e8e254 drm/i915/mtl: Add PTE encode function</div> <div class="ContentPasted0">>>>>></div> <div class="ContentPasted0">>>>>> Are the re-sends for stabilizing the series, or focusing on merge?</div> <div class="ContentPasted0">>>>></div> <div class="ContentPasted0">>>>> v2 was sent just to drop one of patches that has already been merged.</div> <div class="ContentPasted0">>>>></div> <div class="ContentPasted0">>>>>> If the latter then opens are:</div> <div class="ContentPasted0">>>>>></div> <div class="ContentPasted0">>>>>> 1) Link to Mesa MR reviewed and ready to merge.</div> <div class="ContentPasted0">>>>>></div> <div class="ContentPasted0">>>>>> 2) IGT reviewed.</div> <div class="ContentPasted0">>>>>></div> <div class="ContentPasted0">>>>>> 3) I raised an open that get/set_caching should not "lie" but return an</div> <div class="ContentPasted0">>>>>> error if set pat extension has been used. I don't see a good reason not</div> <div class="ContentPasted0">>>>>> to do that.</div> <div class="ContentPasted0">>>>></div> <div class="ContentPasted0">>>>> I don't think it's "lying" to the userspace. the comparison is only valid</div> <div class="ContentPasted0">>>>> for objects created by KMD because only such object uses the pat_index</div> <div class="ContentPasted0">>>>> from the cachelevel_to_pat table.</div> <div class="ContentPasted0">>>></div> <div class="ContentPasted0">>>> Lets double check my understanding is correct. Userspace sequence of</div> <div class="ContentPasted0">>>> operations:</div> <div class="ContentPasted0">>>> 1)</div> <div class="ContentPasted0">>>> obj = gem_create()+set_pat(PAT_UC)</div> <div class="ContentPasted0">>>></div> <div class="ContentPasted0">>>> 2a)</div> >>> get_caching(obj) <div class="ContentPasted1">>>> What gets reported?</div> <div class="ContentPasted1">>></div> <div class="ContentPasted1">>> I see your point here. nice catch.</div> <div class="ContentPasted1">>> That should be handled by,</div> <div class="ContentPasted1">>> if (obj->cachel_level == I915_CACHE_INVAL) /* indicated pat-index is set by userspace */</div> <div class="ContentPasted1">>> return -EOPNOTSUPP;</div> <div class="ContentPasted1">>> Will update the patch.</div> <div class="ContentPasted1">>></div> <div class="ContentPasted1">>>></div> <div class="ContentPasted1">>>> 2b)</div> <div class="ContentPasted1">>>> set_caching(obj, I915_CACHE_LLC)</div> <div class="ContentPasted1">>>> What is the return code?</div> <div class="ContentPasted1">>></div> <div class="ContentPasted1">>> This will either return -EOPNOTSUPP, or ignored because set_pat</div> <div class="ContentPasted1">>> extension was called.</div> <div class="ContentPasted1">></div> <div class="ContentPasted1">> I see that you made it fail instead of fake success in the latest respin</div> <div class="ContentPasted1">> and I definitely agree with that.</div> <div><br class="ContentPasted1"> </div> <div class="ContentPasted1">Thanks for your picky eyes. :)</div> <div><br class="ContentPasted1"> </div> <div class="ContentPasted1">>>></div> <div class="ContentPasted1">>>> If answer to 2a is I915_CACHING_CACHED and to 2b) success, then please</div> <div class="ContentPasted1">>>> state a good reason why both shouldn't return an error.</div> <div class="ContentPasted1">>>></div> <div class="ContentPasted1">>>>></div> <div class="ContentPasted1">>>>>> + Joonas on this one.</div> <div class="ContentPasted1">>>>>></div> <div class="ContentPasted1">>>>>> 4) Refactoring as done is not very pretty and I proposed an idea for a</div> <div class="ContentPasted1">>>>>> nicer approach. Feasible or not, open for discussion.</div> <div class="ContentPasted1">>>>></div> <div class="ContentPasted1">>>>> Still digesting your proposal. but not sure how would you define things</div> <div class="ContentPasted1">>>>> like PAT_UC as that is platform dependent, not a constant.</div> <div class="ContentPasted1">>>></div> <div class="ContentPasted1">>>> "PAT_UC" in my outline was purely a driver specific value, similarly as</div> <div class="ContentPasted1">>>> I915_CACHE_... are.</div> <div class="ContentPasted1">>></div> <div class="ContentPasted1">>> Okay. Then you were suggesting to add a translation table for</div> <div class="ContentPasted1">>> pat_index-to-(PAT-UC/WB/WT)?</div> <div class="ContentPasted1">>> It's going to be a N:1 mapping.</div> > <div class="ContentPasted2">> PAT index to a value, probably a bitmask, built out of bits which define</div> <div class="ContentPasted2">> caching modes. Like "PAT_WB | PAT_1WAY_COHERENT", or just PAT_WB. Would</div> <div class="ContentPasted2">> that approach be enough to express everything?</div> <div><br class="ContentPasted2"> </div> <div class="ContentPasted2">I was thinking about dumping the PAT tables from Bspec into struct intel_device_info {}.</div> <div class="ContentPasted2">But that would be only useful to unify all those *_setup_private_ppat() functions in</div> <div class="ContentPasted2">intel_gtt.c. Kernel doesn't really care much about PAT index except making sure the bits</div> <div class="ContentPasted2">are programmed correctly in PTE.</div> <div><br class="ContentPasted2"> </div> <div class="ContentPasted2">>></div> <div class="ContentPasted2">>>> With the whole point that driver can ask if</div> <div class="ContentPasted2">>>> something is uncached, WT or whatever. Using the platform specific</div> <div class="ContentPasted2">>>> mapping table which converts platform specific obj->pat_index to driver</div> <div class="ContentPasted2">>>> representation of caching modes (PAT_UC etc).</div> <div class="ContentPasted2">>></div> <div class="ContentPasted2">>> Are you suggesting completely remove i915_cache_level and use</div> <div class="ContentPasted2">>> (PAT-UC/WB/WT) instead?</div> <div class="ContentPasted2">></div> <div class="ContentPasted2">> Not completely but throughout the most internal code paths, which would</div> <div class="ContentPasted2">> all just work on PAT index. Basically object always has PAT index set,</div> <div class="ContentPasted2">> with a separate boolean saying whether it came from gem_create_ext or</div> <div class="ContentPasted2">> set_cache_level.</div> <div><br class="ContentPasted2"> </div> <div class="ContentPasted2">What's in my mind as an improvement is to completely remove i915_cache_level.</div> <div class="ContentPasted2">KMD is supposed to abstract the hardware, but in this case, since the desgin</div> <div class="ContentPasted2">is that UMDs understand PAT index (and MOCS as well), having an abstraction</div> <div class="ContentPasted2">in between would only introduce ambiguity and complexity.</div> <div><br class="ContentPasted2"> </div> <div class="ContentPasted2">Also kernel doesn't need to play with all available PAT indices, so it should</div> <div class="ContentPasted2">be okay to add i915->pat_{uc|wb|wt} and initialize them with proper indices</div> <div class="ContentPasted2">respectively. That takes care of the PAT checking as well wherever it's needed,</div> <div class="ContentPasted2">like "if (pat_index == i915->pat_uc)"</div> <div><br class="ContentPasted2"> </div> <div class="ContentPasted2">>> Let's say a KMD object wants to be set as WB, how you get the exact pat_index to use?</div> <div class="ContentPasted2">>> Note that there are multiple PAT indices having caching mod WB, but they are different,</div> <div class="ContentPasted2">>> e.g. do you want just WB or WB with 1-way-coherency or WB with 2-way</div> <div class="ContentPasted2">>> coherency?</div> <div class="ContentPasted2">></div> <div class="ContentPasted2">> Just use the cache_level to pat_index mapping you added in the series?</div> <div class="ContentPasted2">></div> >> Also, in case a checking of pat_index is needed, do we say WB with <div class="ContentPasted3">>> 1-way-coherency is equal to WB or not?</div> <div class="ContentPasted3">></div> <div class="ContentPasted3">> You mean the call sites where i915 is checking the object caching mode?</div> <div class="ContentPasted3">> We have two call sites which check for !I915_CACHE_NONE. Those would</div> <div class="ContentPasted3">> just check if PAT_UC is not set.</div> <div class="ContentPasted3">></div> <div class="ContentPasted3">> Then the one in gpu_write_needs_clflush is checking for neither UC nor</div> <div class="ContentPasted3">> WT, which also directly translates.</div> <div class="ContentPasted3">></div> <div class="ContentPasted3">> For the WB case there aren't any callers but if we just checked for</div> <div class="ContentPasted3">> "base" PAT_WB bit being set that would work.</div> <div class="ContentPasted3">></div> <div class="ContentPasted3">> So in all cases helper which does "return bits_required | bits_set"</div> <div class="ContentPasted3">> seems would work fine.</div> <div class="ContentPasted3">></div> <div class="ContentPasted3">>> BTW, isn't PAT-UC/WB/WT the same kind of abstraction as enum</div> <div class="ContentPasted3">>> i915_cache_level, I'm not</div> <div class="ContentPasted3">>> sure how that would simplify anything.</div> <div class="ContentPasted3">></div> <div class="ContentPasted3">> As I wrote before, I *think* it provides a way of not needing to</div> <div class="ContentPasted3">> sprinkle around i915_gem_get_pat_index and a simpler "has_cache_level".</div> <div class="ContentPasted3">> Conceptually cache_level->pat_index is done only in gem_create_ext and</div> <div class="ContentPasted3">> set_cache_level. Lower level code does not have to consult cache_level</div> <div class="ContentPasted3">> at all. And existence of tables simplifies the pretty printing code to a</div> <div class="ContentPasted3">> platform agnostic loop.</div> <div class="ContentPasted3">></div> <div class="ContentPasted3">> I *think* at least.. We can leave it all for later. My main concern was</div> <div class="ContentPasted3">> that UAPI needs to be clear and solid which it now seems to be.</div> <div><br class="ContentPasted3"> </div> <div class="ContentPasted3">I'm hesitant to do all that I said above in one shot. But I think completely</div> <div class="ContentPasted3">removing enum i915_cache_level is what to do next.</div> <div><br class="ContentPasted3"> </div> <div class="ContentPasted3">-Fei</div> <div><br class="ContentPasted3"> </div> <div class="ContentPasted3">> Regards,</div> <div class="ContentPasted3">></div> <div class="ContentPasted3">> Tvrtko</div> <div class="ContentPasted3">></div> <div class="ContentPasted3">>></div> <div class="ContentPasted3">>>> Quite possible I missed some detail, but if I haven't then it could be</div> >>> a neat and lightweight solution.<br> </div> </body> </html>