On Thu, Sep 09, 2021 at 04:58:50PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 07, 2021 at 11:19:29AM -0700, Matt Roper wrote:
> > On Tue, Sep 07, 2021 at 08:41:06PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 07, 2021 at 10:27:28AM -0700, Matt Roper wrote:
> > > > On Tue, Sep 07, 2021 at 10:46:39PM +0530, Ayaz A Siddiqui wrote:
> > > > > MOCS table of TGL/RKL has MOCS[1] set to L3_UC.
> > > > > While for other gen12 devices we need to set MOCS[1] as L3_WB,
> > > > > So adding a new MOCS table for other gen 12 devices eg. ADL.
> > > > >
> > > > > Fixes: cfbe5291a189 ("drm/i915/gt: Initialize unused MOCS entries with device specific values")
> > > > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > > > Signed-off-by: Ayaz A Siddiqui <ayaz.siddiqui@xxxxxxxxx>
> > > >
> > > > Yep, we overlooked that the TGL table still had an explicit entry for
> > > > I915_MOCS_PTE and wasn't just using an implicit 'unused_entries' lookup
> > > > for MOCS[1]. The new table is the same as the TGL table, just with
> > > > I915_MOCS_PTE (1) removed.
> > >
> > > And just how are people planning on handling display cacheability
> > > control without a PTE MOCS entry? Is Mesa/etc. already making all
> > > external bos uncached on these platforms just in case we might
> > > scan out said bo?
> >
> > MOCS entry 1 has never been considered a valid MOCS table entry on gen12
> > platforms (despite the old #define, it's not actually related to PTE,
> > display, etc. anymore).
>
> So can someone finally explain to me how we're supposed to cache
> anything that might become a scanout buffer later (eg. window system
> buffers)? Or are we just making everything like that UC now, and is
> everyone happy with that? Is userspace actually following that?
Table entry #1 has never had anything to do with scanout on gen12+. I
would assume that UMDs are either using the display entry in the MOCS
table (which is 61 on gen12+) or some other UC entry. If they weren't
properly updated and are still trying to use gen11 MOCS tables, then
they'll continue to work on TGL/RKL (we're not making any changes
there), but they'll just never run properly on the upcoming platforms
like ADL until the userspace enablement of the platform is completed.
>
> We can't just randomly change this stuff in the kernel since it's an
> abi. Userspace has to be in sync.
That's exactly why we're making this change. We need to get this right
_before_ it becomes established as ABI on upcoming platforms. We missed
the boat for TGL/RKL, which is why we're stuck with the "wrong" value in
invalid table entry #1 forever. But the platforms that haven't
established a behavior as pre-existing ABI yet need to be handled
correctly right from the beginning. ABI means we can't change the
behavior of a specific platform between kernel releases; it doesn't mean
that new platform B has to reproduce all the mistakes of old platform A
too. If buggy userspace happened to work by accident on TGL/RKL, that
doesn't mean that it has to accidentally work on ADL hardware too.
Matt
>
> > Userspace shouldn't be using entry 1, since its
> > settings are officially undefined, but because we had already defined a
> > value for it in the driver on our production gen12 platforms (TGL/RKL),
> > we can't change that value now without an ABI break; we just have to
> > leave it be on TGL/RKL.
> >
> > Any userspace trying to use entry 1 on gen12 (or any other undocumented
> > table entry) is still buggy and should still be updated. For the gen12
> > platforms going forward (ADL-S, ADL-P, and anything else that shows up
> > in the future) we can just follow the guidance of setting all
> > invalid/unused entries as L3 cached from day 1.
> >
> >
> > Matt
> >
> > >
> > > >
> > > > Looks good to me,
> > > >
> > > > Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > >
> > > >
> > > > > ---
> > > > > drivers/gpu/drm/i915/gt/intel_mocs.c | 41 +++++++++++++++++++++++++---
> > > > > 1 file changed, 37 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c
> > > > > index e96afd7beb49..c8d289b00de4 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> > > > > @@ -315,6 +315,35 @@ static const struct drm_i915_mocs_entry dg1_mocs_table[] = {
> > > > > MOCS_ENTRY(63, 0, L3_1_UC),
> > > > > };
> > > > >
> > > > > +static const struct drm_i915_mocs_entry gen12_mocs_table[] = {
> > > > > +
> > > > > + GEN11_MOCS_ENTRIES,
> > > > > + /* Implicitly enable L1 - HDC:L1 + L3 + LLC */
> > > > > + MOCS_ENTRY(48,
> > > > > + LE_3_WB | LE_TC_1_LLC | LE_LRUM(3),
> > > > > + L3_3_WB),
> > > > > + /* Implicitly enable L1 - HDC:L1 + L3 */
> > > > > + MOCS_ENTRY(49,
> > > > > + LE_1_UC | LE_TC_1_LLC,
> > > > > + L3_3_WB),
> > > > > + /* Implicitly enable L1 - HDC:L1 + LLC */
> > > > > + MOCS_ENTRY(50,
> > > > > + LE_3_WB | LE_TC_1_LLC | LE_LRUM(3),
> > > > > + L3_1_UC),
> > > > > + /* Implicitly enable L1 - HDC:L1 */
> > > > > + MOCS_ENTRY(51,
> > > > > + LE_1_UC | LE_TC_1_LLC,
> > > > > + L3_1_UC),
> > > > > + /* HW Special Case (CCS) */
> > > > > + MOCS_ENTRY(60,
> > > > > + LE_3_WB | LE_TC_1_LLC | LE_LRUM(3),
> > > > > + L3_1_UC),
> > > > > + /* HW Special Case (Displayable) */
> > > > > + MOCS_ENTRY(61,
> > > > > + LE_1_UC | LE_TC_1_LLC,
> > > > > + L3_3_WB),
> > > > > +};
> > > > > +
> > > > > enum {
> > > > > HAS_GLOBAL_MOCS = BIT(0),
> > > > > HAS_ENGINE_MOCS = BIT(1),
> > > > > @@ -351,14 +380,18 @@ static unsigned int get_mocs_settings(const struct drm_i915_private *i915,
> > > > > table->n_entries = GEN9_NUM_MOCS_ENTRIES;
> > > > > table->uc_index = 1;
> > > > > table->unused_entries_index = 5;
> > > > > - } else if (GRAPHICS_VER(i915) >= 12) {
> > > > > + } else if (IS_TIGERLAKE(i915) || IS_ROCKETLAKE(i915)) {
> > > > > + /* For TGL/RKL, Can't be changed now for ABI reasons */
> > > > > table->size = ARRAY_SIZE(tgl_mocs_table);
> > > > > table->table = tgl_mocs_table;
> > > > > table->n_entries = GEN9_NUM_MOCS_ENTRIES;
> > > > > table->uc_index = 3;
> > > > > - /* For TGL/RKL, Can't be changed now for ABI reasons */
> > > > > - if (!IS_TIGERLAKE(i915) && !IS_ROCKETLAKE(i915))
> > > > > - table->unused_entries_index = 2;
> > > > > + } else if (GRAPHICS_VER(i915) >= 12) {
> > > > > + table->size = ARRAY_SIZE(gen12_mocs_table);
> > > > > + table->table = gen12_mocs_table;
> > > > > + table->n_entries = GEN9_NUM_MOCS_ENTRIES;
> > > > > + table->uc_index = 3;
> > > > > + table->unused_entries_index = 2;
> > > > > } else if (GRAPHICS_VER(i915) == 11) {
> > > > > table->size = ARRAY_SIZE(icl_mocs_table);
> > > > > table->table = icl_mocs_table;
> > > > > --
> > > > > 2.26.2
> > > > >
> > > >
> > > > --
> > > > Matt Roper
> > > > Graphics Software Engineer
> > > > VTT-OSGC Platform Enablement
> > > > Intel Corporation
> > > > (916) 356-2795
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
> > (916) 356-2795
>
> --
> Ville Syrjälä
> Intel
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
