[Intel-gfx] [PATCH v4 2/2] drm/i915/icl: Define MOCS table for Icelake

[Joonas Lahtinen]

Quoting Tomasz Lis (2018-11-05 15:50:21)
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -44,6 +44,8 @@ struct drm_i915_mocs_table {
>  #define LE_SCC(value)          ((value) << 8)
>  #define LE_PFM(value)          ((value) << 11)
>  #define LE_SCF(value)          ((value) << 14)
> +#define LE_CoS(value)          ((value) << 15)

Any specific reason this is not LE_COS (vs. CoS)?

> +#define LE_SSE(value)          ((value) << 17)
>  
>  /* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
>  #define L3_ESC(value)          ((value) << 0)
> @@ -80,21 +82,21 @@ struct drm_i915_mocs_table {
>   * LNCFCMOCS0 - LNCFCMOCS32 registers.
>   *
>   * These tables are intended to be kept reasonably consistent across
> - * platforms. However some of the fields are not applicable to all of
> - * them.
> + * HW platforms, and for ICL+, be identical across OSes. To achieve
> + * that, for Icelake and above, list of entries is published as part
> + * 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 implicitly initialized to the strictest caching
> - * configuration (uncached) to guarantee forwards compatibility with
> - * userspace programs written against more recent kernels providing
> - * additional MOCS entries.
> + * userspace is concerned and shouldn't be relied upon.
>   *
> - * NOTE: These tables MUST start with being uncached and the length
> - *       MUST be less than 63 as the last two registers are reserved
> - *       by the hardware.  These tables are part of the kernel ABI and
> - *       may only be updated incrementally by adding entries at the
> - *       end.
> + * The last two entries are reserved by the hardware. For ICL+ they
> + * should be initialized according to bspec and never used, for older
> + * platforms they should never be written to.
> + *
> + * NOTE: These tables are part of bspec and defined as part of hardware
> + *       interface for ICL+. For older platforms, they are part of kernel
> + *       ABI. It is expected that existing entries will remain constant
> + *       and the tables will only be updated by adding new entries.
>   */
>  
>  #define GEN9_MOCS_TABLE \
> @@ -144,6 +146,222 @@ static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
>         },
>  };
>  
> +#define GEN11_MOCS_TABLE \

#define GEN11_MOCS_ENTRIES would be more truthful, and same applies for
GEN9, of course.

> +       [0] = { \
> +         /* Base - Uncached (Deprecated) */ \
> +         .control_value = LE_CACHEABILITY(LE_UC) | \
> +                          LE_TGT_CACHE(LE_TC_LLC) | \
> +                          LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | \
> +                          LE_PFM(0) | LE_SCF(0) | LE_CoS(0) | LE_SSE(0), \
> +         .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC), \
> +       }, \

My wild assumption would be that these don't please checkpatch.

Maybe worthy fixing the indent while converting the GEN9 to defines,
too?

> +       [1] = { \
> +         /* Base - L3 + LeCC:PAT (Deprecated) */ \
> +         .control_value = LE_CACHEABILITY(LE_PAGETABLE) | \
> +                          LE_TGT_CACHE(LE_TC_LLC) | \
> +                          LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | \
> +                          LE_PFM(0) | LE_SCF(0) | LE_CoS(0) | LE_SSE(0), \
> +         .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB), \

If these are coming off a table, would it make sense to have a dedicated
#define to make them more table-like for easier review?

	.control_value = MOCS_CONTROL_VALUE(0, 0, 0, 0, 0, 0, 0, 0),

Regards, Joonas