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

Fri Jul 26 19:31:04 UTC 2019

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.

>
>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), \
>>