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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@intel.com>
> > > > > > > > Signed-off-by: Ayaz A Siddiqui <ayaz.siddiqui@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?

> 
> > 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.

-- 
Ville Syrjälä
Intel