> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> Sent: Wednesday, October 25, 2023 11:41 PM
> To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3] drm/i915/mtl: Add Wa_14019821291
>
> On Wed, Oct 25, 2023 at 04:06:46PM +0530, Dnyaneshwar Bhadane wrote:
> > This workaround is primarily implemented by the BIOS. However if the
> > BIOS applies the workaround it will reserve a small piece of our DSM
> > (which should be at the top, right below the WOPCM); we just need to
> > keep that region reserved so that nothing else attempts to re-use it.
> >
> > v2: Declare regs in intel_gt_regs.h (Matt Roper)
> >
> > v3: Shift WA implementation before calculation of *base (Matt Roper)
> >
> > Signed-off-by: Dnyaneshwar Bhadane <dnyaneshwar.bhadane@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 17 +++++++++++++++++
> > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 3 +++
> > 2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > index 1a766d8e7cce..192c9a333c0a 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > @@ -404,6 +404,23 @@ static void icl_get_stolen_reserved(struct
> drm_i915_private *i915,
> > MISSING_CASE(reg_val &
> GEN8_STOLEN_RESERVED_SIZE_MASK);
> > }
> >
> > + /* Wa_14019821291 */
> > + if (MEDIA_VER_FULL(i915) == IP_VER(13, 0)) {
> > + /*
> > + * This workaround is primarily implemented by the BIOS. We
> > + * just need to figure out whether the BIOS has applied the
> > + * workaround (meaning the programmed address falls within
> > + * the DSM) and, if so, reserve that part of the DSM to
> > + * prevent accidental reuse. The DSM location should be just
> > + * below the WOPCM.
> > + */
> > + u64 gscpsmi_base = intel_uncore_read64_2x32(uncore,
> > +
> MTL_GSCPSMI_BASEADDR_LSB,
> > +
> MTL_GSCPSMI_BASEADDR_MSB);
>
> I overlooked it while reviewing the previous revisions, but I think we're mixing
> up which regions size/base refer to. Basically the layout of the overall DSM
> stolen memory area is:
>
> [[ usable DSM area ][ GSCPSMI WA area ][ WOPCM ]]
> ^ ^
> | |
> DSM base DSM end
>
> In i915 we have a resource tracking the DSM as a whole, and then also
> another resource tracking the "reserved" subregion of the DSM. Your patch is
> trying to incorporate the gscpsmi workaround area into the reserved
> subregion:
>
> [ usable DSM area ][[ GSCPSMI WA area ][ WOPCM ]]
> ^ ^
> | |
> reserved base reserved end
>
> So regarding the first condition here:
>
> > + if (gscpsmi_base >= *base && gscpsmi_base < *base + *size)
>
> I don't think this is checking the right thing. You want to see whether the
> gscpsmi base address falls somewhere within within the DSM as a whole,
> whereas the base/size you're comparing against above are the preliminary
> bounds for the reserved area. Assuming the gscpsmi address does fall
> somewhere within the DSM area, then we can pretty much assume that the
> BIOS set things up properly and the GSCPSMI workaround area is immediately
> before the WOPCM. I.e., the gscpsmi_base should become the new start of
> the reserved region.
>
> So what you really want is a condition like:
>
> if (gscpsmi_base >= i915->dsm.stolen.start &&
> gscpsmi_base < i915->dsm.stolen.end)
>
> to see if it falls somewhere within the entire DSM area. If it does, then
> everything from gscpsmi_base to the end of the DSM can be considered to be
> the reserved area, and we don't even need to look at the value in
> GEN6_STOLEN_RESERVED to find the WOPCM size.
>
> So maybe the best thing to do is move this condition to the very top of the
> function before we do anything else:
>
> if (gscpsmi_base >= i915->dsm.stolen.start &&
> gscpsmi_base < i915->dsm.stolen.end) {
> *base = gscpsmi_base;
> *size = i915->dsm.stolen.end - gscpsmi_base;
> return;
> }
>
> Then if the GSCPSMI workaround is not in effect we fall back to reading the
> WOPCM size from the register and use that to calculate the reserved region
> base.
>
> This is a bit different from how things work in my Xe patch because Xe isn't
> tracking the reserved region of the DSM, but rather the usable region, so the
> logic is somewhat the inverse of what this i915 function needs.
>
>
> Matt
Thank you, Matt, for the wonderful explanation.
I will address these changes in next(V4) revision.
Regards Dnyaneshwar
>
> > + *size = gscpsmi_base - *base;
> > + }
> > +
> > if (HAS_LMEMBAR_SMEM_STOLEN(i915))
> > /* the base is initialized to stolen top so subtract size to get
> base */
> > *base -= *size;
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > index eecd0a87a647..9de41703fae5 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > @@ -537,6 +537,9 @@
> > #define XEHP_SQCM MCR_REG(0x8724)
> > #define EN_32B_ACCESS REG_BIT(30)
> >
> > +#define MTL_GSCPSMI_BASEADDR_LSB _MMIO(0x880c)
> > +#define MTL_GSCPSMI_BASEADDR_MSB _MMIO(0x8810)
> > +
> > #define HSW_IDICR _MMIO(0x9008)
> > #define IDIHASHMSK(x) (((x) & 0x3f) << 16)
> >
> > --
> > 2.34.1
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
