On 10/6/2023 09:18, John Harrison wrote:
On 10/6/2023 03:20, Nirmoy Das wrote:
On 10/6/2023 12:11 PM, Tvrtko Ursulin wrote:
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:
...
+enum intel_guc_tlb_invalidation_type {
+ INTEL_GUC_TLB_INVAL_FULL = 0x0,
+ INTEL_GUC_TLB_INVAL_GUC = 0x3,
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.
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 ?
The GuC interface spec says:
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.
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.
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.
