[PATCH v7 1/4] drm/i915: Introduce intel_gt_mcr_lock_sanitize()

[PATCH v7 1/4] drm/i915: Introduce intel_gt_mcr_lock_sanitize()

[Nirmoy Das]

Implement intel_gt_mcr_lock_sanitize() to provide a mechanism
for cleaning the steer semaphore when absolutely necessary.

v2: remove unnecessary lock(Andi, Matt)
    improve the kernel doc(Matt)
    s/intel_gt_mcr_lock_clear/intel_gt_mcr_lock_sanitize

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 22 ++++++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_gt_mcr.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
index bf4a933de03a..326c2ed1d99b 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
@@ -419,6 +419,28 @@ void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags)
 		intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
 }
 
+/**
+ * intel_gt_mcr_lock_sanitize - Sanitize MCR steering lock
+ * @gt: GT structure
+ *
+ * This will be used to sanitize the initial status of the hardware lock
+ * during driver load and resume since there won't be any concurrent access
+ * from other agents at those times, but it's possible that boot firmware
+ * may have left the lock in a bad state.
+ *
+ */
+void intel_gt_mcr_lock_sanitize(struct intel_gt *gt)
+{
+	/*
+	 * This gets called at load/resume time, so we shouldn't be
+	 * racing with other driver threads grabbing the mcr lock.
+	 */
+	lockdep_assert_not_held(&gt->mcr_lock);
+
+	if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
+		intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
+}
+
 /**
  * intel_gt_mcr_read - read a specific instance of an MCR register
  * @gt: GT structure
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
index 41684495b7da..01ac565a56a4 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
@@ -11,6 +11,7 @@
 void intel_gt_mcr_init(struct intel_gt *gt);
 void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags);
 void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags);
+void intel_gt_mcr_lock_sanitize(struct intel_gt *gt);
 
 u32 intel_gt_mcr_read(struct intel_gt *gt,
 		      i915_mcr_reg_t reg,
-- 
2.41.0

[PATCH v7 2/4] drm/i915: Introduce the intel_gt_resume_early()

[Nirmoy Das]

Move early resume functions of gt to a proper file.

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_pm.c | 6 ++++++
 drivers/gpu/drm/i915/gt/intel_gt_pm.h | 1 +
 drivers/gpu/drm/i915/i915_driver.c    | 6 ++----
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index 5a942af0a14e..dab73980c9f1 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -216,6 +216,12 @@ void intel_gt_pm_fini(struct intel_gt *gt)
 	intel_rc6_fini(&gt->rc6);
 }
 
+void intel_gt_resume_early(struct intel_gt *gt)
+{
+	intel_uncore_resume_early(gt->uncore);
+	intel_gt_check_and_clear_faults(gt);
+}
+
 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 6c9a46452364..b1eeb5b33918 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
@@ -78,6 +78,7 @@ void intel_gt_pm_fini(struct intel_gt *gt);
 void intel_gt_suspend_prepare(struct intel_gt *gt);
 void intel_gt_suspend_late(struct intel_gt *gt);
 int intel_gt_resume(struct intel_gt *gt);
+void intel_gt_resume_early(struct intel_gt *gt);
 
 void intel_gt_runtime_suspend(struct intel_gt *gt);
 int intel_gt_runtime_resume(struct intel_gt *gt);
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index d50347e5773a..78501a83ba10 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1327,10 +1327,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
 		drm_err(&dev_priv->drm,
 			"Resume prepare failed: %d, continuing anyway\n", ret);
 
-	for_each_gt(gt, dev_priv, i) {
-		intel_uncore_resume_early(gt->uncore);
-		intel_gt_check_and_clear_faults(gt);
-	}
+	for_each_gt(gt, dev_priv, i)
+		intel_gt_resume_early(gt);
 
 	intel_display_power_resume_early(dev_priv);
 
-- 
2.41.0

Re: [PATCH v7 2/4] drm/i915: Introduce the intel_gt_resume_early()

[Andi Shyti]

Hi Nirmoy,

On Thu, Sep 28, 2023 at 03:00:13PM +0200, Nirmoy Das wrote:
> Move early resume functions of gt to a proper file.
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Andi

Re: [Intel-gfx] [PATCH v7 2/4] drm/i915: Introduce the intel_gt_resume_early()

[Andrzej Hajda]

On 28.09.2023 15:00, Nirmoy Das wrote:
> Move early resume functions of gt to a proper file.
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej

> ---
>   drivers/gpu/drm/i915/gt/intel_gt_pm.c | 6 ++++++
>   drivers/gpu/drm/i915/gt/intel_gt_pm.h | 1 +
>   drivers/gpu/drm/i915/i915_driver.c    | 6 ++----
>   3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> index 5a942af0a14e..dab73980c9f1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> @@ -216,6 +216,12 @@ void intel_gt_pm_fini(struct intel_gt *gt)
>   	intel_rc6_fini(&gt->rc6);
>   }
>   
> +void intel_gt_resume_early(struct intel_gt *gt)
> +{
> +	intel_uncore_resume_early(gt->uncore);
> +	intel_gt_check_and_clear_faults(gt);
> +}
> +
>   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 6c9a46452364..b1eeb5b33918 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> @@ -78,6 +78,7 @@ void intel_gt_pm_fini(struct intel_gt *gt);
>   void intel_gt_suspend_prepare(struct intel_gt *gt);
>   void intel_gt_suspend_late(struct intel_gt *gt);
>   int intel_gt_resume(struct intel_gt *gt);
> +void intel_gt_resume_early(struct intel_gt *gt);
>   
>   void intel_gt_runtime_suspend(struct intel_gt *gt);
>   int intel_gt_runtime_resume(struct intel_gt *gt);
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index d50347e5773a..78501a83ba10 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1327,10 +1327,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
>   		drm_err(&dev_priv->drm,
>   			"Resume prepare failed: %d, continuing anyway\n", ret);
>   
> -	for_each_gt(gt, dev_priv, i) {
> -		intel_uncore_resume_early(gt->uncore);
> -		intel_gt_check_and_clear_faults(gt);
> -	}
> +	for_each_gt(gt, dev_priv, i)
> +		intel_gt_resume_early(gt);
>   
>   	intel_display_power_resume_early(dev_priv);
>   

[PATCH v7 3/4] drm/i915: Clean steer semaphore on resume

[Nirmoy Das]

During resume, the steer semaphore on GT1 was observed to be held. The
hardware team has confirmed the safety of clearing steer semaphores
for all GTs during driver load/resume, as no lock acquisitions can occur
in this process by other agents.

v2: reset on resume not in intel_gt_init().
v3: do the reset on intel_gt_resume_early()
v4: do general sanitization for all GTs(Matt)

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_pm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index dab73980c9f1..3461f3e74277 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -13,6 +13,7 @@
 #include "intel_engine_pm.h"
 #include "intel_gt.h"
 #include "intel_gt_clock_utils.h"
+#include "intel_gt_mcr.h"
 #include "intel_gt_pm.h"
 #include "intel_gt_print.h"
 #include "intel_gt_requests.h"
@@ -218,6 +219,15 @@ void intel_gt_pm_fini(struct intel_gt *gt)
 
 void intel_gt_resume_early(struct intel_gt *gt)
 {
+	/*
+	 * Sanitize steer semaphores during driver resume. This is necessary
+	 * to address observed cases of steer semaphores being
+	 * held after a suspend operation. Confirmation from the hardware team
+	 * assures the safety of this operation, as no lock acquisitions
+	 * by other agents occur during driver load/resume process.
+	 */
+	intel_gt_mcr_lock_sanitize(gt);
+
 	intel_uncore_resume_early(gt->uncore);
 	intel_gt_check_and_clear_faults(gt);
 }
-- 
2.41.0

[PATCH v7 4/4] drm/i915/mtl: Skip MCR ops for ring fault register

[Nirmoy Das]

On MTL GEN12_RING_FAULT_REG is not replicated so don't
do mcr based operation for this register.

v2: use MEDIA_VER() instead of GRAPHICS_VER()(Matt).
v3: s/"MEDIA_VER(i915) == 13"/"MEDIA_VER(i915) >= 13"(Matt)
    improve comment.
v4: improve the comment further(Andi)

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt.c      | 13 ++++++++++++-
 drivers/gpu/drm/i915/gt/intel_gt_regs.h |  1 +
 drivers/gpu/drm/i915/i915_gpu_error.c   | 11 ++++++++++-
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 93062c35e072..dff8bba1f5d4 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -262,10 +262,21 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
 				   I915_MASTER_ERROR_INTERRUPT);
 	}
 
-	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
+	/*
+	 * For the media GT, this ring fault register is not replicated,
+	 * so don't do multicast/replicated register read/write operation on it.
+	 */
+	if (MEDIA_VER(i915) >= 13 && gt->type == GT_MEDIA) {
+		intel_uncore_rmw(uncore, XELPMP_RING_FAULT_REG,
+				 RING_FAULT_VALID, 0);
+		intel_uncore_posting_read(uncore,
+					  XELPMP_RING_FAULT_REG);
+
+	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
 		intel_gt_mcr_multicast_rmw(gt, XEHP_RING_FAULT_REG,
 					   RING_FAULT_VALID, 0);
 		intel_gt_mcr_read_any(gt, XEHP_RING_FAULT_REG);
+
 	} else if (GRAPHICS_VER(i915) >= 12) {
 		intel_uncore_rmw(uncore, GEN12_RING_FAULT_REG, RING_FAULT_VALID, 0);
 		intel_uncore_posting_read(uncore, GEN12_RING_FAULT_REG);
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
index cca4bac8f8b0..eecd0a87a647 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
@@ -1084,6 +1084,7 @@
 
 #define GEN12_RING_FAULT_REG			_MMIO(0xcec4)
 #define XEHP_RING_FAULT_REG			MCR_REG(0xcec4)
+#define XELPMP_RING_FAULT_REG			_MMIO(0xcec4)
 #define   GEN8_RING_FAULT_ENGINE_ID(x)		(((x) >> 12) & 0x7)
 #define   RING_FAULT_GTTSEL_MASK		(1 << 11)
 #define   RING_FAULT_SRCID(x)			(((x) >> 3) & 0xff)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index f4ebcfb70289..b4e31e59c799 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1234,7 +1234,16 @@ static void engine_record_registers(struct intel_engine_coredump *ee)
 	if (GRAPHICS_VER(i915) >= 6) {
 		ee->rc_psmi = ENGINE_READ(engine, RING_PSMI_CTL);
 
-		if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50))
+		/*
+		 * For the media GT, this ring fault register is not replicated,
+		 * so don't do multicast/replicated register read/write
+		 * operation on it.
+		 */
+		if (MEDIA_VER(i915) >= 13 && engine->gt->type == GT_MEDIA)
+			ee->fault_reg = intel_uncore_read(engine->uncore,
+							  XELPMP_RING_FAULT_REG);
+
+		else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50))
 			ee->fault_reg = intel_gt_mcr_read_any(engine->gt,
 							      XEHP_RING_FAULT_REG);
 		else if (GRAPHICS_VER(i915) >= 12)
