[Intel-gfx] [PATCH] drm/i915/gt: Add separate MOCS table for Gen12 devices other than TGL/RKL

[Matt Roper]

+Cc a couple more people.

On Thu, Sep 09, 2021 at 10:15:57AM -0700, Matt Roper wrote:
> On Thu, Sep 09, 2021 at 06:09:55PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 09, 2021 at 08:00:02AM -0700, Matt Roper wrote:
> > > On Thu, Sep 09, 2021 at 05:39:26PM +0300, Ville Syrjälä wrote:
> > > > On Thu, Sep 09, 2021 at 07:29:33AM -0700, Matt Roper wrote:
> > > > > 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 at intel.com>
> > > > > > > > > > Signed-off-by: Ayaz A Siddiqui <ayaz.siddiqui at intel.com>
> > > > > > > > > 
> > > > > > > > > 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 61 is meant to to be the new PTE entry wy hasn't it been defines as
> > > > such in the code? And I know for a fact that userspace (Mesa) is not
> > > 
> > > There is no "PTE" entry anymore.  But 61 is already documented as
> > > "displayable" in both the spec and the code:
> > > 
> > >         /* HW Special Case (Displayable) */                                      
> > >         MOCS_ENTRY(61,                             
> > 
> > Why is it called a "HW special case"? I don't think there's any hw
> > magic in there?
> > 
> > And why aren't we setting it to PTE to get some cacheability for
> > window back buffers and such?
> 
> Who is "we" here?  The MOCS table is a pre-defined set of per-platform
> magic numbers.  The software teams don't get to decide what the values
> are, we just program the hardware with the per-platform numbers that
> have been agreed upon as part of a platform-wide stack (everything from
> low-level firmware to high level userspace should be working from the
> same table, defined in the bspec).
> 
> Once we know what the per-platform magic numbers are, we're supposed to
> pick the table entry that matches the behavior we're trying to
> accomplish.  If you want some specific level of cacheability, then you
> select a table row that gives you that.  Maybe 61 isn't the best
> setting, I don't know; userspace can pick whichever defined setting is
> actually best, using the data from the table.  But table row #1 is
> already well-documented as reserved/dontuse across the full stack; the
> fact that row #1 had values similar to PTE on Icelake hardware doesn't
> carry forward to any post-gen11 platform.
> 
> > 
> > > 
> > > > using entry 61. I think there is a massive communication gap here
> > > > where everyone just seems to assume the other side is doing something.
> > > > 
> > > > Could someone actually come up with a clear abi definition for this
> > > > and get all the stakeholders to sign off on it?
> > > 
> > > The agreement between the i915 team, various userspace teams, Windows
> > > driver team, hardware architects, software architects, and bspec writers
> > > was just completed; that's what triggered the kernel updates here (and
> > > I'm guessing is triggering similar work on the UMD side).  It's also why
> > > we held off on removing the force_probe flag on ADL until now since we
> > > couldn't consider enablement of the platform complete until the
> > > agreement and definitions here was finalized.
> > 
> > Can we get that agreement visible on the mailing list? Since MOCS is
> > abi I don't see why we shouldn't follow the normal abi rules for these,
> > ie. post to dri-devel, get acks from relevant people, links to agreed
> > userspace changes, etc.
> 
> The ABI design here was designed and agreed upon years ago, during early
> gen11 development.  The ABI design is that the kernel driver will
> faithfully initialize the hardware with the pre-determined set of magic
> numbers documented by the hardware team.  Since these are
> well-documented and unchanging numbers per-platform, there's no
> ambiguity for userspace, firmware, etc. about what a specific mocs index
> means, and no need to provide additional ABI for userspace to query what
> the kernel used in each row or anything like that.  The specific magic
> numbers are also ABI in the sense that we can't change the set of
> defined values once they're set for a platform (and it's been a long
> road to get the hardware and other OS software teams to understand and
> agree to this requirement), but we don't get to define or overrule what
> the initial values and order of those magic numbers are.
> 
> What is a bit vague in the formal documentation is what should be done
> about the reserved/dontuse table entries.  In theory it wouldn't matter
> since they'd never be used anyway, but in reality userspace can still
> use them by accident, such as by forgetting to update their MOCS
> selection logic from past platforms (e.g., still trying to use row #1 on
> platforms where it isn't defined).  Given that it's legal for entries to
> be added to MOCS tables, but never removed/modified, it follows that we
> should always initialize the undefined entries to fully cached; if a
> MOCS table update happens in the future and new rows show up, they can
> only become more coherent, and any userspace software that was
> incorrectly trying to use them previously will remain functionally
> correct.
> 
> What you're proposing would be a change to existing ABI --- instead of
> following the agreed upon contract, i915 would start defining its own
> set of magic numbers that potentially contradict the documentation that
> every other team is depending on.  We already made this mistake on
> TGL/RKL, so due to an i915 bug we're outside the spec; if entry #1 ever
> becomes a formally defined setting in the future, the rest of the
> software stack will need to explicitly work around i915's bug since we
> can't fix it now without breaking ABI.
> 
> If you really want to redefine how the MOCS ABI works and have i915 no
> longer follow the current contract, I think you need to do the
> following:
> 
>  * Re-add force_probe requirement to ADL and add appropriate Fixes: so
>    that the platform isn't enabled yet.
>  * Document clearly how you want i915 to select the MOCS settings it
>    uses for each table row if we're no longer going to follow the
>    documented values in the bspec.
>  * Provide a way for userspace to determine how i915 has defined the
>    MOCS settings (since they can no longer rely on us following the
>    previously agreed upon contract).
>  * Get acks from all the userspace teams on the new direction you're
>    proposing.
>  * Get an ack from the GuC team to make sure that programming
>    MOCS values differently than documented in the bspec won't have any
>    kind of impact on their operation.
> 
> Alternatively, you could lobby for a new table row #1 to be added to the
> formal MOCS table for gen12.  It's legal for new MOCS entries to show up
> in the future, so if that happens, we can follow up with the
> corresponding change in i915; since we'll be moving to a more coherent
> value (from today's fully cached entry), we'll be becoming more
> permissive from a correctness point of view.  But we absolutely should
> not try to add entries unilaterally in i915 that haven't been formally
> agreed upon because they may clash with a different definition of the
> row that shows up in the future through formal channels.
> 
> 
> 
> Matt
> 
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795