<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 ContentPasted2"> > On 26/04/2023 16:41, Yang, Fei wrote: <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> <div class="ContentPasted0">> get_caching(obj)</div> <div class="ContentPasted0"><span class="ContentPasted1 ContentPasted3" style="margin: 0px; background-color: rgb(255, 255, 255);">> What gets reported?</span><br class="Apple-interchange-newline ContentPasted3"> <br> </div> <div class="ContentPasted0">I see your point here. nice catch.</div> <div class="ContentPasted0">That should be handled by,</div> <div class="ContentPasted0">if (obj->cachel_level == I915_CACHE_INVAL) /* indicated pat-index is set by userspace */</div> <div class="ContentPasted0"><span> return -EOPNOTSUPP;</span><br> </div> <div class="ContentPasted0"><span>Will update the patch.</span></div> <div class="ContentPasted0"><span><br> </span></div> <div class="ContentPasted0"> <div class="ContentPasted1">></div> <div class="ContentPasted1">> 2b)</div> <div class="ContentPasted1">> set_caching(obj, I915_CACHE_LLC)</div> <div><span class="ContentPasted1 ContentPasted4" style="margin: 0px; background-color: rgb(255, 255, 255);">> What is the return code?</span><br class="Apple-interchange-newline ContentPasted4"> <br class="ContentPasted1"> </div> <div class="ContentPasted1">This will either return -EOPNOTSUPP, or ignored because set_pat extension was called.</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><br class="ContentPasted1"> </div> <div class="ContentPasted1">Okay. Then you were suggesting to add a translation table for pat_index-to-(PAT-UC/WB/WT)?</div> <div class="ContentPasted1">It's going to be a N:1 mapping.</div> <div><br class="ContentPasted1"> </div> <div class="ContentPasted1">> With the whole point that driver can ask if</div> <div class="ContentPasted1">> something is uncached, WT or whatever. Using the platform specific</div> <div class="ContentPasted1">> mapping table which converts platform specific obj->pat_index to driver</div> <div class="ContentPasted1">> representation of caching modes (PAT_UC etc).</div> <div><br class="ContentPasted1"> </div> <div class="ContentPasted1">Are you suggesting completely remove i915_cache_level and use (PAT-UC/WB/WT) instead?</div> <div class="ContentPasted1">Let's say a KMD object wants to be set as WB, how you get the exact pat_index to use?</div> </div> Note that there are multiple PAT indices having caching mod WB, but they are different, <div class="ContentPasted2">e.g. do you want just WB or WB with 1-way-coherency or WB with 2-way coherency?</div> <div><br class="ContentPasted2"> </div> <div class="ContentPasted2">Also, in case a checking of pat_index is needed, do we say WB with 1-way-coherency is</div> <div class="ContentPasted2">equal to WB or not?</div> <div><br class="ContentPasted2"> </div> <div class="ContentPasted2">BTW, isn't PAT-UC/WB/WT the same kind of abstraction as enum i915_cache_level, I'm not</div> <div class="ContentPasted2">sure how that would simplify anything.</div> <div><br class="ContentPasted2"> </div> <div class="ContentPasted2">> Quite possible I missed some detail, but if I haven't then it could be</div> <div class="ContentPasted2">> a neat and lightweight solution.</div> <div class="ContentPasted2">></div> <div class="ContentPasted2">> Regards,</div> <div class="ContentPasted2">></div> <div class="ContentPasted2">> Tvrtko</div> </div> </body> </html>