<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> On 10/6/2023 09:18, John Harrison wrote: <blockquote type="cite" cite="mid:cae4f144-2e88-9c3c-985b-849e7f5ff123@intel.com"> On 10/6/2023 03:20, Nirmoy Das wrote: <blockquote type="cite" cite="mid:b95d8be7-c546-1b1c-3975-a4ef6257a28d@intel.com"> On 10/6/2023 12:11 PM, Tvrtko Ursulin wrote: <blockquote type="cite"> Hi, Andi asked me to summarize what I think is unaddressed review feedback so far in order to consolidate and enable hopefully things to move forward. So I will try to re-iterate the comments and questions below. But also note that there is a bunch of new valid comments from John against v7 which I will not repeat. On 05/10/2023 20:35, Jonathan Cavitt wrote: <blockquote type="cite">... +enum intel_guc_tlb_invalidation_type { + INTEL_GUC_TLB_INVAL_FULL = 0x0, + INTEL_GUC_TLB_INVAL_GUC = 0x3, </blockquote> New question - are these names coming from the GuC iface? I find it confusing that full does not include GuC but maybe it is just me. So maybe full should be called GT or something? Although then again it wouldn't be clear GT does not include the GuC.. bummer. GPU? Dunno. Minor confusion I guess so can keep as is. </blockquote> I agree this is bit confusing name. We are using INTEL_GUC_TLB_INVAL_GUC to invalidate ggtt, how about INTEL_GUC_TLB_INVAL_GGTT ? </blockquote> The GuC interface spec says: <blockquote>GUC_TLB_INV_TYPE_TLB_INV_FULL_INTRA_VF = 0x00 Full TLB invalidation within a VF. Invalidates VF’s TLBs only if that VF is active, will invalidate across all engines. GUC_TLB_INV_TYPE_TLB_INV_GUC = 0x03 Guc TLB Invalidation. </blockquote> So the 'GUC' type is not GGTT, it is the TLBs internal to GuC itself is how I would read the above. Whereas 'FULL' is everything that is per-VF, aka everything in the GT that is beyond the GuC level - i.e. the engines, EUs and everything from there on. So I think the INVAL_GUC name is correct. But maybe INVAL_FULL should be called INVAL_VF? Or INVAL_ENGINES if you don't like using the VF term in a non-SRIOV capable driver? John. </blockquote> PS: The function names should also match the type name. Currently the functions are: int intel_guc_invalidate_tlb_full(struct intel_guc *guc); int intel_guc_invalidate_tlb(struct intel_guc *guc); The second should have a suffix to say what is being invalidated - e.g. intel_guc_invalidate_tlb_guc(). The 'guc' in the prefix is just describing the mechanism not the target. So I would read the above as 'full' being a subset of 'blank'. John. </body> </html>