On Fri, Oct 20, 2023 at 02:29:09PM +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 gt/intel_gt_regs.h file
>
> Signed-off-by: Dnyaneshwar Bhadane
> ---
> 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..bb2639d1a824 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> @@ -409,6 +409,23 @@ static void icl_get_stolen_reserved(struct
> drm_i915_private *i915,
> *base -= *size;
> else
> *base = reg_val & GEN11_STOLEN_RESERVED_ADDR_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);
> + if (gscpsmi_base >= *base && gscpsmi_base < *base + *size)
> + *size = gscpsmi_base - *base;
> + }
Right now it looks like you re-calculate the size of the reserved region
to include the gscpsmi workaround space, but you don't update *base,
which is reserved_base in the caller. That will cause the
i915->dsm.reserved resource to get initialized with the old base but the
new size (i.e., it will effectively grow in the wrong direction if you
don't change the base too).
I think the simplest thing to do is just move this workaround above the
if/else that comes right before it. Since the affected platforms here
always take the 'if' branch, that will ensure that *base gets adjusted
downward to account for the larger *size value.
Matt
> }
>
> /*
> 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_SQCMMCR_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
