[Intel-gfx] [PATCH v3] drm/i915/mtl: Add Wa_14019821291

[Bhadane, Dnyaneshwar]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Wednesday, October 25, 2023 11:41 PM
> To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [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 at intel.com>
> > ---
> >  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