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


Matt

> 
> Lucas De Marchi

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