[PATCH v3 00/12] GuC Interrupts/Log updates

[PATCH v3 00/12] GuC Interrupts/Log updates

[Sagar Arun Kamble]

This series addresses following features/fixes:
1. Restructuring to move GuC interrupts related functions to guc.c
2. Making GuC interrupts enable/disable reference based and tying up with
   logging at all places.
3. Handle suspend/resume/reset for GuC interrupts.
4. Logging fixes about RPM wakeref and skipping relay release during
   uc_fini.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Sagar Arun Kamble (12):
  drm/i915: Export low level PM IRQ functions to use from GuC functions
  drm/i915/guc: Move GuC interrupts related functions from i915_irq.c to
    intel_guc.c
  drm/i915/guc: Pass intel_guc struct parameter to GuC interrupts
    functions
  drm/i915/guc: Add description and comments about guc_log_level
    parameter
  drm/i915/guc: Fix GuC interrupts disabling with logging
  drm/i915/guc: Separate creation/release of runtime logging data from
    base logging data
  drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts
  drm/i915/guc: Make guc_log_level parameter immutable
  drm/i915/guc: Make GuC log related functions depend only on log level
  drm/i915/guc: Add client support to enable/disable GuC interrupts
  drm/i915/guc: Restore GuC interrupts across suspend/reset if enabled
  HAX: drm/i915/guc: enable GuC submission/logging for CI

 drivers/gpu/drm/i915/i915_debugfs.c  |   8 +-
 drivers/gpu/drm/i915/i915_drv.c      |   4 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c  |   8 +-
 drivers/gpu/drm/i915/i915_irq.c      |  78 ++-----------------
 drivers/gpu/drm/i915/i915_params.c   |   3 +-
 drivers/gpu/drm/i915/i915_params.h   |   4 +-
 drivers/gpu/drm/i915/intel_display.c |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   7 +-
 drivers/gpu/drm/i915/intel_guc.c     | 141 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_guc.h     |  13 +++-
 drivers/gpu/drm/i915/intel_guc_log.c |  64 +++++++---------
 drivers/gpu/drm/i915/intel_guc_log.h |   8 ++
 drivers/gpu/drm/i915/intel_uc.c      |  29 ++++---
 13 files changed, 225 insertions(+), 144 deletions(-)

-- 
1.9.1

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

[PATCH v3 01/12] drm/i915: Export low level PM IRQ functions to use from GuC functions

[Sagar Arun Kamble]

In order to separate GuC IRQ handling functions from i915_irq.c we need
to export the low level pm irq handlers. Export pm_iir, reset_pm_iir and
enable/disable_pm_irq functions.

v2-v3: Rebase.

Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c  | 8 ++++----
 drivers/gpu/drm/i915/intel_drv.h | 4 ++++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3517c65..7a9e1a7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -306,7 +306,7 @@ void gen5_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask)
 	ilk_update_gt_irq(dev_priv, mask, 0);
 }
 
-static i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv)
+i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv)
 {
 	return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IIR(2) : GEN6_PMIIR;
 }
@@ -369,7 +369,7 @@ void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
 	__gen6_mask_pm_irq(dev_priv, mask);
 }
 
-static void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask)
+void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask)
 {
 	i915_reg_t reg = gen6_pm_iir(dev_priv);
 
@@ -380,7 +380,7 @@ static void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask)
 	POSTING_READ(reg);
 }
 
-static void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mask)
+void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mask)
 {
 	lockdep_assert_held(&dev_priv->irq_lock);
 
@@ -390,7 +390,7 @@ static void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mas
 	/* unmask_pm_irq provides an implicit barrier (POSTING_READ) */
 }
 
-static void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_mask)
+void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_mask)
 {
 	lockdep_assert_held(&dev_priv->irq_lock);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 30f791f..3a7e18c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1234,8 +1234,12 @@ void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 /* i915_irq.c */
 void gen5_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void gen5_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
+i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv);
 void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask);
 void gen6_unmask_pm_irq(struct drm_i915_private *dev_priv, u32 mask);
+void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask);
+void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mask);
+void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_mask);
 void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv);
 void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv);
 void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv);
-- 
1.9.1

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

[PATCH v3 02/12] drm/i915/guc: Move GuC interrupts related functions from i915_irq.c to intel_guc.c

[Sagar Arun Kamble]

GuC interrupts handling is core GuC functionality. Better to keep it
with other core functions in intel_guc.c. Since they are used from
uC functions, GuC logging and i915 irq handling keeping them grouped in
intel_guc.c instead of intel_uc.c.

v2-v3: Rebase.

Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      | 70 +----------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  3 --
 drivers/gpu/drm/i915/intel_guc.c     | 71 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_guc.h     |  4 ++
 drivers/gpu/drm/i915/intel_guc_log.c |  6 +--
 drivers/gpu/drm/i915/intel_uc.c      |  8 ++--
 6 files changed, 81 insertions(+), 81 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7a9e1a7..3f4eff9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -203,7 +203,6 @@ static void gen2_assert_iir_is_zero(struct drm_i915_private *dev_priv,
 } while (0)
 
 static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
-static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
 
 /* For display hotplug interrupt */
 static inline void
@@ -450,38 +449,6 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
 	gen6_reset_rps_interrupts(dev_priv);
 }
 
-void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv)
-{
-	spin_lock_irq(&dev_priv->irq_lock);
-	gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
-	spin_unlock_irq(&dev_priv->irq_lock);
-}
-
-void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
-{
-	spin_lock_irq(&dev_priv->irq_lock);
-	if (!dev_priv->guc.interrupts_enabled) {
-		WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
-				       dev_priv->pm_guc_events);
-		dev_priv->guc.interrupts_enabled = true;
-		gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
-	}
-	spin_unlock_irq(&dev_priv->irq_lock);
-}
-
-void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
-{
-	spin_lock_irq(&dev_priv->irq_lock);
-	dev_priv->guc.interrupts_enabled = false;
-
-	gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
-
-	spin_unlock_irq(&dev_priv->irq_lock);
-	synchronize_irq(dev_priv->drm.irq);
-
-	gen9_reset_guc_interrupts(dev_priv);
-}
-
 /**
  * bdw_update_port_irq - update DE port interrupt
  * @dev_priv: driver private
@@ -1480,7 +1447,7 @@ static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
 		gen6_rps_irq_handler(dev_priv, gt_iir[2]);
 
 	if (gt_iir[2] & dev_priv->pm_guc_events)
-		gen9_guc_irq_handler(dev_priv, gt_iir[2]);
+		intel_guc_irq_handler(dev_priv, gt_iir[2]);
 }
 
 static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
@@ -1757,41 +1724,6 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
-static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
-{
-	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
-		/* Sample the log buffer flush related bits & clear them out now
-		 * itself from the message identity register to minimize the
-		 * probability of losing a flush interrupt, when there are back
-		 * to back flush interrupts.
-		 * There can be a new flush interrupt, for different log buffer
-		 * type (like for ISR), whilst Host is handling one (for DPC).
-		 * Since same bit is used in message register for ISR & DPC, it
-		 * could happen that GuC sets the bit for 2nd interrupt but Host
-		 * clears out the bit on handling the 1st interrupt.
-		 */
-		u32 msg, flush;
-
-		msg = I915_READ(SOFT_SCRATCH(15));
-		flush = msg & (INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED |
-			       INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER);
-		if (flush) {
-			/* Clear the message bits that are handled */
-			I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
-
-			/* Handle flush interrupt in bottom half */
-			queue_work(dev_priv->guc.log.runtime.flush_wq,
-				   &dev_priv->guc.log.runtime.flush_work);
-
-			dev_priv->guc.log.flush_interrupt_count++;
-		} else {
-			/* Not clearing of unhandled event bits won't result in
-			 * re-triggering of the interrupt.
-			 */
-		}
-	}
-}
-
 static void i9xx_pipestat_irq_reset(struct drm_i915_private *dev_priv)
 {
 	enum pipe pipe;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3a7e18c..a70fe4c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1266,9 +1266,6 @@ void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
 				     u8 pipe_mask);
 void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
 				     u8 pipe_mask);
-void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv);
-void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv);
-void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv);
 
 /* intel_crt.c */
 void intel_crt_init(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 50b4725..e95ff2d 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -400,7 +400,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	gen9_disable_guc_interrupts(dev_priv);
+	intel_disable_guc_interrupts(dev_priv);
 
 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
 	/* any value greater than GUC_POWER_D0 */
@@ -446,7 +446,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 		return 0;
 
 	if (i915_modparams.guc_log_level >= 0)
-		gen9_enable_guc_interrupts(dev_priv);
+		intel_enable_guc_interrupts(dev_priv);
 
 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
 	data[1] = GUC_POWER_D0;
@@ -507,3 +507,70 @@ u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
 
 	return wopcm_size;
 }
+
+void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv)
+{
+	spin_lock_irq(&dev_priv->irq_lock);
+	gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
+void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv)
+{
+	spin_lock_irq(&dev_priv->irq_lock);
+	if (!dev_priv->guc.interrupts_enabled) {
+		WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
+				       dev_priv->pm_guc_events);
+		dev_priv->guc.interrupts_enabled = true;
+		gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
+	}
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
+void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv)
+{
+	spin_lock_irq(&dev_priv->irq_lock);
+	dev_priv->guc.interrupts_enabled = false;
+
+	gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
+
+	spin_unlock_irq(&dev_priv->irq_lock);
+	synchronize_irq(dev_priv->drm.irq);
+
+	intel_reset_guc_interrupts(dev_priv);
+}
+
+void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
+{
+	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
+		/* Sample the log buffer flush related bits & clear them out now
+		 * itself from the message identity register to minimize the
+		 * probability of losing a flush interrupt, when there are back
+		 * to back flush interrupts.
+		 * There can be a new flush interrupt, for different log buffer
+		 * type (like for ISR), whilst Host is handling one (for DPC).
+		 * Since same bit is used in message register for ISR & DPC, it
+		 * could happen that GuC sets the bit for 2nd interrupt but Host
+		 * clears out the bit on handling the 1st interrupt.
+		 */
+		u32 msg, flush;
+
+		msg = I915_READ(SOFT_SCRATCH(15));
+		flush = msg & (INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED |
+			       INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER);
+		if (flush) {
+			/* Clear the message bits that are handled */
+			I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
+
+			/* Handle flush interrupt in bottom half */
+			queue_work(dev_priv->guc.log.runtime.flush_wq,
+				   &dev_priv->guc.log.runtime.flush_work);
+
+			dev_priv->guc.log.flush_interrupt_count++;
+		} else {
+			/* Not clearing of unhandled event bits won't result in
+			 * re-triggering of the interrupt.
+			 */
+		}
+	}
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 52856a9..c37d34d 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -131,5 +131,9 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 int intel_guc_resume(struct drm_i915_private *dev_priv);
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
+void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv);
+void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv);
+void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv);
+void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index eaedd63..5cd68f6 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -487,7 +487,7 @@ static void guc_flush_logs(struct intel_guc *guc)
 		return;
 
 	/* First disable the interrupts, will be renabled afterwards */
-	gen9_disable_guc_interrupts(dev_priv);
+	intel_disable_guc_interrupts(dev_priv);
 
 	/* Before initiating the forceful flush, wait for any pending/ongoing
 	 * flush to complete otherwise forceful flush may not actually happen.
@@ -605,7 +605,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 		}
 
 		/* GuC logging is currently the only user of Guc2Host interrupts */
-		gen9_enable_guc_interrupts(dev_priv);
+		intel_enable_guc_interrupts(dev_priv);
 	} else {
 		/* Once logging is disabled, GuC won't generate logs & send an
 		 * interrupt. But there could be some data in the log buffer
@@ -639,7 +639,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	/* GuC logging is currently the only user of Guc2Host interrupts */
-	gen9_disable_guc_interrupts(dev_priv);
+	intel_disable_guc_interrupts(dev_priv);
 	guc_log_runtime_destroy(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 907deac..8e58e5a 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -271,7 +271,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	GEM_BUG_ON(!HAS_GUC(dev_priv));
 
 	guc_disable_communication(guc);
-	gen9_reset_guc_interrupts(dev_priv);
+	intel_reset_guc_interrupts(dev_priv);
 
 	/* init WOPCM */
 	I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
@@ -325,7 +325,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 
 	if (USES_GUC_SUBMISSION(dev_priv)) {
 		if (i915_modparams.guc_log_level >= 0)
-			gen9_enable_guc_interrupts(dev_priv);
+			intel_enable_guc_interrupts(dev_priv);
 
 		ret = intel_guc_submission_enable(guc);
 		if (ret)
@@ -345,7 +345,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	 * We've failed to load the firmware :(
 	 */
 err_interrupts:
-	gen9_disable_guc_interrupts(dev_priv);
+	intel_disable_guc_interrupts(dev_priv);
 err_communication:
 	guc_disable_communication(guc);
 err_log_capture:
@@ -379,5 +379,5 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	guc_disable_communication(guc);
 
 	if (USES_GUC_SUBMISSION(dev_priv))
-		gen9_disable_guc_interrupts(dev_priv);
+		intel_disable_guc_interrupts(dev_priv);
 }
-- 
1.9.1

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

[PATCH v3 03/12] drm/i915/guc: Pass intel_guc struct parameter to GuC interrupts functions

[Sagar Arun Kamble]

GuC interrupts handling functions are GuC specific functions hence update
the parameter from dev_priv to intel_guc struct.

v2-v3: Rebase.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  2 +-
 drivers/gpu/drm/i915/intel_guc.c     | 22 +++++++++++++++-------
 drivers/gpu/drm/i915/intel_guc.h     |  8 ++++----
 drivers/gpu/drm/i915/intel_guc_log.c |  8 +++-----
 drivers/gpu/drm/i915/intel_uc.c      |  8 ++++----
 5 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3f4eff9..a1ae057 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1447,7 +1447,7 @@ static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
 		gen6_rps_irq_handler(dev_priv, gt_iir[2]);
 
 	if (gt_iir[2] & dev_priv->pm_guc_events)
-		intel_guc_irq_handler(dev_priv, gt_iir[2]);
+		intel_guc_irq_handler(&dev_priv->guc, gt_iir[2]);
 }
 
 static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index e95ff2d..14bf508d 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -400,7 +400,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	intel_disable_guc_interrupts(dev_priv);
+	intel_disable_guc_interrupts(guc);
 
 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
 	/* any value greater than GUC_POWER_D0 */
@@ -446,7 +446,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 		return 0;
 
 	if (i915_modparams.guc_log_level >= 0)
-		intel_enable_guc_interrupts(dev_priv);
+		intel_enable_guc_interrupts(guc);
 
 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
 	data[1] = GUC_POWER_D0;
@@ -508,15 +508,19 @@ u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
 	return wopcm_size;
 }
 
-void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv)
+void intel_reset_guc_interrupts(struct intel_guc *guc)
 {
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
 	spin_lock_irq(&dev_priv->irq_lock);
 	gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
-void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv)
+void intel_enable_guc_interrupts(struct intel_guc *guc)
 {
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
 	spin_lock_irq(&dev_priv->irq_lock);
 	if (!dev_priv->guc.interrupts_enabled) {
 		WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
@@ -527,8 +531,10 @@ void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv)
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
-void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv)
+void intel_disable_guc_interrupts(struct intel_guc *guc)
 {
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
 	spin_lock_irq(&dev_priv->irq_lock);
 	dev_priv->guc.interrupts_enabled = false;
 
@@ -537,11 +543,13 @@ void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv)
 	spin_unlock_irq(&dev_priv->irq_lock);
 	synchronize_irq(dev_priv->drm.irq);
 
-	intel_reset_guc_interrupts(dev_priv);
+	intel_reset_guc_interrupts(guc);
 }
 
-void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
+void intel_guc_irq_handler(struct intel_guc *guc, u32 gt_iir)
 {
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
 	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
 		/* Sample the log buffer flush related bits & clear them out now
 		 * itself from the message identity register to minimize the
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index c37d34d..49f33b9 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -131,9 +131,9 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 int intel_guc_resume(struct drm_i915_private *dev_priv);
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
-void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv);
-void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv);
-void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv);
-void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
+void intel_reset_guc_interrupts(struct intel_guc *guc);
+void intel_enable_guc_interrupts(struct intel_guc *guc);
+void intel_disable_guc_interrupts(struct intel_guc *guc);
+void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 5cd68f6..59a9021 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -480,14 +480,12 @@ static void guc_log_capture_logs(struct intel_guc *guc)
 
 static void guc_flush_logs(struct intel_guc *guc)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
 	if (!USES_GUC_SUBMISSION(dev_priv) ||
 	    (i915_modparams.guc_log_level < 0))
 		return;
 
 	/* First disable the interrupts, will be renabled afterwards */
-	intel_disable_guc_interrupts(dev_priv);
+	intel_disable_guc_interrupts(guc);
 
 	/* Before initiating the forceful flush, wait for any pending/ongoing
 	 * flush to complete otherwise forceful flush may not actually happen.
@@ -605,7 +603,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 		}
 
 		/* GuC logging is currently the only user of Guc2Host interrupts */
-		intel_enable_guc_interrupts(dev_priv);
+		intel_enable_guc_interrupts(guc);
 	} else {
 		/* Once logging is disabled, GuC won't generate logs & send an
 		 * interrupt. But there could be some data in the log buffer
@@ -639,7 +637,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	/* GuC logging is currently the only user of Guc2Host interrupts */
-	intel_disable_guc_interrupts(dev_priv);
+	intel_disable_guc_interrupts(&dev_priv->guc);
 	guc_log_runtime_destroy(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 8e58e5a..f82453b 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -271,7 +271,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	GEM_BUG_ON(!HAS_GUC(dev_priv));
 
 	guc_disable_communication(guc);
-	intel_reset_guc_interrupts(dev_priv);
+	intel_reset_guc_interrupts(guc);
 
 	/* init WOPCM */
 	I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
@@ -325,7 +325,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 
 	if (USES_GUC_SUBMISSION(dev_priv)) {
 		if (i915_modparams.guc_log_level >= 0)
-			intel_enable_guc_interrupts(dev_priv);
+			intel_enable_guc_interrupts(guc);
 
 		ret = intel_guc_submission_enable(guc);
 		if (ret)
@@ -345,7 +345,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	 * We've failed to load the firmware :(
 	 */
 err_interrupts:
-	intel_disable_guc_interrupts(dev_priv);
+	intel_disable_guc_interrupts(guc);
 err_communication:
 	guc_disable_communication(guc);
 err_log_capture:
@@ -379,5 +379,5 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	guc_disable_communication(guc);
 
 	if (USES_GUC_SUBMISSION(dev_priv))
-		intel_disable_guc_interrupts(dev_priv);
+		intel_disable_guc_interrupts(guc);
 }
-- 
1.9.1

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

[PATCH v3 04/12] drm/i915/guc: Add description and comments about guc_log_level parameter

[Sagar Arun Kamble]

guc_log_level parameter takes effect when GuC is loaded which is
controlled through enable_guc parameter. Add this relation info.
in parameter description and documentation.
Earlier, this patch was added to sanitize guc_log_level like old
GuC parameters enable_guc_loading/submission. With new parameter
enable_guc, sanitization of guc_log_level is no more needed.

v2: Added documentation to intel_guc_log.c and param description
about GuC loading dependency. (Michal Wajdeczko)

v3: Removed sanitization of module parameter guc_log_level.
Previous review comments not applicable now.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v2
---
 drivers/gpu/drm/i915/i915_params.c   | 3 ++-
 drivers/gpu/drm/i915/intel_guc_log.c | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b5f3eb4..a93a6ca 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = {
 	"(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
 
 i915_param_named(guc_log_level, int, 0400,
-	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
+	"GuC firmware logging level. This takes effect only if GuC is to be "
+	"loaded (depends on enable_guc) (-1:disabled (default), 0-3:enabled)");
 
 i915_param_named_unsafe(guc_firmware_path, charp, 0400,
 	"GuC firmware path to use instead of the default one");
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 59a9021..d0131bc 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -34,6 +34,7 @@
  * DOC: GuC firmware log
  *
  * Firmware log is enabled by setting i915.guc_log_level to non-negative level.
+ * This takes effect only if GuC is to be loaded based on enable_guc.
  * Log data is printed out via reading debugfs i915_guc_log_dump. Reading from
  * i915_guc_load_status will print out firmware loading status and scratch
  * registers value.
-- 
1.9.1

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

[PATCH v3 05/12] drm/i915/guc: Fix GuC interrupts disabling with logging

[Sagar Arun Kamble]

With guc_log_unregister disabling runtime logging and interrupts, there
is no need to disable interrupts during uc_fini_hw hence it is removed.
With GuC CT buffer mechanism, interrupt disabling can be added later at
a point where CT mechanism ceases.

v2: Rebase.

v3: Moved this patch earlier in the series. (Tvrtko)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index f82453b..bc7f549 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -377,7 +377,4 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 		intel_guc_submission_disable(guc);
 
 	guc_disable_communication(guc);
-
-	if (USES_GUC_SUBMISSION(dev_priv))
-		intel_disable_guc_interrupts(guc);
 }
-- 
1.9.1

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

[PATCH v3 06/12] drm/i915/guc: Separate creation/release of runtime logging data from base logging data

[Sagar Arun Kamble]

GuC log runtime/relay channel data will get released during i915
unregister, and only GuC log vma needs to be released during fini. To
achieve this, prepare separate helpers to create/destroy base and runtime
logging.
This separation is also needed to couple runtime log data and interrupts
handling together. In future we might not want to consider runtime data
creation failure as catastrophic to abort GuC load. Then we can ignore
the return error codes from intel_guc_log_runtime_create().

v2: Rebase.

v3: Refined usage of intel_guc_log_destroy and created new function
intel_guc_log_runtime_destroy. (Tvrtko)
Added intel_guc_log_runtime_create to separate the creation part as well.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c     |  8 +++++++-
 drivers/gpu/drm/i915/intel_guc_log.c | 22 ++++++++--------------
 drivers/gpu/drm/i915/intel_guc_log.h |  2 ++
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 14bf508d..0184c86 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -168,9 +168,13 @@ int intel_guc_init(struct intel_guc *guc)
 	if (ret)
 		goto err_shared;
 
-	ret = intel_guc_ads_create(guc);
+	ret = intel_guc_log_runtime_create(guc);
 	if (ret)
 		goto err_log;
+
+	ret = intel_guc_ads_create(guc);
+	if (ret)
+		goto err_log_runtime;
 	GEM_BUG_ON(!guc->ads_vma);
 
 	/* We need to notify the guc whenever we change the GGTT */
@@ -178,6 +182,8 @@ int intel_guc_init(struct intel_guc *guc)
 
 	return 0;
 
+err_log_runtime:
+	intel_guc_log_runtime_destroy(guc);
 err_log:
 	intel_guc_log_destroy(guc);
 err_shared:
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index d0131bc..18d1b49 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -357,7 +357,7 @@ static bool guc_log_has_runtime(struct intel_guc *guc)
 	return guc->log.runtime.buf_addr != NULL;
 }
 
-static int guc_log_runtime_create(struct intel_guc *guc)
+int intel_guc_log_runtime_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	void *vaddr;
@@ -365,6 +365,9 @@ static int guc_log_runtime_create(struct intel_guc *guc)
 	size_t n_subbufs, subbuf_size;
 	int ret;
 
+	if (i915_modparams.guc_log_level < 0)
+		return 0;
+
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
 
 	GEM_BUG_ON(guc_log_has_runtime(guc));
@@ -420,7 +423,7 @@ static int guc_log_runtime_create(struct intel_guc *guc)
 	return ret;
 }
 
-static void guc_log_runtime_destroy(struct intel_guc *guc)
+void intel_guc_log_runtime_destroy(struct intel_guc *guc)
 {
 	/*
 	 * It's possible that the runtime stuff was never allocated because
@@ -446,7 +449,7 @@ static int guc_log_late_setup(struct intel_guc *guc)
 		 * handle log buffer flush interrupts would not have been done yet,
 		 * so do that now.
 		 */
-		ret = guc_log_runtime_create(guc);
+		ret = intel_guc_log_runtime_create(guc);
 		if (ret)
 			goto err;
 	}
@@ -458,7 +461,7 @@ static int guc_log_late_setup(struct intel_guc *guc)
 	return 0;
 
 err_runtime:
-	guc_log_runtime_destroy(guc);
+	intel_guc_log_runtime_destroy(guc);
 err:
 	/* logging will remain off */
 	i915_modparams.guc_log_level = -1;
@@ -536,12 +539,6 @@ int intel_guc_log_create(struct intel_guc *guc)
 
 	guc->log.vma = vma;
 
-	if (i915_modparams.guc_log_level >= 0) {
-		ret = guc_log_runtime_create(guc);
-		if (ret < 0)
-			goto err_vma;
-	}
-
 	/* each allocated unit is a page */
 	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
 		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
@@ -553,8 +550,6 @@ int intel_guc_log_create(struct intel_guc *guc)
 
 	return 0;
 
-err_vma:
-	i915_vma_unpin_and_release(&guc->log.vma);
 err:
 	/* logging will be off */
 	i915_modparams.guc_log_level = -1;
@@ -563,7 +558,6 @@ int intel_guc_log_create(struct intel_guc *guc)
 
 void intel_guc_log_destroy(struct intel_guc *guc)
 {
-	guc_log_runtime_destroy(guc);
 	i915_vma_unpin_and_release(&guc->log.vma);
 }
 
@@ -639,6 +633,6 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	/* GuC logging is currently the only user of Guc2Host interrupts */
 	intel_disable_guc_interrupts(&dev_priv->guc);
-	guc_log_runtime_destroy(&dev_priv->guc);
+	intel_guc_log_runtime_destroy(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index f512cf7..345447a 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -52,6 +52,8 @@ struct intel_guc_log {
 
 int intel_guc_log_create(struct intel_guc *guc);
 void intel_guc_log_destroy(struct intel_guc *guc);
+int intel_guc_log_runtime_create(struct intel_guc *guc);
+void intel_guc_log_runtime_destroy(struct intel_guc *guc);
 int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
 void i915_guc_log_register(struct drm_i915_private *dev_priv);
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
-- 
1.9.1

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

[PATCH v3 07/12] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts

[Sagar Arun Kamble]

Disabling GuC interrupts involves access to GuC IRQ control registers
hence ensure device is RPM awake.

v2: Add comment about need to synchronize flush work and log runtime
    destroy

v3: Moved patch earlier in the series and removed comment about future
work. (Tvrtko)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_guc_log.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 18d1b49..3a895a3 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -632,7 +632,10 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	/* GuC logging is currently the only user of Guc2Host interrupts */
+	intel_runtime_pm_get(dev_priv);
 	intel_disable_guc_interrupts(&dev_priv->guc);
+	intel_runtime_pm_put(dev_priv);
+
 	intel_guc_log_runtime_destroy(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
-- 
1.9.1

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

[PATCH v3 08/12] drm/i915/guc: Make guc_log_level parameter immutable

[Sagar Arun Kamble]

This patch introduces i915 internal state variable in GuC log struct,
"level" which will be copied from guc_log_level modparam during i915
load and thereafter be available for user updates. This will make
guc_log_level parameter immutable.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
 drivers/gpu/drm/i915/intel_guc.c     |  6 +++---
 drivers/gpu/drm/i915/intel_guc_log.c | 23 ++++++++++-------------
 drivers/gpu/drm/i915/intel_guc_log.h |  6 ++++++
 drivers/gpu/drm/i915/intel_uc.c      | 11 +++++++++--
 5 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2bb6307..16f9a95 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2459,7 +2459,7 @@ static int i915_guc_log_control_get(void *data, u64 *val)
 	if (!dev_priv->guc.log.vma)
 		return -EINVAL;
 
-	*val = i915_modparams.guc_log_level;
+	*val = dev_priv->guc.log.level;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 0184c86..d351642 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -253,9 +253,9 @@ void intel_guc_init_params(struct intel_guc *guc)
 
 	params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
 
-	if (i915_modparams.guc_log_level >= 0) {
+	if (guc->log.level >= 0) {
 		params[GUC_CTL_DEBUG] =
-			i915_modparams.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
+			guc->log.level << GUC_LOG_VERBOSITY_SHIFT;
 	} else {
 		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
 	}
@@ -451,7 +451,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	if (i915_modparams.guc_log_level >= 0)
+	if (guc->log.level >= 0)
 		intel_enable_guc_interrupts(guc);
 
 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 3a895a3..d979830 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -148,7 +148,7 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
 	struct dentry *log_dir;
 	int ret;
 
-	if (i915_modparams.guc_log_level < 0)
+	if (guc->log.level < 0)
 		return 0;
 
 	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
@@ -365,7 +365,7 @@ int intel_guc_log_runtime_create(struct intel_guc *guc)
 	size_t n_subbufs, subbuf_size;
 	int ret;
 
-	if (i915_modparams.guc_log_level < 0)
+	if (guc->log.level < 0)
 		return 0;
 
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
@@ -427,7 +427,7 @@ void intel_guc_log_runtime_destroy(struct intel_guc *guc)
 {
 	/*
 	 * It's possible that the runtime stuff was never allocated because
-	 * guc_log_level was < 0 at the time
+	 * guc->log.level was < 0 at the time
 	 **/
 	if (!guc_log_has_runtime(guc))
 		return;
@@ -464,7 +464,7 @@ static int guc_log_late_setup(struct intel_guc *guc)
 	intel_guc_log_runtime_destroy(guc);
 err:
 	/* logging will remain off */
-	i915_modparams.guc_log_level = -1;
+	guc->log.level = -1;
 	return ret;
 }
 
@@ -485,7 +485,7 @@ static void guc_log_capture_logs(struct intel_guc *guc)
 static void guc_flush_logs(struct intel_guc *guc)
 {
 	if (!USES_GUC_SUBMISSION(dev_priv) ||
-	    (i915_modparams.guc_log_level < 0))
+	    guc->log.level < 0)
 		return;
 
 	/* First disable the interrupts, will be renabled afterwards */
@@ -513,9 +513,6 @@ int intel_guc_log_create(struct intel_guc *guc)
 
 	GEM_BUG_ON(guc->log.vma);
 
-	if (i915_modparams.guc_log_level > GUC_LOG_VERBOSITY_MAX)
-		i915_modparams.guc_log_level = GUC_LOG_VERBOSITY_MAX;
-
 	/* The first page is to save log buffer state. Allocate one
 	 * extra page for others in case for overlap */
 	size = (1 + GUC_LOG_DPC_PAGES + 1 +
@@ -552,7 +549,7 @@ int intel_guc_log_create(struct intel_guc *guc)
 
 err:
 	/* logging will be off */
-	i915_modparams.guc_log_level = -1;
+	guc->log.level = -1;
 	return ret;
 }
 
@@ -575,7 +572,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 		return -EINVAL;
 
 	/* This combination doesn't make sense & won't have any effect */
-	if (!log_param.logging_enabled && (i915_modparams.guc_log_level < 0))
+	if (!log_param.logging_enabled && guc->log.level < 0)
 		return 0;
 
 	ret = guc_log_control(guc, log_param.value);
@@ -585,7 +582,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 	}
 
 	if (log_param.logging_enabled) {
-		i915_modparams.guc_log_level = log_param.verbosity;
+		guc->log.level = log_param.verbosity;
 
 		/* If log_level was set as -1 at boot time, then the relay channel file
 		 * wouldn't have been created by now and interrupts also would not have
@@ -608,7 +605,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 		guc_flush_logs(guc);
 
 		/* As logging is disabled, update log level to reflect that */
-		i915_modparams.guc_log_level = -1;
+		guc->log.level = -1;
 	}
 
 	return ret;
@@ -617,7 +614,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 void i915_guc_log_register(struct drm_i915_private *dev_priv)
 {
 	if (!USES_GUC_SUBMISSION(dev_priv) ||
-	    (i915_modparams.guc_log_level < 0))
+	    dev_priv->guc.log.level < 0)
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index 345447a..362bbef 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -33,6 +33,12 @@
 struct intel_guc;
 
 struct intel_guc_log {
+	/*
+	 * Current GuC firmware logging level. This depends on kernel parameter
+	 * guc_log_level during load and thereafter on user requests. It has
+	 * similar semantics as guc_log_level. (-1:disabled, "0-3:enabled)")
+	 */
+	int level;
 	u32 flags;
 	struct i915_vma *vma;
 	/* The runtime stuff gets created only when GuC logging gets enabled */
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index bc7f549..e2e2020 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -107,6 +107,11 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 
 	/* Make sure that sanitization was done */
 	GEM_BUG_ON(i915_modparams.enable_guc < 0);
+
+	if (i915_modparams.guc_log_level > GUC_LOG_VERBOSITY_MAX)
+		i915_modparams.guc_log_level = GUC_LOG_VERBOSITY_MAX;
+
+	dev_priv->guc.log.level = i915_modparams.guc_log_level;
 }
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
@@ -152,7 +157,7 @@ void intel_uc_init_mmio(struct drm_i915_private *dev_priv)
 
 static void guc_capture_load_err_log(struct intel_guc *guc)
 {
-	if (!guc->log.vma || i915_modparams.guc_log_level < 0)
+	if (!guc->log.vma || guc->log.level < 0)
 		return;
 
 	if (!guc->load_err_log)
@@ -324,7 +329,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	}
 
 	if (USES_GUC_SUBMISSION(dev_priv)) {
-		if (i915_modparams.guc_log_level >= 0)
+		if (guc->log.level >= 0)
 			intel_enable_guc_interrupts(guc);
 
 		ret = intel_guc_submission_enable(guc);
@@ -358,6 +363,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (GEM_WARN_ON(ret == -EIO))
 		ret = -EINVAL;
 
+	guc->log.level = -1;
+
 	dev_err(dev_priv->drm.dev, "GuC initialization failed %d\n", ret);
 	return ret;
 }
-- 
1.9.1

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

[PATCH v3 09/12] drm/i915/guc: Make GuC log related functions depend only on log level

[Sagar Arun Kamble]

With GuC log level set properly only for cases where GuC is loaded we can
remove the GuC submission checks from flush_guc_logs and guc_log_register,
unregister and uc_fini_hw functions. It is important to note that GuC log
runtime data has to be freed during driver unregister.
Freeing of that data can't be gated by guc_log_level check because if we
free GuC log runtime only when log level >=0 then it will not be destroyed
when logging is disabled after enabling before driver unload.

Also, with this patch GuC interrupts are enabled first after GuC load if
logging is enabled. GuC to Host interrupts will be needed for GuC CT
buffer recv mechanism and hence we will be adding support to control that
interrupt based on ref. taken by Log or CT recv feature in next patch. To
prepare for that all interrupt updates are now gated by GuC log level
checks.

v2: Rebase. Updated check in i915_guc_log_unregister to be based on
guc_log_level. (Michal Wajdeczko)

v3: Rebase. Made all GuC log related functions depend only log level.
Updated uC init w.r.t enabling of GuC interrupts. Commit message update.
Rebase w.r.t guc_log_level immutable changes. (Tvrtko)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c     |  3 ++-
 drivers/gpu/drm/i915/intel_guc_log.c | 17 +++++++----------
 drivers/gpu/drm/i915/intel_uc.c      | 15 ++++++++-------
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index d351642..36d1bca 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -406,7 +406,8 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	intel_disable_guc_interrupts(guc);
+	if (guc->log.level >= 0)
+		intel_disable_guc_interrupts(guc);
 
 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
 	/* any value greater than GUC_POWER_D0 */
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index d979830..7bc0065 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -484,8 +484,7 @@ static void guc_log_capture_logs(struct intel_guc *guc)
 
 static void guc_flush_logs(struct intel_guc *guc)
 {
-	if (!USES_GUC_SUBMISSION(dev_priv) ||
-	    guc->log.level < 0)
+	if (guc->log.level < 0)
 		return;
 
 	/* First disable the interrupts, will be renabled afterwards */
@@ -613,8 +612,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 
 void i915_guc_log_register(struct drm_i915_private *dev_priv)
 {
-	if (!USES_GUC_SUBMISSION(dev_priv) ||
-	    dev_priv->guc.log.level < 0)
+	if (dev_priv->guc.log.level < 0)
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
@@ -624,14 +622,13 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
 
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 {
-	if (!USES_GUC_SUBMISSION(dev_priv))
-		return;
-
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	/* GuC logging is currently the only user of Guc2Host interrupts */
-	intel_runtime_pm_get(dev_priv);
-	intel_disable_guc_interrupts(&dev_priv->guc);
-	intel_runtime_pm_put(dev_priv);
+	if (dev_priv->guc.log.level >= 0) {
+		intel_runtime_pm_get(dev_priv);
+		intel_disable_guc_interrupts(&dev_priv->guc);
+		intel_runtime_pm_put(dev_priv);
+	}
 
 	intel_guc_log_runtime_destroy(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index e2e2020..fb5edcc 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -318,9 +318,12 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_log_capture;
 
+	if (guc->log.level >= 0)
+		intel_enable_guc_interrupts(guc);
+
 	ret = guc_enable_communication(guc);
 	if (ret)
-		goto err_log_capture;
+		goto err_interrupts;
 
 	if (USES_HUC(dev_priv)) {
 		ret = intel_huc_auth(huc);
@@ -329,12 +332,9 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	}
 
 	if (USES_GUC_SUBMISSION(dev_priv)) {
-		if (guc->log.level >= 0)
-			intel_enable_guc_interrupts(guc);
-
 		ret = intel_guc_submission_enable(guc);
 		if (ret)
-			goto err_interrupts;
+			goto err_communication;
 	}
 
 	dev_info(dev_priv->drm.dev, "GuC firmware version %u.%u\n",
@@ -349,10 +349,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	/*
 	 * We've failed to load the firmware :(
 	 */
-err_interrupts:
-	intel_disable_guc_interrupts(guc);
 err_communication:
 	guc_disable_communication(guc);
+err_interrupts:
+	if (guc->log.level >= 0)
+		intel_disable_guc_interrupts(guc);
 err_log_capture:
 	guc_capture_load_err_log(guc);
 err_out:
-- 
1.9.1

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

[PATCH v3 10/12] drm/i915/guc: Add client support to enable/disable GuC interrupts

[Sagar Arun Kamble]

This patch adds support to enable/disable GuC interrupts for different
features without impacting other's need. Currently GuC log capture and
CT buffer receive mechanisms use the GuC interrupts. GuC interrupts are
currently enabled and disabled in different logging scenarios all gated
by log level.

v2: Rebase with all GuC interrupt handlers moved to intel_guc.c. Handling
multiple clients for GuC interrupts enable/disable.
(Michal Wajdeczko)

v3: Removed spin lock and using test_bit in i915_guc_info. Prepared low
level helpers to get/put GuC interrupts that can be reused during
suspend/resume. (Tvrtko)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  6 +++++
 drivers/gpu/drm/i915/intel_guc.c     | 47 +++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_guc.h     | 11 ++++++---
 drivers/gpu/drm/i915/intel_guc_log.c |  6 ++---
 drivers/gpu/drm/i915/intel_uc.c      |  4 +--
 5 files changed, 54 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 16f9a95..eef4c8b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2340,6 +2340,12 @@ static int i915_guc_info(struct seq_file *m, void *data)
 	GEM_BUG_ON(!guc->execbuf_client);
 	GEM_BUG_ON(!guc->preempt_client);
 
+	seq_puts(m, "GuC Interrupt Clients: ");
+	if (test_bit(GUC_INTR_CLIENT_LOG, &guc->interrupt_clients))
+		seq_puts(m, "GuC Logging\n");
+	else
+		seq_puts(m, "None\n");
+
 	seq_printf(m, "Doorbell map:\n");
 	seq_printf(m, "\t%*pb\n", GUC_NUM_DOORBELLS, guc->doorbell_bitmap);
 	seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", guc->db_cacheline);
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 36d1bca..d356c40 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -407,7 +407,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 		return 0;
 
 	if (guc->log.level >= 0)
-		intel_disable_guc_interrupts(guc);
+		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
 
 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
 	/* any value greater than GUC_POWER_D0 */
@@ -453,7 +453,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 		return 0;
 
 	if (guc->log.level >= 0)
-		intel_enable_guc_interrupts(guc);
+		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
 
 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
 	data[1] = GUC_POWER_D0;
@@ -524,28 +524,51 @@ void intel_reset_guc_interrupts(struct intel_guc *guc)
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
-void intel_enable_guc_interrupts(struct intel_guc *guc)
+static void __intel_get_guc_interrupts(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	lockdep_assert_held(&dev_priv->irq_lock);
+
+	WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
+			       dev_priv->pm_guc_events);
+	gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
+}
+
+void intel_get_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
 	spin_lock_irq(&dev_priv->irq_lock);
-	if (!dev_priv->guc.interrupts_enabled) {
-		WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
-				       dev_priv->pm_guc_events);
-		dev_priv->guc.interrupts_enabled = true;
-		gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
-	}
+
+	if (!guc->interrupt_clients)
+		__intel_get_guc_interrupts(guc);
+	__set_bit(id, &guc->interrupt_clients);
+
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
-void intel_disable_guc_interrupts(struct intel_guc *guc)
+static void __intel_put_guc_interrupts(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	dev_priv->guc.interrupts_enabled = false;
+	lockdep_assert_held(&dev_priv->irq_lock);
 
 	gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
+}
+
+void intel_put_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	spin_lock_irq(&dev_priv->irq_lock);
+
+	__clear_bit(id, &guc->interrupt_clients);
+	if (guc->interrupt_clients) {
+		spin_unlock_irq(&dev_priv->irq_lock);
+		return;
+	}
+	__intel_put_guc_interrupts(guc);
 
 	spin_unlock_irq(&dev_priv->irq_lock);
 	synchronize_irq(dev_priv->drm.irq);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 49f33b9..af74392 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -34,6 +34,11 @@
 #include "intel_uc_fw.h"
 #include "i915_vma.h"
 
+enum guc_intr_client {
+	GUC_INTR_CLIENT_LOG = 0,
+	GUC_INTR_CLIENT_COUNT
+};
+
 struct guc_preempt_work {
 	struct work_struct work;
 	struct intel_engine_cs *engine;
@@ -53,7 +58,7 @@ struct intel_guc {
 	struct drm_i915_gem_object *load_err_log;
 
 	/* intel_guc_recv interrupt related state */
-	bool interrupts_enabled;
+	unsigned long interrupt_clients;
 
 	struct i915_vma *ads_vma;
 	struct i915_vma *stage_desc_pool;
@@ -132,8 +137,8 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 void intel_reset_guc_interrupts(struct intel_guc *guc);
-void intel_enable_guc_interrupts(struct intel_guc *guc);
-void intel_disable_guc_interrupts(struct intel_guc *guc);
+void intel_get_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id);
+void intel_put_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id);
 void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 7bc0065..ddd4f6d 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -488,7 +488,7 @@ static void guc_flush_logs(struct intel_guc *guc)
 		return;
 
 	/* First disable the interrupts, will be renabled afterwards */
-	intel_disable_guc_interrupts(guc);
+	intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
 
 	/* Before initiating the forceful flush, wait for any pending/ongoing
 	 * flush to complete otherwise forceful flush may not actually happen.
@@ -594,7 +594,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 		}
 
 		/* GuC logging is currently the only user of Guc2Host interrupts */
-		intel_enable_guc_interrupts(guc);
+		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
 	} else {
 		/* Once logging is disabled, GuC won't generate logs & send an
 		 * interrupt. But there could be some data in the log buffer
@@ -626,7 +626,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 	/* GuC logging is currently the only user of Guc2Host interrupts */
 	if (dev_priv->guc.log.level >= 0) {
 		intel_runtime_pm_get(dev_priv);
-		intel_disable_guc_interrupts(&dev_priv->guc);
+		intel_put_guc_interrupts(&dev_priv->guc, GUC_INTR_CLIENT_LOG);
 		intel_runtime_pm_put(dev_priv);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index fb5edcc..93ecbf1 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -319,7 +319,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		goto err_log_capture;
 
 	if (guc->log.level >= 0)
-		intel_enable_guc_interrupts(guc);
+		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
 
 	ret = guc_enable_communication(guc);
 	if (ret)
@@ -353,7 +353,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	guc_disable_communication(guc);
 err_interrupts:
 	if (guc->log.level >= 0)
-		intel_disable_guc_interrupts(guc);
+		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
 err_log_capture:
 	guc_capture_load_err_log(guc);
 err_out:
-- 
1.9.1

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

[PATCH v3 11/12] drm/i915/guc: Restore GuC interrupts across suspend/reset if enabled

[Sagar Arun Kamble]

In order to override the disable/enable control of GuC interrupts during
suspend/reset cycle we are creating two new functions suspend/restore
guc_interrupts which check if interrupts were enabled and disable them
on suspend and enable them on resume. They are used to restore interrupts
across reset as well.

Further restructuring of runtime_pm_enable/disable_interrupts and
suspend/restore_guc_interrupts will be done in upcoming patches.

v2: Rebase.

v3: Updated suspend/restore with the new low level get/put functions.
(Tvrtko)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  2 ++
 drivers/gpu/drm/i915/intel_guc.c     | 32 ++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_guc.h     |  2 ++
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0cd3559..2e0db53 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3676,8 +3676,10 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
 		 * The display has been reset as well,
 		 * so need a full re-initialization.
 		 */
+		intel_suspend_guc_interrupts(&dev_priv->guc);
 		intel_runtime_pm_disable_interrupts(dev_priv);
 		intel_runtime_pm_enable_interrupts(dev_priv);
+		intel_restore_guc_interrupts(&dev_priv->guc);
 
 		intel_pps_unlock_regs_wa(dev_priv);
 		intel_modeset_init_hw(dev);
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index d356c40..28a418a 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -406,8 +406,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	if (guc->log.level >= 0)
-		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
+	intel_suspend_guc_interrupts(guc);
 
 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
 	/* any value greater than GUC_POWER_D0 */
@@ -452,8 +451,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	if (guc->log.level >= 0)
-		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
+	intel_restore_guc_interrupts(guc);
 
 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
 	data[1] = GUC_POWER_D0;
@@ -548,6 +546,16 @@ void intel_get_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id)
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
+void intel_restore_guc_interrupts(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	if (guc->interrupt_clients)
+		__intel_get_guc_interrupts(guc);
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
 static void __intel_put_guc_interrupts(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -576,6 +584,22 @@ void intel_put_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id)
 	intel_reset_guc_interrupts(guc);
 }
 
+void intel_suspend_guc_interrupts(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	if (!guc->interrupt_clients) {
+		spin_unlock_irq(&dev_priv->irq_lock);
+		return;
+	}
+	__intel_put_guc_interrupts(guc);
+	spin_unlock_irq(&dev_priv->irq_lock);
+	synchronize_irq(dev_priv->drm.irq);
+
+	intel_reset_guc_interrupts(guc);
+}
+
 void intel_guc_irq_handler(struct intel_guc *guc, u32 gt_iir)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index af74392..2c14781 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -140,5 +140,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 void intel_get_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id);
 void intel_put_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id);
 void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
+void intel_suspend_guc_interrupts(struct intel_guc *guc);
+void intel_restore_guc_interrupts(struct intel_guc *guc);
 
 #endif
-- 
1.9.1

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

[PATCH v3 12/12] HAX: drm/i915/guc: enable GuC submission/logging for CI

[Sagar Arun Kamble]

Also 1) revert ("drm/i915/guc: Assert that we switch between
known ggtt->invalidate functions")
2) fix RPM resume interrupt enabling w.r.t GuC resume
3) disable guc log streaming DRM logs
---
 drivers/gpu/drm/i915/i915_drv.c      | 4 ++--
 drivers/gpu/drm/i915/i915_gem_gtt.c  | 8 ++------
 drivers/gpu/drm/i915/i915_params.h   | 4 ++--
 drivers/gpu/drm/i915/intel_guc_log.c | 2 +-
 4 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c8da9d..c8460c5 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2659,6 +2659,8 @@ static int intel_runtime_resume(struct device *kdev)
 	if (intel_uncore_unclaimed_mmio(dev_priv))
 		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
 
+	intel_runtime_pm_enable_interrupts(dev_priv);
+
 	intel_guc_resume(dev_priv);
 
 	if (IS_GEN9_LP(dev_priv)) {
@@ -2682,8 +2684,6 @@ static int intel_runtime_resume(struct device *kdev)
 	i915_gem_init_swizzling(dev_priv);
 	i915_gem_restore_fences(dev_priv);
 
-	intel_runtime_pm_enable_interrupts(dev_priv);
-
 	/*
 	 * On VLV/CHV display interrupts are part of the display
 	 * power well, so hpd is reinitialized from there. For
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index c5f3938..fdd3345 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3549,8 +3549,6 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
 
 void i915_ggtt_enable_guc(struct drm_i915_private *i915)
 {
-	GEM_BUG_ON(i915->ggtt.invalidate != gen6_ggtt_invalidate);
-
 	i915->ggtt.invalidate = guc_ggtt_invalidate;
 
 	i915_ggtt_invalidate(i915);
@@ -3558,10 +3556,8 @@ void i915_ggtt_enable_guc(struct drm_i915_private *i915)
 
 void i915_ggtt_disable_guc(struct drm_i915_private *i915)
 {
-	/* We should only be called after i915_ggtt_enable_guc() */
-	GEM_BUG_ON(i915->ggtt.invalidate != guc_ggtt_invalidate);
-
-	i915->ggtt.invalidate = gen6_ggtt_invalidate;
+	if (i915->ggtt.invalidate == guc_ggtt_invalidate)
+		i915->ggtt.invalidate = gen6_ggtt_invalidate;
 
 	i915_ggtt_invalidate(i915);
 }
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c963603..25b7e88 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -47,8 +47,8 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc, 0) \
-	param(int, guc_log_level, -1) \
+	param(int, enable_guc, -1) \
+	param(int, guc_log_level, 1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
 	param(int, mmio_debug, 0) \
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index ddd4f6d..da0554c 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -339,7 +339,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 		/* Used rate limited to avoid deluge of messages, logs might be
 		 * getting consumed by User at a slow rate.
 		 */
-		DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
+		//DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
 		guc->log.capture_miss_count++;
 	}
 }
-- 
1.9.1

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

Re: [PATCH v3 03/12] drm/i915/guc: Pass intel_guc struct parameter to GuC interrupts functions

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-01-04 16:21:45)
> GuC interrupts handling functions are GuC specific functions hence update
> the parameter from dev_priv to intel_guc struct.
> 
> v2-v3: Rebase.
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c      |  2 +-
>  drivers/gpu/drm/i915/intel_guc.c     | 22 +++++++++++++++-------
>  drivers/gpu/drm/i915/intel_guc.h     |  8 ++++----
>  drivers/gpu/drm/i915/intel_guc_log.c |  8 +++-----
>  drivers/gpu/drm/i915/intel_uc.c      |  8 ++++----
>  5 files changed, 27 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 3f4eff9..a1ae057 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1447,7 +1447,7 @@ static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
>                 gen6_rps_irq_handler(dev_priv, gt_iir[2]);
>  
>         if (gt_iir[2] & dev_priv->pm_guc_events)
> -               intel_guc_irq_handler(dev_priv, gt_iir[2]);
> +               intel_guc_irq_handler(&dev_priv->guc, gt_iir[2]);
>  }
>  
>  static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index e95ff2d..14bf508d 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -400,7 +400,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
>         if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>                 return 0;
>  
> -       intel_disable_guc_interrupts(dev_priv);
> +       intel_disable_guc_interrupts(guc);
>  
>         data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>         /* any value greater than GUC_POWER_D0 */
> @@ -446,7 +446,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
>                 return 0;
>  
>         if (i915_modparams.guc_log_level >= 0)
> -               intel_enable_guc_interrupts(dev_priv);
> +               intel_enable_guc_interrupts(guc);
>  
>         data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>         data[1] = GUC_POWER_D0;
> @@ -508,15 +508,19 @@ u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
>         return wopcm_size;
>  }
>  
> -void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv)
> +void intel_reset_guc_interrupts(struct intel_guc *guc)
>  {
> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
>         spin_lock_irq(&dev_priv->irq_lock);
>         gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
>         spin_unlock_irq(&dev_priv->irq_lock);
>  }
>  
> -void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv)
> +void intel_enable_guc_interrupts(struct intel_guc *guc)
>  {
> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
>         spin_lock_irq(&dev_priv->irq_lock);
>         if (!dev_priv->guc.interrupts_enabled) {
>                 WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> @@ -527,8 +531,10 @@ void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv)
>         spin_unlock_irq(&dev_priv->irq_lock);
>  }
>  
> -void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv)
> +void intel_disable_guc_interrupts(struct intel_guc *guc)
>  {
> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
>         spin_lock_irq(&dev_priv->irq_lock);
>         dev_priv->guc.interrupts_enabled = false;
>  
> @@ -537,11 +543,13 @@ void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv)
>         spin_unlock_irq(&dev_priv->irq_lock);
>         synchronize_irq(dev_priv->drm.irq);
>  
> -       intel_reset_guc_interrupts(dev_priv);
> +       intel_reset_guc_interrupts(guc);
>  }
>  
> -void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
> +void intel_guc_irq_handler(struct intel_guc *guc, u32 gt_iir)
>  {
> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
>         if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
>                 /* Sample the log buffer flush related bits & clear them out now
>                  * itself from the message identity register to minimize the
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index c37d34d..49f33b9 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -131,9 +131,9 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>  int intel_guc_resume(struct drm_i915_private *dev_priv);
>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
> -void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv);
> -void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv);
> -void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv);
> -void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
> +void intel_reset_guc_interrupts(struct intel_guc *guc);
> +void intel_enable_guc_interrupts(struct intel_guc *guc);
> +void intel_disable_guc_interrupts(struct intel_guc *guc);
> +void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);

You should also consider s/intel_verb_guc/intel_guc_verb/ since we are
now passing intel_guc as the object.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 12/12] HAX: drm/i915/guc: enable GuC submission/logging for CI

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-01-04 16:21:54)
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6c8da9d..c8460c5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2659,6 +2659,8 @@ static int intel_runtime_resume(struct device *kdev)
>         if (intel_uncore_unclaimed_mmio(dev_priv))
>                 DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
>  
> +       intel_runtime_pm_enable_interrupts(dev_priv);
> +
>         intel_guc_resume(dev_priv);
>  
>         if (IS_GEN9_LP(dev_priv)) {
> @@ -2682,8 +2684,6 @@ static int intel_runtime_resume(struct device *kdev)
>         i915_gem_init_swizzling(dev_priv);
>         i915_gem_restore_fences(dev_priv);
>  
> -       intel_runtime_pm_enable_interrupts(dev_priv);

Have I missed the pending patch?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 03/12] drm/i915/guc: Pass intel_guc struct parameter to GuC interrupts functions

[Michal Wajdeczko]

On Thu, 04 Jan 2018 17:23:05 +0100, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Sagar Arun Kamble (2018-01-04 16:21:45)
>> GuC interrupts handling functions are GuC specific functions hence  
>> update
>> the parameter from dev_priv to intel_guc struct.
>>
>> v2-v3: Rebase.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c      |  2 +-
>>  drivers/gpu/drm/i915/intel_guc.c     | 22 +++++++++++++++-------
>>  drivers/gpu/drm/i915/intel_guc.h     |  8 ++++----
>>  drivers/gpu/drm/i915/intel_guc_log.c |  8 +++-----
>>  drivers/gpu/drm/i915/intel_uc.c      |  8 ++++----
>>  5 files changed, 27 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c  
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 3f4eff9..a1ae057 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1447,7 +1447,7 @@ static void gen8_gt_irq_handler(struct  
>> drm_i915_private *dev_priv,
>>                 gen6_rps_irq_handler(dev_priv, gt_iir[2]);
>>
>>         if (gt_iir[2] & dev_priv->pm_guc_events)
>> -               intel_guc_irq_handler(dev_priv, gt_iir[2]);
>> +               intel_guc_irq_handler(&dev_priv->guc, gt_iir[2]);
>>  }
>>
>>  static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index e95ff2d..14bf508d 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -400,7 +400,7 @@ int intel_guc_suspend(struct drm_i915_private  
>> *dev_priv)
>>         if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>                 return 0;
>>
>> -       intel_disable_guc_interrupts(dev_priv);
>> +       intel_disable_guc_interrupts(guc);
>>
>>         data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>>         /* any value greater than GUC_POWER_D0 */
>> @@ -446,7 +446,7 @@ int intel_guc_resume(struct drm_i915_private  
>> *dev_priv)
>>                 return 0;
>>
>>         if (i915_modparams.guc_log_level >= 0)
>> -               intel_enable_guc_interrupts(dev_priv);
>> +               intel_enable_guc_interrupts(guc);
>>
>>         data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>>         data[1] = GUC_POWER_D0;
>> @@ -508,15 +508,19 @@ u32 intel_guc_wopcm_size(struct drm_i915_private  
>> *dev_priv)
>>         return wopcm_size;
>>  }
>>
>> -void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv)
>> +void intel_reset_guc_interrupts(struct intel_guc *guc)
>>  {
>> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>>         spin_lock_irq(&dev_priv->irq_lock);
>>         gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
>>         spin_unlock_irq(&dev_priv->irq_lock);
>>  }
>>
>> -void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv)
>> +void intel_enable_guc_interrupts(struct intel_guc *guc)
>>  {
>> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>>         spin_lock_irq(&dev_priv->irq_lock);
>>         if (!dev_priv->guc.interrupts_enabled) {
>>                 WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>> @@ -527,8 +531,10 @@ void intel_enable_guc_interrupts(struct  
>> drm_i915_private *dev_priv)
>>         spin_unlock_irq(&dev_priv->irq_lock);
>>  }
>>
>> -void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv)
>> +void intel_disable_guc_interrupts(struct intel_guc *guc)
>>  {
>> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>>         spin_lock_irq(&dev_priv->irq_lock);
>>         dev_priv->guc.interrupts_enabled = false;
>>
>> @@ -537,11 +543,13 @@ void intel_disable_guc_interrupts(struct  
>> drm_i915_private *dev_priv)
>>         spin_unlock_irq(&dev_priv->irq_lock);
>>         synchronize_irq(dev_priv->drm.irq);
>>
>> -       intel_reset_guc_interrupts(dev_priv);
>> +       intel_reset_guc_interrupts(guc);
>>  }
>>
>> -void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32  
>> gt_iir)
>> +void intel_guc_irq_handler(struct intel_guc *guc, u32 gt_iir)
>>  {
>> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>>         if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
>>                 /* Sample the log buffer flush related bits & clear  
>> them out now
>>                  * itself from the message identity register to  
>> minimize the
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index c37d34d..49f33b9 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -131,9 +131,9 @@ static inline u32 guc_ggtt_offset(struct i915_vma  
>> *vma)
>>  int intel_guc_resume(struct drm_i915_private *dev_priv);
>>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32  
>> size);
>>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>> -void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv);
>> -void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv);
>> -void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv);
>> -void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32  
>> pm_iir);
>> +void intel_reset_guc_interrupts(struct intel_guc *guc);
>> +void intel_enable_guc_interrupts(struct intel_guc *guc);
>> +void intel_disable_guc_interrupts(struct intel_guc *guc);
>> +void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
>
> You should also consider s/intel_verb_guc/intel_guc_verb/ since we are
> now passing intel_guc as the object.

And additionally also consider squashing this patch with 02/12 (and maybe
01/12 as well) to avoid changing the same lines twice

Michal

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

✗ Fi.CI.BAT: failure for GuC Interrupts/Log updates (rev2)

[Patchwork]

== Series Details ==

Series: GuC Interrupts/Log updates (rev2)
URL   : https://patchwork.freedesktop.org/series/32179/
State : failure

== Summary ==

Series 32179v2 GuC Interrupts/Log updates
https://patchwork.freedesktop.org/api/1.0/series/32179/revisions/2/mbox/

Test core_auth:
        Subgroup basic-auth:
                pass       -> INCOMPLETE (fi-gdg-551)
                pass       -> INCOMPLETE (fi-blb-e6850)
                pass       -> INCOMPLETE (fi-pnv-d510)
                pass       -> INCOMPLETE (fi-bwr-2160)
                pass       -> INCOMPLETE (fi-elk-e7500)
                pass       -> INCOMPLETE (fi-ilk-650)
                pass       -> INCOMPLETE (fi-snb-2520m)
                pass       -> INCOMPLETE (fi-snb-2600)
                pass       -> INCOMPLETE (fi-ivb-3520m)
                pass       -> INCOMPLETE (fi-ivb-3770)
                pass       -> INCOMPLETE (fi-byt-j1900)
                pass       -> INCOMPLETE (fi-byt-n2820)
                pass       -> INCOMPLETE (fi-hsw-4770)
                pass       -> INCOMPLETE (fi-hsw-4770r)
                pass       -> INCOMPLETE (fi-bdw-5557u)
                pass       -> INCOMPLETE (fi-bdw-gvtdvm)
                pass       -> INCOMPLETE (fi-bsw-n3050)
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
Test core_prop_blob:
        Subgroup basic:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-skl-6600u)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6700k2)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> DMESG-WARN (fi-bxt-dsi)
                pass       -> DMESG-WARN (fi-bxt-j4205)
                pass       -> DMESG-WARN (fi-kbl-7500u) fdo#103285
                pass       -> DMESG-WARN (fi-kbl-7560u)
                pass       -> DMESG-WARN (fi-kbl-7567u)
                pass       -> DMESG-WARN (fi-kbl-r)
                pass       -> SKIP       (fi-glk-1)
Test drv_getparams_basic:
        Subgroup basic-eu-total:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-subslice-total:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
Test gem_basic:
        Subgroup bad-close:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
        Subgroup create-close:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
        Subgroup create-fd-close:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
Test gem_busy:
        Subgroup basic-busy-default:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-hang-default:
                pass       -> SKIP       (fi-skl-gvtdvm) fdo#104108 +4
                pass       -> SKIP       (fi-glk-1)
Test gem_close_race:
        Subgroup basic-process:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-threads:
                pass       -> DMESG-WARN (fi-skl-6600u)
                pass       -> DMESG-WARN (fi-skl-6700k2)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-kbl-7560u)
                pass       -> DMESG-WARN (fi-kbl-7567u)
                pass       -> DMESG-WARN (fi-kbl-r)
                pass       -> SKIP       (fi-glk-1)
Test gem_cpu_reloc:
        Subgroup basic:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
Test gem_cs_tlb:
        Subgroup basic-default:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
Test gem_ctx_create:
        Subgroup basic:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-files:
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-glk-1)
WARNING: Long output truncated

eb3dae33ff20a9460034bbfa8a95c1c56c173116 drm-tip: 2018y-01m-04d-13h-53m-17s UTC integration manifest
6049e29e65e7 HAX: drm/i915/guc: enable GuC submission/logging for CI
1eec95567905 drm/i915/guc: Restore GuC interrupts across suspend/reset if enabled
a66e152d7900 drm/i915/guc: Add client support to enable/disable GuC interrupts
b5708ea56e27 drm/i915/guc: Make GuC log related functions depend only on log level
7290c330c8a8 drm/i915/guc: Make guc_log_level parameter immutable
aeb9e89818ed drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts
1f432bfb45e2 drm/i915/guc: Separate creation/release of runtime logging data from base logging data
292e0c1df573 drm/i915/guc: Fix GuC interrupts disabling with logging
f399fbf9cba8 drm/i915/guc: Add description and comments about guc_log_level parameter
39f2aa7bd98b drm/i915/guc: Pass intel_guc struct parameter to GuC interrupts functions
ee3710559a43 drm/i915/guc: Move GuC interrupts related functions from i915_irq.c to intel_guc.c
230bd006971c drm/i915: Export low level PM IRQ functions to use from GuC functions

== Logs ==

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

Re: [PATCH v3 04/12] drm/i915/guc: Add description and comments about guc_log_level parameter

[Michal Wajdeczko]

On Thu, 04 Jan 2018 17:21:46 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> guc_log_level parameter takes effect when GuC is loaded which is
> controlled through enable_guc parameter. Add this relation info.
                                                                  ^^^
Extra "."

> in parameter description and documentation.
> Earlier, this patch was added to sanitize guc_log_level like old
> GuC parameters enable_guc_loading/submission. With new parameter
> enable_guc, sanitization of guc_log_level is no more needed.

Hmm, I think we still need to sanitize log_level if it was wrongly
enabled without enabling GuC first (in intel_uc_sanitize_options).

>
> v2: Added documentation to intel_guc_log.c and param description
> about GuC loading dependency. (Michal Wajdeczko)
>
> v3: Removed sanitization of module parameter guc_log_level.
> Previous review comments not applicable now.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v2
> ---
>  drivers/gpu/drm/i915/i915_params.c   | 3 ++-
>  drivers/gpu/drm/i915/intel_guc_log.c | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c  
> b/drivers/gpu/drm/i915/i915_params.c
> index b5f3eb4..a93a6ca 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = {
>  	"(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
> i915_param_named(guc_log_level, int, 0400,
> -	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
> +	"GuC firmware logging level. This takes effect only if GuC is to be "
> +	"loaded (depends on enable_guc) (-1:disabled (default), 0-3:enabled)");

Btw, I was planing to change above values to follow schema used in other  
modparams:

-1: auto (then it can be controlled by USES_GUC and DRM_I915_DEBUG_GUC)
  0: disabled
  1: enabled (legacy level 0)
  2: enabled (legacy level 1)
  3: enabled (legacy level 2)
  4: enabled (legacy level 3)

So now I'm not sure that I want your patch ;)

> i915_param_named_unsafe(guc_firmware_path, charp, 0400,
>  	"GuC firmware path to use instead of the default one");
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index 59a9021..d0131bc 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -34,6 +34,7 @@
>   * DOC: GuC firmware log
>   *
>   * Firmware log is enabled by setting i915.guc_log_level to  
> non-negative level.
> + * This takes effect only if GuC is to be loaded based on enable_guc.

... based on i915.enable_guc modparam.

>   * Log data is printed out via reading debugfs i915_guc_log_dump.  
> Reading from
>   * i915_guc_load_status will print out firmware loading status and  
> scratch
>   * registers value.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 09/12] drm/i915/guc: Make GuC log related functions depend only on log level

[Michal Wajdeczko]

On Thu, 04 Jan 2018 17:21:51 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> With GuC log level set properly only for cases where GuC is loaded we can
> remove the GuC submission checks from flush_guc_logs and  
> guc_log_register,
> unregister and uc_fini_hw functions. It is important to note that GuC log
> runtime data has to be freed during driver unregister.
> Freeing of that data can't be gated by guc_log_level check because if we
> free GuC log runtime only when log level >=0 then it will not be  
> destroyed
> when logging is disabled after enabling before driver unload.
>
> Also, with this patch GuC interrupts are enabled first after GuC load if
> logging is enabled. GuC to Host interrupts will be needed for GuC CT
> buffer recv mechanism and hence we will be adding support to control that
> interrupt based on ref. taken by Log or CT recv feature in next patch. To
> prepare for that all interrupt updates are now gated by GuC log level
> checks.
>
> v2: Rebase. Updated check in i915_guc_log_unregister to be based on
> guc_log_level. (Michal Wajdeczko)
>
> v3: Rebase. Made all GuC log related functions depend only log level.
> Updated uC init w.r.t enabling of GuC interrupts. Commit message update.
> Rebase w.r.t guc_log_level immutable changes. (Tvrtko)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc.c     |  3 ++-
>  drivers/gpu/drm/i915/intel_guc_log.c | 17 +++++++----------
>  drivers/gpu/drm/i915/intel_uc.c      | 15 ++++++++-------
>  3 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index d351642..36d1bca 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -406,7 +406,8 @@ int intel_guc_suspend(struct drm_i915_private  
> *dev_priv)
>  	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>  		return 0;
> -	intel_disable_guc_interrupts(guc);
> +	if (guc->log.level >= 0)
> +		intel_disable_guc_interrupts(guc);
> 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>  	/* any value greater than GUC_POWER_D0 */
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index d979830..7bc0065 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -484,8 +484,7 @@ static void guc_log_capture_logs(struct intel_guc  
> *guc)
> static void guc_flush_logs(struct intel_guc *guc)
>  {
> -	if (!USES_GUC_SUBMISSION(dev_priv) ||
> -	    guc->log.level < 0)
> +	if (guc->log.level < 0)
>  		return;
> 	/* First disable the interrupts, will be renabled afterwards */
> @@ -613,8 +612,7 @@ int i915_guc_log_control(struct drm_i915_private  
> *dev_priv, u64 control_val)
> void i915_guc_log_register(struct drm_i915_private *dev_priv)
>  {
> -	if (!USES_GUC_SUBMISSION(dev_priv) ||
> -	    dev_priv->guc.log.level < 0)
> +	if (dev_priv->guc.log.level < 0)
>  		return;
> 	mutex_lock(&dev_priv->drm.struct_mutex);
> @@ -624,14 +622,13 @@ void i915_guc_log_register(struct drm_i915_private  
> *dev_priv)
> void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>  {
> -	if (!USES_GUC_SUBMISSION(dev_priv))
> -		return;
> -
>  	mutex_lock(&dev_priv->drm.struct_mutex);
>  	/* GuC logging is currently the only user of Guc2Host interrupts */
> -	intel_runtime_pm_get(dev_priv);
> -	intel_disable_guc_interrupts(&dev_priv->guc);
> -	intel_runtime_pm_put(dev_priv);
> +	if (dev_priv->guc.log.level >= 0) {

Can we move "if (guc->log.level >= 0)" condition check to
intel_guc_[disable|enable]_interrupts functions? Then we
should be able to avoid repeating this check over and over
and it will be also easier for use once we add new CTB check.

> +		intel_runtime_pm_get(dev_priv);
> +		intel_disable_guc_interrupts(&dev_priv->guc);
> +		intel_runtime_pm_put(dev_priv);
> +	}
> 	intel_guc_log_runtime_destroy(&dev_priv->guc);
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index e2e2020..fb5edcc 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -318,9 +318,12 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	if (ret)
>  		goto err_log_capture;
> +	if (guc->log.level >= 0)
> +		intel_enable_guc_interrupts(guc);
> +
>  	ret = guc_enable_communication(guc);
>  	if (ret)
> -		goto err_log_capture;
> +		goto err_interrupts;
> 	if (USES_HUC(dev_priv)) {
>  		ret = intel_huc_auth(huc);
> @@ -329,12 +332,9 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	}
> 	if (USES_GUC_SUBMISSION(dev_priv)) {
> -		if (guc->log.level >= 0)
> -			intel_enable_guc_interrupts(guc);
> -
>  		ret = intel_guc_submission_enable(guc);
>  		if (ret)
> -			goto err_interrupts;
> +			goto err_communication;
>  	}
> 	dev_info(dev_priv->drm.dev, "GuC firmware version %u.%u\n",
> @@ -349,10 +349,11 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	/*
>  	 * We've failed to load the firmware :(
>  	 */
> -err_interrupts:
> -	intel_disable_guc_interrupts(guc);
>  err_communication:
>  	guc_disable_communication(guc);
> +err_interrupts:
> +	if (guc->log.level >= 0)
> +		intel_disable_guc_interrupts(guc);
>  err_log_capture:
>  	guc_capture_load_err_log(guc);
>  err_out:
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 03/12] drm/i915/guc: Pass intel_guc struct parameter to GuC interrupts functions

[Michal Wajdeczko]

On Thu, 04 Jan 2018 17:21:45 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> GuC interrupts handling functions are GuC specific functions hence update
> the parameter from dev_priv to intel_guc struct.
>
> v2-v3: Rebase.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c      |  2 +-
>  drivers/gpu/drm/i915/intel_guc.c     | 22 +++++++++++++++-------
>  drivers/gpu/drm/i915/intel_guc.h     |  8 ++++----
>  drivers/gpu/drm/i915/intel_guc_log.c |  8 +++-----
>  drivers/gpu/drm/i915/intel_uc.c      |  8 ++++----
>  5 files changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c  
> b/drivers/gpu/drm/i915/i915_irq.c
> index 3f4eff9..a1ae057 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1447,7 +1447,7 @@ static void gen8_gt_irq_handler(struct  
> drm_i915_private *dev_priv,
>  		gen6_rps_irq_handler(dev_priv, gt_iir[2]);
> 	if (gt_iir[2] & dev_priv->pm_guc_events)
> -		intel_guc_irq_handler(dev_priv, gt_iir[2]);
> +		intel_guc_irq_handler(&dev_priv->guc, gt_iir[2]);
>  }
> static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index e95ff2d..14bf508d 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -400,7 +400,7 @@ int intel_guc_suspend(struct drm_i915_private  
> *dev_priv)
>  	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>  		return 0;
> -	intel_disable_guc_interrupts(dev_priv);
> +	intel_disable_guc_interrupts(guc);

Hmm, if we disable irq here, then we might have problems with
sending this message to GuC if we use CTB as comm mechanism...

> 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>  	/* any value greater than GUC_POWER_D0 */
> @@ -446,7 +446,7 @@ int intel_guc_resume(struct drm_i915_private  
> *dev_priv)
>  		return 0;
> 	if (i915_modparams.guc_log_level >= 0)
> -		intel_enable_guc_interrupts(dev_priv);
> +		intel_enable_guc_interrupts(guc);
> 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>  	data[1] = GUC_POWER_D0;
> @@ -508,15 +508,19 @@ u32 intel_guc_wopcm_size(struct drm_i915_private  
> *dev_priv)
>  	return wopcm_size;
>  }
> -void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv)
> +void intel_reset_guc_interrupts(struct intel_guc *guc)
>  {
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
> -void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv)
> +void intel_enable_guc_interrupts(struct intel_guc *guc)
>  {
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	if (!dev_priv->guc.interrupts_enabled) {

Just spotted:
as we have "guc" try to use "guc->" instead of "dev_priv->guc."

>  		WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> @@ -527,8 +531,10 @@ void intel_enable_guc_interrupts(struct  
> drm_i915_private *dev_priv)
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
> -void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv)
> +void intel_disable_guc_interrupts(struct intel_guc *guc)
>  {
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	dev_priv->guc.interrupts_enabled = false;

same here (and possibly in other places too)

> @@ -537,11 +543,13 @@ void intel_disable_guc_interrupts(struct  
> drm_i915_private *dev_priv)
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  	synchronize_irq(dev_priv->drm.irq);
> -	intel_reset_guc_interrupts(dev_priv);
> +	intel_reset_guc_interrupts(guc);
>  }
> -void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32  
> gt_iir)
> +void intel_guc_irq_handler(struct intel_guc *guc, u32 gt_iir)
>  {
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
>  	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
>  		/* Sample the log buffer flush related bits & clear them out now
>  		 * itself from the message identity register to minimize the
> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
> b/drivers/gpu/drm/i915/intel_guc.h
> index c37d34d..49f33b9 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -131,9 +131,9 @@ static inline u32 guc_ggtt_offset(struct i915_vma  
> *vma)
>  int intel_guc_resume(struct drm_i915_private *dev_priv);
>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32  
> size);
>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
> -void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv);
> -void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv);
> -void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv);
> -void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32  
> pm_iir);
> +void intel_reset_guc_interrupts(struct intel_guc *guc);
> +void intel_enable_guc_interrupts(struct intel_guc *guc);
> +void intel_disable_guc_interrupts(struct intel_guc *guc);
> +void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
> #endif
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index 5cd68f6..59a9021 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -480,14 +480,12 @@ static void guc_log_capture_logs(struct intel_guc  
> *guc)
> static void guc_flush_logs(struct intel_guc *guc)
>  {
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -
>  	if (!USES_GUC_SUBMISSION(dev_priv) ||
>  	    (i915_modparams.guc_log_level < 0))
>  		return;
> 	/* First disable the interrupts, will be renabled afterwards */
> -	intel_disable_guc_interrupts(dev_priv);
> +	intel_disable_guc_interrupts(guc);
> 	/* Before initiating the forceful flush, wait for any pending/ongoing
>  	 * flush to complete otherwise forceful flush may not actually happen.
> @@ -605,7 +603,7 @@ int i915_guc_log_control(struct drm_i915_private  
> *dev_priv, u64 control_val)
>  		}
> 		/* GuC logging is currently the only user of Guc2Host interrupts */
> -		intel_enable_guc_interrupts(dev_priv);
> +		intel_enable_guc_interrupts(guc);
>  	} else {
>  		/* Once logging is disabled, GuC won't generate logs & send an
>  		 * interrupt. But there could be some data in the log buffer
> @@ -639,7 +637,7 @@ void i915_guc_log_unregister(struct drm_i915_private  
> *dev_priv)
> 	mutex_lock(&dev_priv->drm.struct_mutex);
>  	/* GuC logging is currently the only user of Guc2Host interrupts */
> -	intel_disable_guc_interrupts(dev_priv);
> +	intel_disable_guc_interrupts(&dev_priv->guc);
>  	guc_log_runtime_destroy(&dev_priv->guc);
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 8e58e5a..f82453b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -271,7 +271,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	GEM_BUG_ON(!HAS_GUC(dev_priv));
> 	guc_disable_communication(guc);
> -	intel_reset_guc_interrupts(dev_priv);
> +	intel_reset_guc_interrupts(guc);
> 	/* init WOPCM */
>  	I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
> @@ -325,7 +325,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
> 	if (USES_GUC_SUBMISSION(dev_priv)) {
>  		if (i915_modparams.guc_log_level >= 0)
> -			intel_enable_guc_interrupts(dev_priv);
> +			intel_enable_guc_interrupts(guc);
> 		ret = intel_guc_submission_enable(guc);
>  		if (ret)
> @@ -345,7 +345,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	 * We've failed to load the firmware :(
>  	 */
>  err_interrupts:
> -	intel_disable_guc_interrupts(dev_priv);
> +	intel_disable_guc_interrupts(guc);
>  err_communication:
>  	guc_disable_communication(guc);
>  err_log_capture:
> @@ -379,5 +379,5 @@ void intel_uc_fini_hw(struct drm_i915_private  
> *dev_priv)
>  	guc_disable_communication(guc);
> 	if (USES_GUC_SUBMISSION(dev_priv))
> -		intel_disable_guc_interrupts(dev_priv);
> +		intel_disable_guc_interrupts(guc);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 10/12] drm/i915/guc: Add client support to enable/disable GuC interrupts

[Michal Wajdeczko]

On Thu, 04 Jan 2018 17:21:52 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> This patch adds support to enable/disable GuC interrupts for different
> features without impacting other's need. Currently GuC log capture and
> CT buffer receive mechanisms use the GuC interrupts. GuC interrupts are
> currently enabled and disabled in different logging scenarios all gated
> by log level.
>
> v2: Rebase with all GuC interrupt handlers moved to intel_guc.c. Handling
> multiple clients for GuC interrupts enable/disable.
> (Michal Wajdeczko)
>
> v3: Removed spin lock and using test_bit in i915_guc_info. Prepared low
> level helpers to get/put GuC interrupts that can be reused during
> suspend/resume. (Tvrtko)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  6 +++++
>  drivers/gpu/drm/i915/intel_guc.c     | 47  
> +++++++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_guc.h     | 11 ++++++---
>  drivers/gpu/drm/i915/intel_guc_log.c |  6 ++---
>  drivers/gpu/drm/i915/intel_uc.c      |  4 +--
>  5 files changed, 54 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c  
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 16f9a95..eef4c8b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2340,6 +2340,12 @@ static int i915_guc_info(struct seq_file *m, void  
> *data)
>  	GEM_BUG_ON(!guc->execbuf_client);
>  	GEM_BUG_ON(!guc->preempt_client);
> +	seq_puts(m, "GuC Interrupt Clients: ");
> +	if (test_bit(GUC_INTR_CLIENT_LOG, &guc->interrupt_clients))
> +		seq_puts(m, "GuC Logging\n");
> +	else
> +		seq_puts(m, "None\n");
> +

Maybe this can be done in intel_guc_log.c as part of:

void intel_guc_log_dump(const struct intel_guc_log *log, struct  
drm_printer *p);


>  	seq_printf(m, "Doorbell map:\n");
>  	seq_printf(m, "\t%*pb\n", GUC_NUM_DOORBELLS, guc->doorbell_bitmap);
>  	seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", guc->db_cacheline);
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index 36d1bca..d356c40 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -407,7 +407,7 @@ int intel_guc_suspend(struct drm_i915_private  
> *dev_priv)
>  		return 0;
> 	if (guc->log.level >= 0)
> -		intel_disable_guc_interrupts(guc);
> +		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
> 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>  	/* any value greater than GUC_POWER_D0 */
> @@ -453,7 +453,7 @@ int intel_guc_resume(struct drm_i915_private  
> *dev_priv)
>  		return 0;
> 	if (guc->log.level >= 0)
> -		intel_enable_guc_interrupts(guc);
> +		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
> 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>  	data[1] = GUC_POWER_D0;
> @@ -524,28 +524,51 @@ void intel_reset_guc_interrupts(struct intel_guc  
> *guc)
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
> -void intel_enable_guc_interrupts(struct intel_guc *guc)
> +static void __intel_get_guc_interrupts(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	lockdep_assert_held(&dev_priv->irq_lock);
> +
> +	WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> +			       dev_priv->pm_guc_events);
> +	gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> +}
> +
> +void intel_get_guc_interrupts(struct intel_guc *guc, enum  
> guc_intr_client id)

What about intel_guc_interrupts_get(guc, id) ?

>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> 	spin_lock_irq(&dev_priv->irq_lock);
> -	if (!dev_priv->guc.interrupts_enabled) {
> -		WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> -				       dev_priv->pm_guc_events);
> -		dev_priv->guc.interrupts_enabled = true;
> -		gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> -	}
> +
> +	if (!guc->interrupt_clients)
> +		__intel_get_guc_interrupts(guc);
> +	__set_bit(id, &guc->interrupt_clients);

Do we care about scenarios when we call "get" more than once?
Maybe we need GEM_WARN_ON at the minimum ?

Then I'm wondering if "get/put" are correct if we don't do any
refcounting... maybe "enable/disable" are more appropriate then?

> +
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
> -void intel_disable_guc_interrupts(struct intel_guc *guc)
> +static void __intel_put_guc_interrupts(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	spin_lock_irq(&dev_priv->irq_lock);
> -	dev_priv->guc.interrupts_enabled = false;
> +	lockdep_assert_held(&dev_priv->irq_lock);
> 	gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> +}
> +
> +void intel_put_guc_interrupts(struct intel_guc *guc, enum  
> guc_intr_client id)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +
> +	__clear_bit(id, &guc->interrupt_clients);
> +	if (guc->interrupt_clients) {
> +		spin_unlock_irq(&dev_priv->irq_lock);
> +		return;
> +	}
> +	__intel_put_guc_interrupts(guc);
> 	spin_unlock_irq(&dev_priv->irq_lock);
>  	synchronize_irq(dev_priv->drm.irq);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
> b/drivers/gpu/drm/i915/intel_guc.h
> index 49f33b9..af74392 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -34,6 +34,11 @@
>  #include "intel_uc_fw.h"
>  #include "i915_vma.h"
> +enum guc_intr_client {

Hmm, what about adding intel_ prefix ?

> +	GUC_INTR_CLIENT_LOG = 0,
> +	GUC_INTR_CLIENT_COUNT
> +};
> +
>  struct guc_preempt_work {
>  	struct work_struct work;
>  	struct intel_engine_cs *engine;
> @@ -53,7 +58,7 @@ struct intel_guc {
>  	struct drm_i915_gem_object *load_err_log;
> 	/* intel_guc_recv interrupt related state */
> -	bool interrupts_enabled;
> +	unsigned long interrupt_clients;
> 	struct i915_vma *ads_vma;
>  	struct i915_vma *stage_desc_pool;
> @@ -132,8 +137,8 @@ static inline u32 guc_ggtt_offset(struct i915_vma  
> *vma)
>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32  
> size);
>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>  void intel_reset_guc_interrupts(struct intel_guc *guc);
> -void intel_enable_guc_interrupts(struct intel_guc *guc);
> -void intel_disable_guc_interrupts(struct intel_guc *guc);
> +void intel_get_guc_interrupts(struct intel_guc *guc, enum  
> guc_intr_client id);
> +void intel_put_guc_interrupts(struct intel_guc *guc, enum  
> guc_intr_client id);
>  void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
> #endif
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index 7bc0065..ddd4f6d 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -488,7 +488,7 @@ static void guc_flush_logs(struct intel_guc *guc)
>  		return;
> 	/* First disable the interrupts, will be renabled afterwards */
> -	intel_disable_guc_interrupts(guc);
> +	intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
> 	/* Before initiating the forceful flush, wait for any pending/ongoing
>  	 * flush to complete otherwise forceful flush may not actually happen.
> @@ -594,7 +594,7 @@ int i915_guc_log_control(struct drm_i915_private  
> *dev_priv, u64 control_val)
>  		}
> 		/* GuC logging is currently the only user of Guc2Host interrupts */

I guess above comment is now obsolete ;)

> -		intel_enable_guc_interrupts(guc);
> +		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>  	} else {
>  		/* Once logging is disabled, GuC won't generate logs & send an
>  		 * interrupt. But there could be some data in the log buffer
> @@ -626,7 +626,7 @@ void i915_guc_log_unregister(struct drm_i915_private  
> *dev_priv)
>  	/* GuC logging is currently the only user of Guc2Host interrupts */

Same here.

>  	if (dev_priv->guc.log.level >= 0) {
>  		intel_runtime_pm_get(dev_priv);
> -		intel_disable_guc_interrupts(&dev_priv->guc);
> +		intel_put_guc_interrupts(&dev_priv->guc, GUC_INTR_CLIENT_LOG);
>  		intel_runtime_pm_put(dev_priv);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index fb5edcc..93ecbf1 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -319,7 +319,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  		goto err_log_capture;
> 	if (guc->log.level >= 0)
> -		intel_enable_guc_interrupts(guc);
> +		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
> 	ret = guc_enable_communication(guc);
>  	if (ret)
> @@ -353,7 +353,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	guc_disable_communication(guc);
>  err_interrupts:
>  	if (guc->log.level >= 0)
> -		intel_disable_guc_interrupts(guc);
> +		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>  err_log_capture:
>  	guc_capture_load_err_log(guc);
>  err_out:
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 11/12] drm/i915/guc: Restore GuC interrupts across suspend/reset if enabled

[Michal Wajdeczko]

On Thu, 04 Jan 2018 17:21:53 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> In order to override the disable/enable control of GuC interrupts during
> suspend/reset cycle we are creating two new functions suspend/restore
> guc_interrupts which check if interrupts were enabled and disable them
> on suspend and enable them on resume. They are used to restore interrupts
> across reset as well.
>
> Further restructuring of runtime_pm_enable/disable_interrupts and
> suspend/restore_guc_interrupts will be done in upcoming patches.
>
> v2: Rebase.
>
> v3: Updated suspend/restore with the new low level get/put functions.
> (Tvrtko)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  2 ++
>  drivers/gpu/drm/i915/intel_guc.c     | 32  
> ++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_guc.h     |  2 ++
>  3 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c  
> b/drivers/gpu/drm/i915/intel_display.c
> index 0cd3559..2e0db53 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3676,8 +3676,10 @@ void intel_finish_reset(struct drm_i915_private  
> *dev_priv)
>  		 * The display has been reset as well,
>  		 * so need a full re-initialization.
>  		 */
> +		intel_suspend_guc_interrupts(&dev_priv->guc);
>  		intel_runtime_pm_disable_interrupts(dev_priv);
>  		intel_runtime_pm_enable_interrupts(dev_priv);
> +		intel_restore_guc_interrupts(&dev_priv->guc);
> 		intel_pps_unlock_regs_wa(dev_priv);
>  		intel_modeset_init_hw(dev);
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index d356c40..28a418a 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -406,8 +406,7 @@ int intel_guc_suspend(struct drm_i915_private  
> *dev_priv)
>  	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>  		return 0;
> -	if (guc->log.level >= 0)
> -		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
> +	intel_suspend_guc_interrupts(guc);

Hmm, maybe we should introduce

	intel_uc_suspend(struct drm_i915_private *dev_priv)

which will call separately

	intel_guc_suspend(guc); /* send suspend action */
	intel_guc_suspend_interrupts(guc);

> 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>  	/* any value greater than GUC_POWER_D0 */
> @@ -452,8 +451,7 @@ int intel_guc_resume(struct drm_i915_private  
> *dev_priv)
>  	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>  		return 0;
> -	if (guc->log.level >= 0)
> -		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
> +	intel_restore_guc_interrupts(guc);
> 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>  	data[1] = GUC_POWER_D0;
> @@ -548,6 +546,16 @@ void intel_get_guc_interrupts(struct intel_guc  
> *guc, enum guc_intr_client id)
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
> +void intel_restore_guc_interrupts(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	if (guc->interrupt_clients)
> +		__intel_get_guc_interrupts(guc);
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
>  static void __intel_put_guc_interrupts(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> @@ -576,6 +584,22 @@ void intel_put_guc_interrupts(struct intel_guc  
> *guc, enum guc_intr_client id)
>  	intel_reset_guc_interrupts(guc);
>  }
> +void intel_suspend_guc_interrupts(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	if (!guc->interrupt_clients) {
> +		spin_unlock_irq(&dev_priv->irq_lock);
> +		return;
> +	}
> +	__intel_put_guc_interrupts(guc);
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +	synchronize_irq(dev_priv->drm.irq);
> +
> +	intel_reset_guc_interrupts(guc);
> +}
> +
>  void intel_guc_irq_handler(struct intel_guc *guc, u32 gt_iir)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
> b/drivers/gpu/drm/i915/intel_guc.h
> index af74392..2c14781 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -140,5 +140,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma  
> *vma)
>  void intel_get_guc_interrupts(struct intel_guc *guc, enum  
> guc_intr_client id);
>  void intel_put_guc_interrupts(struct intel_guc *guc, enum  
> guc_intr_client id);
>  void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
> +void intel_suspend_guc_interrupts(struct intel_guc *guc);
> +void intel_restore_guc_interrupts(struct intel_guc *guc);

To match obj-verb pattern:

intel_guc_suspend_interrupts
intel_guc_restore_interrupts

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

Re: [PATCH v3 03/12] drm/i915/guc: Pass intel_guc struct parameter to GuC interrupts functions

[Sagar Arun Kamble]

On 1/4/2018 10:01 PM, Michal Wajdeczko wrote:
> On Thu, 04 Jan 2018 17:23:05 +0100, Chris Wilson 
> <chris@chris-wilson.co.uk> wrote:
>
>> Quoting Sagar Arun Kamble (2018-01-04 16:21:45)
>>> GuC interrupts handling functions are GuC specific functions hence 
>>> update
>>> the parameter from dev_priv to intel_guc struct.
>>>
>>> v2-v3: Rebase.
>>>
>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_irq.c      |  2 +-
>>>  drivers/gpu/drm/i915/intel_guc.c     | 22 +++++++++++++++-------
>>>  drivers/gpu/drm/i915/intel_guc.h     |  8 ++++----
>>>  drivers/gpu/drm/i915/intel_guc_log.c |  8 +++-----
>>>  drivers/gpu/drm/i915/intel_uc.c      |  8 ++++----
>>>  5 files changed, 27 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>>> b/drivers/gpu/drm/i915/i915_irq.c
>>> index 3f4eff9..a1ae057 100644
>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>> @@ -1447,7 +1447,7 @@ static void gen8_gt_irq_handler(struct 
>>> drm_i915_private *dev_priv,
>>>                 gen6_rps_irq_handler(dev_priv, gt_iir[2]);
>>>
>>>         if (gt_iir[2] & dev_priv->pm_guc_events)
>>> -               intel_guc_irq_handler(dev_priv, gt_iir[2]);
>>> +               intel_guc_irq_handler(&dev_priv->guc, gt_iir[2]);
>>>  }
>>>
>>>  static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
>>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>>> b/drivers/gpu/drm/i915/intel_guc.c
>>> index e95ff2d..14bf508d 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>>> @@ -400,7 +400,7 @@ int intel_guc_suspend(struct drm_i915_private 
>>> *dev_priv)
>>>         if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>>                 return 0;
>>>
>>> -       intel_disable_guc_interrupts(dev_priv);
>>> +       intel_disable_guc_interrupts(guc);
>>>
>>>         data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>>>         /* any value greater than GUC_POWER_D0 */
>>> @@ -446,7 +446,7 @@ int intel_guc_resume(struct drm_i915_private 
>>> *dev_priv)
>>>                 return 0;
>>>
>>>         if (i915_modparams.guc_log_level >= 0)
>>> -               intel_enable_guc_interrupts(dev_priv);
>>> +               intel_enable_guc_interrupts(guc);
>>>
>>>         data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>>>         data[1] = GUC_POWER_D0;
>>> @@ -508,15 +508,19 @@ u32 intel_guc_wopcm_size(struct 
>>> drm_i915_private *dev_priv)
>>>         return wopcm_size;
>>>  }
>>>
>>> -void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv)
>>> +void intel_reset_guc_interrupts(struct intel_guc *guc)
>>>  {
>>> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>> +
>>>         spin_lock_irq(&dev_priv->irq_lock);
>>>         gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
>>>         spin_unlock_irq(&dev_priv->irq_lock);
>>>  }
>>>
>>> -void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv)
>>> +void intel_enable_guc_interrupts(struct intel_guc *guc)
>>>  {
>>> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>> +
>>>         spin_lock_irq(&dev_priv->irq_lock);
>>>         if (!dev_priv->guc.interrupts_enabled) {
>>>                 WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>>> @@ -527,8 +531,10 @@ void intel_enable_guc_interrupts(struct 
>>> drm_i915_private *dev_priv)
>>>         spin_unlock_irq(&dev_priv->irq_lock);
>>>  }
>>>
>>> -void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv)
>>> +void intel_disable_guc_interrupts(struct intel_guc *guc)
>>>  {
>>> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>> +
>>>         spin_lock_irq(&dev_priv->irq_lock);
>>>         dev_priv->guc.interrupts_enabled = false;
>>>
>>> @@ -537,11 +543,13 @@ void intel_disable_guc_interrupts(struct 
>>> drm_i915_private *dev_priv)
>>>         spin_unlock_irq(&dev_priv->irq_lock);
>>>         synchronize_irq(dev_priv->drm.irq);
>>>
>>> -       intel_reset_guc_interrupts(dev_priv);
>>> +       intel_reset_guc_interrupts(guc);
>>>  }
>>>
>>> -void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32 
>>> gt_iir)
>>> +void intel_guc_irq_handler(struct intel_guc *guc, u32 gt_iir)
>>>  {
>>> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>> +
>>>         if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
>>>                 /* Sample the log buffer flush related bits & clear 
>>> them out now
>>>                  * itself from the message identity register to 
>>> minimize the
>>> diff --git a/drivers/gpu/drm/i915/intel_guc.h 
>>> b/drivers/gpu/drm/i915/intel_guc.h
>>> index c37d34d..49f33b9 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc.h
>>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>>> @@ -131,9 +131,9 @@ static inline u32 guc_ggtt_offset(struct 
>>> i915_vma *vma)
>>>  int intel_guc_resume(struct drm_i915_private *dev_priv);
>>>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 
>>> size);
>>>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>>> -void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv);
>>> -void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv);
>>> -void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv);
>>> -void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32 
>>> pm_iir);
>>> +void intel_reset_guc_interrupts(struct intel_guc *guc);
>>> +void intel_enable_guc_interrupts(struct intel_guc *guc);
>>> +void intel_disable_guc_interrupts(struct intel_guc *guc);
>>> +void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
>>
>> You should also consider s/intel_verb_guc/intel_guc_verb/ since we are
>> now passing intel_guc as the object.
>
> And additionally also consider squashing this patch with 02/12 (and maybe
> 01/12 as well) to avoid changing the same lines twice
Sure. Will do these changes. Thanks Michal, Chris.
>
> Michal
>

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

Re: [PATCH v3 12/12] HAX: drm/i915/guc: enable GuC submission/logging for CI

[Sagar Arun Kamble]

On 1/4/2018 9:59 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2018-01-04 16:21:54)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 6c8da9d..c8460c5 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -2659,6 +2659,8 @@ static int intel_runtime_resume(struct device *kdev)
>>          if (intel_uncore_unclaimed_mmio(dev_priv))
>>                  DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
>>   
>> +       intel_runtime_pm_enable_interrupts(dev_priv);
>> +
>>          intel_guc_resume(dev_priv);
>>   
>>          if (IS_GEN9_LP(dev_priv)) {
>> @@ -2682,8 +2684,6 @@ static int intel_runtime_resume(struct device *kdev)
>>          i915_gem_init_swizzling(dev_priv);
>>          i915_gem_restore_fences(dev_priv);
>>   
>> -       intel_runtime_pm_enable_interrupts(dev_priv);
> Have I missed the pending patch?
There is suspend/resume related restructuring series for GuC/GEM that is 
in queue post this series.
Was reviewed partly some time back.

Thanks
Sagar
> -Chris

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

Re: [PATCH v3 04/12] drm/i915/guc: Add description and comments about guc_log_level parameter

[Sagar Arun Kamble]

On 1/4/2018 10:22 PM, Michal Wajdeczko wrote:
> On Thu, 04 Jan 2018 17:21:46 +0100, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> guc_log_level parameter takes effect when GuC is loaded which is
>> controlled through enable_guc parameter. Add this relation info.
> ^^^
> Extra "."
>
>> in parameter description and documentation.
>> Earlier, this patch was added to sanitize guc_log_level like old
>> GuC parameters enable_guc_loading/submission. With new parameter
>> enable_guc, sanitization of guc_log_level is no more needed.
>
> Hmm, I think we still need to sanitize log_level if it was wrongly
> enabled without enabling GuC first (in intel_uc_sanitize_options).
I think it would not be harmful as all decisions based on it will be 
gated by USES_GUC.
>
>>
>> v2: Added documentation to intel_guc_log.c and param description
>> about GuC loading dependency. (Michal Wajdeczko)
>>
>> v3: Removed sanitization of module parameter guc_log_level.
>> Previous review comments not applicable now.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v2
>> ---
>>  drivers/gpu/drm/i915/i915_params.c   | 3 ++-
>>  drivers/gpu/drm/i915/intel_guc_log.c | 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>> b/drivers/gpu/drm/i915/i915_params.c
>> index b5f3eb4..a93a6ca 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = {
>>      "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
>> i915_param_named(guc_log_level, int, 0400,
>> -    "GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
>> +    "GuC firmware logging level. This takes effect only if GuC is to 
>> be "
>> +    "loaded (depends on enable_guc) (-1:disabled (default), 
>> 0-3:enabled)");
>
> Btw, I was planing to change above values to follow schema used in 
> other modparams:
>
> -1: auto (then it can be controlled by USES_GUC and DRM_I915_DEBUG_GUC)
>  0: disabled
>  1: enabled (legacy level 0)
>  2: enabled (legacy level 1)
>  3: enabled (legacy level 2)
>  4: enabled (legacy level 3)
>
> So now I'm not sure that I want your patch ;)
>
Makes sense. Will drop this patch.

Thanks
Sagar
>> i915_param_named_unsafe(guc_firmware_path, charp, 0400,
>>      "GuC firmware path to use instead of the default one");
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 59a9021..d0131bc 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -34,6 +34,7 @@
>>   * DOC: GuC firmware log
>>   *
>>   * Firmware log is enabled by setting i915.guc_log_level to 
>> non-negative level.
>> + * This takes effect only if GuC is to be loaded based on enable_guc.
>
> ... based on i915.enable_guc modparam.
>
>>   * Log data is printed out via reading debugfs i915_guc_log_dump. 
>> Reading from
>>   * i915_guc_load_status will print out firmware loading status and 
>> scratch
>>   * registers value.

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

Re: [PATCH v3 09/12] drm/i915/guc: Make GuC log related functions depend only on log level

[Sagar Arun Kamble]

On 1/4/2018 10:45 PM, Michal Wajdeczko wrote:
> On Thu, 04 Jan 2018 17:21:51 +0100, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> With GuC log level set properly only for cases where GuC is loaded we 
>> can
>> remove the GuC submission checks from flush_guc_logs and 
>> guc_log_register,
>> unregister and uc_fini_hw functions. It is important to note that GuC 
>> log
>> runtime data has to be freed during driver unregister.
>> Freeing of that data can't be gated by guc_log_level check because if we
>> free GuC log runtime only when log level >=0 then it will not be 
>> destroyed
>> when logging is disabled after enabling before driver unload.
>>
>> Also, with this patch GuC interrupts are enabled first after GuC load if
>> logging is enabled. GuC to Host interrupts will be needed for GuC CT
>> buffer recv mechanism and hence we will be adding support to control 
>> that
>> interrupt based on ref. taken by Log or CT recv feature in next 
>> patch. To
>> prepare for that all interrupt updates are now gated by GuC log level
>> checks.
>>
>> v2: Rebase. Updated check in i915_guc_log_unregister to be based on
>> guc_log_level. (Michal Wajdeczko)
>>
>> v3: Rebase. Made all GuC log related functions depend only log level.
>> Updated uC init w.r.t enabling of GuC interrupts. Commit message update.
>> Rebase w.r.t guc_log_level immutable changes. (Tvrtko)
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc.c     |  3 ++-
>>  drivers/gpu/drm/i915/intel_guc_log.c | 17 +++++++----------
>>  drivers/gpu/drm/i915/intel_uc.c      | 15 ++++++++-------
>>  3 files changed, 17 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index d351642..36d1bca 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -406,7 +406,8 @@ int intel_guc_suspend(struct drm_i915_private 
>> *dev_priv)
>>      if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>          return 0;
>> -    intel_disable_guc_interrupts(guc);
>> +    if (guc->log.level >= 0)
>> +        intel_disable_guc_interrupts(guc);
>>     data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>>      /* any value greater than GUC_POWER_D0 */
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index d979830..7bc0065 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -484,8 +484,7 @@ static void guc_log_capture_logs(struct intel_guc 
>> *guc)
>> static void guc_flush_logs(struct intel_guc *guc)
>>  {
>> -    if (!USES_GUC_SUBMISSION(dev_priv) ||
>> -        guc->log.level < 0)
>> +    if (guc->log.level < 0)
>>          return;
>>     /* First disable the interrupts, will be renabled afterwards */
>> @@ -613,8 +612,7 @@ int i915_guc_log_control(struct drm_i915_private 
>> *dev_priv, u64 control_val)
>> void i915_guc_log_register(struct drm_i915_private *dev_priv)
>>  {
>> -    if (!USES_GUC_SUBMISSION(dev_priv) ||
>> -        dev_priv->guc.log.level < 0)
>> +    if (dev_priv->guc.log.level < 0)
>>          return;
>>     mutex_lock(&dev_priv->drm.struct_mutex);
>> @@ -624,14 +622,13 @@ void i915_guc_log_register(struct 
>> drm_i915_private *dev_priv)
>> void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>>  {
>> -    if (!USES_GUC_SUBMISSION(dev_priv))
>> -        return;
>> -
>>      mutex_lock(&dev_priv->drm.struct_mutex);
>>      /* GuC logging is currently the only user of Guc2Host interrupts */
>> -    intel_runtime_pm_get(dev_priv);
>> -    intel_disable_guc_interrupts(&dev_priv->guc);
>> -    intel_runtime_pm_put(dev_priv);
>> +    if (dev_priv->guc.log.level >= 0) {
>
> Can we move "if (guc->log.level >= 0)" condition check to
> intel_guc_[disable|enable]_interrupts functions? Then we
> should be able to avoid repeating this check over and over
> and it will be also easier for use once we add new CTB check.
>
I will create a new wrapper function for this logging specific check. 
Will not move the check to
guc_enable|disable_intr as it will be low level handler to 
enable/disable interrupts to be used
by logging, CTB.
>> +        intel_runtime_pm_get(dev_priv);
>> +        intel_disable_guc_interrupts(&dev_priv->guc);
>> +        intel_runtime_pm_put(dev_priv);
>> +    }
>>     intel_guc_log_runtime_destroy(&dev_priv->guc);
>>      mutex_unlock(&dev_priv->drm.struct_mutex);
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index e2e2020..fb5edcc 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -318,9 +318,12 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>      if (ret)
>>          goto err_log_capture;
>> +    if (guc->log.level >= 0)
>> +        intel_enable_guc_interrupts(guc);
>> +
>>      ret = guc_enable_communication(guc);
>>      if (ret)
>> -        goto err_log_capture;
>> +        goto err_interrupts;
>>     if (USES_HUC(dev_priv)) {
>>          ret = intel_huc_auth(huc);
>> @@ -329,12 +332,9 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>      }
>>     if (USES_GUC_SUBMISSION(dev_priv)) {
>> -        if (guc->log.level >= 0)
>> -            intel_enable_guc_interrupts(guc);
>> -
>>          ret = intel_guc_submission_enable(guc);
>>          if (ret)
>> -            goto err_interrupts;
>> +            goto err_communication;
>>      }
>>     dev_info(dev_priv->drm.dev, "GuC firmware version %u.%u\n",
>> @@ -349,10 +349,11 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>      /*
>>       * We've failed to load the firmware :(
>>       */
>> -err_interrupts:
>> -    intel_disable_guc_interrupts(guc);
>>  err_communication:
>>      guc_disable_communication(guc);
>> +err_interrupts:
>> +    if (guc->log.level >= 0)
>> +        intel_disable_guc_interrupts(guc);
>>  err_log_capture:
>>      guc_capture_load_err_log(guc);
>>  err_out:

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

Re: [PATCH v3 03/12] drm/i915/guc: Pass intel_guc struct parameter to GuC interrupts functions

[Sagar Arun Kamble]

On 1/4/2018 10:52 PM, Michal Wajdeczko wrote:
> On Thu, 04 Jan 2018 17:21:45 +0100, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> GuC interrupts handling functions are GuC specific functions hence 
>> update
>> the parameter from dev_priv to intel_guc struct.
>>
>> v2-v3: Rebase.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c      |  2 +-
>>  drivers/gpu/drm/i915/intel_guc.c     | 22 +++++++++++++++-------
>>  drivers/gpu/drm/i915/intel_guc.h     |  8 ++++----
>>  drivers/gpu/drm/i915/intel_guc_log.c |  8 +++-----
>>  drivers/gpu/drm/i915/intel_uc.c      |  8 ++++----
>>  5 files changed, 27 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 3f4eff9..a1ae057 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1447,7 +1447,7 @@ static void gen8_gt_irq_handler(struct 
>> drm_i915_private *dev_priv,
>>          gen6_rps_irq_handler(dev_priv, gt_iir[2]);
>>     if (gt_iir[2] & dev_priv->pm_guc_events)
>> -        intel_guc_irq_handler(dev_priv, gt_iir[2]);
>> +        intel_guc_irq_handler(&dev_priv->guc, gt_iir[2]);
>>  }
>> static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index e95ff2d..14bf508d 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -400,7 +400,7 @@ int intel_guc_suspend(struct drm_i915_private 
>> *dev_priv)
>>      if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>          return 0;
>> -    intel_disable_guc_interrupts(dev_priv);
>> +    intel_disable_guc_interrupts(guc);
>
> Hmm, if we disable irq here, then we might have problems with
> sending this message to GuC if we use CTB as comm mechanism...
>
Yes. This gets fixed in the later patches in the series.
>>     data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>>      /* any value greater than GUC_POWER_D0 */
>> @@ -446,7 +446,7 @@ int intel_guc_resume(struct drm_i915_private 
>> *dev_priv)
>>          return 0;
>>     if (i915_modparams.guc_log_level >= 0)
>> -        intel_enable_guc_interrupts(dev_priv);
>> +        intel_enable_guc_interrupts(guc);
>>     data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>>      data[1] = GUC_POWER_D0;
>> @@ -508,15 +508,19 @@ u32 intel_guc_wopcm_size(struct 
>> drm_i915_private *dev_priv)
>>      return wopcm_size;
>>  }
>> -void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv)
>> +void intel_reset_guc_interrupts(struct intel_guc *guc)
>>  {
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>>      spin_lock_irq(&dev_priv->irq_lock);
>>      gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
>>      spin_unlock_irq(&dev_priv->irq_lock);
>>  }
>> -void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv)
>> +void intel_enable_guc_interrupts(struct intel_guc *guc)
>>  {
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>>      spin_lock_irq(&dev_priv->irq_lock);
>>      if (!dev_priv->guc.interrupts_enabled) {
>
> Just spotted:
> as we have "guc" try to use "guc->" instead of "dev_priv->guc."
>
Ok :)
>> WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>> @@ -527,8 +531,10 @@ void intel_enable_guc_interrupts(struct 
>> drm_i915_private *dev_priv)
>>      spin_unlock_irq(&dev_priv->irq_lock);
>>  }
>> -void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv)
>> +void intel_disable_guc_interrupts(struct intel_guc *guc)
>>  {
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>>      spin_lock_irq(&dev_priv->irq_lock);
>>      dev_priv->guc.interrupts_enabled = false;
>
> same here (and possibly in other places too)
>
Sure.
>> @@ -537,11 +543,13 @@ void intel_disable_guc_interrupts(struct 
>> drm_i915_private *dev_priv)
>>      spin_unlock_irq(&dev_priv->irq_lock);
>>      synchronize_irq(dev_priv->drm.irq);
>> -    intel_reset_guc_interrupts(dev_priv);
>> +    intel_reset_guc_interrupts(guc);
>>  }
>> -void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32 
>> gt_iir)
>> +void intel_guc_irq_handler(struct intel_guc *guc, u32 gt_iir)
>>  {
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>>      if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
>>          /* Sample the log buffer flush related bits & clear them out 
>> now
>>           * itself from the message identity register to minimize the
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h 
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index c37d34d..49f33b9 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -131,9 +131,9 @@ static inline u32 guc_ggtt_offset(struct i915_vma 
>> *vma)
>>  int intel_guc_resume(struct drm_i915_private *dev_priv);
>>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 
>> size);
>>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>> -void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv);
>> -void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv);
>> -void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv);
>> -void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32 
>> pm_iir);
>> +void intel_reset_guc_interrupts(struct intel_guc *guc);
>> +void intel_enable_guc_interrupts(struct intel_guc *guc);
>> +void intel_disable_guc_interrupts(struct intel_guc *guc);
>> +void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
>> #endif
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 5cd68f6..59a9021 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -480,14 +480,12 @@ static void guc_log_capture_logs(struct 
>> intel_guc *guc)
>> static void guc_flush_logs(struct intel_guc *guc)
>>  {
>> -    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> -
>>      if (!USES_GUC_SUBMISSION(dev_priv) ||
>>          (i915_modparams.guc_log_level < 0))
>>          return;
>>     /* First disable the interrupts, will be renabled afterwards */
>> -    intel_disable_guc_interrupts(dev_priv);
>> +    intel_disable_guc_interrupts(guc);
>>     /* Before initiating the forceful flush, wait for any 
>> pending/ongoing
>>       * flush to complete otherwise forceful flush may not actually 
>> happen.
>> @@ -605,7 +603,7 @@ int i915_guc_log_control(struct drm_i915_private 
>> *dev_priv, u64 control_val)
>>          }
>>         /* GuC logging is currently the only user of Guc2Host 
>> interrupts */
>> -        intel_enable_guc_interrupts(dev_priv);
>> +        intel_enable_guc_interrupts(guc);
>>      } else {
>>          /* Once logging is disabled, GuC won't generate logs & send an
>>           * interrupt. But there could be some data in the log buffer
>> @@ -639,7 +637,7 @@ void i915_guc_log_unregister(struct 
>> drm_i915_private *dev_priv)
>>     mutex_lock(&dev_priv->drm.struct_mutex);
>>      /* GuC logging is currently the only user of Guc2Host interrupts */
>> -    intel_disable_guc_interrupts(dev_priv);
>> +    intel_disable_guc_interrupts(&dev_priv->guc);
>>      guc_log_runtime_destroy(&dev_priv->guc);
>>      mutex_unlock(&dev_priv->drm.struct_mutex);
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 8e58e5a..f82453b 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -271,7 +271,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>      GEM_BUG_ON(!HAS_GUC(dev_priv));
>>     guc_disable_communication(guc);
>> -    intel_reset_guc_interrupts(dev_priv);
>> +    intel_reset_guc_interrupts(guc);
>>     /* init WOPCM */
>>      I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
>> @@ -325,7 +325,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>     if (USES_GUC_SUBMISSION(dev_priv)) {
>>          if (i915_modparams.guc_log_level >= 0)
>> -            intel_enable_guc_interrupts(dev_priv);
>> +            intel_enable_guc_interrupts(guc);
>>         ret = intel_guc_submission_enable(guc);
>>          if (ret)
>> @@ -345,7 +345,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>       * We've failed to load the firmware :(
>>       */
>>  err_interrupts:
>> -    intel_disable_guc_interrupts(dev_priv);
>> +    intel_disable_guc_interrupts(guc);
>>  err_communication:
>>      guc_disable_communication(guc);
>>  err_log_capture:
>> @@ -379,5 +379,5 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *dev_priv)
>>      guc_disable_communication(guc);
>>     if (USES_GUC_SUBMISSION(dev_priv))
>> -        intel_disable_guc_interrupts(dev_priv);
>> +        intel_disable_guc_interrupts(guc);
>>  }

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

Re: [PATCH v3 10/12] drm/i915/guc: Add client support to enable/disable GuC interrupts

[Sagar Arun Kamble]

On 1/4/2018 11:09 PM, Michal Wajdeczko wrote:
> On Thu, 04 Jan 2018 17:21:52 +0100, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> This patch adds support to enable/disable GuC interrupts for different
>> features without impacting other's need. Currently GuC log capture and
>> CT buffer receive mechanisms use the GuC interrupts. GuC interrupts are
>> currently enabled and disabled in different logging scenarios all gated
>> by log level.
>>
>> v2: Rebase with all GuC interrupt handlers moved to intel_guc.c. 
>> Handling
>> multiple clients for GuC interrupts enable/disable.
>> (Michal Wajdeczko)
>>
>> v3: Removed spin lock and using test_bit in i915_guc_info. Prepared low
>> level helpers to get/put GuC interrupts that can be reused during
>> suspend/resume. (Tvrtko)
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c  |  6 +++++
>>  drivers/gpu/drm/i915/intel_guc.c     | 47 
>> +++++++++++++++++++++++++++---------
>>  drivers/gpu/drm/i915/intel_guc.h     | 11 ++++++---
>>  drivers/gpu/drm/i915/intel_guc_log.c |  6 ++---
>>  drivers/gpu/drm/i915/intel_uc.c      |  4 +--
>>  5 files changed, 54 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 16f9a95..eef4c8b 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2340,6 +2340,12 @@ static int i915_guc_info(struct seq_file *m, 
>> void *data)
>>      GEM_BUG_ON(!guc->execbuf_client);
>>      GEM_BUG_ON(!guc->preempt_client);
>> +    seq_puts(m, "GuC Interrupt Clients: ");
>> +    if (test_bit(GUC_INTR_CLIENT_LOG, &guc->interrupt_clients))
>> +        seq_puts(m, "GuC Logging\n");
>> +    else
>> +        seq_puts(m, "None\n");
>> +
>
> Maybe this can be done in intel_guc_log.c as part of:
>
> void intel_guc_log_dump(const struct intel_guc_log *log, struct 
> drm_printer *p);
>
Idea is to know as part of GuC global information, which all features 
have enabled GuC to Host Interrupts.
I can add it in the i915_guc_log_dump as to whether interrupt is enabled 
but then I think it will be good to have
it here to know interrupt status together at once we have CTB as well.
>
>>      seq_printf(m, "Doorbell map:\n");
>>      seq_printf(m, "\t%*pb\n", GUC_NUM_DOORBELLS, guc->doorbell_bitmap);
>>      seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", 
>> guc->db_cacheline);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 36d1bca..d356c40 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -407,7 +407,7 @@ int intel_guc_suspend(struct drm_i915_private 
>> *dev_priv)
>>          return 0;
>>     if (guc->log.level >= 0)
>> -        intel_disable_guc_interrupts(guc);
>> +        intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>>     data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>>      /* any value greater than GUC_POWER_D0 */
>> @@ -453,7 +453,7 @@ int intel_guc_resume(struct drm_i915_private 
>> *dev_priv)
>>          return 0;
>>     if (guc->log.level >= 0)
>> -        intel_enable_guc_interrupts(guc);
>> +        intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>>     data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>>      data[1] = GUC_POWER_D0;
>> @@ -524,28 +524,51 @@ void intel_reset_guc_interrupts(struct 
>> intel_guc *guc)
>>      spin_unlock_irq(&dev_priv->irq_lock);
>>  }
>> -void intel_enable_guc_interrupts(struct intel_guc *guc)
>> +static void __intel_get_guc_interrupts(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>> +    lockdep_assert_held(&dev_priv->irq_lock);
>> +
>> +    WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>> +                   dev_priv->pm_guc_events);
>> +    gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>> +}
>> +
>> +void intel_get_guc_interrupts(struct intel_guc *guc, enum 
>> guc_intr_client id)
>
> What about intel_guc_interrupts_get(guc, id) ?
>
>>  {
>>      struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>     spin_lock_irq(&dev_priv->irq_lock);
>> -    if (!dev_priv->guc.interrupts_enabled) {
>> -        WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>> -                       dev_priv->pm_guc_events);
>> -        dev_priv->guc.interrupts_enabled = true;
>> -        gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>> -    }
>> +
>> +    if (!guc->interrupt_clients)
>> +        __intel_get_guc_interrupts(guc);
>> +    __set_bit(id, &guc->interrupt_clients);
>
> Do we care about scenarios when we call "get" more than once?
> Maybe we need GEM_WARN_ON at the minimum ?
>
> Then I'm wondering if "get/put" are correct if we don't do any
> refcounting... maybe "enable/disable" are more appropriate then?
>
enabling/disabling more than once should not be a problem. will update 
the names
as suggested as we don't refcount.
>> +
>>      spin_unlock_irq(&dev_priv->irq_lock);
>>  }
>> -void intel_disable_guc_interrupts(struct intel_guc *guc)
>> +static void __intel_put_guc_interrupts(struct intel_guc *guc)
>>  {
>>      struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> -    spin_lock_irq(&dev_priv->irq_lock);
>> -    dev_priv->guc.interrupts_enabled = false;
>> +    lockdep_assert_held(&dev_priv->irq_lock);
>>     gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>> +}
>> +
>> +void intel_put_guc_interrupts(struct intel_guc *guc, enum 
>> guc_intr_client id)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>> +    spin_lock_irq(&dev_priv->irq_lock);
>> +
>> +    __clear_bit(id, &guc->interrupt_clients);
>> +    if (guc->interrupt_clients) {
>> +        spin_unlock_irq(&dev_priv->irq_lock);
>> +        return;
>> +    }
>> +    __intel_put_guc_interrupts(guc);
>>     spin_unlock_irq(&dev_priv->irq_lock);
>>      synchronize_irq(dev_priv->drm.irq);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h 
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index 49f33b9..af74392 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -34,6 +34,11 @@
>>  #include "intel_uc_fw.h"
>>  #include "i915_vma.h"
>> +enum guc_intr_client {
>
> Hmm, what about adding intel_ prefix ?
>
Forgot about this. will update.
>> +    GUC_INTR_CLIENT_LOG = 0,
>> +    GUC_INTR_CLIENT_COUNT
>> +};
>> +
>>  struct guc_preempt_work {
>>      struct work_struct work;
>>      struct intel_engine_cs *engine;
>> @@ -53,7 +58,7 @@ struct intel_guc {
>>      struct drm_i915_gem_object *load_err_log;
>>     /* intel_guc_recv interrupt related state */
>> -    bool interrupts_enabled;
>> +    unsigned long interrupt_clients;
>>     struct i915_vma *ads_vma;
>>      struct i915_vma *stage_desc_pool;
>> @@ -132,8 +137,8 @@ static inline u32 guc_ggtt_offset(struct i915_vma 
>> *vma)
>>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 
>> size);
>>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>>  void intel_reset_guc_interrupts(struct intel_guc *guc);
>> -void intel_enable_guc_interrupts(struct intel_guc *guc);
>> -void intel_disable_guc_interrupts(struct intel_guc *guc);
>> +void intel_get_guc_interrupts(struct intel_guc *guc, enum 
>> guc_intr_client id);
>> +void intel_put_guc_interrupts(struct intel_guc *guc, enum 
>> guc_intr_client id);
>>  void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
>> #endif
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 7bc0065..ddd4f6d 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -488,7 +488,7 @@ static void guc_flush_logs(struct intel_guc *guc)
>>          return;
>>     /* First disable the interrupts, will be renabled afterwards */
>> -    intel_disable_guc_interrupts(guc);
>> +    intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>>     /* Before initiating the forceful flush, wait for any 
>> pending/ongoing
>>       * flush to complete otherwise forceful flush may not actually 
>> happen.
>> @@ -594,7 +594,7 @@ int i915_guc_log_control(struct drm_i915_private 
>> *dev_priv, u64 control_val)
>>          }
>>         /* GuC logging is currently the only user of Guc2Host 
>> interrupts */
>
> I guess above comment is now obsolete ;)
>
>> -        intel_enable_guc_interrupts(guc);
>> +        intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>>      } else {
>>          /* Once logging is disabled, GuC won't generate logs & send an
>>           * interrupt. But there could be some data in the log buffer
>> @@ -626,7 +626,7 @@ void i915_guc_log_unregister(struct 
>> drm_i915_private *dev_priv)
>>      /* GuC logging is currently the only user of Guc2Host interrupts */
>
> Same here.
>
Yes. will remove.

Thanks
Sagar
>>      if (dev_priv->guc.log.level >= 0) {
>>          intel_runtime_pm_get(dev_priv);
>> -        intel_disable_guc_interrupts(&dev_priv->guc);
>> +        intel_put_guc_interrupts(&dev_priv->guc, GUC_INTR_CLIENT_LOG);
>>          intel_runtime_pm_put(dev_priv);
>>      }
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index fb5edcc..93ecbf1 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -319,7 +319,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>          goto err_log_capture;
>>     if (guc->log.level >= 0)
>> -        intel_enable_guc_interrupts(guc);
>> +        intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>>     ret = guc_enable_communication(guc);
>>      if (ret)
>> @@ -353,7 +353,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>      guc_disable_communication(guc);
>>  err_interrupts:
>>      if (guc->log.level >= 0)
>> -        intel_disable_guc_interrupts(guc);
>> +        intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>>  err_log_capture:
>>      guc_capture_load_err_log(guc);
>>  err_out:

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

Re: [PATCH v3 11/12] drm/i915/guc: Restore GuC interrupts across suspend/reset if enabled

[Sagar Arun Kamble]

On 1/4/2018 11:19 PM, Michal Wajdeczko wrote:
> On Thu, 04 Jan 2018 17:21:53 +0100, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> In order to override the disable/enable control of GuC interrupts during
>> suspend/reset cycle we are creating two new functions suspend/restore
>> guc_interrupts which check if interrupts were enabled and disable them
>> on suspend and enable them on resume. They are used to restore 
>> interrupts
>> across reset as well.
>>
>> Further restructuring of runtime_pm_enable/disable_interrupts and
>> suspend/restore_guc_interrupts will be done in upcoming patches.
>>
>> v2: Rebase.
>>
>> v3: Updated suspend/restore with the new low level get/put functions.
>> (Tvrtko)
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |  2 ++
>>  drivers/gpu/drm/i915/intel_guc.c     | 32 
>> ++++++++++++++++++++++++++++----
>>  drivers/gpu/drm/i915/intel_guc.h     |  2 ++
>>  3 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c 
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 0cd3559..2e0db53 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3676,8 +3676,10 @@ void intel_finish_reset(struct 
>> drm_i915_private *dev_priv)
>>           * The display has been reset as well,
>>           * so need a full re-initialization.
>>           */
>> +        intel_suspend_guc_interrupts(&dev_priv->guc);
>>          intel_runtime_pm_disable_interrupts(dev_priv);
>>          intel_runtime_pm_enable_interrupts(dev_priv);
>> +        intel_restore_guc_interrupts(&dev_priv->guc);
>>         intel_pps_unlock_regs_wa(dev_priv);
>>          intel_modeset_init_hw(dev);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index d356c40..28a418a 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -406,8 +406,7 @@ int intel_guc_suspend(struct drm_i915_private 
>> *dev_priv)
>>      if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>          return 0;
>> -    if (guc->log.level >= 0)
>> -        intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>> +    intel_suspend_guc_interrupts(guc);
>
> Hmm, maybe we should introduce
>
>     intel_uc_suspend(struct drm_i915_private *dev_priv)
>
> which will call separately
>
>     intel_guc_suspend(guc); /* send suspend action */
>     intel_guc_suspend_interrupts(guc);
>
Yes. Ordering was not correct. I have this change as part of 
suspend/resume restructuring from GuC/GEM which will follow
after this. Will keep as it is for now in this patch.
>>     data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>>      /* any value greater than GUC_POWER_D0 */
>> @@ -452,8 +451,7 @@ int intel_guc_resume(struct drm_i915_private 
>> *dev_priv)
>>      if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>          return 0;
>> -    if (guc->log.level >= 0)
>> -        intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>> +    intel_restore_guc_interrupts(guc);
>>     data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>>      data[1] = GUC_POWER_D0;
>> @@ -548,6 +546,16 @@ void intel_get_guc_interrupts(struct intel_guc 
>> *guc, enum guc_intr_client id)
>>      spin_unlock_irq(&dev_priv->irq_lock);
>>  }
>> +void intel_restore_guc_interrupts(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>> +    spin_lock_irq(&dev_priv->irq_lock);
>> +    if (guc->interrupt_clients)
>> +        __intel_get_guc_interrupts(guc);
>> +    spin_unlock_irq(&dev_priv->irq_lock);
>> +}
>> +
>>  static void __intel_put_guc_interrupts(struct intel_guc *guc)
>>  {
>>      struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> @@ -576,6 +584,22 @@ void intel_put_guc_interrupts(struct intel_guc 
>> *guc, enum guc_intr_client id)
>>      intel_reset_guc_interrupts(guc);
>>  }
>> +void intel_suspend_guc_interrupts(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>> +    spin_lock_irq(&dev_priv->irq_lock);
>> +    if (!guc->interrupt_clients) {
>> +        spin_unlock_irq(&dev_priv->irq_lock);
>> +        return;
>> +    }
>> +    __intel_put_guc_interrupts(guc);
>> +    spin_unlock_irq(&dev_priv->irq_lock);
>> +    synchronize_irq(dev_priv->drm.irq);
>> +
>> +    intel_reset_guc_interrupts(guc);
>> +}
>> +
>>  void intel_guc_irq_handler(struct intel_guc *guc, u32 gt_iir)
>>  {
>>      struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h 
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index af74392..2c14781 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -140,5 +140,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma 
>> *vma)
>>  void intel_get_guc_interrupts(struct intel_guc *guc, enum 
>> guc_intr_client id);
>>  void intel_put_guc_interrupts(struct intel_guc *guc, enum 
>> guc_intr_client id);
>>  void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
>> +void intel_suspend_guc_interrupts(struct intel_guc *guc);
>> +void intel_restore_guc_interrupts(struct intel_guc *guc);
>
> To match obj-verb pattern:
>
> intel_guc_suspend_interrupts
> intel_guc_restore_interrupts
>
Yes. Will update.

Thanks
Sagar
>> #endif

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

Re: [PATCH v3 04/12] drm/i915/guc: Add description and comments about guc_log_level parameter

[Sagar Arun Kamble]

On 1/5/2018 10:24 AM, Sagar Arun Kamble wrote:
>
>
> On 1/4/2018 10:22 PM, Michal Wajdeczko wrote:
>> On Thu, 04 Jan 2018 17:21:46 +0100, Sagar Arun Kamble 
>> <sagar.a.kamble@intel.com> wrote:
>>
>>> guc_log_level parameter takes effect when GuC is loaded which is
>>> controlled through enable_guc parameter. Add this relation info.
>> ^^^
>> Extra "."
>>
>>> in parameter description and documentation.
>>> Earlier, this patch was added to sanitize guc_log_level like old
>>> GuC parameters enable_guc_loading/submission. With new parameter
>>> enable_guc, sanitization of guc_log_level is no more needed.
>>
>> Hmm, I think we still need to sanitize log_level if it was wrongly
>> enabled without enabling GuC first (in intel_uc_sanitize_options).
> I think it would not be harmful as all decisions based on it will be 
> gated by USES_GUC.
I was wrong. i915_guc_log_register/unregister were changed by my series 
to only rely on guc_log_level.
I have added HAS_GUC check in those function in v4 patch. Is that option 
okay or we should sanitize this parameter?
>>
>>>
>>> v2: Added documentation to intel_guc_log.c and param description
>>> about GuC loading dependency. (Michal Wajdeczko)
>>>
>>> v3: Removed sanitization of module parameter guc_log_level.
>>> Previous review comments not applicable now.
>>>
>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v2
>>> ---
>>>  drivers/gpu/drm/i915/i915_params.c   | 3 ++-
>>>  drivers/gpu/drm/i915/intel_guc_log.c | 1 +
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>>> b/drivers/gpu/drm/i915/i915_params.c
>>> index b5f3eb4..a93a6ca 100644
>>> --- a/drivers/gpu/drm/i915/i915_params.c
>>> +++ b/drivers/gpu/drm/i915/i915_params.c
>>> @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = {
>>>      "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
>>> i915_param_named(guc_log_level, int, 0400,
>>> -    "GuC firmware logging level (-1:disabled (default), 
>>> 0-3:enabled)");
>>> +    "GuC firmware logging level. This takes effect only if GuC is 
>>> to be "
>>> +    "loaded (depends on enable_guc) (-1:disabled (default), 
>>> 0-3:enabled)");
>>
>> Btw, I was planing to change above values to follow schema used in 
>> other modparams:
>>
>> -1: auto (then it can be controlled by USES_GUC and DRM_I915_DEBUG_GUC)
>>  0: disabled
>>  1: enabled (legacy level 0)
>>  2: enabled (legacy level 1)
>>  3: enabled (legacy level 2)
>>  4: enabled (legacy level 3)
>>
>> So now I'm not sure that I want your patch ;)
>>
> Makes sense. Will drop this patch.
>
> Thanks
> Sagar
>>> i915_param_named_unsafe(guc_firmware_path, charp, 0400,
>>>      "GuC firmware path to use instead of the default one");
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>>> b/drivers/gpu/drm/i915/intel_guc_log.c
>>> index 59a9021..d0131bc 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>>> @@ -34,6 +34,7 @@
>>>   * DOC: GuC firmware log
>>>   *
>>>   * Firmware log is enabled by setting i915.guc_log_level to 
>>> non-negative level.
>>> + * This takes effect only if GuC is to be loaded based on enable_guc.
>>
>> ... based on i915.enable_guc modparam.
>>
>>>   * Log data is printed out via reading debugfs i915_guc_log_dump. 
>>> Reading from
>>>   * i915_guc_load_status will print out firmware loading status and 
>>> scratch
>>>   * registers value.
>

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