> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> Sent: Thursday, September 2, 2021 5:15 AM
> To: Siddiqui, Ayaz A <ayaz.siddiqui@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH V3 5/8] drm/i915/gt: Initialize unused MOCS
> entries with device specific values
>
> On Mon, Aug 30, 2021 at 09:52:37PM +0530, Ayaz A Siddiqui wrote:
> > Historically we've initialized all undefined/reserved entries in a
> > platform's MOCS table to the contents of table entry #1 (i.e.,
> > I915_MOCS_PTE).
> > Going forward, we can't assume that table entry #1 will always contain
> > suitable values to use for undefined/reserved table indices. We'll
> > allow a platform-specific table index to be selected at table
> > initialization time in these cases.
> >
> > This new mechanism to select L3 WB entry will be applicable for all
> > the Gen12+ platforms except TGL and RKL.
> >
> > Since TGL and RLK are already in production so their mocs settings are
> > intact to avoid ABI break.
> >
> > Signed-off-by: Ayaz A Siddiqui <ayaz.siddiqui@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/intel_mocs.c | 41
> > +++++++++++++++-------------
> > 1 file changed, 22 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c
> > b/drivers/gpu/drm/i915/gt/intel_mocs.c
> > index 82eafa8d22453..a97cc08e5a395 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> > @@ -25,6 +25,7 @@ struct drm_i915_mocs_table {
> > unsigned int n_entries;
> > const struct drm_i915_mocs_entry *table;
> > u8 uc_index;
> > + u8 unused_entries_index;
> > };
> >
> > enum register_type {
> > @@ -113,18 +114,25 @@ struct drm_i915_aux_table {
> > *
> > * Entries not part of the following tables are undefined as far as
> > * 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 and will be initialized to an invalid value.
> > + * they will be initialized to PTE. Gen >= 12 don't have a setting
> > + for
> > + * PTE and those platforms except TGL/RKL will be initialized L3 WB
> > + to
> > + * catch accidental use of reserved and unused mocs indexes.
> > *
> > * The last few 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
> > + * NOTE1: 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, for specific hardware platform, existing
> > * entries will remain constant and the table will only be updated by
> > * adding new entries, filling unused positions.
> > + *
> > + * NOTE2: For GEN >= 12 except TGL and RKL, reserved and unspecified
> MOCS
> > + * indices have been set to L3 WB. These reserved entries should never
> > + * be used, they may be changed to low performant variants with
> better
> > + * coherency in the future if more entries are needed.
> > + * For TGL/RKL, all the unspecified MOCS indexes are mapped to L3 UC.
> > */
> > #define GEN9_MOCS_ENTRIES \
> > MOCS_ENTRY(I915_MOCS_UNCACHED, \
> > @@ -307,17 +315,9 @@ static const struct drm_i915_mocs_entry
> > icl_mocs_table[] = { };
> >
> > static const struct drm_i915_mocs_entry dg1_mocs_table[] = {
> > - /* Error */
> > - MOCS_ENTRY(0, 0, L3_0_DIRECT),
> >
> > /* UC */
> > MOCS_ENTRY(1, 0, L3_1_UC),
> > -
> > - /* Reserved */
> > - MOCS_ENTRY(2, 0, L3_0_DIRECT),
> > - MOCS_ENTRY(3, 0, L3_0_DIRECT),
> > - MOCS_ENTRY(4, 0, L3_0_DIRECT),
> > -
> > /* WB - L3 */
> > MOCS_ENTRY(5, 0, L3_3_WB),
> > /* WB - L3 50% */
> > @@ -469,6 +469,7 @@ static unsigned int get_mocs_settings(const struct
> drm_i915_private *i915,
> > table->table = dg1_mocs_table;
> > table->n_entries = GEN9_NUM_MOCS_ENTRIES;
> > table->uc_index = 1;
> > + table->unused_entries_index = 5;
> > } else if (GRAPHICS_VER(i915) >= 12) {
> > table->size = ARRAY_SIZE(tgl_mocs_table);
> > table->table = tgl_mocs_table;
>
> Should we add
>
> table->unused_entries_index = 1;
>
> to the rest of the platforms here since that's what we're doing in
> get_entry_l3cc() anyway?
>
> Also I think we can rely on that to avoid the need to add a new MOCS table
> for ADL in the next patch; we can just use the same table but set a different
> unused_entries_index. E.g.,
>
> } else if (IS_TIGERLAKE(i915) || IS_ROCKETLAKE(i915)) {
> ...
> /* UC: Can't be changed now for ABI reasons */
> table->unused_entries_index = 1;
> } else if (GRAPHICS_VER(i915) >= 12) {
> ...
> /* L3 */
> table->unused_entries_index = 2;
> } else if (GRAPHICS_VER(i915) == 11) {
> ...
> table->unused_entries_index = 1;
> ...
>
} else if (GRAPHICS_VER(i915) >= 12) {
table->size = ARRAY_SIZE(tgl_mocs_table);
table->table = tgl_mocs_table;
table->n_entries = GEN9_NUM_MOCS_ENTRIES;
table->uc_index = 3;
if (!IS_TIGERLAKE(i915) || !IS_ROCKETLAKE(i915))
table->unused_entries_index = 2;
}
I 'm thinking to add like this so that there is no change for gen11/Gen9 and TGL/RKL flow.
>
> > @@ -600,16 +601,17 @@ int apply_mocs_aux_regs_ctx(struct
> i915_request
> > *rq) }
> >
> > /*
> > - * Get control_value from MOCS entry taking into account when it's not
> used:
> > - * I915_MOCS_PTE's value is returned in this case.
> > + * Get control_value from MOCS entry taking into account when it's
> > + not used
> > + * then if unused_entries_index is non-zero then its value will be
> > + returned
> > + * otherwise I915_MOCS_PTE's value is returned in this case.
> > */
> > static u32 get_entry_control(const struct drm_i915_mocs_table *table,
> > unsigned int index)
> > {
> > if (index < table->size && table->table[index].used)
> > return table->table[index].control_value;
> > -
> > - return table->table[I915_MOCS_PTE].control_value;
> > + index = table->unused_entries_index ? : I915_MOCS_PTE;
>
> As noted above, I'd rather see us do this right in the MOCS settings setup.
> Then we'd probably just want a
> drm_WARN_ON(!table->unused_entries_index) to make sure we don't
> forget to explicitly set it for any platforms.
If we have to add a warn on for table->unused_entries_index,
The we should add unused_entries_index when we should add this for all table
and change code like this
- return table->table[I915_MOCS_PTE].control_value;
+ return table->table[table->unused_entries_index].control_value;
What ever you suggest I'll take care
-Ayaz
> > }
> >
> > #define for_each_mocs(mocs, t, i) \
> > @@ -650,16 +652,17 @@ static void init_mocs_table(struct
> > intel_engine_cs *engine, }
> >
> > /*
> > - * Get l3cc_value from MOCS entry taking into account when it's not used:
> > - * I915_MOCS_PTE's value is returned in this case.
> > + * Get l3cc_value from MOCS entry taking into account when it's not
> > + used
> > + * then if unused_entries_index is not zero then its value will be
> > + returned
> > + * otherwise I915_MOCS_PTE's value is returned in this case.
> > */
> > static u16 get_entry_l3cc(const struct drm_i915_mocs_table *table,
> > unsigned int index)
> > {
> > if (index < table->size && table->table[index].used)
> > return table->table[index].l3cc_value;
> > -
> > - return table->table[I915_MOCS_PTE].l3cc_value;
> > + index = table->unused_entries_index ? : I915_MOCS_PTE;
>
> Ditto.
>
>
>
> Matt
>
> > + return table->table[index].l3cc_value;
> > }
> >
> > static u32 l3cc_combine(u16 low, u16 high)
> > --
> > 2.26.2
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
