Re: [PATCH v3 1/3] drm/i915/gen9: Clean up MOCS table definitions

[Francisco Jerez <currojerez@riseup.net>]

[-- Attachment #1.1.1: Type: text/plain, Size: 5559 bytes --]

Hi Imre,

Imre Deak <imre.deak@intel.com> writes:

> Use named struct initializers for clarity. Also fix the target cache
> definition to reflect its role in GEN9 onwards. On GEN8 a TC value of 0
> meant ELLC but on GEN9+ it means the TC and LRU controls are taken from
> the PTE.
>
I vaguely recall bringing up this apparent inconsistency with the
hardware spec during review of the original series of patches
programming the MOCS tables.  IIRC TC value 0 behaved as ELLC-only on
the simulator, but the hardware docs claimed it would take the target
cache from the PTE controls, not sure what the real hardware actually
does.  Maybe Peter can confirm whether this was intentional?

> No functional change, igt/gem_mocs_settings still passing after this
> change.
>
> v2: (Chris)
> - Add back the hexa literals for the entries.
>   Add note that igt/gem_mocs_settings still passes.
>
> CC: Rong R Yang <rong.r.yang@intel.com>
> CC: Yakui Zhao <yakui.zhao@intel.com>
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_mocs.c | 88 +++++++++++++++++++++++++++------------
>  1 file changed, 61 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index 3c1482b..d36e609 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -66,9 +66,10 @@ struct drm_i915_mocs_table {
>  #define L3_WB			3
>  
>  /* Target cache */
> -#define ELLC			0
> -#define LLC			1
> -#define LLC_ELLC		2
> +#define LE_TC_PAGETABLE		0
> +#define LE_TC_LLC		1
> +#define LE_TC_LLC_ELLC		2
> +#define LE_TC_LLC_ELLC_ALT	3
>  
>  /*
>   * MOCS tables
> @@ -96,34 +97,67 @@ struct drm_i915_mocs_table {
>   *       end.
>   */
>  static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
> -	/* { 0x00000009, 0x0010 } */
> -	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
> -	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
> -	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
> -	/* { 0x00000038, 0x0030 } */
> -	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
> -	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
> -	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
> -	/* { 0x0000003b, 0x0030 } */
> -	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
> -	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
> -	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
> +	{ /* 0x00000009 */
> +	  .control_value = LE_CACHEABILITY(LE_UC) |
> +			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> +			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	},
> +	{
> +	  /* 0x00000038 */
> +	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
> +			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> +			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0030 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	},
> +	{
> +	  /* 0x0000003b */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> +			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0030 */
> +	  .l3cc_value =   L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	},
>  };
>  
>  /* NOTE: the LE_TGT_CACHE is not used on Broxton */
>  static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
> -	/* { 0x00000009, 0x0010 } */
> -	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
> -	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
> -	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
> -	/* { 0x00000038, 0x0030 } */
> -	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
> -	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
> -	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
> -	/* { 0x0000003b, 0x0030 } */
> -	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
> -	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
> -	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
> +	{
> +	  /* 0x00000009 */
> +	  .control_value = LE_CACHEABILITY(LE_UC) |
> +			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> +			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	},
> +	{
> +	  /* 0x00000038 */
> +	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
> +			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> +			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +
> +	  /* 0x0030 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	},
> +	{
> +	  /* 0x0000003b */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> +			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +
> +	  /* 0x0030 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	},
>  };
>  
>  /**
> -- 
> 2.5.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx