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: 1829 bytes --]

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.

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