[Intel-gfx] [Patch v3 1/2] drm/i915: Move reg_in_range_table

[Intel-gfx] [Patch v3 1/2] drm/i915: Move reg_in_range_table

[Matt Atwood]

Function reg_in_range_table is useful in more places, move function to
intel_uncore.h to make available to more places.

Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c    | 13 +------------
 drivers/gpu/drm/i915/intel_uncore.h | 12 ++++++++++++
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 7b1c8de2f9cb3..5e5dc73621379 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -219,6 +219,7 @@
 #include "i915_perf.h"
 #include "i915_perf_oa_regs.h"
 #include "i915_reg.h"
+#include "intel_uncore.h"
 
 /* HW requires this to be a power of two, between 128k and 16M, though driver
  * is currently generally designed assuming the largest 16M size is used such
@@ -4324,18 +4325,6 @@ static bool gen8_is_valid_flex_addr(struct i915_perf *perf, u32 addr)
 	return false;
 }
 
-static bool reg_in_range_table(u32 addr, const struct i915_range *table)
-{
-	while (table->start || table->end) {
-		if (addr >= table->start && addr <= table->end)
-			return true;
-
-		table++;
-	}
-
-	return false;
-}
-
 #define REG_EQUAL(addr, mmio) \
 	((addr) == i915_mmio_reg_offset(mmio))
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index f419c311a0dea..1e85eaec1fc5a 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -496,6 +496,18 @@ static inline int intel_uncore_write_and_verify(struct intel_uncore *uncore,
 	return (reg_val & mask) != expected_val ? -EINVAL : 0;
 }
 
+static inline bool reg_in_range_table(u32 addr, const struct i915_range *table)
+{
+	while (table->start || table->end) {
+		if (addr >= table->start && addr <= table->end)
+			return true;
+
+		table++;
+	}
+
+	return false;
+}
+
 static inline void __iomem *intel_uncore_regs(struct intel_uncore *uncore)
 {
 	return uncore->regs;
-- 
2.40.1

[Intel-gfx] [Patch v3 2/2] drm/i915: Introduce Wa_1401127433

[Matt Atwood]

Wa_14011274333 applies to gen 12.0->12.55. There are regions of
registers that restore to default values on resume from rc6. The
hardware has a built in register (CTX_WA_PTR), that can point to a
region of memory full of commands to restore non default values on rc6
resume.

Based off patch by Tilak Tangudu.

v2: Use correct lineage number, more generically apply workarounds for
all registers impacted, move workaround to gt/intel_workarounds.c
(MattR)

v3: Correctly use intel_engine_cs, intel_engine_regs, use
reg_in_range_table() for wa registers impacted search, move the majority
of functional changes into intel_workarounds.c, free up pinned memory on
engine tear down, be more verbose in commit message and wa comments, use
generic function for platforms applied to. (MattR)

Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c    |   2 +
 drivers/gpu/drm/i915/gt/intel_engine_regs.h  |   4 +
 drivers/gpu/drm/i915/gt/intel_engine_types.h |   3 +
 drivers/gpu/drm/i915/gt/intel_workarounds.c  | 114 +++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_workarounds.h  |   1 +
 5 files changed, 124 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 40687806d22a6..d3628462bfb3e 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -1573,6 +1573,8 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 	intel_wa_list_free(&engine->ctx_wa_list);
 	intel_wa_list_free(&engine->wa_list);
 	intel_wa_list_free(&engine->whitelist);
+
+	intel_ctx_wa_bb_fini(engine);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_regs.h b/drivers/gpu/drm/i915/gt/intel_engine_regs.h
index a8eac59e37793..becb466f5910d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_regs.h
@@ -251,6 +251,10 @@
 #define GEN11_VECS_SFC_USAGE(base)		_MMIO((base) + 0x2014)
 #define   GEN11_VECS_SFC_USAGE_BIT		(1 << 0)
 
+#define CTX_WA_PTR(base)			_MMIO((base) + 0x2058)
+#define   CTX_WA_PTR_ADDR_MASK			REG_GENMASK(31, 12)
+#define   CTX_WA_VALID				REG_BIT(0)
+
 #define RING_HWS_PGA_GEN6(base)	_MMIO((base) + 0x2080)
 
 #define GEN12_HCP_SFC_LOCK_STATUS(base)		_MMIO((base) + 0x2914)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 960e6be2042fe..95ff3e1adf11e 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -388,6 +388,9 @@ struct intel_engine_cs {
 	u32 context_size;
 	u32 mmio_base;
 
+	u32 *ctx_wa_bb;
+	struct i915_vma *vma;
+
 	struct intel_engine_tlb_inv tlb_inv;
 
 	/*
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 9bc0654efdc0c..1d33555039725 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -5,6 +5,8 @@
 
 #include "i915_drv.h"
 #include "i915_reg.h"
+#include "i915_perf.h"
+#include "gem/i915_gem_internal.h"
 #include "intel_context.h"
 #include "intel_engine_pm.h"
 #include "intel_engine_regs.h"
@@ -14,6 +16,7 @@
 #include "intel_gt_print.h"
 #include "intel_gt_regs.h"
 #include "intel_ring.h"
+#include "intel_uncore.h"
 #include "intel_workarounds.h"
 
 /**
@@ -2985,6 +2988,105 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li
 	}
 }
 
+static const struct i915_range tdl_power_ctx[] = {
+	{ .start = 0xe100, .end = 0xe100 },
+	{ .start = 0xe180, .end = 0xe194 },
+	{},
+};
+
+static void
+emit_ctx_wa_bb(struct intel_engine_cs *engine, struct i915_wa_list *wal)
+{
+	const struct i915_wa *wa;
+	struct intel_uncore *uncore = engine->gt->uncore;
+	int i, count = 0;
+	u32 *cs = engine->ctx_wa_bb;
+
+	*cs++ = MI_LOAD_REGISTER_IMM(1);
+	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
+		if (reg_in_range_table(i915_mmio_reg_offset(wa->mcr_reg), tdl_power_ctx)) {
+			*cs++ = i915_mmio_reg_offset(wa->mcr_reg);
+			*cs++ = wa->set;
+			count++;
+		}
+	}
+	*cs++ = MI_BATCH_BUFFER_END;
+
+	if (count != 0) {
+		i915_gem_object_flush_map(engine->vma->obj);
+		intel_uncore_write(uncore, CTX_WA_PTR(engine->mmio_base),
+				   REG_FIELD_PREP(CTX_WA_PTR_ADDR_MASK,
+						  i915_vma_offset(engine->vma) & GENMASK(31, 12)) |
+				   CTX_WA_VALID);
+	}
+}
+
+static int ctx_wa_bb_init(struct intel_engine_cs *engine)
+{
+	struct drm_i915_private *i915 = engine->i915;
+	struct intel_gt *gt = engine->gt;
+	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_internal(i915, PAGE_SIZE);
+	if (IS_ERR(obj))
+		return PTR_ERR(obj);
+
+	vma = i915_vma_instance(obj, &gt->ggtt->vm, NULL);
+	if (IS_ERR(vma)) {
+		err = PTR_ERR(vma);
+		goto err;
+	}
+
+	engine->vma = vma;
+	i915_gem_ww_ctx_init(&ww, true);
+
+retry:
+	err = i915_gem_object_lock(obj, &ww);
+	if (!err)
+		err = i915_ggtt_pin(engine->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;
+	}
+	engine->ctx_wa_bb = batch;
+
+	return 0;
+
+err_unpin:
+	i915_vma_unpin(engine->vma);
+
+err_ww_fini:
+	if (err == -EDEADLK) {
+		err = i915_gem_ww_ctx_backoff(&ww);
+		if (!err)
+			goto retry;
+	}
+
+	i915_gem_ww_ctx_fini(&ww);
+	i915_vma_put(engine->vma);
+
+err:
+	i915_gem_object_put(obj);
+	return err;
+}
+
+void intel_ctx_wa_bb_fini(struct intel_engine_cs *engine)
+{
+	i915_vma_unpin_and_release(&engine->vma, I915_VMA_RELEASE_MAP);
+}
+
+#define NEEDS_CTX_WABB(engine) ( \
+	IS_GFX_GT_IP_RANGE(engine->gt, IP_VER(12, 0), IP_VER(12, 55)) && \
+	engine->class == RENDER_CLASS)
+
 static void
 engine_init_workarounds(struct intel_engine_cs *engine, struct i915_wa_list *wal)
 {
@@ -3007,6 +3109,18 @@ engine_init_workarounds(struct intel_engine_cs *engine, struct i915_wa_list *wal
 		rcs_engine_wa_init(engine, wal);
 	else
 		xcs_engine_wa_init(engine, wal);
+
+	/* Wa_14011274333
+	 * After the workaround list has been populated some platforms have
+	 * regions of addresses that do not retain their values after resuming
+	 * from rc6. For these platforms, add all workarounds in these regions
+	 * to a batch buffer, this batch buffer is stored in memory and
+	 * executes on every rc6 resume.
+	 */
+	if (NEEDS_CTX_WABB(engine)) {
+		ctx_wa_bb_init(engine);
+		emit_ctx_wa_bb(engine, wal);
+	}
 }
 
 void intel_engine_init_workarounds(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.h b/drivers/gpu/drm/i915/gt/intel_workarounds.h
index 9beaab77c7f0b..fe8946b0c7b67 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.h
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.h
@@ -36,4 +36,5 @@ void intel_engine_apply_workarounds(struct intel_engine_cs *engine);
 int intel_engine_verify_workarounds(struct intel_engine_cs *engine,
 				    const char *from);
 
+void intel_ctx_wa_bb_fini(struct intel_engine_cs *engine);
 #endif
-- 
2.40.1

Re: [Intel-gfx] [Patch v3 2/2] drm/i915: Introduce Wa_1401127433

[Matt Roper]

You're missing a digit in the patch subject.

On Wed, Nov 29, 2023 at 12:51:22PM -0800, Matt Atwood wrote:
> Wa_14011274333 applies to gen 12.0->12.55. There are regions of
> registers that restore to default values on resume from rc6. The

It would be more clear to write this sentence as "The TDL power context
is not properly restored on RC6 exit, causing registers in the context
to revert to their hardware default values on RC6 exit."

> hardware has a built in register (CTX_WA_PTR), that can point to a
> region of memory full of commands to restore non default values on rc6
> resume.
> 
> Based off patch by Tilak Tangudu.
> 
> v2: Use correct lineage number, more generically apply workarounds for
> all registers impacted, move workaround to gt/intel_workarounds.c
> (MattR)
> 
> v3: Correctly use intel_engine_cs, intel_engine_regs, use
> reg_in_range_table() for wa registers impacted search, move the majority
> of functional changes into intel_workarounds.c, free up pinned memory on
> engine tear down, be more verbose in commit message and wa comments, use
> generic function for platforms applied to. (MattR)
> 
> Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c    |   2 +
>  drivers/gpu/drm/i915/gt/intel_engine_regs.h  |   4 +
>  drivers/gpu/drm/i915/gt/intel_engine_types.h |   3 +
>  drivers/gpu/drm/i915/gt/intel_workarounds.c  | 114 +++++++++++++++++++
>  drivers/gpu/drm/i915/gt/intel_workarounds.h  |   1 +
>  5 files changed, 124 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 40687806d22a6..d3628462bfb3e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -1573,6 +1573,8 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
>  	intel_wa_list_free(&engine->ctx_wa_list);
>  	intel_wa_list_free(&engine->wa_list);
>  	intel_wa_list_free(&engine->whitelist);
> +
> +	intel_ctx_wa_bb_fini(engine);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_regs.h b/drivers/gpu/drm/i915/gt/intel_engine_regs.h
> index a8eac59e37793..becb466f5910d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_regs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_regs.h
> @@ -251,6 +251,10 @@
>  #define GEN11_VECS_SFC_USAGE(base)		_MMIO((base) + 0x2014)
>  #define   GEN11_VECS_SFC_USAGE_BIT		(1 << 0)
>  
> +#define CTX_WA_PTR(base)			_MMIO((base) + 0x2058)

This isn't the right offset; you've accidentally pre-added the RCS base.
The definition should be (base + 0x58).

> +#define   CTX_WA_PTR_ADDR_MASK			REG_GENMASK(31, 12)
> +#define   CTX_WA_VALID				REG_BIT(0)
> +
>  #define RING_HWS_PGA_GEN6(base)	_MMIO((base) + 0x2080)
>  
>  #define GEN12_HCP_SFC_LOCK_STATUS(base)		_MMIO((base) + 0x2914)
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 960e6be2042fe..95ff3e1adf11e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -388,6 +388,9 @@ struct intel_engine_cs {
>  	u32 context_size;
>  	u32 mmio_base;
>  
> +	u32 *ctx_wa_bb;
> +	struct i915_vma *vma;

You should pick a more descriptive name here.  Otherwise nobody will
know what this is a vma for.

> +
>  	struct intel_engine_tlb_inv tlb_inv;
>  
>  	/*
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 9bc0654efdc0c..1d33555039725 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -5,6 +5,8 @@
>  
>  #include "i915_drv.h"
>  #include "i915_reg.h"
> +#include "i915_perf.h"
> +#include "gem/i915_gem_internal.h"
>  #include "intel_context.h"
>  #include "intel_engine_pm.h"
>  #include "intel_engine_regs.h"
> @@ -14,6 +16,7 @@
>  #include "intel_gt_print.h"
>  #include "intel_gt_regs.h"
>  #include "intel_ring.h"
> +#include "intel_uncore.h"
>  #include "intel_workarounds.h"
>  
>  /**
> @@ -2985,6 +2988,105 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li
>  	}
>  }
>  
> +static const struct i915_range tdl_power_ctx[] = {
> +	{ .start = 0xe100, .end = 0xe100 },
> +	{ .start = 0xe180, .end = 0xe194 },
> +	{},
> +};
> +
> +static void
> +emit_ctx_wa_bb(struct intel_engine_cs *engine, struct i915_wa_list *wal)
> +{
> +	const struct i915_wa *wa;
> +	struct intel_uncore *uncore = engine->gt->uncore;
> +	int i, count = 0;
> +	u32 *cs = engine->ctx_wa_bb;
> +
> +	*cs++ = MI_LOAD_REGISTER_IMM(1);

I think you meant to come back and patch up the length of the
instruction after emitting however many registers we find, right?  It
looks like you forgot that part.

> +	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> +		if (reg_in_range_table(i915_mmio_reg_offset(wa->mcr_reg), tdl_power_ctx)) {
> +			*cs++ = i915_mmio_reg_offset(wa->mcr_reg);
> +			*cs++ = wa->set;

Part of the reason this is so easy to handle is because all of the
registers in the TDL power context are masked, so we don't have to worry
about clobbering unrelated bits when re-programming the workaround.  You
might want to add a comment noting that so that it doesn't look like
we're just reprogramming the whole register here.

> +			count++;
> +		}
> +	}
> +	*cs++ = MI_BATCH_BUFFER_END;
> +
> +	if (count != 0) {
> +		i915_gem_object_flush_map(engine->vma->obj);
> +		intel_uncore_write(uncore, CTX_WA_PTR(engine->mmio_base),
> +				   REG_FIELD_PREP(CTX_WA_PTR_ADDR_MASK,
> +						  i915_vma_offset(engine->vma) & GENMASK(31, 12)) |
> +				   CTX_WA_VALID);

CTX_WA_PTR gets cleared on render domain reset, so you need to make sure
it gets re-applied properly as well.  You should probably do this actual
register write in intel_engine_apply_workarounds() (to make sure it's
re-applied on the non-GuC platforms) and also add this register to the
guc ADS regset so that the GuC will save/restore it around resets on
ADL-P.

If it turned out you didn't actually need the batchbuffer, you could
release its object here and clear the vma reference in the engine.  The
presence/absence of that pointer can be used as the condition to
determine whether the register should be written in
intel_engine_apply_workarounds().


> +	}
> +}
> +
> +static int ctx_wa_bb_init(struct intel_engine_cs *engine)
> +{
> +	struct drm_i915_private *i915 = engine->i915;
> +	struct intel_gt *gt = engine->gt;
> +	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_internal(i915, PAGE_SIZE);
> +	if (IS_ERR(obj))
> +		return PTR_ERR(obj);
> +
> +	vma = i915_vma_instance(obj, &gt->ggtt->vm, NULL);
> +	if (IS_ERR(vma)) {
> +		err = PTR_ERR(vma);
> +		goto err;
> +	}
> +
> +	engine->vma = vma;
> +	i915_gem_ww_ctx_init(&ww, true);
> +
> +retry:
> +	err = i915_gem_object_lock(obj, &ww);
> +	if (!err)
> +		err = i915_ggtt_pin(engine->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;
> +	}
> +	engine->ctx_wa_bb = batch;
> +
> +	return 0;

Aren't we still holding locks in the wait-wound context?

> +
> +err_unpin:
> +	i915_vma_unpin(engine->vma);
> +
> +err_ww_fini:
> +	if (err == -EDEADLK) {
> +		err = i915_gem_ww_ctx_backoff(&ww);
> +		if (!err)
> +			goto retry;
> +	}
> +
> +	i915_gem_ww_ctx_fini(&ww);
> +	i915_vma_put(engine->vma);
> +
> +err:
> +	i915_gem_object_put(obj);
> +	return err;
> +}
> +
> +void intel_ctx_wa_bb_fini(struct intel_engine_cs *engine)
> +{
> +	i915_vma_unpin_and_release(&engine->vma, I915_VMA_RELEASE_MAP);

You might want to clear the vma pointer as well to allow this function
to be used in more places, as noted above.

> +}
> +
> +#define NEEDS_CTX_WABB(engine) ( \

It might be better to put the name of the workaround in the macro here.
There are lots of potential reasons why we might want to execute a batch
on RC6 exit; this workaround is just one specific reason, but there may
be others in the future.

> +	IS_GFX_GT_IP_RANGE(engine->gt, IP_VER(12, 0), IP_VER(12, 55)) && \

This isn't needed on 12.55.  It was a pre-production-only workaround for
DG2-G10, and it never applied to DG2-G11 or DG2-G12.  So 12.00 - 12.10
would be the proper range.

Note that if you did need to program this on DG2, then you would have
also needed to take Wa_18018699957 into account, which involves
switching to use BB_PER_CTX_PTR instead of CTX_WA_PTR.


> +	engine->class == RENDER_CLASS)
> +
>  static void
>  engine_init_workarounds(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>  {
> @@ -3007,6 +3109,18 @@ engine_init_workarounds(struct intel_engine_cs *engine, struct i915_wa_list *wal
>  		rcs_engine_wa_init(engine, wal);
>  	else
>  		xcs_engine_wa_init(engine, wal);
> +
> +	/* Wa_14011274333
> +	 * After the workaround list has been populated some platforms have
> +	 * regions of addresses that do not retain their values after resuming
> +	 * from rc6. For these platforms, add all workarounds in these regions
> +	 * to a batch buffer, this batch buffer is stored in memory and
> +	 * executes on every rc6 resume.
> +	 */
> +	if (NEEDS_CTX_WABB(engine)) {
> +		ctx_wa_bb_init(engine);

This function returns errors, but you're not handling them here, so
failure will be silently ignored.


Matt

> +		emit_ctx_wa_bb(engine, wal);
> +	}
>  }
>  
>  void intel_engine_init_workarounds(struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.h b/drivers/gpu/drm/i915/gt/intel_workarounds.h
> index 9beaab77c7f0b..fe8946b0c7b67 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.h
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.h
> @@ -36,4 +36,5 @@ void intel_engine_apply_workarounds(struct intel_engine_cs *engine);
>  int intel_engine_verify_workarounds(struct intel_engine_cs *engine,
>  				    const char *from);
>  
> +void intel_ctx_wa_bb_fini(struct intel_engine_cs *engine);
>  #endif
> -- 
> 2.40.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v3,1/2] drm/i915: Move reg_in_range_table

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/2] drm/i915: Move reg_in_range_table
URL   : https://patchwork.freedesktop.org/series/127082/
State : warning

