[Intel-gfx] [PATCH 2/4] drm/i915/tgl: Define MOCS entries for Tigerlake

Fri Jul 26 20:17:06 UTC 2019

On 7/26/19 12:31 PM, Lucas De Marchi wrote:
> On Fri, Jul 26, 2019 at 11:38:16AM -0700, Daniele Ceraolo Spurio wrote:
>>
>>
>> On 7/25/19 5:12 PM, Lucas De Marchi wrote:
>>> From: Tomasz Lis <tomasz.lis at intel.com>
>>>
>>> The MOCS table is published as part of bspec, and versioned. Entries
>>> are supposed to never be modified, but new ones can be added. Adding
>>> entries increases table version. The patch includes version 1 entries.
>>>
>>> Two of the 3 legacy entries used for gen9 are no longer expected to 
>>> work.
>>> Although we are changing the gen11 table, those changes are supposed to
>>> be backward compatible since we are only touching previously undefined
>>> entries.
>>>
>>> v2: Add the missing entries in 49-51 range and replace "HW reserved"
>>>     terminology to what it actually is: L1 is implicitly enabled 
>>> (from Daniele)
>>>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> Signed-off-by: Tomasz Lis <tomasz.lis at intel.com>
>>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/gt/intel_mocs.c | 37 +++++++++++++++++++++++++---
>>>  1 file changed, 34 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c 
>>> b/drivers/gpu/drm/i915/gt/intel_mocs.c
>>> index 290a5e9b90b9..ca370c7487f9 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
>>> @@ -62,6 +62,10 @@ struct drm_i915_mocs_table {
>>>  #define GEN11_NUM_MOCS_ENTRIES    64  /* 63-64 are reserved, but 
>>> configured. */
>>>  /* (e)LLC caching options */
>>> +/*
>>> + * Note: LE_0_PAGETABLE works only up to Gen11; for newer gens it means
>>> + * the same as LE_UC
>>> + */
>>>  #define LE_0_PAGETABLE        _LE_CACHEABILITY(0)
>>>  #define LE_1_UC            _LE_CACHEABILITY(1)
>>>  #define LE_2_WT            _LE_CACHEABILITY(2)
>>> @@ -100,8 +104,9 @@ struct drm_i915_mocs_table {
>>>   * of bspec.
>>>   *
>>>   * Entries not part of the following tables are undefined as far as
>>> - * userspace is concerned and shouldn't be relied upon.  For the time
>>> - * being they will be initialized to PTE.
>>> + * userspace is concerned and shouldn't be relied upon.  For Gen < 12
>>> + * they will be initialized to PTE. Gen >= 12 onwards don't have a 
>>> setting for
>>> + * PTE. We use the same value, but that actually means Uncached.
>>>   *
>>>   * The last two entries are reserved by the hardware. For ICL+ they
>>>   * should be initialized according to bspec and never used, for older
>>> @@ -137,11 +142,13 @@ static const struct drm_i915_mocs_entry 
>>> broxton_mocs_table[] = {
>>>  };
>>>  #define GEN11_MOCS_ENTRIES \
>>> -    /* Base - Uncached (Deprecated) */ \
>>> +    /* Gen11: Base - Uncached (Deprecated) */ \
>>> +    /* Gen12+: Base - Error (Reserved for Non-Use) */ \
>>>      MOCS_ENTRY(I915_MOCS_UNCACHED, \
>>>             LE_1_UC | LE_TC_1_LLC, \
>>>             L3_1_UC), \
>>>      /* Base - L3 + LeCC:PAT (Deprecated) */ \
>>> +    /* Gen12+: Base - Reserved */ \
>>>      MOCS_ENTRY(I915_MOCS_PTE, \
>>>             LE_0_PAGETABLE | LE_TC_1_LLC, \
>>>             L3_3_WB), \
>>
>>
>> I've double-checked the specs and noticed that they now say that MOCS 
>> 0 and 1 should be set to all zeros for Gen12 (possibly just to 
>> highlight the fact that they're deprecated/reserved). These are not 
>> supposes to be used so programming them to different values shouldn't 
>> matter, but it might be worth sticking to the specs and add a separate 
>> Gen12 table.
> 
> I noticed that too, but while implementing the IGT tests. These are 0,
> but they are RO, it doesn't matter what we write on them.
> 
> I could add a comment here and change the IGT test to check they are in
> fact 0 (rather than setting them as unused). Would this cover your
> concern? I feel reluctant to add a big table like this per platform if
> there's a wayto avoid it.
> 

Yes, that works for me.

Daniele

> 
>>
>> I've confirmed all the other entries in the gen11 table are kept the 
>> same in the TGL one, and the ones added below match the table as well.
>>
>>> @@ -233,6 +240,30 @@ static const struct drm_i915_mocs_entry 
>>> broxton_mocs_table[] = {
>>>      MOCS_ENTRY(23, \
>>>             LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_RSC(1) | 
>>> LE_SCC(7), \
>>>             L3_3_WB), \
>>> +    /* Gen12+: Implicitly enable L1 - HDC:L1 + L3 + LLC */ \
>>> +    MOCS_ENTRY(48, \
>>> +           LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
>>> +           L3_3_WB), \
>>> +    /* Gen12+: Implicitly enable L1 - HDC:L1 + L3 */ \
>>> +    MOCS_ENTRY(49, \
>>> +           LE_1_UC | LE_TC_1_LLC, \
>>> +           L3_3_WB), \
>>> +    /* Gen12+: Implicitly enable L1 - HDC:L1 + LLC */ \
>>> +    MOCS_ENTRY(50, \
>>> +           LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
>>> +           L3_1_UC), \
>>> +    /* Gen12+: Implicitly enable L1 - HDC:L1 */ \
>>> +    MOCS_ENTRY(51, \
>>> +           LE_1_UC | LE_TC_1_LLC, \
>>> +           L3_1_UC), \
>>> +    /* Gen12+: HW Reserved - HW Special Case (CCS) */ \
>>
>> Entries 60 and 61 are not reserved for HW usage but are special cases 
>> that trigger implicit behaviors. I'd drop the "HW Reserved" tag and 
>> leave just "HW Special Case"
> 
> ok
> 
> thanks
> Lucas De Marchi
> 
>>
>> Daniele
>>
>>> +    MOCS_ENTRY(60, \
>>> +           LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
>>> +           L3_1_UC), \
>>> +    /* Gen12+: HW Reserved - HW Special Case (Displayable) */ \
>>> +    MOCS_ENTRY(61, \
>>> +           LE_1_UC | LE_TC_1_LLC | LE_SCF(1), \
>>> +           L3_3_WB), \
>>>      /* HW Reserved - SW program but never use */ \
>>>      MOCS_ENTRY(62, \
>>>             LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
>>>