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@xxxxxxxxx>
> > > > >
> > > > > Bspec: 45101, 72161
> > > > > Signed-off-by: Ayaz A Siddiqui <ayaz.siddiqui@xxxxxxxxx>
> > > > > Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx>
> > > > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > > > ---
> > > > > 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