-- 
2.41.0

Re: [Intel-gfx] [PATCH v7 4/4] drm/i915/mtl: Skip MCR ops for ring fault register

[Andrzej Hajda]

On 28.09.2023 15:00, Nirmoy Das wrote:
> On MTL GEN12_RING_FAULT_REG is not replicated so don't
> do mcr based operation for this register.
> 
> v2: use MEDIA_VER() instead of GRAPHICS_VER()(Matt).
> v3: s/"MEDIA_VER(i915) == 13"/"MEDIA_VER(i915) >= 13"(Matt)
>      improve comment.
> v4: improve the comment further(Andi)
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_gt.c      | 13 ++++++++++++-
>   drivers/gpu/drm/i915/gt/intel_gt_regs.h |  1 +
>   drivers/gpu/drm/i915/i915_gpu_error.c   | 11 ++++++++++-
>   3 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 93062c35e072..dff8bba1f5d4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -262,10 +262,21 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
>   				   I915_MASTER_ERROR_INTERRUPT);
>   	}
>   
> -	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
> +	/*
> +	 * For the media GT, this ring fault register is not replicated,
> +	 * so don't do multicast/replicated register read/write operation on it.
> +	 */
> +	if (MEDIA_VER(i915) >= 13 && gt->type == GT_MEDIA) {
> +		intel_uncore_rmw(uncore, XELPMP_RING_FAULT_REG,
> +				 RING_FAULT_VALID, 0);
> +		intel_uncore_posting_read(uncore,
> +					  XELPMP_RING_FAULT_REG);
> +
> +	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {

WA 14017387313 suggests to "remove Semaphore acquisition steps for all 
GAM ranges" (XELPMP_RING_FAULT_REG is in GAM range), just FYI.

Regards
Andrzej


>   		intel_gt_mcr_multicast_rmw(gt, XEHP_RING_FAULT_REG,
>   					   RING_FAULT_VALID, 0);
>   		intel_gt_mcr_read_any(gt, XEHP_RING_FAULT_REG);
> +
>   	} else if (GRAPHICS_VER(i915) >= 12) {
>   		intel_uncore_rmw(uncore, GEN12_RING_FAULT_REG, RING_FAULT_VALID, 0);
>   		intel_uncore_posting_read(uncore, GEN12_RING_FAULT_REG);
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> index cca4bac8f8b0..eecd0a87a647 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> @@ -1084,6 +1084,7 @@
>   
>   #define GEN12_RING_FAULT_REG			_MMIO(0xcec4)
>   #define XEHP_RING_FAULT_REG			MCR_REG(0xcec4)
> +#define XELPMP_RING_FAULT_REG			_MMIO(0xcec4)
>   #define   GEN8_RING_FAULT_ENGINE_ID(x)		(((x) >> 12) & 0x7)
>   #define   RING_FAULT_GTTSEL_MASK		(1 << 11)
>   #define   RING_FAULT_SRCID(x)			(((x) >> 3) & 0xff)
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index f4ebcfb70289..b4e31e59c799 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1234,7 +1234,16 @@ static void engine_record_registers(struct intel_engine_coredump *ee)
>   	if (GRAPHICS_VER(i915) >= 6) {
>   		ee->rc_psmi = ENGINE_READ(engine, RING_PSMI_CTL);
>   
> -		if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50))
> +		/*
> +		 * For the media GT, this ring fault register is not replicated,
> +		 * so don't do multicast/replicated register read/write
> +		 * operation on it.
> +		 */
> +		if (MEDIA_VER(i915) >= 13 && engine->gt->type == GT_MEDIA)
> +			ee->fault_reg = intel_uncore_read(engine->uncore,
> +							  XELPMP_RING_FAULT_REG);
> +
> +		else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50))
>   			ee->fault_reg = intel_gt_mcr_read_any(engine->gt,
>   							      XEHP_RING_FAULT_REG);
>   		else if (GRAPHICS_VER(i915) >= 12)

