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

Thu Dec 13 19:45:59 UTC 2018

On Thu, Dec 13, 2018 at 5:46 AM Joonas Lahtinen
<joonas.lahtinen at linux.intel.com> wrote:
>
> 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.

yes, it passes.

$ dim checkpatch  drm-tip/drm-tip..HEAD  drm-intel
e02fab02e635 drm/i915/skl: Rework MOCS tables to keep common part in a define
261119c69063 drm/i915/icl: Define MOCS table for Icelake

This being inside a macro is probably what makes checkpatch to ignore
the indentation.

Lucas De Marchi