Re: [Intel-gfx] [PATCH v7 2/5] drm/i915: Define and use GuC and CTB TLB invalidation routines

[John Harrison <john.c.harrison@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2405 bytes --]

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.

[-- Attachment #2: Type: text/html, Size: 3414 bytes --]