== Summary ==

Error: dim checkpatch failed
553cf478868e drm/i915: Move reg_in_range_table
349983425e84 drm/i915: Introduce Wa_1401127433
-:188: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'engine' - possible side-effects?
#188: FILE: drivers/gpu/drm/i915/gt/intel_workarounds.c:3078:
+#define NEEDS_CTX_WABB(engine) ( \
+	IS_GFX_GT_IP_RANGE(engine->gt, IP_VER(12, 0), IP_VER(12, 55)) && \
+	engine->class == RENDER_CLASS)

-:188: CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'engine' may be better as '(engine)' to avoid precedence issues
#188: FILE: drivers/gpu/drm/i915/gt/intel_workarounds.c:3078:
+#define NEEDS_CTX_WABB(engine) ( \
+	IS_GFX_GT_IP_RANGE(engine->gt, IP_VER(12, 0), IP_VER(12, 55)) && \
+	engine->class == RENDER_CLASS)

total: 0 errors, 0 warnings, 2 checks, 170 lines checked

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [v3,1/2] drm/i915: Move reg_in_range_table

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/2] drm/i915: Move reg_in_range_table
URL   : https://patchwork.freedesktop.org/series/127082/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v3,1/2] drm/i915: Move reg_in_range_table

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 7458 bytes --]

