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

Chris Wilson
2019-02-22T21:48:31Z

Quoting Kumar Valsan, Prathap (2019-02-22 21:32:55)
> On Fri, Feb 22, 2019 at 03:21:18PM +0000, Chris Wilson wrote:
> > Quoting Lis, Tomasz (2019-02-22 15:17:34)
> > >
> > >
> > > 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.
> As you pointed out On Gen11, "-dirty" subcases are failing.
> Test is not able to write to registers.
> >
> > We shouldn't be able to do nonpriv writes to it then. Hopefully those
> > writes are already being dropped?
> So probably on latest hardware, test should still try to write and call
> it pass if the dirty values are not being written?

Yes, that would be useful confirmation that one context can't shoot
another in the back, and that our global MOCS table remain invariant.
-Chris