[Intel-gfx] [PATCH 1/3] drm/i915: Create a blitter context for GGTT updates

[Intel-gfx] [PATCH 1/3] drm/i915: Create a blitter context for GGTT updates

[Oak Zeng]

From: Nirmoy Das <nirmoy.das@intel.com>

Create a separate blitter context if a platform requires
GGTT updates using MI_UPDATE_GTT blitter command.

Subsequent patch will introduce methods to update
GGTT using this blitter context and MI_UPDATE_GTT blitter
command.

v2: Fix a typo in comment. (Oak)

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
Signed-off-by: Oak Zeng <oak.zeng@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine.h       |  4 ++
 drivers/gpu/drm/i915/gt/intel_engine_cs.c    | 44 +++++++++++++++++++-
 drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 ++
 drivers/gpu/drm/i915/gt/intel_gtt.c          |  4 ++
 drivers/gpu/drm/i915/gt/intel_gtt.h          |  2 +
 5 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index b58c30ac8ef0..ee36db2fdaa7 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -170,6 +170,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
 #define I915_GEM_HWS_SEQNO		0x40
 #define I915_GEM_HWS_SEQNO_ADDR		(I915_GEM_HWS_SEQNO * sizeof(u32))
 #define I915_GEM_HWS_MIGRATE		(0x42 * sizeof(u32))
+#define I915_GEM_HWS_GGTT_BLIT		0x46
+#define I915_GEM_HWS_GGTT_BLIT_ADDR	(I915_GEM_HWS_GGTT_BLIT * sizeof(u32))
 #define I915_GEM_HWS_PXP		0x60
 #define I915_GEM_HWS_PXP_ADDR		(I915_GEM_HWS_PXP * sizeof(u32))
 #define I915_GEM_HWS_GSC		0x62
@@ -356,4 +358,6 @@ u64 intel_clamp_preempt_timeout_ms(struct intel_engine_cs *engine, u64 value);
 u64 intel_clamp_stop_timeout_ms(struct intel_engine_cs *engine, u64 value);
 u64 intel_clamp_timeslice_duration_ms(struct intel_engine_cs *engine, u64 value);
 
+void intel_engine_blitter_context_set_ready(struct intel_gt *gt, bool ready);
+bool intel_engine_blitter_context_ready(struct intel_gt *gt);
 #endif /* _INTEL_RINGBUFFER_H_ */
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index dfb69fc977a0..d8c492a507a4 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -27,6 +27,7 @@
 #include "intel_gt_mcr.h"
 #include "intel_gt_pm.h"
 #include "intel_gt_requests.h"
+#include "intel_gtt.h"
 #include "intel_lrc.h"
 #include "intel_lrc_reg.h"
 #include "intel_reset.h"
@@ -1419,6 +1420,34 @@ void intel_engine_destroy_pinned_context(struct intel_context *ce)
 	intel_context_put(ce);
 }
 
