Re: [igt-dev] [PATCH i-g-t v2] i915/gem_mocs_settings: Add mocs table for icelake

["Kumar Valsan, Prathap" <prathap.kumar.valsan@intel.com>]

On Fri, Feb 22, 2019 at 04:17:34PM +0100, Lis, Tomasz wrote:
> 
> 
> On 2019-02-22 15:17, Prathap Kumar Valsan wrote:
> > From: "Kumar Valsan, Prathap" <prathap.kumar.valsan@intel.com>
> > 
> > This patch adds mocs table for icelake with expected L3 and eDRAM
> > control values.
> > 
> > Signed-off-by: Kumar Valsan, Prathap <prathap.kumar.valsan@intel.com>
> > ---
> > Changes in v2:
> > - Cleaned up the code based on review comments from Lucas and Chris
> >   tests/i915/gem_mocs_settings.c | 96 ++++++++++++++++++++++++++--------
> >   1 file changed, 73 insertions(+), 23 deletions(-)
> > 
> > diff --git a/tests/i915/gem_mocs_settings.c b/tests/i915/gem_mocs_settings.c
> > index 5b3b6bc1..6d111076 100644
> > --- a/tests/i915/gem_mocs_settings.c
> > +++ b/tests/i915/gem_mocs_settings.c
> > @@ -33,6 +33,9 @@
> >   #include "igt_sysfs.h"
> >   #define MAX_NUMBER_MOCS_REGISTERS	(64)
> > +#define GEN9_NUM_MOCS_ENTRIES   62  /* 62 out of 64 - 63 & 64 are reserved. */
> > +#define GEN11_NUM_MOCS_ENTRIES  64  /* 63-64 are reserved, but configured. */
> > +
> >   enum {
> >   	NONE,
> >   	RESET,
> > @@ -72,36 +75,66 @@ struct mocs_table {
> >   	const struct mocs_entry	*table;
> >   };
> > +static const struct mocs_entry icelake_mocs_pte = {0x00000004, 0x0030};
> > +static const struct mocs_entry mocs_pte = {0x00000038, 0x0030};
> > +
> >   /* The first entries in the MOCS tables are defined by uABI */
> > -static const struct mocs_entry skylake_mocs_table[] = {
> > -	{ 0x00000009, 0x0010 },
> > -	{ 0x00000038, 0x0030 },
> > -	{ 0x0000003b, 0x0030 },
> > +static const struct mocs_entry icelake_mocs_table[GEN11_NUM_MOCS_ENTRIES] = {
> > +	[0 ... GEN11_NUM_MOCS_ENTRIES - 1] = icelake_mocs_pte,
> > +	[0]  = { 0x00000005, 0x0010},
> > +	[1]  = { 0x00000004, 0x0030},
> > +	[2]  = { 0x00000037, 0x0030},
> > +	[3]  = { 0x00000005, 0x0010},
> > +	[4]  = { 0x00000005, 0x0030},
> > +	[5]  = { 0x00000037, 0x0010},
> > +	[6]  = { 0x00000017, 0x0010},
> > +	[7]  = { 0x00000017, 0x0030},
> > +	[8]  = { 0x00000027, 0x0010},
> > +	[9]  = { 0x00000027, 0x0030},
> > +	[10] = { 0x00000077, 0x0010},
> > +	[11] = { 0x00000077, 0x0030},
> > +	[12] = { 0x00000057, 0x0010},
> > +	[13] = { 0x00000057, 0x0030},
> > +	[14] = { 0x00000067, 0x0010},
> > +	[15] = { 0x00000067, 0x0030},
> > +	[18] = { 0x00060037, 0x0030},
> > +	[19] = { 0x00000737, 0x0030},
> > +	[20] = { 0x00000337, 0x0030},
> > +	[21] = { 0x00000137, 0x0030},
> > +	[22] = { 0x000003b7, 0x0030},
> > +	[23] = { 0x000007b7, 0x0030},
> > +	[62] = { 0x00000037, 0x0010},
> > +	[63] = { 0x00000037, 0x0010},
> > +};
> > +
> > +static const struct mocs_entry dirty_icelake_mocs_table[GEN11_NUM_MOCS_ENTRIES] = {
> > +	[0 ... GEN11_NUM_MOCS_ENTRIES - 1] = { 0x0007FFFF, 0x003F },
> >   };
> > -static const struct mocs_entry dirty_skylake_mocs_table[] = {
> > -	{ 0x00003FFF, 0x003F }, /* no snoop bit */
> > -	{ 0x00003FFF, 0x003F },
> > -	{ 0x00003FFF, 0x003F },
> > +static const struct mocs_entry skylake_mocs_table[GEN9_NUM_MOCS_ENTRIES] = {
> > +	[0 ... GEN9_NUM_MOCS_ENTRIES - 1] = mocs_pte,
> > +	[0] = { 0x00000009, 0x0010},
> > +	[1] = { 0x00000038, 0x0030},
> > +	[2] = { 0x0000003b, 0x0030},
> >   };
> > -static const struct mocs_entry broxton_mocs_table[] = {
> > -	{ 0x00000009, 0x0010 },
> > -	{ 0x00000038, 0x0030 },
> > -	{ 0x00000039, 0x0030 },
> > +static const struct mocs_entry dirty_skylake_mocs_table[GEN9_NUM_MOCS_ENTRIES] = {
> > +	[0 ... GEN9_NUM_MOCS_ENTRIES - 1] = { 0x00003FFF, 0x003F },
> >   };
> > -static const struct mocs_entry dirty_broxton_mocs_table[] = {
> > -	{ 0x00007FFF, 0x003F },
> > -	{ 0x00007FFF, 0x003F },
> > -	{ 0x00007FFF, 0x003F },
> > +static const struct mocs_entry broxton_mocs_table[GEN9_NUM_MOCS_ENTRIES] = {
> > +	[0 ... GEN9_NUM_MOCS_ENTRIES - 1] = mocs_pte,
> > +	[0] = { 0x00000009, 0x0010},
> > +	[1] = { 0x00000038, 0x0030},
> > +	[2] = { 0x00000039, 0x0030},
> >   };
> > -static const uint32_t write_values[] = {
> > -	0xFFFFFFFF,
> > -	0xFFFFFFFF,
> > -	0xFFFFFFFF,
> > -	0xFFFFFFFF
> > +static const struct mocs_entry dirty_broxton_mocs_table[GEN9_NUM_MOCS_ENTRIES] = {
> > +	[0 ... GEN9_NUM_MOCS_ENTRIES - 1] = { 0x00007FFF, 0x003F },
> > +};
> > +
> > +static const uint32_t write_values[MAX_NUMBER_MOCS_REGISTERS] = {
> > +	[0 ... MAX_NUMBER_MOCS_REGISTERS - 1] = 0xFFFFFFFF,
> >   };
> >   static bool get_mocs_settings(int fd, struct mocs_table *table, bool dirty)
> > @@ -127,6 +160,15 @@ static bool get_mocs_settings(int fd, struct mocs_table *table, bool dirty)
> >   			table->table = broxton_mocs_table;
> >   		}
> >   		result = true;
> > +	} else if (IS_ICELAKE(devid)) {
> > +		if (dirty) {
> > +			table->size  = ARRAY_SIZE(dirty_icelake_mocs_table);
> > +			table->table = dirty_icelake_mocs_table;
> > +		} else {
> > +			table->size  = ARRAY_SIZE(icelake_mocs_table);
> > +			table->table = icelake_mocs_table;
> > +		}
> > +		result = true;
> >   	}
> >   	return result;
> > @@ -374,13 +416,21 @@ static void check_mocs_values(int fd, unsigned engine, uint32_t ctx_id, bool dir
> >   static void write_dirty_mocs(int fd, unsigned engine, uint32_t ctx_id)
> >   {
> > +	uint32_t devid = intel_get_drm_devid(fd);
> > +	int num_of_mocs_entries;
> > +
> > +	if (IS_ICELAKE(devid))
> > +		num_of_mocs_entries = GEN11_NUM_MOCS_ENTRIES;
> > +	else
> > +		num_of_mocs_entries = GEN9_NUM_MOCS_ENTRIES;
> > +
> >   	write_registers(fd, ctx_id, get_engine_base(engine),
> > -			write_values, ARRAY_SIZE(write_values),
> > +			write_values, num_of_mocs_entries,
> >   			engine);
> >   	if (engine == I915_EXEC_RENDER)
> >   		write_registers(fd, ctx_id, GEN9_LNCFCMOCS0,
> > -				write_values, ARRAY_SIZE(write_values),
> > +				write_values, num_of_mocs_entries/2,
> >   				engine);
> >   }
> The "-dirty" subcase of the test doesn't seem suitable to current hardware..
> 
> The table is now global. So writing it from one context and checking if
> another context is unaffected is bound to fail.
> 
> Shouldn't the test for gen11+ be restricted to reading the values and
> checking whether they match pre-defined array?
> (we may still do a test if everything is fine after reset/suspend, that
> won't hurt)
> And if we are writing dirty values, shouldn't there be a restoration to
> original values afterwards?
> 
> -Tomasz

I am observing following behaviour on Icelake platform

 "-dirty" subcase of the test fails in checking l3 control register.

  Tomasz mentioned that the table is now golbal. However, for control
  registers(LLC/eDRAM), the context is getting saved and restored
  between contexts like the Gen9. If the table was global this shouldn't
  be the expected behaviour. Also i don't see a reference in bspec which
  stated mocs table is global for GEN11+.

  Now for L3 control registers the behaviour is different. This is where
  test is failing in "-dirty" subcase. For L3 control registers, writing
  dirty values are somehow ignored by the hardware. So checking the L3
  Control registers after writing the dirty values fails.


-Prathap
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev