[PATCH 0/6] Call uC functions from GT ones

[PATCH 0/6] Call uC functions from GT ones

[Daniele Ceraolo Spurio]

The plan is to completely hide intel_uc under intel_gt and this series
starts in that direction by moving the init early and suspend/resume
functions.
The _init and init_hw have for now been skipped as they should be
covered when gt_init_hw is introduced; however, the init_hw path has
been simplified a bit by moving the wopcm init call inside the uc one.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>

Daniele Ceraolo Spurio (6):
  drm/i915/uc: move uC WOPCM setup in uc_init_hw
  drm/i915: move gt_cleanup_early out of gem_cleanup_early
  drm/i915/uc: move uc early functions inside the gt ones
  drm/i915/gt: introduce intel_gt_runtime_suspend/resume
  drm/i915/uc: move uc_resume under gt_resume
  drm/i915/gt: introduce intel_gt_suspend

 drivers/gpu/drm/i915/gem/i915_gem_pm.c | 11 +---
 drivers/gpu/drm/i915/gt/intel_gt.c     |  2 +
 drivers/gpu/drm/i915/gt/intel_gt_pm.c  | 28 ++++++++++
 drivers/gpu/drm/i915/gt/intel_gt_pm.h  |  3 ++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c  | 72 +++++++++++++++++++++++++-
 drivers/gpu/drm/i915/gt/uc/intel_uc.h  |  2 +
 drivers/gpu/drm/i915/i915_drv.c        | 21 ++++----
 drivers/gpu/drm/i915/i915_gem.c        | 10 +---
 drivers/gpu/drm/i915/intel_wopcm.c     | 68 ------------------------
 drivers/gpu/drm/i915/intel_wopcm.h     |  3 --
 10 files changed, 116 insertions(+), 104 deletions(-)

-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/6] drm/i915/uc: move uC WOPCM setup in uc_init_hw

[Daniele Ceraolo Spurio]

The register we write are not WOPCM regs but uC ones related to how
GuC and HuC are going to use the WOPCM, so it makes logical sense
for them to be programmed as part of uc_init_hw. The wopcm map on the
other side is not uC-specific (although that is our main use-case), so
keep that separate.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc.c | 62 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c       |  8 +---
 drivers/gpu/drm/i915/intel_wopcm.c    | 68 ---------------------------
 drivers/gpu/drm/i915/intel_wopcm.h    |  3 --
 4 files changed, 63 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index fafa9be1e12a..2f71f3704c46 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -390,6 +390,63 @@ void intel_uc_sanitize(struct intel_uc *uc)
 	__uc_sanitize(uc);
 }
 
+static int
+write_and_verify(struct intel_gt *gt,
+		 i915_reg_t reg, u32 val, u32 mask, u32 locked_bit)
+{
+	struct intel_uncore *uncore = gt->uncore;
+	u32 reg_val;
+
+	GEM_BUG_ON(val & ~mask);
+
+	intel_uncore_write(uncore, reg, val);
+
+	reg_val = intel_uncore_read(uncore, reg);
+
+	return (reg_val & mask) != (val | locked_bit) ? -EIO : 0;
+}
+
+/* Initialize and verify the uC regs related to uC positioning in WOPCM */
+int uc_wopcm_init_hw(struct intel_uc *uc)
+{
+	struct intel_gt *gt = uc_to_gt(uc);
+	struct intel_wopcm *wopcm = &gt->i915->wopcm;
+	struct intel_uncore *uncore = gt->uncore;
+	u32 huc_agent;
+	u32 mask;
+	int err;
+
+	GEM_BUG_ON(!HAS_GT_UC(gt->i915));
+	GEM_BUG_ON(!intel_uc_is_using_guc(uc));
+	GEM_BUG_ON(!wopcm->guc.size);
+	GEM_BUG_ON(!wopcm->guc.base);
+
+	err = write_and_verify(gt, GUC_WOPCM_SIZE, wopcm->guc.size,
+			       GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
+			       GUC_WOPCM_SIZE_LOCKED);
+	if (err)
+		goto err_out;
+
+	huc_agent = intel_uc_is_using_huc(uc) ? HUC_LOADING_AGENT_GUC : 0;
+	mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
+	err = write_and_verify(gt, DMA_GUC_WOPCM_OFFSET,
+			       wopcm->guc.base | huc_agent, mask,
+			       GUC_WOPCM_OFFSET_VALID);
+	if (err)
+		goto err_out;
+
+	return 0;
+
+err_out:
+	DRM_ERROR("Failed to init WOPCM registers:\n");
+	DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
+		  intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET));
+	DRM_ERROR("GUC_WOPCM_SIZE=%#x\n",
+		  intel_uncore_read(uncore, GUC_WOPCM_SIZE));
+
+	return err;
+}
+
 int intel_uc_init_hw(struct intel_uc *uc)
 {
 	struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
@@ -402,6 +459,10 @@ int intel_uc_init_hw(struct intel_uc *uc)
 
 	GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
 
+	ret = uc_wopcm_init_hw(uc);
+	if (ret)
+		goto out;
+
 	guc_reset_interrupts(guc);
 
 	/* WaEnableuKernelHeaderValidFix:skl */
@@ -486,6 +547,7 @@ int intel_uc_init_hw(struct intel_uc *uc)
 	if (GEM_WARN_ON(ret == -EIO))
 		ret = -EINVAL;
 
+out:
 	dev_err(i915->drm.dev, "GuC initialization failed %d\n", ret);
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 01dd0d1d9bf6..ae4e7cc3e3f9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1239,14 +1239,8 @@ int i915_gem_init_hw(struct drm_i915_private *i915)
 		goto out;
 	}
 
-	ret = intel_wopcm_init_hw(&i915->wopcm, gt);
-	if (ret) {
-		DRM_ERROR("Enabling WOPCM failed (%d)\n", ret);
-		goto out;
-	}
-
 	/* We can't enable contexts until all firmware is loaded */
-	ret = intel_uc_init_hw(&i915->gt.uc);
+	ret = intel_uc_init_hw(&gt->uc);
 	if (ret) {
 		DRM_ERROR("Enabling uc failed (%d)\n", ret);
 		goto out;
diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index 0e86a9e85b49..d9973c0b0384 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -224,71 +224,3 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 
 	return 0;
 }
-
-static int
-write_and_verify(struct intel_gt *gt,
-		 i915_reg_t reg, u32 val, u32 mask, u32 locked_bit)
-{
-	struct intel_uncore *uncore = gt->uncore;
-	u32 reg_val;
-
-	GEM_BUG_ON(val & ~mask);
-
-	intel_uncore_write(uncore, reg, val);
-
-	reg_val = intel_uncore_read(uncore, reg);
-
-	return (reg_val & mask) != (val | locked_bit) ? -EIO : 0;
-}
-
-/**
- * intel_wopcm_init_hw() - Setup GuC WOPCM registers.
- * @wopcm: pointer to intel_wopcm.
- * @gt: pointer to the containing GT
- *
- * Setup the GuC WOPCM size and offset registers with the calculated values. It
- * will verify the register values to make sure the registers are locked with
- * correct values.
- *
- * Return: 0 on success. -EIO if registers were locked with incorrect values.
- */
-int intel_wopcm_init_hw(struct intel_wopcm *wopcm, struct intel_gt *gt)
-{
-	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
-	struct intel_uncore *uncore = gt->uncore;
-	u32 huc_agent;
-	u32 mask;
-	int err;
-
-	if (!USES_GUC(i915))
-		return 0;
-
-	GEM_BUG_ON(!HAS_GT_UC(i915));
-	GEM_BUG_ON(!wopcm->guc.size);
-	GEM_BUG_ON(!wopcm->guc.base);
-
-	err = write_and_verify(gt, GUC_WOPCM_SIZE, wopcm->guc.size,
-			       GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
-			       GUC_WOPCM_SIZE_LOCKED);
-	if (err)
-		goto err_out;
-
-	huc_agent = USES_HUC(i915) ? HUC_LOADING_AGENT_GUC : 0;
-	mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
-	err = write_and_verify(gt, DMA_GUC_WOPCM_OFFSET,
-			       wopcm->guc.base | huc_agent, mask,
-			       GUC_WOPCM_OFFSET_VALID);
-	if (err)
-		goto err_out;
-
-	return 0;
-
-err_out:
-	DRM_ERROR("Failed to init WOPCM registers:\n");
-	DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
-		  intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET));
-	DRM_ERROR("GUC_WOPCM_SIZE=%#x\n",
-		  intel_uncore_read(uncore, GUC_WOPCM_SIZE));
-
-	return err;
-}
diff --git a/drivers/gpu/drm/i915/intel_wopcm.h b/drivers/gpu/drm/i915/intel_wopcm.h
index 56aaed4d64ff..e1f0f66aaa44 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.h
+++ b/drivers/gpu/drm/i915/intel_wopcm.h
@@ -9,8 +9,6 @@
 
 #include <linux/types.h>
 