Re: [Intel-gfx] [PATCH v7 4/4] drm/i915/mtl: Skip MCR ops for ring fault register

[Matt Roper]

On Fri, Sep 29, 2023 at 12:14:37AM +0200, Andrzej Hajda wrote:
> On 28.09.2023 15:00, Nirmoy Das wrote:
> > On MTL GEN12_RING_FAULT_REG is not replicated so don't
> > do mcr based operation for this register.
> > 
> > v2: use MEDIA_VER() instead of GRAPHICS_VER()(Matt).
> > v3: s/"MEDIA_VER(i915) == 13"/"MEDIA_VER(i915) >= 13"(Matt)
> >      improve comment.
> > v4: improve the comment further(Andi)
> > 
> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
> > Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_gt.c      | 13 ++++++++++++-
> >   drivers/gpu/drm/i915/gt/intel_gt_regs.h |  1 +
> >   drivers/gpu/drm/i915/i915_gpu_error.c   | 11 ++++++++++-
> >   3 files changed, 23 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> > index 93062c35e072..dff8bba1f5d4 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -262,10 +262,21 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
> >   				   I915_MASTER_ERROR_INTERRUPT);
> >   	}
> > -	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
> > +	/*
> > +	 * For the media GT, this ring fault register is not replicated,
> > +	 * so don't do multicast/replicated register read/write operation on it.
> > +	 */
> > +	if (MEDIA_VER(i915) >= 13 && gt->type == GT_MEDIA) {
> > +		intel_uncore_rmw(uncore, XELPMP_RING_FAULT_REG,
> > +				 RING_FAULT_VALID, 0);
> > +		intel_uncore_posting_read(uncore,
> > +					  XELPMP_RING_FAULT_REG);
> > +
> > +	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
> 
> WA 14017387313 suggests to "remove Semaphore acquisition steps for all GAM
> ranges" (XELPMP_RING_FAULT_REG is in GAM range), just FYI.

We've actually looked at that workaround before and decided that it
doesn't make sense to implement it on Linux.  The background for that
workaround is due to Windows driver design; their driver potentially
tries to access some MCR registers from within an interrupt handler,
which would cause problems if non-IRQ code grabs the semaphore, gets
interrupted, and then the interrupt handler deadlocks while also trying
to acquire it.  On Linux, we never access MCR registers from an
interrupt handler, so we're not susceptible to that issue.


Matt

> 
> Regards
> Andrzej
> 
> 
> >   		intel_gt_mcr_multicast_rmw(gt, XEHP_RING_FAULT_REG,
> >   					   RING_FAULT_VALID, 0);
> >   		intel_gt_mcr_read_any(gt, XEHP_RING_FAULT_REG);
> > +
> >   	} else if (GRAPHICS_VER(i915) >= 12) {
> >   		intel_uncore_rmw(uncore, GEN12_RING_FAULT_REG, RING_FAULT_VALID, 0);
> >   		intel_uncore_posting_read(uncore, GEN12_RING_FAULT_REG);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > index cca4bac8f8b0..eecd0a87a647 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > @@ -1084,6 +1084,7 @@
> >   #define GEN12_RING_FAULT_REG			_MMIO(0xcec4)
> >   #define XEHP_RING_FAULT_REG			MCR_REG(0xcec4)
> > +#define XELPMP_RING_FAULT_REG			_MMIO(0xcec4)
> >   #define   GEN8_RING_FAULT_ENGINE_ID(x)		(((x) >> 12) & 0x7)
> >   #define   RING_FAULT_GTTSEL_MASK		(1 << 11)
> >   #define   RING_FAULT_SRCID(x)			(((x) >> 3) & 0xff)
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index f4ebcfb70289..b4e31e59c799 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -1234,7 +1234,16 @@ static void engine_record_registers(struct intel_engine_coredump *ee)
> >   	if (GRAPHICS_VER(i915) >= 6) {
> >   		ee->rc_psmi = ENGINE_READ(engine, RING_PSMI_CTL);
> > -		if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50))
> > +		/*
> > +		 * For the media GT, this ring fault register is not replicated,
> > +		 * so don't do multicast/replicated register read/write
> > +		 * operation on it.
> > +		 */
> > +		if (MEDIA_VER(i915) >= 13 && engine->gt->type == GT_MEDIA)
> > +			ee->fault_reg = intel_uncore_read(engine->uncore,
> > +							  XELPMP_RING_FAULT_REG);
> > +
> > +		else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50))
> >   			ee->fault_reg = intel_gt_mcr_read_any(engine->gt,
> >   							      XEHP_RING_FAULT_REG);
> >   		else if (GRAPHICS_VER(i915) >= 12)
> 

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

Re: [PATCH v7 1/4] drm/i915: Introduce intel_gt_mcr_lock_sanitize()

[Andi Shyti]

Hi Nirmoy,

On Thu, Sep 28, 2023 at 03:00:12PM +0200, Nirmoy Das wrote:
> Implement intel_gt_mcr_lock_sanitize() to provide a mechanism
> for cleaning the steer semaphore when absolutely necessary.
> 
> v2: remove unnecessary lock(Andi, Matt)
>     improve the kernel doc(Matt)
>     s/intel_gt_mcr_lock_clear/intel_gt_mcr_lock_sanitize
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Andi

Re: [PATCH v7 1/4] drm/i915: Introduce intel_gt_mcr_lock_sanitize()

[Matt Roper]

On Thu, Sep 28, 2023 at 03:00:12PM +0200, Nirmoy Das wrote:
> Implement intel_gt_mcr_lock_sanitize() to provide a mechanism
> for cleaning the steer semaphore when absolutely necessary.
> 
> v2: remove unnecessary lock(Andi, Matt)
>     improve the kernel doc(Matt)
>     s/intel_gt_mcr_lock_clear/intel_gt_mcr_lock_sanitize
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 22 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/gt/intel_gt_mcr.h |  1 +
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> index bf4a933de03a..326c2ed1d99b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> @@ -419,6 +419,28 @@ void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags)
>  		intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
>  }
>  
> +/**
> + * intel_gt_mcr_lock_sanitize - Sanitize MCR steering lock
> + * @gt: GT structure
> + *
> + * This will be used to sanitize the initial status of the hardware lock
> + * during driver load and resume since there won't be any concurrent access
> + * from other agents at those times, but it's possible that boot firmware
> + * may have left the lock in a bad state.
> + *
> + */
> +void intel_gt_mcr_lock_sanitize(struct intel_gt *gt)
> +{
> +	/*
> +	 * This gets called at load/resume time, so we shouldn't be
> +	 * racing with other driver threads grabbing the mcr lock.
> +	 */
> +	lockdep_assert_not_held(&gt->mcr_lock);
> +
> +	if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
> +		intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
> +}
> +
>  /**
>   * intel_gt_mcr_read - read a specific instance of an MCR register
>   * @gt: GT structure
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> index 41684495b7da..01ac565a56a4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> @@ -11,6 +11,7 @@
>  void intel_gt_mcr_init(struct intel_gt *gt);
>  void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags);
>  void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags);
> +void intel_gt_mcr_lock_sanitize(struct intel_gt *gt);
>  
>  u32 intel_gt_mcr_read(struct intel_gt *gt,
>  		      i915_mcr_reg_t reg,
> -- 
> 2.41.0
> 

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

Re: [Intel-gfx] [PATCH v7 1/4] drm/i915: Introduce intel_gt_mcr_lock_sanitize()

[Andrzej Hajda]

On 28.09.2023 15:00, Nirmoy Das wrote:
> Implement intel_gt_mcr_lock_sanitize() to provide a mechanism
> for cleaning the steer semaphore when absolutely necessary.
> 
> v2: remove unnecessary lock(Andi, Matt)
>      improve the kernel doc(Matt)
>      s/intel_gt_mcr_lock_clear/intel_gt_mcr_lock_sanitize
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 22 ++++++++++++++++++++++
>   drivers/gpu/drm/i915/gt/intel_gt_mcr.h |  1 +
>   2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> index bf4a933de03a..326c2ed1d99b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> @@ -419,6 +419,28 @@ void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags)
>   		intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
>   }
>   
> +/**
> + * intel_gt_mcr_lock_sanitize - Sanitize MCR steering lock
> + * @gt: GT structure
> + *
> + * This will be used to sanitize the initial status of the hardware lock
> + * during driver load and resume since there won't be any concurrent access
> + * from other agents at those times, but it's possible that boot firmware
> + * may have left the lock in a bad state.
> + *
> + */
> +void intel_gt_mcr_lock_sanitize(struct intel_gt *gt)
> +{
> +	/*
> +	 * This gets called at load/resume time, so we shouldn't be
> +	 * racing with other driver threads grabbing the mcr lock.
> +	 */
> +	lockdep_assert_not_held(&gt->mcr_lock);
> +
> +	if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
> +		intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);