+void intel_engine_blitter_context_set_ready(struct intel_gt *gt, bool ready)
+{
+	struct intel_engine_cs *engine = gt->engine[BCS0];
+
+	if (engine && engine->blitter_context)
+		atomic_set(&engine->blitter_context_ready, ready ? 1 : 0);
+}
+
+bool intel_engine_blitter_context_ready(struct intel_gt *gt)
+{
+	struct intel_engine_cs *engine = gt->engine[BCS0];
+
+	if (engine)
+		return atomic_read(&engine->blitter_context_ready) == 1;
+
+	return false;
+}
+
+static struct intel_context *
+create_ggtt_blitter_context(struct intel_engine_cs *engine)
+{
+	static struct lock_class_key kernel;
+
+	/* MI_UPDATE_GTT can insert up to 512 PTE entries so get a bigger ring */
+	return intel_engine_create_pinned_context(engine, engine->gt->vm, SZ_512K,
+						  I915_GEM_HWS_GGTT_BLIT_ADDR,
+						  &kernel, "ggtt_blitter_context");
+}
 static struct intel_context *
 create_kernel_context(struct intel_engine_cs *engine)
 {
@@ -1442,7 +1471,7 @@ create_kernel_context(struct intel_engine_cs *engine)
  */
 static int engine_init_common(struct intel_engine_cs *engine)
 {
-	struct intel_context *ce;
+	struct intel_context *ce, *bce = NULL;
 	int ret;
 
 	engine->set_default_submission(engine);
@@ -1458,6 +1487,15 @@ static int engine_init_common(struct intel_engine_cs *engine)
 	ce = create_kernel_context(engine);
 	if (IS_ERR(ce))
 		return PTR_ERR(ce);
+	/*
+	 * Create a separate pinned context for GGTT update using blitter
+	 * if a platform require such service.
+	 */
+	if (i915_ggtt_require_blitter(engine->i915) && engine->id == BCS0) {
+		bce = create_ggtt_blitter_context(engine);
+		if (IS_ERR(bce))
+			return PTR_ERR(bce);
+	}
 
 	ret = measure_breadcrumb_dw(ce);
 	if (ret < 0)
@@ -1465,6 +1503,7 @@ static int engine_init_common(struct intel_engine_cs *engine)
 
 	engine->emit_fini_breadcrumb_dw = ret;
 	engine->kernel_context = ce;
+	engine->blitter_context = bce;
 
 	return 0;
 
@@ -1537,6 +1576,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 
 	if (engine->kernel_context)
 		intel_engine_destroy_pinned_context(engine->kernel_context);
+	if (engine->blitter_context)
+		intel_engine_destroy_pinned_context(engine->blitter_context);
+
 
 	GEM_BUG_ON(!llist_empty(&engine->barrier_tasks));
 	cleanup_status_page(engine);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index e99a6fa03d45..62095c0d8783 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -415,6 +415,9 @@ struct intel_engine_cs {
 	struct llist_head barrier_tasks;
 
 	struct intel_context *kernel_context; /* pinned */
+	struct intel_context *blitter_context; /* pinned, only for BCS0 */
+	/* mark the blitter engine's availability status */
+	atomic_t blitter_context_ready;
 
 	/**
 	 * pinned_contexts_list: List of pinned contexts. This list is only
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
index 13944a14ea2d..9c77c97670fe 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -21,6 +21,10 @@
 #include "intel_gt_regs.h"
 #include "intel_gtt.h"
 
+bool i915_ggtt_require_blitter(struct drm_i915_private *i915)
+{
+	return IS_METEORLAKE(i915);
+}
 
 static bool intel_ggtt_update_needs_vtd_wa(struct drm_i915_private *i915)
 {
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
index 4d6296cdbcfd..9710eb031fb2 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -688,4 +688,6 @@ static inline struct sgt_dma {
 	return (struct sgt_dma){ sg, addr, addr + sg_dma_len(sg) };
 }
 
+bool i915_ggtt_require_blitter(struct drm_i915_private *i915);
+
 #endif
-- 
2.26.3

[Intel-gfx] [PATCH 2/3] drm/i915: Implement GGTT update method with blitter

[Oak Zeng]

From: Nirmoy Das <nirmoy.das@intel.com>

Implement GGTT update method with blitter command, MI_UPDATE_GTT
and install those handlers if a platform requires that.

v2: Make sure we hold the GT wakeref and Blitter engine wakeref before
we call mutex_lock/intel_context_enter below. When GT/engine are not
awake, the intel_context_enter calls into some runtime pm function which
can end up with kmalloc/fs_reclaim. But trigger fs_reclaim holding a
mutex lock is not allowed because shrinker can also try to hold the same
mutex lock. It is a circular lock. So hold the GT/blitter engine wakeref
before calling mutex_lock, to fix the circular lock. (Oak)

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
Signed-off-by: Oak Zeng <oak.zeng@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_ggtt.c | 198 +++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index dd0ed941441a..d445cb015257 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -15,18 +15,23 @@
 #include "display/intel_display.h"
 #include "gem/i915_gem_lmem.h"
 
+#include "intel_context.h"
 #include "intel_ggtt_gmch.h"
+#include "intel_gpu_commands.h"
 #include "intel_gt.h"
 #include "intel_gt_regs.h"
 #include "intel_pci_config.h"
+#include "intel_ring.h"
 #include "i915_drv.h"
 #include "i915_pci.h"
+#include "i915_request.h"
 #include "i915_scatterlist.h"
 #include "i915_utils.h"
 #include "i915_vgpu.h"
 
 #include "intel_gtt.h"
 #include "gen8_ppgtt.h"
+#include "intel_engine_pm.h"
 
 static void i915_ggtt_color_adjust(const struct drm_mm_node *node,
 				   unsigned long color,
@@ -252,6 +257,103 @@ u64 gen8_ggtt_pte_encode(dma_addr_t addr,
 	return pte;
 }
 
+static bool should_update_ggtt_with_blit(struct i915_ggtt *ggtt)
+{
+	struct intel_gt *gt = ggtt->vm.gt;
+
+	return intel_engine_blitter_context_ready(gt);
+}
+
+static bool gen8_ggtt_blit_fixed_pte(struct i915_ggtt *ggtt, u32 offset,
+				     u32 num_entries, const gen8_pte_t pte)
+{
+	struct intel_gt *gt = ggtt->vm.gt;
+	struct i915_sched_attr attr = {};
+	struct i915_request *rq;
+	struct intel_context *ce;
+	bool wakeref;
+	u32 *cs;
+
+	if (!num_entries)
+		return true;
+
+	ce = gt->engine[BCS0]->blitter_context;
+	if (!ce) {
+		drm_dbg(&ggtt->vm.i915->drm, "Failed to get blitter context\n");
+		return false;
+	}
+
+	/*
+	 * If the GT is not awake already at this stage then fallback
+	 * to pci based GGTT update otherwise __intel_wakeref_get_first()
+	 * would conflict with fs_reclaim trying to allocate memory while
+	 * doing rpm_resume().
+	 */
+	wakeref = intel_gt_pm_get_if_awake(gt);
+	if (!wakeref) {
+		drm_dbg(&ggtt->vm.i915->drm, "GT is not awake, fallback to CPU GGTT update\n");
+		return false;
+	}
+
+	if (!intel_engine_pm_get_if_awake(gt->engine[BCS0])) {
+		drm_dbg(&ggtt->vm.i915->drm, "Blitter engine is not awake, fallback to CPU GGTT update\n");
+		goto err_gt_pm_put;
+	}
+
+	while (num_entries) {
+		/* MI_UPDATE_GTT can update 512 entries in a single command */
+		u32 n_ptes = min_t(u32, 512, num_entries);
+
+		mutex_lock(&ce->timeline->mutex);
+		intel_context_enter(ce);
+		rq = __i915_request_create(ce, GFP_NOWAIT | GFP_ATOMIC);
+		intel_context_exit(ce);
+		if (IS_ERR(rq)) {
+			drm_dbg(&ggtt->vm.i915->drm,
+				"Failed to get blitter request\n");
+			goto err_unlock;
+		}
+
+		cs = intel_ring_begin(rq, 2 * n_ptes + 2);
+		if (IS_ERR(cs)) {
+			drm_dbg(&ggtt->vm.i915->drm,
+				"Failed to begin ring for GGTT blitter\n");
+			goto err_rq;
+		}
+
+		*cs++ = MI_UPDATE_GTT | (2 * n_ptes);
+		*cs++ = offset << 12;
+		memset64((u64 *)cs, pte, n_ptes);
+		cs += n_ptes * 2;
+		intel_ring_advance(rq, cs);
+
+		i915_request_get(rq);
+		__i915_request_commit(rq);
+		__i915_request_queue(rq, &attr);
+
+		mutex_unlock(&ce->timeline->mutex);
+		/* This will break if the request is complete or after engine reset */
+		i915_request_wait(rq, 0, MAX_SCHEDULE_TIMEOUT);
+		i915_request_put(rq);
+
+		num_entries -= n_ptes;
+	}
+
+	ggtt->invalidate(ggtt);
+	intel_engine_pm_put(gt->engine[BCS0]);
+	intel_gt_pm_put(gt);
+	return true;
+
+err_rq:
+	i915_request_put(rq);
+err_unlock:
+	mutex_unlock(&ce->timeline->mutex);
+	intel_engine_pm_put(gt->engine[BCS0]);
+err_gt_pm_put:
+	intel_gt_pm_put(gt);
+	return false;
+}
+
 static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte)
 {
 	writeq(pte, addr);
@@ -272,6 +374,22 @@ static void gen8_ggtt_insert_page(struct i915_address_space *vm,
 	ggtt->invalidate(ggtt);
 }
 
+static void gen8_ggtt_insert_page_blit(struct i915_address_space *vm,
+				       dma_addr_t addr, u64 offset,
+				       unsigned int pat_index, u32 flags)
+{
+	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
+	gen8_pte_t pte;
+
+	pte = ggtt->vm.pte_encode(addr, pat_index, flags);
+	if (should_update_ggtt_with_blit(i915_vm_to_ggtt(vm)) &&
+	    gen8_ggtt_blit_fixed_pte(ggtt, offset, 1, pte))
+		return;
+
+	gen8_ggtt_insert_page(vm, addr, offset, pat_index, flags);
+	ggtt->invalidate(ggtt);
+}
+
 static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 				     struct i915_vma_resource *vma_res,
 				     unsigned int pat_index,
@@ -311,6 +429,52 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 	ggtt->invalidate(ggtt);
 }
 
+static void __gen8_ggtt_insert_entries_blit(struct i915_address_space *vm,
+					    struct i915_vma_resource *vma_res,
+					    unsigned int pat_index, u32 flags)
+{
+	gen8_pte_t pte_encode;
+	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
+	struct sgt_iter iter;
+	dma_addr_t addr;
+	u64 start, end;
+
+	pte_encode = ggtt->vm.pte_encode(0, pat_index, flags);
+	start = (vma_res->start - vma_res->guard) / I915_GTT_PAGE_SIZE;
+	end = start + vma_res->guard / I915_GTT_PAGE_SIZE;
+	if (!gen8_ggtt_blit_fixed_pte(ggtt, start, end - start, vm->scratch[0]->encode))
+		goto err;
+	start = end;
+
+	end += (vma_res->node_size + vma_res->guard) / I915_GTT_PAGE_SIZE;
+	/* TODO: MI_UPDATE_GTT can update 511 entries in a single command. */
+	for_each_sgt_daddr(addr, iter, vma_res->bi.pages) {
+		if (!gen8_ggtt_blit_fixed_pte(ggtt, start++, 1, pte_encode | addr))
+			goto err;
+	}
+
+	if (!gen8_ggtt_blit_fixed_pte(ggtt, start, end - start, vm->scratch[0]->encode))
+		goto err;
+
+	return;
+
+err:
+	drm_dbg(&ggtt->vm.i915->drm, "falling back to gen8_ggtt_insert_entries\n");
+	gen8_ggtt_insert_entries(vm, vma_res, pat_index, flags);
+}
+
+static void gen8_ggtt_insert_entries_blit(struct i915_address_space *vm,
+					  struct i915_vma_resource *vma_res,
+					  unsigned int pat_index, u32 flags)
+{
+	if (!should_update_ggtt_with_blit(i915_vm_to_ggtt(vm))) {
+		gen8_ggtt_insert_entries(vm, vma_res, pat_index, flags);
+		return;
+	}
+
+	__gen8_ggtt_insert_entries_blit(vm, vma_res, pat_index, flags);
+}
+
 static void gen8_ggtt_clear_range(struct i915_address_space *vm,
 				  u64 start, u64 length)
 {
@@ -332,6 +496,34 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
 		gen8_set_pte(&gtt_base[i], scratch_pte);
 }
 
+static void gen8_ggtt_scratch_range_blit(struct i915_address_space *vm,
+					 u64 start, u64 length)
+{
+	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
+	unsigned int first_entry = start / I915_GTT_PAGE_SIZE;
+	unsigned int num_entries = length / I915_GTT_PAGE_SIZE;
+	const gen8_pte_t scratch_pte = vm->scratch[0]->encode;
+	gen8_pte_t __iomem *gtt_base =
+		(gen8_pte_t __iomem *)ggtt->gsm + first_entry;
+	const int max_entries = ggtt_total_entries(ggtt) - first_entry;
+	int i;
+
+	if (WARN(num_entries > max_entries,
+		 "First entry = %d; Num entries = %d (max=%d)\n",
+		 first_entry, num_entries, max_entries))
+		num_entries = max_entries;
+
+	if (should_update_ggtt_with_blit(ggtt) &&
+	    gen8_ggtt_blit_fixed_pte(ggtt, first_entry,
+				     num_entries, scratch_pte))
+		return;
+
+	for (i = 0; i < num_entries; i++)
+		gen8_set_pte(&gtt_base[i], scratch_pte);
+
+	ggtt->invalidate(ggtt);
+}
+
 static void gen6_ggtt_insert_page(struct i915_address_space *vm,
 				  dma_addr_t addr,
 				  u64 offset,
@@ -997,6 +1189,12 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
 			I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND;
 	}
 
+	if (i915_ggtt_require_blitter(i915)) {
+		ggtt->vm.scratch_range = gen8_ggtt_scratch_range_blit;
+		ggtt->vm.insert_page = gen8_ggtt_insert_page_blit;
+		ggtt->vm.insert_entries = gen8_ggtt_insert_entries_blit;
+	}
+
 	if (intel_uc_wants_guc(&ggtt->vm.gt->uc))
 		ggtt->invalidate = guc_ggtt_invalidate;
 	else
-- 
2.26.3

[Intel-gfx] [PATCH 3/3] drm/i915: Enable GGTT blitting in MTL

[Oak Zeng]

From: Nirmoy Das <nirmoy.das@intel.com>

MTL can hang because of a HW bug while parallel reading/writing
from/to LMEM/GTTMMADR BAR so try to reduce GGTT update
related pci transactions with blitter command as recommended
for Wa_22018444074.

To issue blitter commands, the driver must be primed to receive
requests. Maintain blitter-based GGTT update disablement until driver
probing completes. Moreover, implement a temporary disablement
of blitter prior to entering suspend, followed by re-enablement
post-resume. This is acceptable as those transition periods are
mostly single threaded.

v2: Disable GGTT blitter prior to runtime suspend and re-enable
after runtime resume. (Oak)

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
Signed-off-by: Oak Zeng <oak.zeng@intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index f8dbee7a5af7..6afe0adc8ddb 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -815,6 +815,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	i915_welcome_messages(i915);
 
 	i915->do_release = true;
+	intel_engine_blitter_context_set_ready(to_gt(i915), true);
 
 	return 0;
 
@@ -855,6 +856,7 @@ void i915_driver_remove(struct drm_i915_private *i915)
 {
 	intel_wakeref_t wakeref;
 
+	intel_engine_blitter_context_set_ready(to_gt(i915), false);
 	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
 
 	i915_driver_unregister(i915);
@@ -1077,6 +1079,8 @@ static int i915_drm_suspend(struct drm_device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
 	pci_power_t opregion_target_state;
 
+	intel_engine_blitter_context_set_ready(to_gt(dev_priv), false);
+
 	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
 
 	/* We do a lot of poking in a lot of registers, make sure they work
@@ -1264,6 +1268,7 @@ static int i915_drm_resume(struct drm_device *dev)
 	intel_gvt_resume(dev_priv);
 
 	enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
+	intel_engine_blitter_context_set_ready(to_gt(dev_priv), true);
 
 	return 0;
 }
@@ -1515,6 +1520,7 @@ static int intel_runtime_suspend(struct device *kdev)
 	if (drm_WARN_ON_ONCE(&dev_priv->drm, !HAS_RUNTIME_PM(dev_priv)))
 		return -ENODEV;
 
+	intel_engine_blitter_context_set_ready(to_gt(dev_priv), false);
 	drm_dbg(&dev_priv->drm, "Suspending device\n");
 
 	disable_rpm_wakeref_asserts(rpm);
@@ -1669,6 +1675,8 @@ static int intel_runtime_resume(struct device *kdev)
 	else
 		drm_dbg(&dev_priv->drm, "Device resumed\n");
 
+	intel_engine_blitter_context_set_ready(to_gt(dev_priv), true);
+
 	return ret;
 }
 
-- 
2.26.3

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Enable GGTT blitting in MTL

[Matt Roper]

On Tue, Aug 22, 2023 at 11:28:59AM -0400, Oak Zeng wrote:
> From: Nirmoy Das <nirmoy.das@intel.com>
> 
> MTL can hang because of a HW bug while parallel reading/writing
> from/to LMEM/GTTMMADR BAR so try to reduce GGTT update
> related pci transactions with blitter command as recommended
> for Wa_22018444074.

Drive-by comment:  this isn't a valid workaround number.  22018444074 is
a per-platform record number, whereas workarounds should always be
identified by their cross-platform lineage number, which will stay
constant if the workaround winds up extending to future platforms as
well.  So in this case, the workaround should be referred to as
Wa_13010847436.


Matt

> 
> To issue blitter commands, the driver must be primed to receive
> requests. Maintain blitter-based GGTT update disablement until driver
> probing completes. Moreover, implement a temporary disablement
> of blitter prior to entering suspend, followed by re-enablement
> post-resume. This is acceptable as those transition periods are
> mostly single threaded.
> 
> v2: Disable GGTT blitter prior to runtime suspend and re-enable
> after runtime resume. (Oak)
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> Signed-off-by: Oak Zeng <oak.zeng@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index f8dbee7a5af7..6afe0adc8ddb 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -815,6 +815,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	i915_welcome_messages(i915);
>  
>  	i915->do_release = true;
> +	intel_engine_blitter_context_set_ready(to_gt(i915), true);
>  
>  	return 0;
>  
> @@ -855,6 +856,7 @@ void i915_driver_remove(struct drm_i915_private *i915)
>  {
>  	intel_wakeref_t wakeref;
>  
> +	intel_engine_blitter_context_set_ready(to_gt(i915), false);
>  	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>  
>  	i915_driver_unregister(i915);
> @@ -1077,6 +1079,8 @@ static int i915_drm_suspend(struct drm_device *dev)
>  	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>  	pci_power_t opregion_target_state;
>  
> +	intel_engine_blitter_context_set_ready(to_gt(dev_priv), false);
> +
>  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
>  
>  	/* We do a lot of poking in a lot of registers, make sure they work
> @@ -1264,6 +1268,7 @@ static int i915_drm_resume(struct drm_device *dev)
>  	intel_gvt_resume(dev_priv);
>  
>  	enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> +	intel_engine_blitter_context_set_ready(to_gt(dev_priv), true);
>  
>  	return 0;
>  }
> @@ -1515,6 +1520,7 @@ static int intel_runtime_suspend(struct device *kdev)
>  	if (drm_WARN_ON_ONCE(&dev_priv->drm, !HAS_RUNTIME_PM(dev_priv)))
>  		return -ENODEV;
>  
> +	intel_engine_blitter_context_set_ready(to_gt(dev_priv), false);
>  	drm_dbg(&dev_priv->drm, "Suspending device\n");
>  
>  	disable_rpm_wakeref_asserts(rpm);
> @@ -1669,6 +1675,8 @@ static int intel_runtime_resume(struct device *kdev)
>  	else
>  		drm_dbg(&dev_priv->drm, "Device resumed\n");
>  
> +	intel_engine_blitter_context_set_ready(to_gt(dev_priv), true);
> +
>  	return ret;
>  }
>  
> -- 
> 2.26.3
> 

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

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Enable GGTT blitting in MTL

[Zeng, Oak]

Thanks,
Oak

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@intel.com>
> Sent: August 24, 2023 11:54 AM
> To: Zeng, Oak <oak.zeng@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Shyti, Andi <andi.shyti@intel.com>;
> chris.p.wilson@linux.intel.com; Das, Nirmoy <nirmoy.das@intel.com>
> Subject: Re: [Intel-gfx] [PATCH 3/3] drm/i915: Enable GGTT blitting in MTL
> 
> On Tue, Aug 22, 2023 at 11:28:59AM -0400, Oak Zeng wrote:
> > From: Nirmoy Das <nirmoy.das@intel.com>
> >
> > MTL can hang because of a HW bug while parallel reading/writing
> > from/to LMEM/GTTMMADR BAR so try to reduce GGTT update
> > related pci transactions with blitter command as recommended
> > for Wa_22018444074.
> 
> Drive-by comment:  this isn't a valid workaround number.  22018444074 is
> a per-platform record number, whereas workarounds should always be
> identified by their cross-platform lineage number, which will stay
> constant if the workaround winds up extending to future platforms as
> well.  So in this case, the workaround should be referred to as
> Wa_13010847436.

Spoke with Matt offline. Will keep lineage number 13010847436. Also put a soc HSD number 14019519902

Thanks Matt for explanation!
Oak

> 
> 
> Matt
> 
> >
> > To issue blitter commands, the driver must be primed to receive
> > requests. Maintain blitter-based GGTT update disablement until driver
> > probing completes. Moreover, implement a temporary disablement
> > of blitter prior to entering suspend, followed by re-enablement
> > post-resume. This is acceptable as those transition periods are
> > mostly single threaded.
> >
> > v2: Disable GGTT blitter prior to runtime suspend and re-enable
> > after runtime resume. (Oak)
> >
> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > Signed-off-by: Oak Zeng <oak.zeng@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> > index f8dbee7a5af7..6afe0adc8ddb 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -815,6 +815,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
> >  	i915_welcome_messages(i915);
> >
> >  	i915->do_release = true;
> > +	intel_engine_blitter_context_set_ready(to_gt(i915), true);
> >
> >  	return 0;
> >
> > @@ -855,6 +856,7 @@ void i915_driver_remove(struct drm_i915_private *i915)
> >  {
> >  	intel_wakeref_t wakeref;
> >
> > +	intel_engine_blitter_context_set_ready(to_gt(i915), false);
> >  	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> >
> >  	i915_driver_unregister(i915);
> > @@ -1077,6 +1079,8 @@ static int i915_drm_suspend(struct drm_device *dev)
> >  	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> >  	pci_power_t opregion_target_state;
> >
> > +	intel_engine_blitter_context_set_ready(to_gt(dev_priv), false);
> > +
> >  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> >
> >  	/* We do a lot of poking in a lot of registers, make sure they work
> > @@ -1264,6 +1268,7 @@ static int i915_drm_resume(struct drm_device *dev)
> >  	intel_gvt_resume(dev_priv);
> >
> >  	enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> > +	intel_engine_blitter_context_set_ready(to_gt(dev_priv), true);
> >
> >  	return 0;
> >  }
> > @@ -1515,6 +1520,7 @@ static int intel_runtime_suspend(struct device *kdev)
> >  	if (drm_WARN_ON_ONCE(&dev_priv-
> >drm, !HAS_RUNTIME_PM(dev_priv)))
> >  		return -ENODEV;
> >
> > +	intel_engine_blitter_context_set_ready(to_gt(dev_priv), false);
> >  	drm_dbg(&dev_priv->drm, "Suspending device\n");
> >
> >  	disable_rpm_wakeref_asserts(rpm);
> > @@ -1669,6 +1675,8 @@ static int intel_runtime_resume(struct device *kdev)
> >  	else
> >  		drm_dbg(&dev_priv->drm, "Device resumed\n");
> >
> > +	intel_engine_blitter_context_set_ready(to_gt(dev_priv), true);
> > +
> >  	return ret;
> >  }
> >
> > --
> > 2.26.3
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915: Create a blitter context for GGTT updates

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Create a blitter context for GGTT updates
URL   : https://patchwork.freedesktop.org/series/122748/
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 [1/3] drm/i915: Create a blitter context for GGTT updates

[Patchwork]

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

== Series Details ==

Series: series starting with [1/3] drm/i915: Create a blitter context for GGTT updates
URL   : https://patchwork.freedesktop.org/series/122748/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13549 -> Patchwork_122748v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_122748v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_122748v1, please notify your bug team 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_122748v1/index.html

Participating hosts (41 -> 39)
------------------------------

  Missing    (2): bat-dg2-9 fi-snb-2520m 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@mman:
    - bat-dg2-11:         NOTRUN -> [TIMEOUT][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122748v1/bat-dg2-11/igt@i915_selftest@live@mman.html

  * igt@i915_suspend@basic-s2idle-without-i915:
    - bat-dg2-11:         NOTRUN -> [WARN][2]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122748v1/bat-dg2-11/igt@i915_suspend@basic-s2idle-without-i915.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@gem_contexts:
    - bat-mtlp-6:         [PASS][3] -> [ABORT][4] ([i915#8630])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13549/bat-mtlp-6/igt@i915_selftest@live@gem_contexts.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122748v1/bat-mtlp-6/igt@i915_selftest@live@gem_contexts.html

  * igt@i915_suspend@basic-s3-without-i915:
    - bat-adlp-9:         NOTRUN -> [INCOMPLETE][5] ([i915#7443])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122748v1/bat-adlp-9/igt@i915_suspend@basic-s3-without-i915.html
    - bat-dg2-11:         NOTRUN -> [INCOMPLETE][6] ([i915#4817])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122748v1/bat-dg2-11/igt@i915_suspend@basic-s3-without-i915.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_lrc:
    - bat-adlp-9:         [INCOMPLETE][7] ([i915#4983] / [i915#7913]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13549/bat-adlp-9/igt@i915_selftest@live@gt_lrc.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122748v1/bat-adlp-9/igt@i915_selftest@live@gt_lrc.html
    - bat-dg2-11:         [INCOMPLETE][9] ([i915#7609] / [i915#7913]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13549/bat-dg2-11/igt@i915_selftest@live@gt_lrc.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122748v1/bat-dg2-11/igt@i915_selftest@live@gt_lrc.html

  
  [i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#7443]: https://gitlab.freedesktop.org/drm/intel/issues/7443
  [i915#7609]: https://gitlab.freedesktop.org/drm/intel/issues/7609
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#8630]: https://gitlab.freedesktop.org/drm/intel/issues/8630


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

  * Linux: CI_DRM_13549 -> Patchwork_122748v1

  CI-20190529: 20190529
  CI_DRM_13549: daa7b246575041e069f151cfbc69d07e321bdc01 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7451: 5d48d1fb231f449fe2f80cda14ea7a1ecfda59fa @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_122748v1: daa7b246575041e069f151cfbc69d07e321bdc01 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

60448642fa81 drm/i915: Enable GGTT blitting in MTL
a53ef76b297d drm/i915: Implement GGTT update method with blitter
f224bfd0ab1b drm/i915: Create a blitter context for GGTT updates

== Logs ==

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

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

Re: [Intel-gfx] [PATCH 1/3] drm/i915: Create a blitter context for GGTT updates

[Matt Roper]

On Tue, Aug 22, 2023 at 11:28:57AM -0400, Oak Zeng wrote:
> From: Nirmoy Das <nirmoy.das@intel.com>
> 
> Create a separate blitter context if a platform requires
> GGTT updates using MI_UPDATE_GTT blitter command.
> 
> Subsequent patch will introduce methods to update
> GGTT using this blitter context and MI_UPDATE_GTT blitter
> command.
> 
> v2: Fix a typo in comment. (Oak)
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> Signed-off-by: Oak Zeng <oak.zeng@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine.h       |  4 ++
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c    | 44 +++++++++++++++++++-
>  drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 ++
>  drivers/gpu/drm/i915/gt/intel_gtt.c          |  4 ++
>  drivers/gpu/drm/i915/gt/intel_gtt.h          |  2 +
>  5 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index b58c30ac8ef0..ee36db2fdaa7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -170,6 +170,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
>  #define I915_GEM_HWS_SEQNO		0x40
>  #define I915_GEM_HWS_SEQNO_ADDR		(I915_GEM_HWS_SEQNO * sizeof(u32))
>  #define I915_GEM_HWS_MIGRATE		(0x42 * sizeof(u32))
> +#define I915_GEM_HWS_GGTT_BLIT		0x46
> +#define I915_GEM_HWS_GGTT_BLIT_ADDR	(I915_GEM_HWS_GGTT_BLIT * sizeof(u32))
>  #define I915_GEM_HWS_PXP		0x60
>  #define I915_GEM_HWS_PXP_ADDR		(I915_GEM_HWS_PXP * sizeof(u32))
>  #define I915_GEM_HWS_GSC		0x62
> @@ -356,4 +358,6 @@ u64 intel_clamp_preempt_timeout_ms(struct intel_engine_cs *engine, u64 value);
>  u64 intel_clamp_stop_timeout_ms(struct intel_engine_cs *engine, u64 value);
>  u64 intel_clamp_timeslice_duration_ms(struct intel_engine_cs *engine, u64 value);
>  
> +void intel_engine_blitter_context_set_ready(struct intel_gt *gt, bool ready);
> +bool intel_engine_blitter_context_ready(struct intel_gt *gt);
>  #endif /* _INTEL_RINGBUFFER_H_ */
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index dfb69fc977a0..d8c492a507a4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -27,6 +27,7 @@
>  #include "intel_gt_mcr.h"
>  #include "intel_gt_pm.h"
>  #include "intel_gt_requests.h"
> +#include "intel_gtt.h"
>  #include "intel_lrc.h"
>  #include "intel_lrc_reg.h"
>  #include "intel_reset.h"
> @@ -1419,6 +1420,34 @@ void intel_engine_destroy_pinned_context(struct intel_context *ce)
>  	intel_context_put(ce);
>  }
>  
> +void intel_engine_blitter_context_set_ready(struct intel_gt *gt, bool ready)
> +{
> +	struct intel_engine_cs *engine = gt->engine[BCS0];
> +
> +	if (engine && engine->blitter_context)
> +		atomic_set(&engine->blitter_context_ready, ready ? 1 : 0);
> +}
> +
> +bool intel_engine_blitter_context_ready(struct intel_gt *gt)
> +{
> +	struct intel_engine_cs *engine = gt->engine[BCS0];
> +
> +	if (engine)
> +		return atomic_read(&engine->blitter_context_ready) == 1;
> +
> +	return false;
> +}
> +
> +static struct intel_context *
> +create_ggtt_blitter_context(struct intel_engine_cs *engine)
> +{
> +	static struct lock_class_key kernel;
> +
> +	/* MI_UPDATE_GTT can insert up to 512 PTE entries so get a bigger ring */
> +	return intel_engine_create_pinned_context(engine, engine->gt->vm, SZ_512K,
> +						  I915_GEM_HWS_GGTT_BLIT_ADDR,
> +						  &kernel, "ggtt_blitter_context");
> +}
>  static struct intel_context *
>  create_kernel_context(struct intel_engine_cs *engine)
>  {
> @@ -1442,7 +1471,7 @@ create_kernel_context(struct intel_engine_cs *engine)
>   */
>  static int engine_init_common(struct intel_engine_cs *engine)
>  {
> -	struct intel_context *ce;
> +	struct intel_context *ce, *bce = NULL;
>  	int ret;
>  
>  	engine->set_default_submission(engine);
> @@ -1458,6 +1487,15 @@ static int engine_init_common(struct intel_engine_cs *engine)
>  	ce = create_kernel_context(engine);
>  	if (IS_ERR(ce))
>  		return PTR_ERR(ce);
> +	/*
> +	 * Create a separate pinned context for GGTT update using blitter
> +	 * if a platform require such service.
> +	 */
> +	if (i915_ggtt_require_blitter(engine->i915) && engine->id == BCS0) {
> +		bce = create_ggtt_blitter_context(engine);
> +		if (IS_ERR(bce))
> +			return PTR_ERR(bce);
> +	}
>  
>  	ret = measure_breadcrumb_dw(ce);
>  	if (ret < 0)
> @@ -1465,6 +1503,7 @@ static int engine_init_common(struct intel_engine_cs *engine)
>  
>  	engine->emit_fini_breadcrumb_dw = ret;
>  	engine->kernel_context = ce;
> +	engine->blitter_context = bce;
>  
>  	return 0;
>  
> @@ -1537,6 +1576,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
>  
>  	if (engine->kernel_context)
>  		intel_engine_destroy_pinned_context(engine->kernel_context);
> +	if (engine->blitter_context)
> +		intel_engine_destroy_pinned_context(engine->blitter_context);
> +
>  
>  	GEM_BUG_ON(!llist_empty(&engine->barrier_tasks));
>  	cleanup_status_page(engine);
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index e99a6fa03d45..62095c0d8783 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -415,6 +415,9 @@ struct intel_engine_cs {
>  	struct llist_head barrier_tasks;
>  
>  	struct intel_context *kernel_context; /* pinned */
> +	struct intel_context *blitter_context; /* pinned, only for BCS0 */
> +	/* mark the blitter engine's availability status */
> +	atomic_t blitter_context_ready;
>  
>  	/**
>  	 * pinned_contexts_list: List of pinned contexts. This list is only
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
> index 13944a14ea2d..9c77c97670fe 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
> @@ -21,6 +21,10 @@
>  #include "intel_gt_regs.h"
>  #include "intel_gtt.h"
>  
> +bool i915_ggtt_require_blitter(struct drm_i915_private *i915)
> +{
> +	return IS_METEORLAKE(i915);

Drive-by comment:  this workaround is tied to the Xe_LPM+ media IP, not
to the MTL platform.  Other platforms that re-use Xe_LPM+ IP will also
be affected, whereas MTL platforms that lack media, or integrate a
different media chiplet will not be affected.  So the condition here
should be:

        /* Wa_13010847436 */
        return MEDIA_VER_FULL(i915) == IP_VER(13, 0);

But does this even belong in this patch?  It sounds like patch #3 of the
series is where you intended to hook up this programming to the specific
workaround.


Matt

> +}
>  
>  static bool intel_ggtt_update_needs_vtd_wa(struct drm_i915_private *i915)
>  {
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
> index 4d6296cdbcfd..9710eb031fb2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> @@ -688,4 +688,6 @@ static inline struct sgt_dma {
>  	return (struct sgt_dma){ sg, addr, addr + sg_dma_len(sg) };
>  }
>  
> +bool i915_ggtt_require_blitter(struct drm_i915_private *i915);
> +
>  #endif
> -- 
> 2.26.3
> 

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

Re: [Intel-gfx] [PATCH 1/3] drm/i915: Create a blitter context for GGTT updates

[Zeng, Oak]

Thanks,
Oak

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@intel.com>
> Sent: August 24, 2023 11:52 AM
> To: Zeng, Oak <oak.zeng@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Shyti, Andi <andi.shyti@intel.com>;
> chris.p.wilson@linux.intel.com; Das, Nirmoy <nirmoy.das@intel.com>
> Subject: Re: [Intel-gfx] [PATCH 1/3] drm/i915: Create a blitter context for GGTT
> updates
> 
> On Tue, Aug 22, 2023 at 11:28:57AM -0400, Oak Zeng wrote:
> > From: Nirmoy Das <nirmoy.das@intel.com>
> >
> > Create a separate blitter context if a platform requires
> > GGTT updates using MI_UPDATE_GTT blitter command.
> >
> > Subsequent patch will introduce methods to update
> > GGTT using this blitter context and MI_UPDATE_GTT blitter
> > command.
> >
> > v2: Fix a typo in comment. (Oak)
> >
> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > Signed-off-by: Oak Zeng <oak.zeng@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_engine.h       |  4 ++
> >  drivers/gpu/drm/i915/gt/intel_engine_cs.c    | 44 +++++++++++++++++++-
> >  drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 ++
> >  drivers/gpu/drm/i915/gt/intel_gtt.c          |  4 ++
> >  drivers/gpu/drm/i915/gt/intel_gtt.h          |  2 +
> >  5 files changed, 56 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
> b/drivers/gpu/drm/i915/gt/intel_engine.h
> > index b58c30ac8ef0..ee36db2fdaa7 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> > @@ -170,6 +170,8 @@ intel_write_status_page(struct intel_engine_cs *engine,
> int reg, u32 value)
> >  #define I915_GEM_HWS_SEQNO		0x40
> >  #define I915_GEM_HWS_SEQNO_ADDR		(I915_GEM_HWS_SEQNO
> * sizeof(u32))
> >  #define I915_GEM_HWS_MIGRATE		(0x42 * sizeof(u32))
> > +#define I915_GEM_HWS_GGTT_BLIT		0x46
> > +#define I915_GEM_HWS_GGTT_BLIT_ADDR
> 	(I915_GEM_HWS_GGTT_BLIT * sizeof(u32))
> >  #define I915_GEM_HWS_PXP		0x60
> >  #define I915_GEM_HWS_PXP_ADDR		(I915_GEM_HWS_PXP *
> sizeof(u32))
> >  #define I915_GEM_HWS_GSC		0x62
> > @@ -356,4 +358,6 @@ u64 intel_clamp_preempt_timeout_ms(struct
> intel_engine_cs *engine, u64 value);
> >  u64 intel_clamp_stop_timeout_ms(struct intel_engine_cs *engine, u64 value);
> >  u64 intel_clamp_timeslice_duration_ms(struct intel_engine_cs *engine, u64
> value);
> >
> > +void intel_engine_blitter_context_set_ready(struct intel_gt *gt, bool ready);
> > +bool intel_engine_blitter_context_ready(struct intel_gt *gt);
> >  #endif /* _INTEL_RINGBUFFER_H_ */
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > index dfb69fc977a0..d8c492a507a4 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > @@ -27,6 +27,7 @@
> >  #include "intel_gt_mcr.h"
> >  #include "intel_gt_pm.h"
> >  #include "intel_gt_requests.h"
> > +#include "intel_gtt.h"
> >  #include "intel_lrc.h"
> >  #include "intel_lrc_reg.h"
> >  #include "intel_reset.h"
> > @@ -1419,6 +1420,34 @@ void intel_engine_destroy_pinned_context(struct
> intel_context *ce)
> >  	intel_context_put(ce);
> >  }
> >
> > +void intel_engine_blitter_context_set_ready(struct intel_gt *gt, bool ready)
> > +{
> > +	struct intel_engine_cs *engine = gt->engine[BCS0];
> > +
> > +	if (engine && engine->blitter_context)
> > +		atomic_set(&engine->blitter_context_ready, ready ? 1 : 0);
> > +}
> > +
> > +bool intel_engine_blitter_context_ready(struct intel_gt *gt)
> > +{
> > +	struct intel_engine_cs *engine = gt->engine[BCS0];
> > +
> > +	if (engine)
> > +		return atomic_read(&engine->blitter_context_ready) == 1;
> > +
> > +	return false;
> > +}
> > +
> > +static struct intel_context *
> > +create_ggtt_blitter_context(struct intel_engine_cs *engine)
> > +{
> > +	static struct lock_class_key kernel;
> > +
> > +	/* MI_UPDATE_GTT can insert up to 512 PTE entries so get a bigger ring
> */
> > +	return intel_engine_create_pinned_context(engine, engine->gt->vm,
> SZ_512K,
> > +
> I915_GEM_HWS_GGTT_BLIT_ADDR,
> > +						  &kernel, "ggtt_blitter_context");
> > +}
> >  static struct intel_context *
> >  create_kernel_context(struct intel_engine_cs *engine)
> >  {
> > @@ -1442,7 +1471,7 @@ create_kernel_context(struct intel_engine_cs
> *engine)
> >   */
> >  static int engine_init_common(struct intel_engine_cs *engine)
> >  {
> > -	struct intel_context *ce;
> > +	struct intel_context *ce, *bce = NULL;
> >  	int ret;
> >
> >  	engine->set_default_submission(engine);
> > @@ -1458,6 +1487,15 @@ static int engine_init_common(struct
> intel_engine_cs *engine)
> >  	ce = create_kernel_context(engine);
> >  	if (IS_ERR(ce))
> >  		return PTR_ERR(ce);
> > +	/*
> > +	 * Create a separate pinned context for GGTT update using blitter
> > +	 * if a platform require such service.
> > +	 */
> > +	if (i915_ggtt_require_blitter(engine->i915) && engine->id == BCS0) {
> > +		bce = create_ggtt_blitter_context(engine);
> > +		if (IS_ERR(bce))
> > +			return PTR_ERR(bce);
> > +	}
> >
> >  	ret = measure_breadcrumb_dw(ce);
> >  	if (ret < 0)
> > @@ -1465,6 +1503,7 @@ static int engine_init_common(struct intel_engine_cs
> *engine)
> >
> >  	engine->emit_fini_breadcrumb_dw = ret;
> >  	engine->kernel_context = ce;
> > +	engine->blitter_context = bce;
> >
> >  	return 0;
> >
> > @@ -1537,6 +1576,9 @@ void intel_engine_cleanup_common(struct
> intel_engine_cs *engine)
> >
> >  	if (engine->kernel_context)
> >  		intel_engine_destroy_pinned_context(engine->kernel_context);
> > +	if (engine->blitter_context)
> > +		intel_engine_destroy_pinned_context(engine->blitter_context);
> > +
> >
> >  	GEM_BUG_ON(!llist_empty(&engine->barrier_tasks));
> >  	cleanup_status_page(engine);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > index e99a6fa03d45..62095c0d8783 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > @@ -415,6 +415,9 @@ struct intel_engine_cs {
> >  	struct llist_head barrier_tasks;
> >
> >  	struct intel_context *kernel_context; /* pinned */
> > +	struct intel_context *blitter_context; /* pinned, only for BCS0 */
> > +	/* mark the blitter engine's availability status */
> > +	atomic_t blitter_context_ready;
> >
> >  	/**
> >  	 * pinned_contexts_list: List of pinned contexts. This list is only
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
> b/drivers/gpu/drm/i915/gt/intel_gtt.c
> > index 13944a14ea2d..9c77c97670fe 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
> > @@ -21,6 +21,10 @@
> >  #include "intel_gt_regs.h"
> >  #include "intel_gtt.h"
> >
> > +bool i915_ggtt_require_blitter(struct drm_i915_private *i915)
> > +{
> > +	return IS_METEORLAKE(i915);
> 
> Drive-by comment:  this workaround is tied to the Xe_LPM+ media IP, not
> to the MTL platform.  Other platforms that re-use Xe_LPM+ IP will also
> be affected, whereas MTL platforms that lack media, or integrate a
> different media chiplet will not be affected.  So the condition here
> should be:
> 
>         /* Wa_13010847436 */
>         return MEDIA_VER_FULL(i915) == IP_VER(13, 0);

Issue was observed on LNL A0 (fixed in A1), MTL. 

Not sure whether it is fixed on ARL or not.

For LNL A0, there might be a different wa so this software wa is not needed. Double confirming.

For now let's only enable MTL. 

> 
> But does this even belong in this patch?  It sounds like patch #3 of the
> series is where you intended to hook up this programming to the specific
> workaround.

This function is called from patch 2. So will keep it to patch 1.

Oak

> 
> 
> Matt
> 
> > +}
> >
> >  static bool intel_ggtt_update_needs_vtd_wa(struct drm_i915_private *i915)
> >  {
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
> b/drivers/gpu/drm/i915/gt/intel_gtt.h
> > index 4d6296cdbcfd..9710eb031fb2 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> > @@ -688,4 +688,6 @@ static inline struct sgt_dma {
> >  	return (struct sgt_dma){ sg, addr, addr + sg_dma_len(sg) };
> >  }
> >
> > +bool i915_ggtt_require_blitter(struct drm_i915_private *i915);
> > +
> >  #endif
> > --
> > 2.26.3
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation

Re: [Intel-gfx] [PATCH 1/3] drm/i915: Create a blitter context for GGTT updates

[Zeng, Oak]

Thanks,
Oak

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Zeng,
> Oak
> Sent: August 24, 2023 4:38 PM
> To: Roper, Matthew D <matthew.d.roper@intel.com>
> Cc: chris.p.wilson@linux.intel.com; intel-gfx@lists.freedesktop.org; Shyti, Andi
> <andi.shyti@intel.com>; Das, Nirmoy <nirmoy.das@intel.com>
> Subject: Re: [Intel-gfx] [PATCH 1/3] drm/i915: Create a blitter context for GGTT
> updates
> 
> 
> 
> Thanks,
> Oak
> 
> > -----Original Message-----
> > From: Roper, Matthew D <matthew.d.roper@intel.com>
> > Sent: August 24, 2023 11:52 AM
> > To: Zeng, Oak <oak.zeng@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Shyti, Andi <andi.shyti@intel.com>;
> > chris.p.wilson@linux.intel.com; Das, Nirmoy <nirmoy.das@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH 1/3] drm/i915: Create a blitter context for GGTT
> > updates
> >
> > On Tue, Aug 22, 2023 at 11:28:57AM -0400, Oak Zeng wrote:
> > > From: Nirmoy Das <nirmoy.das@intel.com>
> > >
> > > Create a separate blitter context if a platform requires
> > > GGTT updates using MI_UPDATE_GTT blitter command.
> > >
> > > Subsequent patch will introduce methods to update
> > > GGTT using this blitter context and MI_UPDATE_GTT blitter
> > > command.
> > >
> > > v2: Fix a typo in comment. (Oak)
> > >
> > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > > Signed-off-by: Oak Zeng <oak.zeng@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_engine.h       |  4 ++
> > >  drivers/gpu/drm/i915/gt/intel_engine_cs.c    | 44 +++++++++++++++++++-
> > >  drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 ++
> > >  drivers/gpu/drm/i915/gt/intel_gtt.c          |  4 ++
> > >  drivers/gpu/drm/i915/gt/intel_gtt.h          |  2 +
> > >  5 files changed, 56 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
> > b/drivers/gpu/drm/i915/gt/intel_engine.h
> > > index b58c30ac8ef0..ee36db2fdaa7 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> > > @@ -170,6 +170,8 @@ intel_write_status_page(struct intel_engine_cs
> *engine,
> > int reg, u32 value)
> > >  #define I915_GEM_HWS_SEQNO		0x40
> > >  #define I915_GEM_HWS_SEQNO_ADDR		(I915_GEM_HWS_SEQNO
> > * sizeof(u32))
> > >  #define I915_GEM_HWS_MIGRATE		(0x42 * sizeof(u32))
> > > +#define I915_GEM_HWS_GGTT_BLIT		0x46
> > > +#define I915_GEM_HWS_GGTT_BLIT_ADDR
> > 	(I915_GEM_HWS_GGTT_BLIT * sizeof(u32))
> > >  #define I915_GEM_HWS_PXP		0x60
> > >  #define I915_GEM_HWS_PXP_ADDR		(I915_GEM_HWS_PXP *
> > sizeof(u32))
> > >  #define I915_GEM_HWS_GSC		0x62
> > > @@ -356,4 +358,6 @@ u64 intel_clamp_preempt_timeout_ms(struct
> > intel_engine_cs *engine, u64 value);
> > >  u64 intel_clamp_stop_timeout_ms(struct intel_engine_cs *engine, u64
> value);
> > >  u64 intel_clamp_timeslice_duration_ms(struct intel_engine_cs *engine, u64
> > value);
> > >
> > > +void intel_engine_blitter_context_set_ready(struct intel_gt *gt, bool ready);
> > > +bool intel_engine_blitter_context_ready(struct intel_gt *gt);
> > >  #endif /* _INTEL_RINGBUFFER_H_ */
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > index dfb69fc977a0..d8c492a507a4 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > @@ -27,6 +27,7 @@
> > >  #include "intel_gt_mcr.h"
> > >  #include "intel_gt_pm.h"
> > >  #include "intel_gt_requests.h"
> > > +#include "intel_gtt.h"
> > >  #include "intel_lrc.h"
> > >  #include "intel_lrc_reg.h"
> > >  #include "intel_reset.h"
> > > @@ -1419,6 +1420,34 @@ void intel_engine_destroy_pinned_context(struct
> > intel_context *ce)
> > >  	intel_context_put(ce);
> > >  }
> > >
> > > +void intel_engine_blitter_context_set_ready(struct intel_gt *gt, bool ready)
> > > +{
> > > +	struct intel_engine_cs *engine = gt->engine[BCS0];
> > > +
> > > +	if (engine && engine->blitter_context)
> > > +		atomic_set(&engine->blitter_context_ready, ready ? 1 : 0);
> > > +}
> > > +
> > > +bool intel_engine_blitter_context_ready(struct intel_gt *gt)
> > > +{
> > > +	struct intel_engine_cs *engine = gt->engine[BCS0];
> > > +
> > > +	if (engine)
> > > +		return atomic_read(&engine->blitter_context_ready) == 1;
> > > +
> > > +	return false;
> > > +}
> > > +
> > > +static struct intel_context *
> > > +create_ggtt_blitter_context(struct intel_engine_cs *engine)
> > > +{
> > > +	static struct lock_class_key kernel;
> > > +
> > > +	/* MI_UPDATE_GTT can insert up to 512 PTE entries so get a bigger ring
> > */
> > > +	return intel_engine_create_pinned_context(engine, engine->gt->vm,
> > SZ_512K,
> > > +
> > I915_GEM_HWS_GGTT_BLIT_ADDR,
> > > +						  &kernel, "ggtt_blitter_context");
> > > +}
> > >  static struct intel_context *
> > >  create_kernel_context(struct intel_engine_cs *engine)
> > >  {
> > > @@ -1442,7 +1471,7 @@ create_kernel_context(struct intel_engine_cs
> > *engine)
> > >   */
> > >  static int engine_init_common(struct intel_engine_cs *engine)
> > >  {
> > > -	struct intel_context *ce;
> > > +	struct intel_context *ce, *bce = NULL;
> > >  	int ret;
> > >
> > >  	engine->set_default_submission(engine);
> > > @@ -1458,6 +1487,15 @@ static int engine_init_common(struct
> > intel_engine_cs *engine)
> > >  	ce = create_kernel_context(engine);
> > >  	if (IS_ERR(ce))
> > >  		return PTR_ERR(ce);
> > > +	/*
> > > +	 * Create a separate pinned context for GGTT update using blitter
> > > +	 * if a platform require such service.
> > > +	 */
> > > +	if (i915_ggtt_require_blitter(engine->i915) && engine->id == BCS0) {
> > > +		bce = create_ggtt_blitter_context(engine);
> > > +		if (IS_ERR(bce))
> > > +			return PTR_ERR(bce);
> > > +	}
> > >
> > >  	ret = measure_breadcrumb_dw(ce);
> > >  	if (ret < 0)
> > > @@ -1465,6 +1503,7 @@ static int engine_init_common(struct
> intel_engine_cs
> > *engine)
> > >
> > >  	engine->emit_fini_breadcrumb_dw = ret;
> > >  	engine->kernel_context = ce;
> > > +	engine->blitter_context = bce;
> > >
> > >  	return 0;
> > >
> > > @@ -1537,6 +1576,9 @@ void intel_engine_cleanup_common(struct
> > intel_engine_cs *engine)
> > >
> > >  	if (engine->kernel_context)
> > >  		intel_engine_destroy_pinned_context(engine->kernel_context);
> > > +	if (engine->blitter_context)
> > > +		intel_engine_destroy_pinned_context(engine->blitter_context);
> > > +
> > >
> > >  	GEM_BUG_ON(!llist_empty(&engine->barrier_tasks));
> > >  	cleanup_status_page(engine);
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > index e99a6fa03d45..62095c0d8783 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > @@ -415,6 +415,9 @@ struct intel_engine_cs {
> > >  	struct llist_head barrier_tasks;
> > >
> > >  	struct intel_context *kernel_context; /* pinned */
> > > +	struct intel_context *blitter_context; /* pinned, only for BCS0 */
> > > +	/* mark the blitter engine's availability status */
> > > +	atomic_t blitter_context_ready;
> > >
> > >  	/**
> > >  	 * pinned_contexts_list: List of pinned contexts. This list is only
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
> > b/drivers/gpu/drm/i915/gt/intel_gtt.c
> > > index 13944a14ea2d..9c77c97670fe 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
> > > @@ -21,6 +21,10 @@
> > >  #include "intel_gt_regs.h"
> > >  #include "intel_gtt.h"
> > >
> > > +bool i915_ggtt_require_blitter(struct drm_i915_private *i915)
> > > +{
> > > +	return IS_METEORLAKE(i915);
> >
> > Drive-by comment:  this workaround is tied to the Xe_LPM+ media IP, not
> > to the MTL platform.  Other platforms that re-use Xe_LPM+ IP will also
> > be affected, whereas MTL platforms that lack media, or integrate a
> > different media chiplet will not be affected.  So the condition here
> > should be:
> >
> >         /* Wa_13010847436 */
> >         return MEDIA_VER_FULL(i915) == IP_VER(13, 0);
> 
> Issue was observed on LNL A0 (fixed in A1), MTL.
> 
> Not sure whether it is fixed on ARL or not.
> 
> For LNL A0, there might be a different wa so this software wa is not needed.
> Double confirming.
> 
> For now let's only enable MTL.


I just confirmed this HW bug is also on ARL. But ARL code is not ready yet. Will enable it once it is ready.

Oak
> 
> >
> > But does this even belong in this patch?  It sounds like patch #3 of the
> > series is where you intended to hook up this programming to the specific
> > workaround.
> 
> This function is called from patch 2. So will keep it to patch 1.
> 
> Oak
> 
> >
> >
> > Matt
> >
> > > +}
> > >
> > >  static bool intel_ggtt_update_needs_vtd_wa(struct drm_i915_private *i915)
> > >  {
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
> > b/drivers/gpu/drm/i915/gt/intel_gtt.h
> > > index 4d6296cdbcfd..9710eb031fb2 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> > > @@ -688,4 +688,6 @@ static inline struct sgt_dma {
> > >  	return (struct sgt_dma){ sg, addr, addr + sg_dma_len(sg) };
> > >  }
> > >
> > > +bool i915_ggtt_require_blitter(struct drm_i915_private *i915);
> > > +
> > >  #endif
> > > --
> > > 2.26.3
> > >
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation

[Intel-gfx] [PATCH 1/3] drm/i915: Create a bind context for GGTT updates

[Oak Zeng]

From: Nirmoy Das <nirmoy.das@intel.com>

Create a separate blitter context if a platform requires
GGTT updates using MI_UPDATE_GTT blitter command.

Subsequent patch will introduce methods to update
GGTT using this blitter context and MI_UPDATE_GTT blitter
command.

v2: Fix a typo in comment. (Oak)
v3: s/blitter_context/bind_context/g (Chris)

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
Signed-off-by: Oak Zeng <oak.zeng@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine.h       |  4 ++
 drivers/gpu/drm/i915/gt/intel_engine_cs.c    | 44 +++++++++++++++++++-
 drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 ++
 drivers/gpu/drm/i915/gt/intel_gtt.c          |  4 ++
 drivers/gpu/drm/i915/gt/intel_gtt.h          |  2 +
 5 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index b58c30ac8ef0..c3b2ac8bad08 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -170,6 +170,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
 #define I915_GEM_HWS_SEQNO		0x40
 #define I915_GEM_HWS_SEQNO_ADDR		(I915_GEM_HWS_SEQNO * sizeof(u32))
 #define I915_GEM_HWS_MIGRATE		(0x42 * sizeof(u32))
+#define I915_GEM_HWS_GGTT_BLIT		0x46
+#define I915_GEM_HWS_GGTT_BLIT_ADDR	(I915_GEM_HWS_GGTT_BLIT * sizeof(u32))
 #define I915_GEM_HWS_PXP		0x60
 #define I915_GEM_HWS_PXP_ADDR		(I915_GEM_HWS_PXP * sizeof(u32))
 #define I915_GEM_HWS_GSC		0x62
@@ -356,4 +358,6 @@ u64 intel_clamp_preempt_timeout_ms(struct intel_engine_cs *engine, u64 value);
 u64 intel_clamp_stop_timeout_ms(struct intel_engine_cs *engine, u64 value);
 u64 intel_clamp_timeslice_duration_ms(struct intel_engine_cs *engine, u64 value);
 
+void intel_engine_bind_context_set_ready(struct intel_gt *gt, bool ready);
+bool intel_engine_bind_context_ready(struct intel_gt *gt);
 #endif /* _INTEL_RINGBUFFER_H_ */
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index dfb69fc977a0..d316f9eb914a 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -27,6 +27,7 @@
 #include "intel_gt_mcr.h"
 #include "intel_gt_pm.h"
 #include "intel_gt_requests.h"
+#include "intel_gtt.h"
 #include "intel_lrc.h"
 #include "intel_lrc_reg.h"
 #include "intel_reset.h"
@@ -1419,6 +1420,34 @@ void intel_engine_destroy_pinned_context(struct intel_context *ce)
 	intel_context_put(ce);
 }
 
+void intel_engine_bind_context_set_ready(struct intel_gt *gt, bool ready)
+{
+	struct intel_engine_cs *engine = gt->engine[BCS0];
+
+	if (engine && engine->bind_context)
+		atomic_set(&engine->bind_context_ready, ready ? 1 : 0);
+}
+
+bool intel_engine_bind_context_ready(struct intel_gt *gt)
+{
+	struct intel_engine_cs *engine = gt->engine[BCS0];
+
+	if (engine)
+		return atomic_read(&engine->bind_context_ready) == 1;
+
+	return false;
+}
+
+static struct intel_context *
+create_ggtt_bind_context(struct intel_engine_cs *engine)
+{
+	static struct lock_class_key kernel;
+
+	/* MI_UPDATE_GTT can insert up to 512 PTE entries so get a bigger ring */
+	return intel_engine_create_pinned_context(engine, engine->gt->vm, SZ_512K,
+						  I915_GEM_HWS_GGTT_BLIT_ADDR,
+						  &kernel, "ggtt_bind_context");
+}
 static struct intel_context *
 create_kernel_context(struct intel_engine_cs *engine)
 {
@@ -1442,7 +1471,7 @@ create_kernel_context(struct intel_engine_cs *engine)
  */
 static int engine_init_common(struct intel_engine_cs *engine)
 {
-	struct intel_context *ce;
+	struct intel_context *ce, *bce = NULL;
 	int ret;
 
 	engine->set_default_submission(engine);
@@ -1458,6 +1487,15 @@ static int engine_init_common(struct intel_engine_cs *engine)
 	ce = create_kernel_context(engine);
 	if (IS_ERR(ce))
 		return PTR_ERR(ce);
+	/*
+	 * Create a separate pinned context for GGTT update using blitter
+	 * if a platform require such service.
+	 */
+	if (i915_ggtt_require_blitter(engine->i915) && engine->id == BCS0) {
+		bce = create_ggtt_bind_context(engine);
+		if (IS_ERR(bce))
+			return PTR_ERR(bce);
+	}
 
 	ret = measure_breadcrumb_dw(ce);
 	if (ret < 0)
@@ -1465,6 +1503,7 @@ static int engine_init_common(struct intel_engine_cs *engine)
 
 	engine->emit_fini_breadcrumb_dw = ret;
 	engine->kernel_context = ce;
+	engine->bind_context = bce;
 
 	return 0;
 
@@ -1537,6 +1576,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 
 	if (engine->kernel_context)
 		intel_engine_destroy_pinned_context(engine->kernel_context);
+	if (engine->bind_context)
+		intel_engine_destroy_pinned_context(engine->bind_context);
+
 
 	GEM_BUG_ON(!llist_empty(&engine->barrier_tasks));
 	cleanup_status_page(engine);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index e99a6fa03d45..a1fea76698f3 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -415,6 +415,9 @@ struct intel_engine_cs {
 	struct llist_head barrier_tasks;
 
 	struct intel_context *kernel_context; /* pinned */
+	struct intel_context *bind_context; /* pinned, only for BCS0 */
+	/* mark the blitter engine's availability status */
+	atomic_t bind_context_ready;
 
 	/**
 	 * pinned_contexts_list: List of pinned contexts. This list is only
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
index 13944a14ea2d..9c77c97670fe 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -21,6 +21,10 @@
 #include "intel_gt_regs.h"
 #include "intel_gtt.h"
 
+bool i915_ggtt_require_blitter(struct drm_i915_private *i915)
+{
+	return IS_METEORLAKE(i915);
+}
 
 static bool intel_ggtt_update_needs_vtd_wa(struct drm_i915_private *i915)
 {
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
index 4d6296cdbcfd..9710eb031fb2 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -688,4 +688,6 @@ static inline struct sgt_dma {
 	return (struct sgt_dma){ sg, addr, addr + sg_dma_len(sg) };
 }
 
+bool i915_ggtt_require_blitter(struct drm_i915_private *i915);
+
 #endif
-- 
2.26.3

[Intel-gfx] [PATCH 3/3] drm/i915: Enable GGTT blitting in MTL

[Oak Zeng]

From: Nirmoy Das <nirmoy.das@intel.com>

MTL can hang because of a HW bug while parallel reading/writing
from/to LMEM/GTTMMADR BAR so try to reduce GGTT update
related pci transactions with blitter command as recommended
for Wa_13010847436 and Wa_14019519902.

To issue blitter commands, the driver must be primed to receive
requests. Maintain blitter-based GGTT update disablement until driver
probing completes. Moreover, implement a temporary disablement
of blitter prior to entering suspend, followed by re-enablement
post-resume. This is acceptable as those transition periods are
mostly single threaded.

v2: Disable GGTT blitter prior to runtime suspend and re-enable
after runtime resume. (Oak)
v3: s/blitter_context/bind_context/g (Chris)

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
Signed-off-by: Oak Zeng <oak.zeng@intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index f8dbee7a5af7..26521de190a7 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -815,6 +815,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	i915_welcome_messages(i915);
 
 	i915->do_release = true;
+	intel_engine_bind_context_set_ready(to_gt(i915), true);
 
 	return 0;
 
@@ -855,6 +856,7 @@ void i915_driver_remove(struct drm_i915_private *i915)
 {
 	intel_wakeref_t wakeref;
 
+	intel_engine_bind_context_set_ready(to_gt(i915), false);
 	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
 
 	i915_driver_unregister(i915);
@@ -1077,6 +1079,8 @@ static int i915_drm_suspend(struct drm_device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
 	pci_power_t opregion_target_state;
 
+	intel_engine_bind_context_set_ready(to_gt(dev_priv), false);
+
 	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
 
 	/* We do a lot of poking in a lot of registers, make sure they work
@@ -1264,6 +1268,7 @@ static int i915_drm_resume(struct drm_device *dev)
 	intel_gvt_resume(dev_priv);
 
 	enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
+	intel_engine_bind_context_set_ready(to_gt(dev_priv), true);
 
 	return 0;
 }
@@ -1515,6 +1520,7 @@ static int intel_runtime_suspend(struct device *kdev)
 	if (drm_WARN_ON_ONCE(&dev_priv->drm, !HAS_RUNTIME_PM(dev_priv)))
 		return -ENODEV;
 
+	intel_engine_bind_context_set_ready(to_gt(dev_priv), false);
 	drm_dbg(&dev_priv->drm, "Suspending device\n");
 
 	disable_rpm_wakeref_asserts(rpm);
@@ -1669,6 +1675,8 @@ static int intel_runtime_resume(struct device *kdev)
 	else
 		drm_dbg(&dev_priv->drm, "Device resumed\n");
 
+	intel_engine_bind_context_set_ready(to_gt(dev_priv), true);
+
 	return ret;
 }
 
-- 
2.26.3

[Intel-gfx] [PATCH 1/3] drm/i915: Create a blitter context for GGTT updates

[Andi Shyti]

From: Nirmoy Das <nirmoy.das@intel.com>

Create a separate blitter context if a platform requires
GGTT updates using MI_UPDATE_GTT blitter command.

Subsequent patch will introduce methods to update
GGTT using this blitter context and MI_UPDATE_GTT blitter
command.

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine.h       |  4 ++
 drivers/gpu/drm/i915/gt/intel_engine_cs.c    | 44 +++++++++++++++++++-
 drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 ++
 drivers/gpu/drm/i915/gt/intel_gtt.c          |  4 ++
 drivers/gpu/drm/i915/gt/intel_gtt.h          |  2 +
 5 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index b58c30ac8ef02..ee36db2fdaa7c 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -170,6 +170,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
 #define I915_GEM_HWS_SEQNO		0x40
 #define I915_GEM_HWS_SEQNO_ADDR		(I915_GEM_HWS_SEQNO * sizeof(u32))
 #define I915_GEM_HWS_MIGRATE		(0x42 * sizeof(u32))
+#define I915_GEM_HWS_GGTT_BLIT		0x46
+#define I915_GEM_HWS_GGTT_BLIT_ADDR	(I915_GEM_HWS_GGTT_BLIT * sizeof(u32))
 #define I915_GEM_HWS_PXP		0x60
 #define I915_GEM_HWS_PXP_ADDR		(I915_GEM_HWS_PXP * sizeof(u32))
 #define I915_GEM_HWS_GSC		0x62
@@ -356,4 +358,6 @@ u64 intel_clamp_preempt_timeout_ms(struct intel_engine_cs *engine, u64 value);
 u64 intel_clamp_stop_timeout_ms(struct intel_engine_cs *engine, u64 value);
 u64 intel_clamp_timeslice_duration_ms(struct intel_engine_cs *engine, u64 value);
 
+void intel_engine_blitter_context_set_ready(struct intel_gt *gt, bool ready);
+bool intel_engine_blitter_context_ready(struct intel_gt *gt);
 #endif /* _INTEL_RINGBUFFER_H_ */
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index ee15486fed0da..9871ee5ab754b 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -27,6 +27,7 @@
 #include "intel_gt_mcr.h"
 #include "intel_gt_pm.h"
 #include "intel_gt_requests.h"
+#include "intel_gtt.h"
 #include "intel_lrc.h"
 #include "intel_lrc_reg.h"
 #include "intel_reset.h"
@@ -1419,6 +1420,34 @@ void intel_engine_destroy_pinned_context(struct intel_context *ce)
 	intel_context_put(ce);
 }
 
+void intel_engine_blitter_context_set_ready(struct intel_gt *gt, bool ready)
+{
+	struct intel_engine_cs *engine = gt->engine[BCS0];
+
+	if (engine && engine->blitter_context)
+		atomic_set(&engine->blitter_context_ready, ready ? 1 : 0);
+}
+
+bool intel_engine_blitter_context_ready(struct intel_gt *gt)
+{
+	struct intel_engine_cs *engine = gt->engine[BCS0];
+
+	if (engine)
+		return atomic_read(&engine->blitter_context_ready) == 1;
+
+	return false;
+}
+
+static struct intel_context *
+create_ggtt_blitter_context(struct intel_engine_cs *engine)
+{
+	static struct lock_class_key kernel;
+
+	/* MI_UPDATE_GTT can insert upto 512 PTE entries so get a bigger ring */
+	return intel_engine_create_pinned_context(engine, engine->gt->vm, SZ_512K,
+						  I915_GEM_HWS_GGTT_BLIT_ADDR,
+						  &kernel, "ggtt_blitter_context");
+}
 static struct intel_context *
 create_kernel_context(struct intel_engine_cs *engine)
 {
@@ -1442,7 +1471,7 @@ create_kernel_context(struct intel_engine_cs *engine)
  */
 static int engine_init_common(struct intel_engine_cs *engine)
 {
-	struct intel_context *ce;
+	struct intel_context *ce, *bce = NULL;
 	int ret;
 
 	engine->set_default_submission(engine);
@@ -1458,6 +1487,15 @@ static int engine_init_common(struct intel_engine_cs *engine)
 	ce = create_kernel_context(engine);
 	if (IS_ERR(ce))
 		return PTR_ERR(ce);
+	/*
+	 * Create a separate pinned context for GGTT update using blitter
+	 * if a platform require such service.
+	 */
+	if (i915_ggtt_require_blitter(engine->i915) && engine->id == BCS0) {
+		bce = create_ggtt_blitter_context(engine);
+		if (IS_ERR(bce))
+			return PTR_ERR(bce);
+	}
 
 	ret = measure_breadcrumb_dw(ce);
 	if (ret < 0)
@@ -1465,6 +1503,7 @@ static int engine_init_common(struct intel_engine_cs *engine)
 
 	engine->emit_fini_breadcrumb_dw = ret;
 	engine->kernel_context = ce;
+	engine->blitter_context = bce;
 
 	return 0;
 
@@ -1537,6 +1576,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 
 	if (engine->kernel_context)
 		intel_engine_destroy_pinned_context(engine->kernel_context);
+	if (engine->blitter_context)
+		intel_engine_destroy_pinned_context(engine->blitter_context);
+
 
 	GEM_BUG_ON(!llist_empty(&engine->barrier_tasks));
 	cleanup_status_page(engine);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index e99a6fa03d453..62095c0d8783d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -415,6 +415,9 @@ struct intel_engine_cs {
 	struct llist_head barrier_tasks;
 
 	struct intel_context *kernel_context; /* pinned */
+	struct intel_context *blitter_context; /* pinned, only for BCS0 */
+	/* mark the blitter engine's availability status */
+	atomic_t blitter_context_ready;
 
 	/**
 	 * pinned_contexts_list: List of pinned contexts. This list is only
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
index 13944a14ea2d1..9c77c97670fea 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -21,6 +21,10 @@
 #include "intel_gt_regs.h"
 #include "intel_gtt.h"
 
+bool i915_ggtt_require_blitter(struct drm_i915_private *i915)
+{
+	return IS_METEORLAKE(i915);
+}
 
 static bool intel_ggtt_update_needs_vtd_wa(struct drm_i915_private *i915)
 {
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
index 4d6296cdbcfdd..9710eb031fb2c 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -688,4 +688,6 @@ static inline struct sgt_dma {
 	return (struct sgt_dma){ sg, addr, addr + sg_dma_len(sg) };
 }
 
+bool i915_ggtt_require_blitter(struct drm_i915_private *i915);
+
 #endif
-- 
2.40.1

[Intel-gfx] [PATCH 3/3] drm/i915: Enable GGTT blitting in MTL

[Andi Shyti]

From: Nirmoy Das <nirmoy.das@intel.com>

MTL can hang because of a HW bug while parallel reading/writing
from/to LMEM/GTTMMADR BAR so try to reduce GGTT update
related pci transactions with blitter command as recommended
for Wa_22018444074.

To issue blitter commands, the driver must be primed to receive
requests. Maintain blitter-based GGTT update disablement until driver
probing completes. Moreover, implement a temporary disablement
of blitter prior to entering suspend, followed by re-enablement
post-resume. This is acceptable as those transition periods are
mostly single threaded.

v2: Disable GGTT blitter prior to runtime suspend and re-enable
after runtime resume.

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
Signed-off-by: Oak Zeng <oak.zeng@intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index ec4d26b3c17cc..a893a5e8496ef 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -812,6 +812,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	i915_welcome_messages(i915);
 
 	i915->do_release = true;
+	intel_engine_blitter_context_set_ready(to_gt(i915), true);
 
 	return 0;
 
@@ -852,6 +853,7 @@ void i915_driver_remove(struct drm_i915_private *i915)
 {
 	intel_wakeref_t wakeref;
 
+	intel_engine_blitter_context_set_ready(to_gt(i915), false);
 	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
 
 	i915_driver_unregister(i915);
@@ -1074,6 +1076,8 @@ static int i915_drm_suspend(struct drm_device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
 	pci_power_t opregion_target_state;
 
+	intel_engine_blitter_context_set_ready(to_gt(dev_priv), false);
+
 	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
 
 	/* We do a lot of poking in a lot of registers, make sure they work
@@ -1261,6 +1265,7 @@ static int i915_drm_resume(struct drm_device *dev)
 	intel_gvt_resume(dev_priv);
 
 	enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
+	intel_engine_blitter_context_set_ready(to_gt(dev_priv), true);
 
 	return 0;
 }
@@ -1512,6 +1517,7 @@ static int intel_runtime_suspend(struct device *kdev)
 	if (drm_WARN_ON_ONCE(&dev_priv->drm, !HAS_RUNTIME_PM(dev_priv)))
 		return -ENODEV;
 
+	intel_engine_blitter_context_set_ready(to_gt(dev_priv), false);
 	drm_dbg(&dev_priv->drm, "Suspending device\n");
 
 	disable_rpm_wakeref_asserts(rpm);
@@ -1666,6 +1672,8 @@ static int intel_runtime_resume(struct device *kdev)
 	else
 		drm_dbg(&dev_priv->drm, "Device resumed\n");
 
+	intel_engine_blitter_context_set_ready(to_gt(dev_priv), true);
+
 	return ret;
 }
 
-- 
2.40.1