-struct intel_gt;
-
 /**
  * struct intel_wopcm - Overall WOPCM info and WOPCM regions.
  * @size: Size of overall WOPCM.
@@ -43,6 +41,5 @@ static inline u32 intel_wopcm_guc_size(struct intel_wopcm *wopcm)
 
 void intel_wopcm_init_early(struct intel_wopcm *wopcm);
 int intel_wopcm_init(struct intel_wopcm *wopcm);
-int intel_wopcm_init_hw(struct intel_wopcm *wopcm, struct intel_gt *gt);
 
 #endif
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/6] drm/i915: move gt_cleanup_early out of gem_cleanup_early

[Daniele Ceraolo Spurio]

We don't call the init_early function from within the gem code, so we
shouldn't do it for the cleanup either.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 2 ++
 drivers/gpu/drm/i915/i915_gem.c | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f2d3d754af37..934e605e2466 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -951,6 +951,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
 	intel_uc_cleanup_early(&dev_priv->gt.uc);
 	i915_gem_cleanup_early(dev_priv);
 err_workqueues:
+	intel_gt_cleanup_early(&dev_priv->gt);
 	i915_workqueues_cleanup(dev_priv);
 	return ret;
 }
@@ -966,6 +967,7 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv)
 	intel_power_domains_cleanup(dev_priv);
 	intel_uc_cleanup_early(&dev_priv->gt.uc);
 	i915_gem_cleanup_early(dev_priv);
+	intel_gt_cleanup_early(&dev_priv->gt);
 	i915_workqueues_cleanup(dev_priv);
 
 	pm_qos_remove_request(&dev_priv->sb_qos);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ae4e7cc3e3f9..2c7dc3404759 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1674,8 +1674,6 @@ void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
 	GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
 	WARN_ON(dev_priv->mm.shrink_count);
 
-	intel_gt_cleanup_early(&dev_priv->gt);
-
 	i915_gemfs_fini(dev_priv);
 }
 
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/6] drm/i915/uc: move uc early functions inside the gt ones

[Daniele Ceraolo Spurio]

uc is a subcomponent of GT, so initialize/clean it as part of it. The
wopcm_init_early doesn't have to be happen before the uC one, but since
in other parts of the code we consider wopcm first do the same for
consistency.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt.c |  2 ++
 drivers/gpu/drm/i915/i915_drv.c    | 10 ++++------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index f7e69db4019d..c0b846ecee09 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -22,6 +22,7 @@ void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915)
 	intel_gt_init_hangcheck(gt);
 	intel_gt_init_reset(gt);
 	intel_gt_pm_init_early(gt);
+	intel_uc_init_early(&gt->uc);
 }
 
 void intel_gt_init_hw(struct drm_i915_private *i915)
@@ -246,5 +247,6 @@ void intel_gt_fini_scratch(struct intel_gt *gt)
 
 void intel_gt_cleanup_early(struct intel_gt *gt)
 {
+	intel_uc_cleanup_early(&gt->uc);
 	intel_gt_fini_reset(gt);
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 934e605e2466..3cd3be69dbad 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -921,6 +921,8 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
 	if (ret < 0)
 		return ret;
 
+	intel_wopcm_init_early(&dev_priv->wopcm);
+
 	intel_gt_init_early(&dev_priv->gt, dev_priv);
 
 	ret = i915_gem_init_early(dev_priv);
@@ -930,13 +932,11 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
 	/* This must be called before any calls to HAS_PCH_* */
 	intel_detect_pch(dev_priv);
 
-	intel_wopcm_init_early(&dev_priv->wopcm);
-	intel_uc_init_early(&dev_priv->gt.uc);
 	intel_pm_setup(dev_priv);
 	intel_init_dpio(dev_priv);
 	ret = intel_power_domains_init(dev_priv);
 	if (ret < 0)
-		goto err_uc;
+		goto err_gem;
 	intel_irq_init(dev_priv);
 	intel_init_display_hooks(dev_priv);
 	intel_init_clock_gating_hooks(dev_priv);
@@ -947,8 +947,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
 
 	return 0;
 
-err_uc:
-	intel_uc_cleanup_early(&dev_priv->gt.uc);
+err_gem:
 	i915_gem_cleanup_early(dev_priv);
 err_workqueues:
 	intel_gt_cleanup_early(&dev_priv->gt);
@@ -965,7 +964,6 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv)
 {
 	intel_irq_fini(dev_priv);
 	intel_power_domains_cleanup(dev_priv);
-	intel_uc_cleanup_early(&dev_priv->gt.uc);
 	i915_gem_cleanup_early(dev_priv);
 	intel_gt_cleanup_early(&dev_priv->gt);
 	i915_workqueues_cleanup(dev_priv);
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/6] drm/i915/gt: introduce intel_gt_runtime_suspend/resume

[Daniele Ceraolo Spurio]

To be called from the top level runtime functions, to hide the
gt-specific bits (mainly related to intel_uc).

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_pm.c | 12 ++++++++++++
 drivers/gpu/drm/i915/gt/intel_gt_pm.h |  2 ++
 drivers/gpu/drm/i915/i915_drv.c       |  9 +++------
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index 65c0d0c9d543..1a32e3e523c0 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -164,3 +164,15 @@ int intel_gt_resume(struct intel_gt *gt)
 
 	return err;
 }
+
+void intel_gt_runtime_suspend(struct intel_gt *gt)
+{
+	intel_uc_runtime_suspend(&gt->uc);
+}
+
+int intel_gt_runtime_resume(struct intel_gt *gt)
+{
+	intel_gt_init_swizzling(gt);
+
+	return intel_uc_resume(&gt->uc);
+}
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
index ba960e1fc209..527894fe1345 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
@@ -23,5 +23,7 @@ void intel_gt_pm_init_early(struct intel_gt *gt);
 
 void intel_gt_sanitize(struct intel_gt *gt, bool force);
 int intel_gt_resume(struct intel_gt *gt);
+void intel_gt_runtime_suspend(struct intel_gt *gt);
+int intel_gt_runtime_resume(struct intel_gt *gt);
 
 #endif /* INTEL_GT_PM_H */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3cd3be69dbad..bc7ffda10e5c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2925,7 +2925,7 @@ static int intel_runtime_suspend(struct device *kdev)
 	 */
 	i915_gem_runtime_suspend(dev_priv);
 
-	intel_uc_runtime_suspend(&dev_priv->gt.uc);
+	intel_gt_runtime_suspend(&dev_priv->gt);
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
 
@@ -2950,9 +2950,8 @@ static int intel_runtime_suspend(struct device *kdev)
 
 		intel_runtime_pm_enable_interrupts(dev_priv);
 
-		intel_uc_resume(&dev_priv->gt.uc);
+		intel_gt_runtime_resume(&dev_priv->gt);
 
-		intel_gt_init_swizzling(&dev_priv->gt);
 		i915_gem_restore_fences(dev_priv);
 
 		enable_rpm_wakeref_asserts(rpm);
@@ -3047,13 +3046,11 @@ static int intel_runtime_resume(struct device *kdev)
 
 	intel_runtime_pm_enable_interrupts(dev_priv);
 
-	intel_uc_resume(&dev_priv->gt.uc);
-
 	/*
 	 * No point of rolling back things in case of an error, as the best
 	 * we can do is to hope that things will still work (and disable RPM).
 	 */