== Series Details ==

Series: series starting with [v3,1/2] drm/i915: Move reg_in_range_table
URL   : https://patchwork.freedesktop.org/series/127082/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13951 -> Patchwork_127082v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_127082v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_127082v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/index.html

Participating hosts (36 -> 37)
------------------------------

  Additional (3): bat-dg2-8 bat-kbl-2 bat-dg2-9 
  Missing    (2): fi-snb-2520m bat-dg1-5 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_127082v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - bat-rpls-1:         [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-rpls-1/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-rpls-1/igt@i915_module_load@load.html
    - bat-adlp-6:         [PASS][3] -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-adlp-6/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-adlp-6/igt@i915_module_load@load.html
    - fi-rkl-11600:       [PASS][5] -> [ABORT][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/fi-rkl-11600/igt@i915_module_load@load.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/fi-rkl-11600/igt@i915_module_load@load.html
    - bat-adls-5:         [PASS][7] -> [ABORT][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-adls-5/igt@i915_module_load@load.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-adls-5/igt@i915_module_load@load.html
    - bat-dg1-7:          [PASS][9] -> [ABORT][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-dg1-7/igt@i915_module_load@load.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-dg1-7/igt@i915_module_load@load.html
    - bat-adlp-9:         [PASS][11] -> [ABORT][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-adlp-9/igt@i915_module_load@load.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-adlp-9/igt@i915_module_load@load.html
    - bat-dg2-11:         [PASS][13] -> [ABORT][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-dg2-11/igt@i915_module_load@load.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-dg2-11/igt@i915_module_load@load.html
    - bat-adln-1:         [PASS][15] -> [ABORT][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-adln-1/igt@i915_module_load@load.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-adln-1/igt@i915_module_load@load.html
    - bat-adlm-1:         [PASS][17] -> [ABORT][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-adlm-1/igt@i915_module_load@load.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-adlm-1/igt@i915_module_load@load.html
    - bat-rplp-1:         [PASS][19] -> [ABORT][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-rplp-1/igt@i915_module_load@load.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-rplp-1/igt@i915_module_load@load.html
    - fi-tgl-1115g4:      [PASS][21] -> [ABORT][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/fi-tgl-1115g4/igt@i915_module_load@load.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/fi-tgl-1115g4/igt@i915_module_load@load.html
    - bat-atsm-1:         [PASS][23] -> [ABORT][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-atsm-1/igt@i915_module_load@load.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-atsm-1/igt@i915_module_load@load.html
    - bat-dg2-9:          NOTRUN -> [ABORT][25]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-dg2-9/igt@i915_module_load@load.html
    - bat-adlp-11:        [PASS][26] -> [ABORT][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-adlp-11/igt@i915_module_load@load.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-adlp-11/igt@i915_module_load@load.html
    - bat-dg2-8:          NOTRUN -> [ABORT][28]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-dg2-8/igt@i915_module_load@load.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_module_load@load:
    - {bat-dg2-13}:       [PASS][29] -> [ABORT][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-dg2-13/igt@i915_module_load@load.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-dg2-13/igt@i915_module_load@load.html
    - {bat-dg2-14}:       [PASS][31] -> [ABORT][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13951/bat-dg2-14/igt@i915_module_load@load.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-dg2-14/igt@i915_module_load@load.html

  
Known issues
------------

  Here are the changes found in Patchwork_127082v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@fbdev@info:
    - bat-kbl-2:          NOTRUN -> [SKIP][33] ([fdo#109271] / [i915#1849])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-kbl-2/igt@fbdev@info.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-kbl-2:          NOTRUN -> [SKIP][34] ([fdo#109271]) +20 other tests skip
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-kbl-2/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence:
    - bat-kbl-2:          NOTRUN -> [SKIP][35] ([fdo#109271] / [i915#1845]) +14 other tests skip
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/bat-kbl-2/igt@kms_pipe_crc_basic@read-crc-frame-sequence.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849


Build changes
-------------

  * Linux: CI_DRM_13951 -> Patchwork_127082v1

  CI-20190529: 20190529
  CI_DRM_13951: 7bd342323d5bd405b02fd21cd78f157f363278ac @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7610: 7610
  Patchwork_127082v1: 7bd342323d5bd405b02fd21cd78f157f363278ac @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

9c7f0e14fd6e drm/i915: Introduce Wa_1401127433
ba3630268a1c drm/i915: Move reg_in_range_table

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127082v1/index.html

[-- Attachment #2: Type: text/html, Size: 8583 bytes --]

Re: [Intel-gfx] [Patch v3 1/2] drm/i915: Move reg_in_range_table

[Rodrigo Vivi]

On Wed, Nov 29, 2023 at 12:51:21PM -0800, Matt Atwood wrote:
> Function reg_in_range_table is useful in more places, move function to
> intel_uncore.h to make available to more places.
> 
> Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_perf.c    | 13 +------------
>  drivers/gpu/drm/i915/intel_uncore.h | 12 ++++++++++++
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 7b1c8de2f9cb3..5e5dc73621379 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -219,6 +219,7 @@
>  #include "i915_perf.h"
>  #include "i915_perf_oa_regs.h"
>  #include "i915_reg.h"
> +#include "intel_uncore.h"
>  
>  /* HW requires this to be a power of two, between 128k and 16M, though driver
>   * is currently generally designed assuming the largest 16M size is used such
> @@ -4324,18 +4325,6 @@ static bool gen8_is_valid_flex_addr(struct i915_perf *perf, u32 addr)
>  	return false;
>  }
>  
> -static bool reg_in_range_table(u32 addr, const struct i915_range *table)
> -{
> -	while (table->start || table->end) {
> -		if (addr >= table->start && addr <= table->end)
> -			return true;
> -
> -		table++;
> -	}
> -
> -	return false;
> -}
> -
>  #define REG_EQUAL(addr, mmio) \
>  	((addr) == i915_mmio_reg_offset(mmio))
>  
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index f419c311a0dea..1e85eaec1fc5a 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -496,6 +496,18 @@ static inline int intel_uncore_write_and_verify(struct intel_uncore *uncore,
>  	return (reg_val & mask) != expected_val ? -EINVAL : 0;
>  }
>  
> +static inline bool reg_in_range_table(u32 addr, const struct i915_range *table)

exported functions should carry the name of the component that is exposing it.

something like intel_uncore_reg_in_range()

but also, based on your second patch, this doesn't seem to be the right thing to do.

although I hate the i915_utils.h, that sounds like a better place for a function like this.

or on a second thought... maybe just duplicate the logic that is inside this
function if only one extra call... or maybe duplicate this function along
the other table definition instead of having the table in one place and using
the helper from another place.

> +{
> +	while (table->start || table->end) {
> +		if (addr >= table->start && addr <= table->end)
> +			return true;
> +
> +		table++;
> +	}
> +
> +	return false;
> +}
> +
>  static inline void __iomem *intel_uncore_regs(struct intel_uncore *uncore)
>  {
>  	return uncore->regs;
> -- 
> 2.40.1
> 

Re: [Intel-gfx] [Patch v3 1/2] drm/i915: Move reg_in_range_table

[Matt Roper]

On Thu, Nov 30, 2023 at 03:00:59PM -0500, Rodrigo Vivi wrote:
> On Wed, Nov 29, 2023 at 12:51:21PM -0800, Matt Atwood wrote:
> > Function reg_in_range_table is useful in more places, move function to
> > intel_uncore.h to make available to more places.
> > 
> > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_perf.c    | 13 +------------
> >  drivers/gpu/drm/i915/intel_uncore.h | 12 ++++++++++++
> >  2 files changed, 13 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 7b1c8de2f9cb3..5e5dc73621379 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -219,6 +219,7 @@
> >  #include "i915_perf.h"
> >  #include "i915_perf_oa_regs.h"
> >  #include "i915_reg.h"
> > +#include "intel_uncore.h"
> >  
> >  /* HW requires this to be a power of two, between 128k and 16M, though driver
> >   * is currently generally designed assuming the largest 16M size is used such
> > @@ -4324,18 +4325,6 @@ static bool gen8_is_valid_flex_addr(struct i915_perf *perf, u32 addr)
> >  	return false;
> >  }
> >  
> > -static bool reg_in_range_table(u32 addr, const struct i915_range *table)
> > -{
> > -	while (table->start || table->end) {
> > -		if (addr >= table->start && addr <= table->end)
> > -			return true;
> > -
> > -		table++;
> > -	}
> > -
> > -	return false;
> > -}
> > -
> >  #define REG_EQUAL(addr, mmio) \
> >  	((addr) == i915_mmio_reg_offset(mmio))
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> > index f419c311a0dea..1e85eaec1fc5a 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.h
> > +++ b/drivers/gpu/drm/i915/intel_uncore.h
> > @@ -496,6 +496,18 @@ static inline int intel_uncore_write_and_verify(struct intel_uncore *uncore,
> >  	return (reg_val & mask) != expected_val ? -EINVAL : 0;
> >  }
> >  
> > +static inline bool reg_in_range_table(u32 addr, const struct i915_range *table)
> 
> exported functions should carry the name of the component that is exposing it.
> 
> something like intel_uncore_reg_in_range()
> 
> but also, based on your second patch, this doesn't seem to be the right thing to do.
> 
> although I hate the i915_utils.h, that sounds like a better place for a function like this.

If the function leaves intel_uncore, we should probably move i915_range
to wherever it ends up as well since that's defined in this header at
the moment.  Today intel_uncore is where pretty much all of the general
register handling is, even though "uncore" is a bit of a misnomer for
most of what we're actually doing.

This function probably doesn't need to be an inline either.  Just a
normal function in a .c would probably be best.


Matt

> 
> or on a second thought... maybe just duplicate the logic that is inside this
> function if only one extra call... or maybe duplicate this function along
> the other table definition instead of having the table in one place and using
> the helper from another place.
> 
> > +{
> > +	while (table->start || table->end) {
> > +		if (addr >= table->start && addr <= table->end)
> > +			return true;
> > +
> > +		table++;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static inline void __iomem *intel_uncore_regs(struct intel_uncore *uncore)
> >  {
> >  	return uncore->regs;
> > -- 
> > 2.40.1
> > 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation