Re: [Intel-gfx] [PATCH] drm/i915: Introduce Wa_14011274333
On Wed, Jul 12, 2023 at 04:34:15PM -0700, Matt Atwood wrote:
> Wa_14011274333 applies to RKL, ADL-S, ADL-P and TGL. ALlocate buffer
> pinned to GGTT and add WA to restore impacted registers.
You should explain the approach you're taking to implement this
workaround since someone reading the workaround description isn't really
going to understand how your code here addresses it.
>
> v2: use correct lineage number, more generically apply workarounds for
> all registers impacted, move workaround to gt/intel_workarounds.c
> (MattR)
>
> Based off patch by Tilak Tangudu.
>
> Signed-off-by: Matt Atwood
> ---
> drivers/gpu/drm/i915/gt/intel_gt_regs.h | 5 ++
> drivers/gpu/drm/i915/gt/intel_rc6.c | 63 +
> drivers/gpu/drm/i915/gt/intel_rc6_types.h | 3 +
> drivers/gpu/drm/i915/gt/intel_workarounds.c | 40 +
> 4 files changed, 111 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> index 718cb2c80f79e..eaee35ecbc8d3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> @@ -83,6 +83,11 @@
> #define MTL_MCR_GROUPIDREG_GENMASK(11, 8)
> #define MTL_MCR_INSTANCEID REG_GENMASK(3, 0)
>
> +#define CTX_WA_PTR _MMIO(0x2058)
CTX_WA_PTR is a register that exists for every engine, not just the RCS
engine. This should be a parameterized definition in the engine reg
file, not here.
> +#define CTX_WA_PTR_ADDR_MASKREG_GENMASK(31,12)
> +#define CTX_WA_TYPE_MASKREG_GENMASK(4,3)
> +#define CTX_WA_VALIDREG_BIT(0)
> +
> #define IPEIR_I965 _MMIO(0x2064)
> #define IPEHR_I965 _MMIO(0x2068)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c
> b/drivers/gpu/drm/i915/gt/intel_rc6.c
> index 58bb1c55294c9..6baa341814da7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
> @@ -14,6 +14,7 @@
> #include "intel_gt.h"
> #include "intel_gt_pm.h"
> #include "intel_gt_regs.h"
> +#include "intel_gpu_commands.h"
> #include "intel_pcode.h"
> #include "intel_rc6.h"
>
> @@ -53,6 +54,65 @@ static struct drm_i915_private *rc6_to_i915(struct
> intel_rc6 *rc)
> return rc6_to_gt(rc)->i915;
> }
>
> +static int rc6_wa_bb_ctx_init(struct intel_rc6 *rc6)
> +{
> + struct drm_i915_private *i915 = rc6_to_i915(rc6);
> + struct intel_gt *gt = rc6_to_gt(rc6);
> + struct drm_i915_gem_object *obj;
> + struct i915_vma *vma;
> + void *batch;
> + struct i915_gem_ww_ctx ww;
> + int err;
> +
> + obj = i915_gem_object_create_shmem(i915, PAGE_SIZE);
Should we be using i915_gem_object_create_internal() here? This is
something that's permanently pinned and we don't really need a swappable
storage, right?
> + if (IS_ERR(obj))
> + return PTR_ERR(obj);
> +
> + vma = i915_vma_instance(obj, >->ggtt->vm, NULL);
> + if (IS_ERR(vma)){
> + err = PTR_ERR(vma);
> + goto err;
> + }
> + rc6->vma=vma;
Coding style isn't correct on this line.
> + i915_gem_ww_ctx_init(&ww, true);
> +retry:
> + err = i915_gem_object_lock(obj, &ww);
> + if (!err)
> + err = i915_ggtt_pin(rc6->vma, &ww, 0, PIN_HIGH);
> + if (err)
> + goto err_ww_fini;
> +
> + batch = i915_gem_object_pin_map(obj, I915_MAP_WB);
> + if (IS_ERR(batch)) {
> + err = PTR_ERR(batch);
> + goto err_unpin;
> + }
> + rc6->rc6_wa_bb = batch;
> + return 0;
> +err_unpin:
> + if (err)
Isn't this redundant?
> + i915_vma_unpin(rc6->vma);
> +err_ww_fini:
> + if (err == -EDEADLK) {
> + err = i915_gem_ww_ctx_backoff(&ww);
> + if (!err)
> + goto retry;
> + }
> + i915_gem_ww_ctx_fini(&ww);
> +
> + if (err)
And isn't this redundant too?
> + i915_vma_put(rc6->vma);
> +err:
> + i915_gem_object_put(obj);
> + return err;
> +}
> +
> +void rc6_wa_bb_ctx_wa_fini(struct intel_rc6 *rc6)
> +{
> + i915_vma_unpin(rc6->vma);
> + i915_vma_put(rc6->vma);
Don't we need to put the object here too? Actually, can we use
i915_vma_unpin_and_release here?
Also, is this function ever called anywhere?
> +}
> +
> static void gen11_rc6_enable(struct intel_rc6 *rc6)
> {
> struct intel_gt *gt = rc6_to_gt(rc6);
> @@ -616,6 +676,9 @@ void intel_rc6_init(struct intel_rc6 *rc6)
> err = chv_rc6_init(rc6);
> else if (IS_VALLEYVIEW(i915))
> err = vlv_rc6_init(rc6);
> + else if ((GRAPHICS_VER_FULL(i915) >= IP_VER(12, 0)) &&
> + (GRAPHICS_VER_FULL(i915) <= IP_VER(12, 70)))
This doesn't match the platforms that the commit message said it should
apply to.
Also, if we have complicated version checks for a workaround tha
Re: [Intel-gfx] [PATCH] drm/i915: Introduce Wa_14011274333
On 13/07/2023 02:34, Matt Atwood wrote:
Wa_14011274333 applies to RKL, ADL-S, ADL-P and TGL. ALlocate buffer
pinned to GGTT and add WA to restore impacted registers.
v2: use correct lineage number, more generically apply workarounds for
all registers impacted, move workaround to gt/intel_workarounds.c
(MattR)
Based off patch by Tilak Tangudu.
Signed-off-by: Matt Atwood
I applied this patch to drm-tip and as far as I can tell it doesn't fix
the problem of the SAMPLER_MODE register loosing its bit0 programming.
-Lionel
---
drivers/gpu/drm/i915/gt/intel_gt_regs.h | 5 ++
drivers/gpu/drm/i915/gt/intel_rc6.c | 63 +
drivers/gpu/drm/i915/gt/intel_rc6_types.h | 3 +
drivers/gpu/drm/i915/gt/intel_workarounds.c | 40 +
4 files changed, 111 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
index 718cb2c80f79e..eaee35ecbc8d3 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
@@ -83,6 +83,11 @@
#define MTL_MCR_GROUPID REG_GENMASK(11, 8)
#define MTL_MCR_INSTANCEID REG_GENMASK(3, 0)
+#define CTX_WA_PTR_MMIO(0x2058)
+#define CTX_WA_PTR_ADDR_MASK REG_GENMASK(31,12)
+#define CTX_WA_TYPE_MASK REG_GENMASK(4,3)
+#define CTX_WA_VALID REG_BIT(0)
+
#define IPEIR_I965_MMIO(0x2064)
#define IPEHR_I965_MMIO(0x2068)
diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c
index 58bb1c55294c9..6baa341814da7 100644
--- a/drivers/gpu/drm/i915/gt/intel_rc6.c
+++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
@@ -14,6 +14,7 @@
#include "intel_gt.h"
#include "intel_gt_pm.h"
#include "intel_gt_regs.h"
+#include "intel_gpu_commands.h"
#include "intel_pcode.h"
#include "intel_rc6.h"
@@ -53,6 +54,65 @@ static struct drm_i915_private *rc6_to_i915(struct intel_rc6 *rc)
return rc6_to_gt(rc)->i915;
}
+static int rc6_wa_bb_ctx_init(struct intel_rc6 *rc6)
+{
+ struct drm_i915_private *i915 = rc6_to_i915(rc6);
+ struct intel_gt *gt = rc6_to_gt(rc6);
+ struct drm_i915_gem_object *obj;
+ struct i915_vma *vma;
+ void *batch;
+ struct i915_gem_ww_ctx ww;
+ int err;
+
+ obj = i915_gem_object_create_shmem(i915, PAGE_SIZE);
+ if (IS_ERR(obj))
+ return PTR_ERR(obj);
+
+ vma = i915_vma_instance(obj, >->ggtt->vm, NULL);
+ if (IS_ERR(vma)){
+ err = PTR_ERR(vma);
+ goto err;
+ }
+ rc6->vma=vma;
+ i915_gem_ww_ctx_init(&ww, true);
+retry:
+ err = i915_gem_object_lock(obj, &ww);
+ if (!err)
+ err = i915_ggtt_pin(rc6->vma, &ww, 0, PIN_HIGH);
+ if (err)
+ goto err_ww_fini;
+
+ batch = i915_gem_object_pin_map(obj, I915_MAP_WB);
+ if (IS_ERR(batch)) {
+ err = PTR_ERR(batch);
+ goto err_unpin;
+ }
+ rc6->rc6_wa_bb = batch;
+ return 0;
+err_unpin:
+ if (err)
+ i915_vma_unpin(rc6->vma);
+err_ww_fini:
+ if (err == -EDEADLK) {
+ err = i915_gem_ww_ctx_backoff(&ww);
+ if (!err)
+ goto retry;
+ }
+ i915_gem_ww_ctx_fini(&ww);
+
+ if (err)
+ i915_vma_put(rc6->vma);
+err:
+ i915_gem_object_put(obj);
+ return err;
+}
+
+void rc6_wa_bb_ctx_wa_fini(struct intel_rc6 *rc6)
+{
+ i915_vma_unpin(rc6->vma);
+ i915_vma_put(rc6->vma);
+}
+
static void gen11_rc6_enable(struct intel_rc6 *rc6)
{
struct intel_gt *gt = rc6_to_gt(rc6);
@@ -616,6 +676,9 @@ void intel_rc6_init(struct intel_rc6 *rc6)
err = chv_rc6_init(rc6);
else if (IS_VALLEYVIEW(i915))
err = vlv_rc6_init(rc6);
+ else if ((GRAPHICS_VER_FULL(i915) >= IP_VER(12, 0)) &&
+(GRAPHICS_VER_FULL(i915) <= IP_VER(12, 70)))
+ err = rc6_wa_bb_ctx_init(rc6);
else
err = 0;
diff --git a/drivers/gpu/drm/i915/gt/intel_rc6_types.h b/drivers/gpu/drm/i915/gt/intel_rc6_types.h
index cd4587098162a..643fd4e839ad4 100644
--- a/drivers/gpu/drm/i915/gt/intel_rc6_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_rc6_types.h
@@ -33,6 +33,9 @@ struct intel_rc6 {
struct drm_i915_gem_object *pctx;
+ u32 *rc6_wa_bb;
+ struct i915_vma *vma;
+
bool supported : 1;
bool enabled : 1;
bool manual : 1;
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 4d2dece960115..d20afb318d857 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -14,6 +14,7 @@
#include "intel_gt_regs.h"
#include "intel_ring.h"
#include "intel_workarounds.h"
+#include "intel_r
[Intel-gfx] [PATCH] drm/i915: Introduce Wa_14011274333
Wa_14011274333 applies to RKL, ADL-S, ADL-P and TGL. ALlocate buffer
pinned to GGTT and add WA to restore impacted registers.
v2: use correct lineage number, more generically apply workarounds for
all registers impacted, move workaround to gt/intel_workarounds.c
(MattR)
Based off patch by Tilak Tangudu.
Signed-off-by: Matt Atwood
---
drivers/gpu/drm/i915/gt/intel_gt_regs.h | 5 ++
drivers/gpu/drm/i915/gt/intel_rc6.c | 63 +
drivers/gpu/drm/i915/gt/intel_rc6_types.h | 3 +
drivers/gpu/drm/i915/gt/intel_workarounds.c | 40 +
4 files changed, 111 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
index 718cb2c80f79e..eaee35ecbc8d3 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
@@ -83,6 +83,11 @@
#define MTL_MCR_GROUPID REG_GENMASK(11, 8)
#define MTL_MCR_INSTANCEID REG_GENMASK(3, 0)
+#define CTX_WA_PTR _MMIO(0x2058)
+#define CTX_WA_PTR_ADDR_MASK REG_GENMASK(31,12)
+#define CTX_WA_TYPE_MASK REG_GENMASK(4,3)
+#define CTX_WA_VALID REG_BIT(0)
+
#define IPEIR_I965 _MMIO(0x2064)
#define IPEHR_I965 _MMIO(0x2068)
diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c
b/drivers/gpu/drm/i915/gt/intel_rc6.c
index 58bb1c55294c9..6baa341814da7 100644
--- a/drivers/gpu/drm/i915/gt/intel_rc6.c
+++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
@@ -14,6 +14,7 @@
#include "intel_gt.h"
#include "intel_gt_pm.h"
#include "intel_gt_regs.h"
+#include "intel_gpu_commands.h"
#include "intel_pcode.h"
#include "intel_rc6.h"
@@ -53,6 +54,65 @@ static struct drm_i915_private *rc6_to_i915(struct intel_rc6
*rc)
return rc6_to_gt(rc)->i915;
}
+static int rc6_wa_bb_ctx_init(struct intel_rc6 *rc6)
+{
+ struct drm_i915_private *i915 = rc6_to_i915(rc6);
+ struct intel_gt *gt = rc6_to_gt(rc6);
+ struct drm_i915_gem_object *obj;
+ struct i915_vma *vma;
+ void *batch;
+ struct i915_gem_ww_ctx ww;
+ int err;
+
+ obj = i915_gem_object_create_shmem(i915, PAGE_SIZE);
+ if (IS_ERR(obj))
+ return PTR_ERR(obj);
+
+ vma = i915_vma_instance(obj, >->ggtt->vm, NULL);
+ if (IS_ERR(vma)){
+ err = PTR_ERR(vma);
+ goto err;
+ }
+ rc6->vma=vma;
+ i915_gem_ww_ctx_init(&ww, true);
+retry:
+ err = i915_gem_object_lock(obj, &ww);
+ if (!err)
+ err = i915_ggtt_pin(rc6->vma, &ww, 0, PIN_HIGH);
+ if (err)
+ goto err_ww_fini;
+
+ batch = i915_gem_object_pin_map(obj, I915_MAP_WB);
+ if (IS_ERR(batch)) {
+ err = PTR_ERR(batch);
+ goto err_unpin;
+ }
+ rc6->rc6_wa_bb = batch;
+ return 0;
+err_unpin:
+ if (err)
+ i915_vma_unpin(rc6->vma);
+err_ww_fini:
+ if (err == -EDEADLK) {
+ err = i915_gem_ww_ctx_backoff(&ww);
+ if (!err)
+ goto retry;
+ }
+ i915_gem_ww_ctx_fini(&ww);
+
+ if (err)
+ i915_vma_put(rc6->vma);
+err:
+ i915_gem_object_put(obj);
+ return err;
+}
+
+void rc6_wa_bb_ctx_wa_fini(struct intel_rc6 *rc6)
+{
+ i915_vma_unpin(rc6->vma);
+ i915_vma_put(rc6->vma);
+}
+
static void gen11_rc6_enable(struct intel_rc6 *rc6)
{
struct intel_gt *gt = rc6_to_gt(rc6);
@@ -616,6 +676,9 @@ void intel_rc6_init(struct intel_rc6 *rc6)
err = chv_rc6_init(rc6);
else if (IS_VALLEYVIEW(i915))
err = vlv_rc6_init(rc6);
+ else if ((GRAPHICS_VER_FULL(i915) >= IP_VER(12, 0)) &&
+(GRAPHICS_VER_FULL(i915) <= IP_VER(12, 70)))
+ err = rc6_wa_bb_ctx_init(rc6);
else
err = 0;
diff --git a/drivers/gpu/drm/i915/gt/intel_rc6_types.h
b/drivers/gpu/drm/i915/gt/intel_rc6_types.h
index cd4587098162a..643fd4e839ad4 100644
--- a/drivers/gpu/drm/i915/gt/intel_rc6_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_rc6_types.h
@@ -33,6 +33,9 @@ struct intel_rc6 {
struct drm_i915_gem_object *pctx;
+ u32 *rc6_wa_bb;
+ struct i915_vma *vma;
+
bool supported : 1;
bool enabled : 1;
bool manual : 1;
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 4d2dece960115..d20afb318d857 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -14,6 +14,7 @@
#include "intel_gt_regs.h"
#include "intel_ring.h"
#include "intel_workarounds.h"
+#include "intel_rc6.h"
/**
* DOC: Hardware workarounds
@@ -3132,6 +3133,41 @@ general_render_compute_wa_init(struct intel_engine_cs
*engine, struct i915_wa_li
true);
}
+stati