I wonder if it wouldn't be useful to check and report if it is locked 
before unconditional release, no strong feelings.

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej


> +}
> +
>   /**
>    * intel_gt_mcr_read - read a specific instance of an MCR register
>    * @gt: GT structure
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> index 41684495b7da..01ac565a56a4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> @@ -11,6 +11,7 @@
>   void intel_gt_mcr_init(struct intel_gt *gt);
>   void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags);
>   void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags);
> +void intel_gt_mcr_lock_sanitize(struct intel_gt *gt);
>   
>   u32 intel_gt_mcr_read(struct intel_gt *gt,
>   		      i915_mcr_reg_t reg,

Re: [Intel-gfx] [PATCH v7 1/4] drm/i915: Introduce intel_gt_mcr_lock_sanitize()

[Nirmoy Das]

On 9/28/2023 11:42 PM, Andrzej Hajda wrote:
> On 28.09.2023 15:00, Nirmoy Das wrote:
>> Implement intel_gt_mcr_lock_sanitize() to provide a mechanism
>> for cleaning the steer semaphore when absolutely necessary.
>>
>> v2: remove unnecessary lock(Andi, Matt)
>>      improve the kernel doc(Matt)
>>      s/intel_gt_mcr_lock_clear/intel_gt_mcr_lock_sanitize
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 22 ++++++++++++++++++++++
>>   drivers/gpu/drm/i915/gt/intel_gt_mcr.h |  1 +
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c 
>> b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>> index bf4a933de03a..326c2ed1d99b 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>> @@ -419,6 +419,28 @@ void intel_gt_mcr_unlock(struct intel_gt *gt, 
>> unsigned long flags)
>>           intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
>>   }
>>   +/**
>> + * intel_gt_mcr_lock_sanitize - Sanitize MCR steering lock
>> + * @gt: GT structure
>> + *
>> + * This will be used to sanitize the initial status of the hardware 
>> lock
>> + * during driver load and resume since there won't be any concurrent 
>> access
>> + * from other agents at those times, but it's possible that boot 
>> firmware
>> + * may have left the lock in a bad state.
>> + *
>> + */
>> +void intel_gt_mcr_lock_sanitize(struct intel_gt *gt)
>> +{
>> +    /*
>> +     * This gets called at load/resume time, so we shouldn't be
>> +     * racing with other driver threads grabbing the mcr lock.
>> +     */
>> +    lockdep_assert_not_held(&gt->mcr_lock);
>> +
>> +    if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
>> +        intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
>
> I wonder if it wouldn't be useful to check and report if it is locked 
> before unconditional release, no strong feelings.
Not so useful for user but may be as debug log if we need.
>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>


Thanks,

Nirmoy

>
> Regards
> Andrzej
>
>
>> +}
>> +
>>   /**
>>    * intel_gt_mcr_read - read a specific instance of an MCR register
>>    * @gt: GT structure
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h 
>> b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
>> index 41684495b7da..01ac565a56a4 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
>> @@ -11,6 +11,7 @@
>>   void intel_gt_mcr_init(struct intel_gt *gt);
>>   void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags);
>>   void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags);
>> +void intel_gt_mcr_lock_sanitize(struct intel_gt *gt);
>>     u32 intel_gt_mcr_read(struct intel_gt *gt,
>>                 i915_mcr_reg_t reg,
>

Re: [PATCH v7 1/4] drm/i915: Introduce intel_gt_mcr_lock_sanitize()

[Nirmoy Das]

Thanks reviewing this series. Merged it in  gt-next so hopefully we have 
bit greener CI for MTL now.


Regards,

Nirmoy

On 9/28/2023 3:00 PM, Nirmoy Das wrote:
> Implement intel_gt_mcr_lock_sanitize() to provide a mechanism
> for cleaning the steer semaphore when absolutely necessary.
>
> v2: remove unnecessary lock(Andi, Matt)
>      improve the kernel doc(Matt)
>      s/intel_gt_mcr_lock_clear/intel_gt_mcr_lock_sanitize
>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 22 ++++++++++++++++++++++
>   drivers/gpu/drm/i915/gt/intel_gt_mcr.h |  1 +
>   2 files changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> index bf4a933de03a..326c2ed1d99b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> @@ -419,6 +419,28 @@ void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags)
>   		intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
>   }
>   
> +/**
> + * intel_gt_mcr_lock_sanitize - Sanitize MCR steering lock
> + * @gt: GT structure
> + *
> + * This will be used to sanitize the initial status of the hardware lock
> + * during driver load and resume since there won't be any concurrent access
> + * from other agents at those times, but it's possible that boot firmware
> + * may have left the lock in a bad state.
> + *
> + */
> +void intel_gt_mcr_lock_sanitize(struct intel_gt *gt)
> +{
> +	/*
> +	 * This gets called at load/resume time, so we shouldn't be
> +	 * racing with other driver threads grabbing the mcr lock.
> +	 */
> +	lockdep_assert_not_held(&gt->mcr_lock);
> +
> +	if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
> +		intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
> +}
> +
>   /**
>    * intel_gt_mcr_read - read a specific instance of an MCR register
>    * @gt: GT structure
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> index 41684495b7da..01ac565a56a4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> @@ -11,6 +11,7 @@
>   void intel_gt_mcr_init(struct intel_gt *gt);
>   void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags);
>   void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags);
> +void intel_gt_mcr_lock_sanitize(struct intel_gt *gt);
>   
>   u32 intel_gt_mcr_read(struct intel_gt *gt,
>   		      i915_mcr_reg_t reg,