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

[Matt Roper]

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

> +			*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