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

[Joonas Lahtinen]

Quoting Tvrtko Ursulin (2018-12-10 17:17:29)
> 
> On 07/11/2018 15:16, Tomasz Lis wrote:
> > The table has been unified across OSes to minimize virtualization overhead.
> > 
> > The MOCS table is now published as part of bspec, and versioned. Entries
> > are supposed to never be modified, but new ones can be added. Adding
> > entries increases table version. The patch includes version 1 entries.
> > 
> > Meaning of each entry is now explained in bspec, and user mode clients
> > are expected to know what each entry means. The 3 entries used for previous
> > platforms are still compatible with their legacy definitions, but that is
> > not guaranteed to be true for future platforms.
> > 
> > v2: Fixed SCC values, improved commit comment (Daniele)
> > v3: Improved MOCS table comment (Daniele)
> > v4: Moved new entries below gen9 ones. Put common entries into
> >      definition to be used in multiple arrays. (Lucas)
> > v5: Made defines for or-ing flags. Renamed macros from MOCS_TABLE
> >      to MOCS_ENTRIES. Switched LE_CoS to upper case. (Joonas)
> > v6: Removed definitions of reserved entries. (Michal)
> >      Increased limit of entries sent to the hardware on gen11+.
> > 
> > BSpec: 34007
> > BSpec: 560
> > Signed-off-by: Tomasz Lis <tomasz.lis at intel.com>
> > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com> (v4)
> 
> R-b upgrade needed here as well.
> 
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > Cc: Zhenyu Wang <zhenyuw at linux.intel.com>
> > Cc: Zhi A Wang <zhi.a.wang at intel.com>
> > Cc: Anuj Phogat <anuj.phogat at intel.com>
> > Cc: Adam Cetnerowski <adam.cetnerowski at intel.com>
> > Cc: Piotr Rozenfeld <piotr.rozenfeld at intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_mocs.c | 222 +++++++++++++++++++++++++++++++++-----
> >   1 file changed, 197 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> > index 8d08a7b..4eb05c6 100644
> > --- a/drivers/gpu/drm/i915/intel_mocs.c
> > +++ 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)
> > +#define LE_SSE(value)                ((value) << 17)
> >   
> >   /* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
> >   #define L3_ESC(value)               ((value) << 0)
> > @@ -52,6 +54,10 @@ struct drm_i915_mocs_table {
> >   
> >   /* Helper defines */
> >   #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. */
> > +
> > +#define NUM_MOCS_ENTRIES(i915) \
> > +     (INTEL_GEN(i915) < 11 ? GEN9_NUM_MOCS_ENTRIES : GEN11_NUM_MOCS_ENTRIES)
> 
> I have a suggestion to make this a bit more elegant by avoiding a 
> sprinkle of conditionals throughout the code - since I count ten 
> non-debug call sites of this macros.
> 
> Since the MOCS code seems nicely driven from struct drm_i915_mocs_table, 
> the suggestion is to store both the used and maximum valid number of 
> entries per platform in that structure.
> 
> It's all nicely consolidated in get_mocs_settings, where all the sanity 
> checks could be moved as well. Other bits of the code would then just 
> trust the table.
> 
> >   
> >   /* (e)LLC caching options */
> >   #define LE_PAGETABLE                0
> > @@ -80,21 +86,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.
> > + *
> > + * 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 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.
> > + * 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 MOCS_CONTROL_VALUE(lecc, tc, lrum, daom, ersc, scc, pfm, scf) \
> > @@ -147,6 +153,167 @@ static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
> >   #undef MOCS_CONTROL_VALUE
> >   #undef MOCS_L3CC_VALUE
> >   
> > +#define MOCS_CONTROL_VALUE(lecc, tc, lrum, daom, ersc, scc, pfm, scf, cos, sse) \
> > +     (LE_CACHEABILITY(lecc) | LE_TGT_CACHE(tc) | \
> > +     LE_LRUM(lrum) | LE_AOM(daom) | LE_RSC(ersc) | LE_SCC(scc) | \
> > +     LE_PFM(pfm) | LE_SCF(scf) | LE_COS(cos) | LE_SSE(sse))
> > +
> > +#define MOCS_L3CC_VALUE(esc, scc, l3cc) \
> > +     (L3_ESC(esc) | L3_SCC(scc) | L3_CACHEABILITY(l3cc))
> > +
> > +#define GEN11_MOCS_ENTRIES \
> > +     [0] = { \
> > +       /* Base - Uncached (Deprecated) */ \
> > +       .control_value = MOCS_CONTROL_VALUE(LE_UC, LE_TC_LLC, \
> > +                                           0, 0, 0, 0, 0, 0, 0, 0), \
> > +       .l3cc_value = MOCS_L3CC_VALUE(0, 0, L3_UC), \

These indents don't follow the guidelines. Make sure checkpatch
doesn't complain.

Regards, Joonas