-	intel_gt_init_swizzling(&dev_priv->gt);
+	intel_gt_runtime_resume(&dev_priv->gt);
 	i915_gem_restore_fences(dev_priv);
 
 	/*
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/6] drm/i915/uc: move uc_resume under gt_resume

[Daniele Ceraolo Spurio]

intel_uc is part of intel_gt so it makes logical sense for it to be
resumed as part of it. Note that, since gt_resume is also called during
the init flow, a state variable has been added to intel_uc to avoid
asking an already running GuC to resume.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gem/i915_gem_pm.c |  2 --
 drivers/gpu/drm/i915/gt/intel_gt_pm.c  |  3 +++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c  | 10 ++++++++--
 drivers/gpu/drm/i915/gt/uc/intel_uc.h  |  2 ++
 4 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
index b5561cbdc5ea..25610de3961b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
@@ -265,8 +265,6 @@ void i915_gem_resume(struct drm_i915_private *i915)
 	if (intel_gt_resume(&i915->gt))
 		goto err_wedged;
 
-	intel_uc_resume(&i915->gt.uc);
-
 	/* Always reload a context for powersaving. */
 	if (!i915_gem_load_power_context(i915))
 		goto err_wedged;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index 1a32e3e523c0..2250ffbd2f32 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -162,6 +162,9 @@ int intel_gt_resume(struct intel_gt *gt)
 	}
 	intel_gt_pm_put(gt);
 
+	if (!err)
+		err = intel_uc_resume(&gt->uc);
+
 	return err;
 }
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 2f71f3704c46..a9a893c5fbe0 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -380,6 +380,8 @@ static void __uc_sanitize(struct intel_uc *uc)
 	intel_guc_sanitize(guc);
 
 	__intel_uc_reset_hw(uc);
+
+	uc->suspended = false;
 }
 
 void intel_uc_sanitize(struct intel_uc *uc)
@@ -598,6 +600,8 @@ void intel_uc_runtime_suspend(struct intel_uc *uc)
 		DRM_DEBUG_DRIVER("Failed to suspend GuC, err=%d", err);
 
 	guc_disable_communication(guc);
+
+	uc->suspended = true;
 }
 
 void intel_uc_suspend(struct intel_uc *uc)
@@ -605,7 +609,7 @@ void intel_uc_suspend(struct intel_uc *uc)
 	struct intel_guc *guc = &uc->guc;
 	intel_wakeref_t wakeref;
 
-	if (!intel_guc_is_running(guc))
+	if (!intel_guc_is_running(guc) || uc->suspended)
 		return;
 
 	with_intel_runtime_pm(&uc_to_gt(uc)->i915->runtime_pm, wakeref)
@@ -617,7 +621,7 @@ int intel_uc_resume(struct intel_uc *uc)
 	struct intel_guc *guc = &uc->guc;
 	int err;
 
-	if (!intel_guc_is_running(guc))
+	if (!intel_guc_is_running(guc) || !uc->suspended)
 		return 0;
 
 	guc_enable_communication(guc);
@@ -628,5 +632,7 @@ int intel_uc_resume(struct intel_uc *uc)
 		return err;
 	}
 
+	uc->suspended = false;
+
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index fe3362fd7706..ea58fc164d1e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -31,6 +31,8 @@
 struct intel_uc {
 	struct intel_guc guc;
 	struct intel_huc huc;
+
+	bool suspended;
 };
 
 void intel_uc_init_early(struct intel_uc *uc);
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/6] drm/i915/gt: introduce intel_gt_suspend

[Daniele Ceraolo Spurio]

For symmetry with intel_gt_resume and to hide more stuff from the top
level under intel_gt. Note that the switch_to_kernel_context_sync has
not been moved dure to the locking and ordering requirements that exist
at the moment.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_pm.c |  9 +--------
 drivers/gpu/drm/i915/gt/intel_gt_pm.c  | 13 +++++++++++++
 drivers/gpu/drm/i915/gt/intel_gt_pm.h  |  1 +
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
index 25610de3961b..9f72ce8b5a6f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
@@ -163,18 +163,11 @@ void i915_gem_suspend(struct drm_i915_private *i915)
 
 	mutex_unlock(&i915->drm.struct_mutex);
 
-	/*
-	 * Assert that we successfully flushed all the work and
-	 * reset the GPU back to its idle, low power state.
-	 */
-	GEM_BUG_ON(i915->gt.awake);
 	flush_work(&i915->gem.idle_work);
 
-	cancel_delayed_work_sync(&i915->gt.hangcheck.work);
+	intel_gt_suspend(&i915->gt);
 
 	i915_gem_drain_freed_objects(i915);
-
-	intel_uc_suspend(&i915->gt.uc);
 }
 
 static struct drm_i915_gem_object *first_mm_object(struct list_head *list)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index 2250ffbd2f32..9c9efcde994d 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -127,6 +127,19 @@ void intel_gt_sanitize(struct intel_gt *gt, bool force)
 		__intel_engine_reset(engine, false);
 }
 
