Re: [Intel-gfx] [PATCH 03/11] drm/i915/pvc: Define MOCS table for PVC

[Matt Roper <matthew.d.roper@intel.com>]

On Mon, May 02, 2022 at 12:27:29PM -0700, Lucas De Marchi wrote:
> On Mon, May 02, 2022 at 11:50:22AM -0700, Matt Roper wrote:
> > On Mon, May 02, 2022 at 11:39:48AM -0700, Lucas De Marchi wrote:
> > > On Mon, May 02, 2022 at 09:50:23AM -0700, Matt Roper wrote:
> > > > On Mon, May 02, 2022 at 09:34:09AM -0700, Matt Roper wrote:
> > > > > From: Ayaz A Siddiqui <ayaz.siddiqui@intel.com>
> > > > >
> > > > > Bspec: 45101, 72161
> > > > > Signed-off-by: Ayaz A Siddiqui <ayaz.siddiqui@intel.com>
> > > > > Signed-off-by: Fei Yang <fei.yang@intel.com>
> > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/gt/intel_gt_types.h    |  1 +
> > > > >  drivers/gpu/drm/i915/gt/intel_mocs.c        | 24 ++++++++++++++++++++-
> > > > >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 13 ++++++++---
> > > > >  drivers/gpu/drm/i915/i915_drv.h             |  2 ++
> > > > >  drivers/gpu/drm/i915/i915_pci.c             |  3 ++-
> > > > >  drivers/gpu/drm/i915/intel_device_info.h    |  1 +
> > > > >  6 files changed, 39 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > > index b06611c1d4ad..7853ea194ea6 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > > @@ -221,6 +221,7 @@ struct intel_gt {
> > > > >
> > > > >  	struct {
> > > > >  		u8 uc_index;
> > > > > +		u8 wb_index; /* Only for platforms listed in Bspec: 72161 */
> > > > >  	} mocs;
> > > > >
> > > > >  	struct intel_pxp pxp;
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c
> > > > > index c4c37585ae8c..265812589f87 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> > > > > @@ -23,6 +23,7 @@ struct drm_i915_mocs_table {
> > > > >  	unsigned int n_entries;
> > > > >  	const struct drm_i915_mocs_entry *table;
> > > > >  	u8 uc_index;
> > > > > +	u8 wb_index; /* Only for platforms listed in Bspec: 72161 */
> > > > >  	u8 unused_entries_index;
> > > > >  };
> > > > >
> > > > > @@ -47,6 +48,7 @@ struct drm_i915_mocs_table {
> > > > >
> > > > >  /* Helper defines */
> > > > >  #define GEN9_NUM_MOCS_ENTRIES	64  /* 63-64 are reserved, but configured. */
> > > > > +#define PVC_NUM_MOCS_ENTRIES	3
> > > >
> > > > Should this be 4?  The value here should reflect the number of entries
> > > > that can defined in hardware rather than the size of the table we're
> > > > asked to program.  Since there are two registers (each with a high and a
> > > > low entry), that would imply we should set 4 here to ensure that the
> > > > fourth entry is initialized according to unused_entries_index rather
> > > > than left at whatever the hardware defaults might be.
> > > 
> > > not sure I understand what you mean here. The n_entries specifies, as
> > > you said, the number of entries we can have. Bspec 45101 shows entries
> > > for indexes 0, 1 and 2. As does the pvc_mocs_table below.
> > > 
> > > Also, from bspec 44509:
> > > "For PVC, only 3 MOCS states are supported. The allowed index values are
> > > in range [0, 2]..."
> > > 
> > > So, I don't think we want to program any fourth entry.
> > 
> > We don't have a choice; the fourth entry lives in the same register as
> > the third entry, so no matter what we're writing _something_ to those
> > bits.  The question is whether we should write all 0's or whether we
> > should treat it like other platforms and ensure it's initialized to the
> > unused entry values.  Entry #4 isn't supposed to be used, but if buggy
> > userspace tries to use it, we probably still want well-defined behavior,
> > just like it an invalid entry gets used on any other platform.
> 
> Now I understand what you were talking about:  each register houses 2
> entries. For PVC we have LNCFCMOCS0 and LNCFCMOCS1. Humn... looking at
> for_each_l3cc(), that is actually handled and the rest of the register
> is initialized with the value pointed by unused_entries_index.

Yep, you're right.  It looks like we still do a get_entry_l3cc() for the
upper entry of the final register, and that will return the unused_entry
value if it's out of bounds.  In that case I don't have any concerns
here.


Matt

> 
> Such situation would only happen for the last entry, which implies the
> handling for odd size works for this as well.
> 
> Lucas De Marchi
> 
> > 
> > 
> > Matt
> > 
> > > 
> > > Lucas De Marchi
> > 
> > -- 
> > 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