| Message ID | 20231025103646.3315818-1-dnyaneshwar.bhadane@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/i915/mtl: Add Wa_14019821291 | expand |
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@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 >
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Wednesday, October 25, 2023 11:41 PM > To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com> > Cc: intel-gfx@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@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
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); + if (gscpsmi_base >= *base && gscpsmi_base < *base + *size) + *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)
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@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(+)