+void intel_gt_suspend(struct intel_gt *gt)
+{
+	/*
+	 * Assert that we successfully flushed all the work and
+	 * reset the GPU back to its idle, low power state.
+	 */
+	GEM_BUG_ON(gt->awake);
+
+	cancel_delayed_work_sync(&gt->hangcheck.work);
+
+	intel_uc_suspend(&gt->uc);
+}
+
 int intel_gt_resume(struct intel_gt *gt)
 {
 	struct intel_engine_cs *engine;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
index 527894fe1345..4f29ac880ca8 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
@@ -22,6 +22,7 @@ void intel_gt_pm_put(struct intel_gt *gt);
 void intel_gt_pm_init_early(struct intel_gt *gt);
 
 void intel_gt_sanitize(struct intel_gt *gt, bool force);
+void intel_gt_suspend(struct intel_gt *gt);
 int intel_gt_resume(struct intel_gt *gt);
 void intel_gt_runtime_suspend(struct intel_gt *gt);
 int intel_gt_runtime_resume(struct intel_gt *gt);
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for Call uC functions from GT ones

[Patchwork]

== Series Details ==

Series: Call uC functions from GT ones
URL   : https://patchwork.freedesktop.org/series/64406/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/uc: move uC WOPCM setup in uc_init_hw
+drivers/gpu/drm/i915/gt/uc/intel_uc.c:410:5: warning: symbol 'uc_wopcm_init_hw' was not declared. Should it be static?

Commit: drm/i915: move gt_cleanup_early out of gem_cleanup_early
Okay!

Commit: drm/i915/uc: move uc early functions inside the gt ones
Okay!

Commit: drm/i915/gt: introduce intel_gt_runtime_suspend/resume
Okay!

Commit: drm/i915/uc: move uc_resume under gt_resume
Okay!

Commit: drm/i915/gt: introduce intel_gt_suspend
Okay!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Call uC functions from GT ones

[Patchwork]

== Series Details ==

Series: Call uC functions from GT ones
URL   : https://patchwork.freedesktop.org/series/64406/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6576 -> Patchwork_13793
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       [PASS][1] -> [WARN][2] ([fdo#109380])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/fi-kbl-7567u/igt@kms_chamelium@common-hpd-after-suspend.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/fi-kbl-7567u/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-cml-u2:          [PASS][3] -> [FAIL][4] ([fdo#109483])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          [PASS][5] -> [FAIL][6] ([fdo#103167])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-c:
    - fi-kbl-7567u:       [PASS][7] -> [SKIP][8] ([fdo#109271]) +23 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/fi-kbl-7567u/igt@kms_pipe_crc_basic@read-crc-pipe-c.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/fi-kbl-7567u/igt@kms_pipe_crc_basic@read-crc-pipe-c.html

  * igt@prime_vgem@basic-write:
    - fi-icl-u3:          [PASS][9] -> [DMESG-WARN][10] ([fdo#107724])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/fi-icl-u3/igt@prime_vgem@basic-write.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/fi-icl-u3/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@kms_busy@basic-flip-a:
    - fi-kbl-7567u:       [SKIP][11] ([fdo#109271] / [fdo#109278]) -> [PASS][12] +2 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/fi-kbl-7567u/igt@kms_busy@basic-flip-a.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/fi-kbl-7567u/igt@kms_busy@basic-flip-a.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          [FAIL][13] ([fdo#109483]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109380]: https://bugs.freedesktop.org/show_bug.cgi?id=109380
  [fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483


Participating hosts (51 -> 45)
------------------------------

  Missing    (6): fi-byt-squawks fi-bsw-cyan fi-pnv-d510 fi-icl-y fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6576 -> Patchwork_13793

  CI-20190529: 20190529
  CI_DRM_6576: 4040b4c4ab647422d82100c8b091d34b6a82f572 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5115: 21be7a02ac8a8ff46b561c36a69e4dd5a0c2938b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13793: 184a57b1d31a2094d1ac88756c82b56478f9072a @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

184a57b1d31a drm/i915/gt: introduce intel_gt_suspend
ecbc778d9f02 drm/i915/uc: move uc_resume under gt_resume
e648b6830b52 drm/i915/gt: introduce intel_gt_runtime_suspend/resume
74b1723bd1e7 drm/i915/uc: move uc early functions inside the gt ones
855580793f1d drm/i915: move gt_cleanup_early out of gem_cleanup_early
57ae57f734f6 drm/i915/uc: move uC WOPCM setup in uc_init_hw

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915: move gt_cleanup_early out of gem_cleanup_early

[Tvrtko Ursulin]

On 30/07/2019 00:47, Daniele Ceraolo Spurio wrote:
> We don't call the init_early function from within the gem code, so we
> shouldn't do it for the cleanup either.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c | 2 ++
>   drivers/gpu/drm/i915/i915_gem.c | 2 --
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f2d3d754af37..934e605e2466 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -951,6 +951,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
>   	intel_uc_cleanup_early(&dev_priv->gt.uc);
>   	i915_gem_cleanup_early(dev_priv);
>   err_workqueues:
> +	intel_gt_cleanup_early(&dev_priv->gt);
>   	i915_workqueues_cleanup(dev_priv);
>   	return ret;
>   }
> @@ -966,6 +967,7 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv)
>   	intel_power_domains_cleanup(dev_priv);
>   	intel_uc_cleanup_early(&dev_priv->gt.uc);
>   	i915_gem_cleanup_early(dev_priv);
> +	intel_gt_cleanup_early(&dev_priv->gt);
>   	i915_workqueues_cleanup(dev_priv);
>   
>   	pm_qos_remove_request(&dev_priv->sb_qos);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ae4e7cc3e3f9..2c7dc3404759 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1674,8 +1674,6 @@ void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
>   	GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
>   	WARN_ON(dev_priv->mm.shrink_count);
>   
> -	intel_gt_cleanup_early(&dev_priv->gt);
> -
>   	i915_gemfs_fini(dev_priv);
>   }
>   
> 

I have a nagging feeling it was me who added this asymmetry.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915/gt: introduce intel_gt_suspend

[Tvrtko Ursulin]

On 30/07/2019 00:47, Daniele Ceraolo Spurio wrote:
> For symmetry with intel_gt_resume and to hide more stuff from the top
> level under intel_gt. Note that the switch_to_kernel_context_sync has
> not been moved dure to the locking and ordering requirements that exist

Typo in due.

Should we add intel_gt_suspend_early/late, with the former dealing with 
switch_to_kernel_context_sync? Although that raises an interesting 
conundrum since it operates on GEM and GT. Best then to leave it 
unwrapped for now I think.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> at the moment.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_pm.c |  9 +--------
>   drivers/gpu/drm/i915/gt/intel_gt_pm.c  | 13 +++++++++++++
>   drivers/gpu/drm/i915/gt/intel_gt_pm.h  |  1 +
>   3 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> index 25610de3961b..9f72ce8b5a6f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> @@ -163,18 +163,11 @@ void i915_gem_suspend(struct drm_i915_private *i915)
>   
>   	mutex_unlock(&i915->drm.struct_mutex);
>   
> -	/*
> -	 * Assert that we successfully flushed all the work and
> -	 * reset the GPU back to its idle, low power state.
> -	 */
> -	GEM_BUG_ON(i915->gt.awake);
>   	flush_work(&i915->gem.idle_work);
>   
> -	cancel_delayed_work_sync(&i915->gt.hangcheck.work);
> +	intel_gt_suspend(&i915->gt);
>   
>   	i915_gem_drain_freed_objects(i915);
> -
> -	intel_uc_suspend(&i915->gt.uc);
>   }
>   
>   static struct drm_i915_gem_object *first_mm_object(struct list_head *list)
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> index 2250ffbd2f32..9c9efcde994d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> @@ -127,6 +127,19 @@ void intel_gt_sanitize(struct intel_gt *gt, bool force)
>   		__intel_engine_reset(engine, false);
>   }
>   
> +void intel_gt_suspend(struct intel_gt *gt)
> +{
> +	/*
> +	 * Assert that we successfully flushed all the work and
> +	 * reset the GPU back to its idle, low power state.
> +	 */
> +	GEM_BUG_ON(gt->awake);
> +
> +	cancel_delayed_work_sync(&gt->hangcheck.work);
> +
> +	intel_uc_suspend(&gt->uc);
> +}
> +
>   int intel_gt_resume(struct intel_gt *gt)
>   {
>   	struct intel_engine_cs *engine;
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> index 527894fe1345..4f29ac880ca8 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> @@ -22,6 +22,7 @@ void intel_gt_pm_put(struct intel_gt *gt);
>   void intel_gt_pm_init_early(struct intel_gt *gt);
>   
>   void intel_gt_sanitize(struct intel_gt *gt, bool force);
> +void intel_gt_suspend(struct intel_gt *gt);
>   int intel_gt_resume(struct intel_gt *gt);
>   void intel_gt_runtime_suspend(struct intel_gt *gt);
>   int intel_gt_runtime_resume(struct intel_gt *gt);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915/gt: introduce intel_gt_runtime_suspend/resume

[Tvrtko Ursulin]

On 30/07/2019 00:47, Daniele Ceraolo Spurio wrote:
> To be called from the top level runtime functions, to hide the
> gt-specific bits (mainly related to intel_uc).

Looks okay to me.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

P.S. Added Andi and Ram to Cc since PM/RPS task.


> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_gt_pm.c | 12 ++++++++++++
>   drivers/gpu/drm/i915/gt/intel_gt_pm.h |  2 ++
>   drivers/gpu/drm/i915/i915_drv.c       |  9 +++------
>   3 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> index 65c0d0c9d543..1a32e3e523c0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> @@ -164,3 +164,15 @@ int intel_gt_resume(struct intel_gt *gt)
>   
>   	return err;
>   }
> +
> +void intel_gt_runtime_suspend(struct intel_gt *gt)
> +{
> +	intel_uc_runtime_suspend(&gt->uc);
> +}
> +
> +int intel_gt_runtime_resume(struct intel_gt *gt)
> +{
> +	intel_gt_init_swizzling(gt);
> +
> +	return intel_uc_resume(&gt->uc);
> +}
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> index ba960e1fc209..527894fe1345 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> @@ -23,5 +23,7 @@ void intel_gt_pm_init_early(struct intel_gt *gt);
>   
>   void intel_gt_sanitize(struct intel_gt *gt, bool force);
>   int intel_gt_resume(struct intel_gt *gt);
> +void intel_gt_runtime_suspend(struct intel_gt *gt);
> +int intel_gt_runtime_resume(struct intel_gt *gt);
>   
>   #endif /* INTEL_GT_PM_H */
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3cd3be69dbad..bc7ffda10e5c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2925,7 +2925,7 @@ static int intel_runtime_suspend(struct device *kdev)
>   	 */
>   	i915_gem_runtime_suspend(dev_priv);
>   
> -	intel_uc_runtime_suspend(&dev_priv->gt.uc);
> +	intel_gt_runtime_suspend(&dev_priv->gt);
>   
>   	intel_runtime_pm_disable_interrupts(dev_priv);
>   
> @@ -2950,9 +2950,8 @@ static int intel_runtime_suspend(struct device *kdev)
>   
>   		intel_runtime_pm_enable_interrupts(dev_priv);
>   
> -		intel_uc_resume(&dev_priv->gt.uc);
> +		intel_gt_runtime_resume(&dev_priv->gt);
>   
> -		intel_gt_init_swizzling(&dev_priv->gt);
>   		i915_gem_restore_fences(dev_priv);
>   
>   		enable_rpm_wakeref_asserts(rpm);
> @@ -3047,13 +3046,11 @@ static int intel_runtime_resume(struct device *kdev)
>   
>   	intel_runtime_pm_enable_interrupts(dev_priv);
>   
> -	intel_uc_resume(&dev_priv->gt.uc);
> -
>   	/*
>   	 * No point of rolling back things in case of an error, as the best
>   	 * we can do is to hope that things will still work (and disable RPM).
>   	 */
> -	intel_gt_init_swizzling(&dev_priv->gt);
> +	intel_gt_runtime_resume(&dev_priv->gt);
>   	i915_gem_restore_fences(dev_priv);
>   
>   	/*
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915: move gt_cleanup_early out of gem_cleanup_early

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-30 08:19:18)
> 
> On 30/07/2019 00:47, Daniele Ceraolo Spurio wrote:
> > We don't call the init_early function from within the gem code, so we
> > shouldn't do it for the cleanup either.
> > 
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.c | 2 ++
> >   drivers/gpu/drm/i915/i915_gem.c | 2 --
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index f2d3d754af37..934e605e2466 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -951,6 +951,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
> >       intel_uc_cleanup_early(&dev_priv->gt.uc);
> >       i915_gem_cleanup_early(dev_priv);
> >   err_workqueues:
> > +     intel_gt_cleanup_early(&dev_priv->gt);
> >       i915_workqueues_cleanup(dev_priv);
> >       return ret;
> >   }
> > @@ -966,6 +967,7 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv)
> >       intel_power_domains_cleanup(dev_priv);
> >       intel_uc_cleanup_early(&dev_priv->gt.uc);
> >       i915_gem_cleanup_early(dev_priv);
> > +     intel_gt_cleanup_early(&dev_priv->gt);

Note the change in naming convention, intel_gt_driver_late_release().
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm/i915/uc: move uc_resume under gt_resume

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-07-30 00:47:26)
> intel_uc is part of intel_gt so it makes logical sense for it to be
> resumed as part of it. Note that, since gt_resume is also called during
> the init flow, a state variable has been added to intel_uc to avoid
> asking an already running GuC to resume.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_pm.c |  2 --
>  drivers/gpu/drm/i915/gt/intel_gt_pm.c  |  3 +++
>  drivers/gpu/drm/i915/gt/uc/intel_uc.c  | 10 ++++++++--
>  drivers/gpu/drm/i915/gt/uc/intel_uc.h  |  2 ++
>  4 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> index b5561cbdc5ea..25610de3961b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> @@ -265,8 +265,6 @@ void i915_gem_resume(struct drm_i915_private *i915)
>         if (intel_gt_resume(&i915->gt))
>                 goto err_wedged;
>  
> -       intel_uc_resume(&i915->gt.uc);
> -
>         /* Always reload a context for powersaving. */
>         if (!i915_gem_load_power_context(i915))
>                 goto err_wedged;

This sequence itself does not belong to i915_gem_resume() and needs to
be lifted to intel_gt_resume (that is establishing the baseline GT power
context). So I would rather postpone this to try and avoid having to
introduce bool suspended if at all possible.

Another rule of thumb to consider is that we should be able to throw gt
initialisation into an async task (and I'm considering an async task
per engine, although for a large part we can achieve asynchronicity via
HW queues).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915/gt: introduce intel_gt_suspend

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-30 08:33:28)
> 
> On 30/07/2019 00:47, Daniele Ceraolo Spurio wrote:
> > For symmetry with intel_gt_resume and to hide more stuff from the top
> > level under intel_gt. Note that the switch_to_kernel_context_sync has
> > not been moved dure to the locking and ordering requirements that exist
> 
> Typo in due.
> 
> Should we add intel_gt_suspend_early/late, with the former dealing with 
> switch_to_kernel_context_sync? Although that raises an interesting 
> conundrum since it operates on GEM and GT. Best then to leave it 
> unwrapped for now I think.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Ah, I wouldn't apply this patch yet for exactly that reason: we
shouldn't be driving GT suspend from inside GEM, so would wait until we
have a few more pieces of the puzzle reviewed^W unravelled.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for Call uC functions from GT ones

[Patchwork]

== Series Details ==

Series: Call uC functions from GT ones
URL   : https://patchwork.freedesktop.org/series/64406/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6576_full -> Patchwork_13793_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@in-flight-suspend:
    - shard-kbl:          [PASS][1] -> [INCOMPLETE][2] ([fdo#103665])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-kbl6/igt@gem_eio@in-flight-suspend.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-kbl2/igt@gem_eio@in-flight-suspend.html

  * igt@gem_exec_suspend@basic-s3:
    - shard-skl:          [PASS][3] -> [INCOMPLETE][4] ([fdo#104108])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-skl8/igt@gem_exec_suspend@basic-s3.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-skl6/igt@gem_exec_suspend@basic-s3.html

  * igt@kms_cursor_crc@pipe-c-cursor-alpha-opaque:
    - shard-glk:          [PASS][5] -> [FAIL][6] ([fdo#103232])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-glk3/igt@kms_cursor_crc@pipe-c-cursor-alpha-opaque.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-glk5/igt@kms_cursor_crc@pipe-c-cursor-alpha-opaque.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-apl:          [PASS][7] -> [DMESG-WARN][8] ([fdo#108566]) +3 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-apl3/igt@kms_flip@flip-vs-suspend-interruptible.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-apl1/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-badstride:
    - shard-iclb:         [PASS][9] -> [FAIL][10] ([fdo#103167]) +7 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-badstride.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-badstride.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-kbl:          [PASS][11] -> [DMESG-WARN][12] ([fdo#108566])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-kbl1/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-kbl4/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([fdo#108145])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-skl4/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-skl1/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_psr@psr2_sprite_plane_move:
    - shard-iclb:         [PASS][15] -> [SKIP][16] ([fdo#109441])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-iclb7/igt@kms_psr@psr2_sprite_plane_move.html

  * igt@perf@polling:
    - shard-skl:          [PASS][17] -> [FAIL][18] ([fdo#110728])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-skl2/igt@perf@polling.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-skl6/igt@perf@polling.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-kbl:          [DMESG-WARN][19] ([fdo#108566]) -> [PASS][20] +9 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-kbl3/igt@gem_ctx_isolation@rcs0-s3.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-kbl3/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [SKIP][21] ([fdo#110854]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-iclb6/igt@gem_exec_balancer@smoke.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-iclb4/igt@gem_exec_balancer@smoke.html

  * igt@gem_exec_suspend@basic-s3:
    - shard-snb:          [DMESG-WARN][23] ([fdo#110684] / [fdo#111115]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-snb6/igt@gem_exec_suspend@basic-s3.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-snb5/igt@gem_exec_suspend@basic-s3.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-skl:          [INCOMPLETE][25] ([fdo#110741]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-skl6/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-skl7/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          [FAIL][27] ([fdo#105363]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-glk5/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-glk9/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt:
    - shard-iclb:         [FAIL][29] ([fdo#103167]) -> [PASS][30] +4 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-apl:          [DMESG-WARN][31] ([fdo#108566]) -> [PASS][32] +3 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-apl5/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-apl8/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [FAIL][33] ([fdo#108145] / [fdo#110403]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-skl3/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-skl7/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_psr@psr2_suspend:
    - shard-iclb:         [SKIP][35] ([fdo#109441]) -> [PASS][36] +2 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-iclb6/igt@kms_psr@psr2_suspend.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-iclb2/igt@kms_psr@psr2_suspend.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [FAIL][37] ([fdo#99912]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-kbl7/igt@kms_setmode@basic.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-kbl3/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-c-wait-forked-busy-hang:
    - shard-iclb:         [INCOMPLETE][39] ([fdo#107713]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-iclb7/igt@kms_vblank@pipe-c-wait-forked-busy-hang.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-iclb8/igt@kms_vblank@pipe-c-wait-forked-busy-hang.html

  * igt@prime_busy@wait-hang-vebox:
    - shard-glk:          [DMESG-WARN][41] ([fdo#111256]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6576/shard-glk7/igt@prime_busy@wait-hang-vebox.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/shard-glk1/igt@prime_busy@wait-hang-vebox.html

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110684]: https://bugs.freedesktop.org/show_bug.cgi?id=110684
  [fdo#110728]: https://bugs.freedesktop.org/show_bug.cgi?id=110728
  [fdo#110741]: https://bugs.freedesktop.org/show_bug.cgi?id=110741
  [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
  [fdo#111115]: https://bugs.freedesktop.org/show_bug.cgi?id=111115
  [fdo#111256]: https://bugs.freedesktop.org/show_bug.cgi?id=111256
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6576 -> Patchwork_13793

  CI-20190529: 20190529
  CI_DRM_6576: 4040b4c4ab647422d82100c8b091d34b6a82f572 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5115: 21be7a02ac8a8ff46b561c36a69e4dd5a0c2938b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13793: 184a57b1d31a2094d1ac88756c82b56478f9072a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13793/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915/uc: move uC WOPCM setup in uc_init_hw

[Michal Wajdeczko]

On Tue, 30 Jul 2019 01:47:22 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> The register we write are not WOPCM regs but uC ones related to how
> GuC and HuC are going to use the WOPCM, so it makes logical sense
> for them to be programmed as part of uc_init_hw. The wopcm map on the

s/wopcm/WOPCM

> other side is not uC-specific (although that is our main use-case), so
> keep that separate.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_uc.c | 62 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem.c       |  8 +---
>  drivers/gpu/drm/i915/intel_wopcm.c    | 68 ---------------------------
>  drivers/gpu/drm/i915/intel_wopcm.h    |  3 --
>  4 files changed, 63 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index fafa9be1e12a..2f71f3704c46 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -390,6 +390,63 @@ void intel_uc_sanitize(struct intel_uc *uc)
>  	__uc_sanitize(uc);
>  }
> +static int
> +write_and_verify(struct intel_gt *gt,
> +		 i915_reg_t reg, u32 val, u32 mask, u32 locked_bit)

as this function is more 'uncore' than 'gt' I would define it as:

static inline bool
intel_uncore_write_and_verify(struct intel_uncore *uncore,
                               i915_reg_t reg, u32 value,
                               u32 expected_value, u32 mask)
in intel_uncore.h

> +{
> +	struct intel_uncore *uncore = gt->uncore;
> +	u32 reg_val;
> +
> +	GEM_BUG_ON(val & ~mask);
> +
> +	intel_uncore_write(uncore, reg, val);
> +
> +	reg_val = intel_uncore_read(uncore, reg);
> +
> +	return (reg_val & mask) != (val | locked_bit) ? -EIO : 0;
> +}
> +
> +/* Initialize and verify the uC regs related to uC positioning in WOPCM  
> */
> +int uc_wopcm_init_hw(struct intel_uc *uc)

static int uc_init_wopcm()

> +{
> +	struct intel_gt *gt = uc_to_gt(uc);
> +	struct intel_wopcm *wopcm = &gt->i915->wopcm;
> +	struct intel_uncore *uncore = gt->uncore;
> +	u32 huc_agent;
> +	u32 mask;
> +	int err;
> +
> +	GEM_BUG_ON(!HAS_GT_UC(gt->i915));
> +	GEM_BUG_ON(!intel_uc_is_using_guc(uc));
> +	GEM_BUG_ON(!wopcm->guc.size);

on one hand there is intel_wopcm_guc_size() that can be used here,
but on other hand there is no intel_wopcm_guc_base ;(

> +	GEM_BUG_ON(!wopcm->guc.base);
> +
> +	err = write_and_verify(gt, GUC_WOPCM_SIZE, wopcm->guc.size,
> +			       GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
> +			       GUC_WOPCM_SIZE_LOCKED);

hmm, as these are write-once registers, maybe we should write only once
(not here) and only verify every time in uc_init_hw ?

> +	if (err)
> +		goto err_out;
> +
> +	huc_agent = intel_uc_is_using_huc(uc) ? HUC_LOADING_AGENT_GUC : 0;
> +	mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
> +	err = write_and_verify(gt, DMA_GUC_WOPCM_OFFSET,
> +			       wopcm->guc.base | huc_agent, mask,
> +			       GUC_WOPCM_OFFSET_VALID);
> +	if (err)
> +		goto err_out;
> +
> +	return 0;
> +
> +err_out:
> +	DRM_ERROR("Failed to init WOPCM registers:\n");

In commit msg you said that these are not WOPCM registers ;)

> +	DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
> +		  intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET));
> +	DRM_ERROR("GUC_WOPCM_SIZE=%#x\n",
> +		  intel_uncore_read(uncore, GUC_WOPCM_SIZE));

btw, we can avoid extra read by reporting already failed write
in intel_uncore_write_and_verify()

> +
> +	return err;
> +}
> +
>  int intel_uc_init_hw(struct intel_uc *uc)
>  {
>  	struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
> @@ -402,6 +459,10 @@ int intel_uc_init_hw(struct intel_uc *uc)
> 	GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
> +	ret = uc_wopcm_init_hw(uc);
> +	if (ret)
> +		goto out;

it should be harmless to reuse existing err_out label

> +
>  	guc_reset_interrupts(guc);
> 	/* WaEnableuKernelHeaderValidFix:skl */
> @@ -486,6 +547,7 @@ int intel_uc_init_hw(struct intel_uc *uc)
>  	if (GEM_WARN_ON(ret == -EIO))
>  		ret = -EINVAL;
> +out:
>  	dev_err(i915->drm.dev, "GuC initialization failed %d\n", ret);
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem.c  
> b/drivers/gpu/drm/i915/i915_gem.c
> index 01dd0d1d9bf6..ae4e7cc3e3f9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1239,14 +1239,8 @@ int i915_gem_init_hw(struct drm_i915_private  
> *i915)
>  		goto out;
>  	}
> -	ret = intel_wopcm_init_hw(&i915->wopcm, gt);
> -	if (ret) {
> -		DRM_ERROR("Enabling WOPCM failed (%d)\n", ret);
> -		goto out;
> -	}
> -
>  	/* We can't enable contexts until all firmware is loaded */
> -	ret = intel_uc_init_hw(&i915->gt.uc);
> +	ret = intel_uc_init_hw(&gt->uc);
>  	if (ret) {
>  		DRM_ERROR("Enabling uc failed (%d)\n", ret);
>  		goto out;
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c  
> b/drivers/gpu/drm/i915/intel_wopcm.c
> index 0e86a9e85b49..d9973c0b0384 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -224,71 +224,3 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
> 	return 0;
>  }
> -
> -static int
> -write_and_verify(struct intel_gt *gt,
> -		 i915_reg_t reg, u32 val, u32 mask, u32 locked_bit)
> -{
> -	struct intel_uncore *uncore = gt->uncore;
> -	u32 reg_val;
> -
> -	GEM_BUG_ON(val & ~mask);
> -
> -	intel_uncore_write(uncore, reg, val);
> -
> -	reg_val = intel_uncore_read(uncore, reg);
> -
> -	return (reg_val & mask) != (val | locked_bit) ? -EIO : 0;
> -}
> -
> -/**
> - * intel_wopcm_init_hw() - Setup GuC WOPCM registers.
> - * @wopcm: pointer to intel_wopcm.
> - * @gt: pointer to the containing GT
> - *
> - * Setup the GuC WOPCM size and offset registers with the calculated  
> values. It
> - * will verify the register values to make sure the registers are  
> locked with
> - * correct values.
> - *
> - * Return: 0 on success. -EIO if registers were locked with incorrect  
> values.
> - */
> -int intel_wopcm_init_hw(struct intel_wopcm *wopcm, struct intel_gt *gt)
> -{
> -	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> -	struct intel_uncore *uncore = gt->uncore;
> -	u32 huc_agent;
> -	u32 mask;
> -	int err;
> -
> -	if (!USES_GUC(i915))
> -		return 0;
> -
> -	GEM_BUG_ON(!HAS_GT_UC(i915));
> -	GEM_BUG_ON(!wopcm->guc.size);
> -	GEM_BUG_ON(!wopcm->guc.base);
> -
> -	err = write_and_verify(gt, GUC_WOPCM_SIZE, wopcm->guc.size,
> -			       GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
> -			       GUC_WOPCM_SIZE_LOCKED);
> -	if (err)
> -		goto err_out;
> -
> -	huc_agent = USES_HUC(i915) ? HUC_LOADING_AGENT_GUC : 0;
> -	mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
> -	err = write_and_verify(gt, DMA_GUC_WOPCM_OFFSET,
> -			       wopcm->guc.base | huc_agent, mask,
> -			       GUC_WOPCM_OFFSET_VALID);
> -	if (err)
> -		goto err_out;
> -
> -	return 0;
> -
> -err_out:
> -	DRM_ERROR("Failed to init WOPCM registers:\n");
> -	DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
> -		  intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET));
> -	DRM_ERROR("GUC_WOPCM_SIZE=%#x\n",
> -		  intel_uncore_read(uncore, GUC_WOPCM_SIZE));
> -
> -	return err;
> -}
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.h  
> b/drivers/gpu/drm/i915/intel_wopcm.h
> index 56aaed4d64ff..e1f0f66aaa44 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.h
> +++ b/drivers/gpu/drm/i915/intel_wopcm.h
> @@ -9,8 +9,6 @@
> #include <linux/types.h>
> -struct intel_gt;
> -
>  /**
>   * struct intel_wopcm - Overall WOPCM info and WOPCM regions.
>   * @size: Size of overall WOPCM.
> @@ -43,6 +41,5 @@ static inline u32 intel_wopcm_guc_size(struct  
> intel_wopcm *wopcm)
> void intel_wopcm_init_early(struct intel_wopcm *wopcm);
>  int intel_wopcm_init(struct intel_wopcm *wopcm);
> -int intel_wopcm_init_hw(struct intel_wopcm *wopcm, struct intel_gt *gt);
> #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915/uc: move uc early functions inside the gt ones

[Michal Wajdeczko]

On Tue, 30 Jul 2019 01:47:24 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> uc is a subcomponent of GT, so initialize/clean it as part of it. The
> wopcm_init_early doesn't have to be happen before the uC one, but since
> in other parts of the code we consider wopcm first do the same for
> consistency.

nit: s/uc/uC
nit: s/wopcm/WOPCM

>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915: move gt_cleanup_early out of gem_cleanup_early

[Daniele Ceraolo Spurio]

On 7/30/19 1:01 AM, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-30 08:19:18)
>>
>> On 30/07/2019 00:47, Daniele Ceraolo Spurio wrote:
>>> We don't call the init_early function from within the gem code, so we
>>> shouldn't do it for the cleanup either.
>>>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_drv.c | 2 ++
>>>    drivers/gpu/drm/i915/i915_gem.c | 2 --
>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>> index f2d3d754af37..934e605e2466 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -951,6 +951,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
>>>        intel_uc_cleanup_early(&dev_priv->gt.uc);
>>>        i915_gem_cleanup_early(dev_priv);
>>>    err_workqueues:
>>> +     intel_gt_cleanup_early(&dev_priv->gt);
>>>        i915_workqueues_cleanup(dev_priv);
>>>        return ret;
>>>    }
>>> @@ -966,6 +967,7 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv)
>>>        intel_power_domains_cleanup(dev_priv);
>>>        intel_uc_cleanup_early(&dev_priv->gt.uc);
>>>        i915_gem_cleanup_early(dev_priv);
>>> +     intel_gt_cleanup_early(&dev_priv->gt);
> 
> Note the change in naming convention, intel_gt_driver_late_release().
> -Chris
> 

Does it make sense to flip only the gt function? it'd look terribly out 
of place close to all those other cleanup_early() calls. I can follow up 
with a patch to flip them all at the same time if that works for you.

Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915/gt: introduce intel_gt_suspend

[Daniele Ceraolo Spurio]

On 7/30/19 1:12 AM, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-30 08:33:28)
>>
>> On 30/07/2019 00:47, Daniele Ceraolo Spurio wrote:
>>> For symmetry with intel_gt_resume and to hide more stuff from the top
>>> level under intel_gt. Note that the switch_to_kernel_context_sync has
>>> not been moved dure to the locking and ordering requirements that exist
>>
>> Typo in due.
>>
>> Should we add intel_gt_suspend_early/late, with the former dealing with
>> switch_to_kernel_context_sync? Although that raises an interesting
>> conundrum since it operates on GEM and GT. Best then to leave it
>> unwrapped for now I think.
>>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Ah, I wouldn't apply this patch yet for exactly that reason: we
> shouldn't be driving GT suspend from inside GEM, so would wait until we
> have a few more pieces of the puzzle reviewed^W unravelled.
> -Chris
> 

Ok, I'll drop the last 2 patches in the series for now.

Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915: move gt_cleanup_early out of gem_cleanup_early

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-07-30 15:56:57)
> 
> 
> On 7/30/19 1:01 AM, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-30 08:19:18)
> >>
> >> On 30/07/2019 00:47, Daniele Ceraolo Spurio wrote:
> >>> We don't call the init_early function from within the gem code, so we
> >>> shouldn't do it for the cleanup either.
> >>>
> >>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_drv.c | 2 ++
> >>>    drivers/gpu/drm/i915/i915_gem.c | 2 --
> >>>    2 files changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >>> index f2d3d754af37..934e605e2466 100644
> >>> --- a/drivers/gpu/drm/i915/i915_drv.c
> >>> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >>> @@ -951,6 +951,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
> >>>        intel_uc_cleanup_early(&dev_priv->gt.uc);
> >>>        i915_gem_cleanup_early(dev_priv);
> >>>    err_workqueues:
> >>> +     intel_gt_cleanup_early(&dev_priv->gt);
> >>>        i915_workqueues_cleanup(dev_priv);
> >>>        return ret;
> >>>    }
> >>> @@ -966,6 +967,7 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv)
> >>>        intel_power_domains_cleanup(dev_priv);
> >>>        intel_uc_cleanup_early(&dev_priv->gt.uc);
> >>>        i915_gem_cleanup_early(dev_priv);
> >>> +     intel_gt_cleanup_early(&dev_priv->gt);
> > 
> > Note the change in naming convention, intel_gt_driver_late_release().
> > -Chris
> > 
> 
> Does it make sense to flip only the gt function? it'd look terribly out 
> of place close to all those other cleanup_early() calls. I can follow up 
> with a patch to flip them all at the same time if that works for you.

Baby steps, the consistency is in the function callgraphs -- it looks
out of place to have late_release call cleanup_early :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC PATCH] drm/i915/uc: uc_wopcm_init_hw() can be static

[kbuild test robot]

Fixes: 6b6fa175ec57 ("drm/i915/uc: move uC WOPCM setup in uc_init_hw")
Signed-off-by: kbuild test robot <lkp@intel.com>
---
 intel_uc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 2f71f3704c4671..383f048e490092 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -407,7 +407,7 @@ write_and_verify(struct intel_gt *gt,
 }
 
 /* Initialize and verify the uC regs related to uC positioning in WOPCM */
-int uc_wopcm_init_hw(struct intel_uc *uc)
+static int uc_wopcm_init_hw(struct intel_uc *uc)
 {
 	struct intel_gt *gt = uc_to_gt(uc);
 	struct intel_wopcm *wopcm = &gt->i915->wopcm;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915/uc: move uC WOPCM setup in uc_init_hw

[kbuild test robot]

Hi Daniele,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[cannot apply to v5.3-rc2 next-20190730]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Daniele-Ceraolo-Spurio/Call-uC-functions-from-GT-ones/20190730-131043
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-rc1-7-g2b96cd8-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/gpu/drm/i915/gt/uc/intel_uc.c:410:5: sparse: sparse: symbol 'uc_wopcm_init_hw' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915/uc: move uC WOPCM setup in uc_init_hw

[Daniele Ceraolo Spurio]

On 7/30/19 7:39 AM, Michal Wajdeczko wrote:
> On Tue, 30 Jul 2019 01:47:22 +0200, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@intel.com> wrote:
> 
>> The register we write are not WOPCM regs but uC ones related to how
>> GuC and HuC are going to use the WOPCM, so it makes logical sense
>> for them to be programmed as part of uc_init_hw. The wopcm map on the
> 
> s/wopcm/WOPCM
> 
>> other side is not uC-specific (although that is our main use-case), so
>> keep that separate.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>  drivers/gpu/drm/i915/gt/uc/intel_uc.c | 62 ++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/i915_gem.c       |  8 +---
>>  drivers/gpu/drm/i915/intel_wopcm.c    | 68 ---------------------------
>>  drivers/gpu/drm/i915/intel_wopcm.h    |  3 --
>>  4 files changed, 63 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index fafa9be1e12a..2f71f3704c46 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -390,6 +390,63 @@ void intel_uc_sanitize(struct intel_uc *uc)
>>      __uc_sanitize(uc);
>>  }
>> +static int
>> +write_and_verify(struct intel_gt *gt,
>> +         i915_reg_t reg, u32 val, u32 mask, u32 locked_bit)
> 
> as this function is more 'uncore' than 'gt' I would define it as:
> 
> static inline bool
> intel_uncore_write_and_verify(struct intel_uncore *uncore,
>                                i915_reg_t reg, u32 value,
>                                u32 expected_value, u32 mask)
> in intel_uncore.h
> 

ok

>> +{
>> +    struct intel_uncore *uncore = gt->uncore;
>> +    u32 reg_val;
>> +
>> +    GEM_BUG_ON(val & ~mask);
>> +
>> +    intel_uncore_write(uncore, reg, val);
>> +
>> +    reg_val = intel_uncore_read(uncore, reg);
>> +
>> +    return (reg_val & mask) != (val | locked_bit) ? -EIO : 0;
>> +}
>> +
>> +/* Initialize and verify the uC regs related to uC positioning in 
>> WOPCM */
>> +int uc_wopcm_init_hw(struct intel_uc *uc)
> 
> static int uc_init_wopcm()
> 
>> +{
>> +    struct intel_gt *gt = uc_to_gt(uc);
>> +    struct intel_wopcm *wopcm = &gt->i915->wopcm;
>> +    struct intel_uncore *uncore = gt->uncore;
>> +    u32 huc_agent;
>> +    u32 mask;
>> +    int err;
>> +
>> +    GEM_BUG_ON(!HAS_GT_UC(gt->i915));
>> +    GEM_BUG_ON(!intel_uc_is_using_guc(uc));
>> +    GEM_BUG_ON(!wopcm->guc.size);
> 
> on one hand there is intel_wopcm_guc_size() that can be used here,
> but on other hand there is no intel_wopcm_guc_base ;(
> 
>> +    GEM_BUG_ON(!wopcm->guc.base);
>> +
>> +    err = write_and_verify(gt, GUC_WOPCM_SIZE, wopcm->guc.size,
>> +                   GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
>> +                   GUC_WOPCM_SIZE_LOCKED);
> 
> hmm, as these are write-once registers, maybe we should write only once
> (not here) and only verify every time in uc_init_hw ?
> 

AFAIK they don't survive deep sleep states even if they're write once, 
so we do need to write them again in some occasions. We could read them 
first and only write them if they're not locked, but IMO it's simpler to 
just unconditionally emit the write every time.

>> +    if (err)
>> +        goto err_out;
>> +
>> +    huc_agent = intel_uc_is_using_huc(uc) ? HUC_LOADING_AGENT_GUC : 0;
>> +    mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
>> +    err = write_and_verify(gt, DMA_GUC_WOPCM_OFFSET,
>> +                   wopcm->guc.base | huc_agent, mask,
>> +                   GUC_WOPCM_OFFSET_VALID);
>> +    if (err)
>> +        goto err_out;
>> +
>> +    return 0;
>> +
>> +err_out:
>> +    DRM_ERROR("Failed to init WOPCM registers:\n");
> 
> In commit msg you said that these are not WOPCM registers ;)
> 

ops :)

>> +    DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
>> +          intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET));
>> +    DRM_ERROR("GUC_WOPCM_SIZE=%#x\n",
>> +          intel_uncore_read(uncore, GUC_WOPCM_SIZE));
> 
> btw, we can avoid extra read by reporting already failed write
> in intel_uncore_write_and_verify()
> 

intel_uncore_write_and_verify(0 is better kept generic using a bool 
return IMO. We only do the extra reads in an error path anyway, so it 
shouldn't be an issue.

>> +
>> +    return err;
>> +}
>> +
>>  int intel_uc_init_hw(struct intel_uc *uc)
>>  {
>>      struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
>> @@ -402,6 +459,10 @@ int intel_uc_init_hw(struct intel_uc *uc)
>>     GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
>> +    ret = uc_wopcm_init_hw(uc);
>> +    if (ret)
>> +        goto out;
> 
> it should be harmless to reuse existing err_out label
> 

ack.

Thanks,
Daniele

>> +
>>      guc_reset_interrupts(guc);
>>     /* WaEnableuKernelHeaderValidFix:skl */
>> @@ -486,6 +547,7 @@ int intel_uc_init_hw(struct intel_uc *uc)
>>      if (GEM_WARN_ON(ret == -EIO))
>>          ret = -EINVAL;
>> +out:
>>      dev_err(i915->drm.dev, "GuC initialization failed %d\n", ret);
>>      return ret;
>>  }
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index 01dd0d1d9bf6..ae4e7cc3e3f9 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1239,14 +1239,8 @@ int i915_gem_init_hw(struct drm_i915_private 
>> *i915)
>>          goto out;
>>      }
>> -    ret = intel_wopcm_init_hw(&i915->wopcm, gt);
>> -    if (ret) {
>> -        DRM_ERROR("Enabling WOPCM failed (%d)\n", ret);
>> -        goto out;
>> -    }
>> -
>>      /* We can't enable contexts until all firmware is loaded */
>> -    ret = intel_uc_init_hw(&i915->gt.uc);
>> +    ret = intel_uc_init_hw(&gt->uc);
>>      if (ret) {
>>          DRM_ERROR("Enabling uc failed (%d)\n", ret);
>>          goto out;
>> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
>> b/drivers/gpu/drm/i915/intel_wopcm.c
>> index 0e86a9e85b49..d9973c0b0384 100644
>> --- a/drivers/gpu/drm/i915/intel_wopcm.c
>> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
>> @@ -224,71 +224,3 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>>     return 0;
>>  }
>> -
>> -static int
>> -write_and_verify(struct intel_gt *gt,
>> -         i915_reg_t reg, u32 val, u32 mask, u32 locked_bit)
>> -{
>> -    struct intel_uncore *uncore = gt->uncore;
>> -    u32 reg_val;
>> -
>> -    GEM_BUG_ON(val & ~mask);
>> -
>> -    intel_uncore_write(uncore, reg, val);
>> -
>> -    reg_val = intel_uncore_read(uncore, reg);
>> -
>> -    return (reg_val & mask) != (val | locked_bit) ? -EIO : 0;
>> -}
>> -
>> -/**
>> - * intel_wopcm_init_hw() - Setup GuC WOPCM registers.
>> - * @wopcm: pointer to intel_wopcm.
>> - * @gt: pointer to the containing GT
>> - *
>> - * Setup the GuC WOPCM size and offset registers with the calculated 
>> values. It
>> - * will verify the register values to make sure the registers are 
>> locked with
>> - * correct values.
>> - *
>> - * Return: 0 on success. -EIO if registers were locked with incorrect 
>> values.
>> - */
>> -int intel_wopcm_init_hw(struct intel_wopcm *wopcm, struct intel_gt *gt)
>> -{
>> -    struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
>> -    struct intel_uncore *uncore = gt->uncore;
>> -    u32 huc_agent;
>> -    u32 mask;
>> -    int err;
>> -
>> -    if (!USES_GUC(i915))
>> -        return 0;
>> -
>> -    GEM_BUG_ON(!HAS_GT_UC(i915));
>> -    GEM_BUG_ON(!wopcm->guc.size);
>> -    GEM_BUG_ON(!wopcm->guc.base);
>> -
>> -    err = write_and_verify(gt, GUC_WOPCM_SIZE, wopcm->guc.size,
>> -                   GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
>> -                   GUC_WOPCM_SIZE_LOCKED);
>> -    if (err)
>> -        goto err_out;
>> -
>> -    huc_agent = USES_HUC(i915) ? HUC_LOADING_AGENT_GUC : 0;
>> -    mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
>> -    err = write_and_verify(gt, DMA_GUC_WOPCM_OFFSET,
>> -                   wopcm->guc.base | huc_agent, mask,
>> -                   GUC_WOPCM_OFFSET_VALID);
>> -    if (err)
>> -        goto err_out;
>> -
>> -    return 0;
>> -
>> -err_out:
>> -    DRM_ERROR("Failed to init WOPCM registers:\n");
>> -    DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
>> -          intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET));
>> -    DRM_ERROR("GUC_WOPCM_SIZE=%#x\n",
>> -          intel_uncore_read(uncore, GUC_WOPCM_SIZE));
>> -
>> -    return err;
>> -}
>> diff --git a/drivers/gpu/drm/i915/intel_wopcm.h 
>> b/drivers/gpu/drm/i915/intel_wopcm.h
>> index 56aaed4d64ff..e1f0f66aaa44 100644
>> --- a/drivers/gpu/drm/i915/intel_wopcm.h
>> +++ b/drivers/gpu/drm/i915/intel_wopcm.h
>> @@ -9,8 +9,6 @@
>> #include <linux/types.h>
>> -struct intel_gt;
>> -
>>  /**
>>   * struct intel_wopcm - Overall WOPCM info and WOPCM regions.
>>   * @size: Size of overall WOPCM.
>> @@ -43,6 +41,5 @@ static inline u32 intel_wopcm_guc_size(struct 
>> intel_wopcm *wopcm)
>> void intel_wopcm_init_early(struct intel_wopcm *wopcm);
>>  int intel_wopcm_init(struct intel_wopcm *wopcm);
>> -int intel_wopcm_init_hw(struct intel_wopcm *wopcm, struct intel_gt *gt);
>> #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx