[PATCH 00/11] GuC Interrupts/Log updates

[PATCH 00/11] GuC Interrupts/Log updates

[Sagar Arun Kamble]

This series addresses following features/fixes:
1. Restructuring to control GuC interrupts from guc.c functions
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
   submission_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 (11):
  drm/i915: Export low level PM IRQ functions to control 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: Sanitize module parameter guc_log_level
  drm/i915/guc: Make GuC log related functions depend only on log level
  drm/i915/guc: Only release GuC log object during submission_fini
  drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts
  drm/i915/guc: Add client support to enable/disable GuC interrupts
  drm/i915/guc: Fix GuC interrupts disabling with Logging
  drm/i915/guc: Skip interrupt enabling if logging is already enabled
  drm/i915/guc: Restore GuC interrupts across suspend/reset if enabled

 drivers/gpu/drm/i915/i915_debugfs.c        |   8 ++
 drivers/gpu/drm/i915/i915_guc_submission.c |   2 +-
 drivers/gpu/drm/i915/i915_irq.c            |  78 ++-----------------
 drivers/gpu/drm/i915/i915_params.c         |   4 +-
 drivers/gpu/drm/i915/intel_display.c       |   2 +
 drivers/gpu/drm/i915/intel_drv.h           |   7 +-
 drivers/gpu/drm/i915/intel_guc.c           | 121 ++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_guc.h           |  13 +++-
 drivers/gpu/drm/i915/intel_guc_log.c       |  33 +++++---
 drivers/gpu/drm/i915/intel_uc.c            |  32 ++++----
 10 files changed, 192 insertions(+), 108 deletions(-)

-- 
1.9.1

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

[PATCH 01/11] drm/i915: Export low level PM IRQ functions to control 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.

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>
---
 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 b1296a5..caa6283 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 d61985f..792d8ea 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1237,8 +1237,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 02/11] 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, i915 irq handling keeping them grouped in
intel_guc.c instead of intel_uc.c.

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>
---
 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 caa6283..84a4bf3 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
@@ -1474,7 +1441,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)
@@ -1751,41 +1718,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 792d8ea..39c76ba 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1269,9 +1269,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 10037c0..3a64ae1 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -274,7 +274,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);
 
 	ctx = dev_priv->kernel_context;
 
@@ -302,7 +302,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);
 
 	ctx = dev_priv->kernel_context;
 
@@ -367,3 +367,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 418450b..8b26505 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -116,5 +116,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 76d3eb1..8120208 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -510,7 +510,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.
@@ -628,7 +628,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
@@ -662,7 +662,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 25bd162..b9b9947a0 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -158,7 +158,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		return 0;
 
 	guc_disable_communication(guc);
-	gen9_reset_guc_interrupts(dev_priv);
+	intel_reset_guc_interrupts(dev_priv);
 
 	/* We need to notify the guc whenever we change the GGTT */
 	i915_ggtt_enable_guc(dev_priv);
@@ -215,7 +215,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	intel_huc_auth(&dev_priv->huc);
 	if (i915_modparams.enable_guc_submission) {
 		if (i915_modparams.guc_log_level >= 0)
-			gen9_enable_guc_interrupts(dev_priv);
+			intel_enable_guc_interrupts(dev_priv);
 
 		ret = i915_guc_submission_enable(dev_priv);
 		if (ret)
@@ -241,7 +241,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	 */
 err_interrupts:
 	guc_disable_communication(guc);
-	gen9_disable_guc_interrupts(dev_priv);
+	intel_disable_guc_interrupts(dev_priv);
 err_log_capture:
 	guc_capture_load_err_log(guc);
 err_submission:
@@ -282,7 +282,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	guc_disable_communication(&dev_priv->guc);
 
 	if (i915_modparams.enable_guc_submission) {
-		gen9_disable_guc_interrupts(dev_priv);
+		intel_disable_guc_interrupts(dev_priv);
 		i915_guc_submission_fini(dev_priv);
 	}
 
-- 
1.9.1

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

[PATCH 03/11] 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.

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_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 84a4bf3..1a5c026 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1441,7 +1441,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 3a64ae1..31f25e5 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -274,7 +274,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);
 
 	ctx = dev_priv->kernel_context;
 
@@ -302,7 +302,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);
 
 	ctx = dev_priv->kernel_context;
 
@@ -368,15 +368,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)) &
@@ -387,8 +391,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;
 
@@ -397,11 +403,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 8b26505..e89b4ae 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -116,9 +116,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 8120208..f53c663 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -503,14 +503,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 (!i915_modparams.enable_guc_submission ||
 	    (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.
@@ -628,7 +626,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
@@ -662,7 +660,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 b9b9947a0..62738ad 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -158,7 +158,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		return 0;
 
 	guc_disable_communication(guc);
-	intel_reset_guc_interrupts(dev_priv);
+	intel_reset_guc_interrupts(guc);
 
 	/* We need to notify the guc whenever we change the GGTT */
 	i915_ggtt_enable_guc(dev_priv);
@@ -215,7 +215,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	intel_huc_auth(&dev_priv->huc);
 	if (i915_modparams.enable_guc_submission) {
 		if (i915_modparams.guc_log_level >= 0)
-			intel_enable_guc_interrupts(dev_priv);
+			intel_enable_guc_interrupts(guc);
 
 		ret = i915_guc_submission_enable(dev_priv);
 		if (ret)
@@ -241,7 +241,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	 */
 err_interrupts:
 	guc_disable_communication(guc);
-	intel_disable_guc_interrupts(dev_priv);
+	intel_disable_guc_interrupts(guc);
 err_log_capture:
 	guc_capture_load_err_log(guc);
 err_submission:
@@ -282,7 +282,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	guc_disable_communication(&dev_priv->guc);
 
 	if (i915_modparams.enable_guc_submission) {
-		intel_disable_guc_interrupts(dev_priv);
+		intel_disable_guc_interrupts(&dev_priv->guc);
 		i915_guc_submission_fini(dev_priv);
 	}
 
-- 
1.9.1

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

[PATCH 04/11] drm/i915/guc: Sanitize module parameter guc_log_level

[Sagar Arun Kamble]

Parameter guc_log_level needs to be sanitized based on GuC support and
enable_guc_loading parameter since it depends on them like
enable_guc_submission. This will make GuC logging paths independent of
enable_guc_submission parameter in further patches.

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

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_params.c   |  4 +++-
 drivers/gpu/drm/i915/intel_guc_log.c |  1 +
 drivers/gpu/drm/i915/intel_uc.c      | 10 +++++++---
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b4faeb6..774c56e 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly = {
 	"(-1=auto, 0=never [default], 1=if available, 2=required)");
 
 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_loading) (-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 f53c663..200f0a1 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_loading.
  * 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.
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 62738ad..8feefcd 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_GUC(dev_priv)) {
 		if (i915_modparams.enable_guc_loading > 0 ||
-		    i915_modparams.enable_guc_submission > 0)
+		    i915_modparams.enable_guc_submission > 0 ||
+		    i915_modparams.guc_log_level > 0)
 			DRM_INFO("Ignoring GuC options, no hardware\n");
 
 		i915_modparams.enable_guc_loading = 0;
 		i915_modparams.enable_guc_submission = 0;
+		i915_modparams.guc_log_level = -1;
 		return;
 	}
 
@@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 			i915_modparams.enable_guc_loading = 0;
 	}
 
-	/* Can't enable guc submission without guc loaded */
-	if (!i915_modparams.enable_guc_loading)
+	/* Can't enable guc submission and logging without guc loaded */
+	if (!i915_modparams.enable_guc_loading) {
 		i915_modparams.enable_guc_submission = 0;
+		i915_modparams.guc_log_level = -1;
+	}
 
 	/* A negative value means "use platform default" */
 	if (i915_modparams.enable_guc_submission < 0)
-- 
1.9.1

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

[PATCH 05/11] drm/i915/guc: Make GuC log related functions depend only on log level

[Sagar Arun Kamble]

With guc_log_level parameter sanitized we can remove the GuC submission
checks from flush_guc_logs and i915_guc_log_register/unregister and
intel_uc_fini_hw. It is important to note that GuC log runtime/relay
channel 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 and driver
will try to destroy during cleanup/fini_hw where relay will be NULL
already.

With this patch GuC interrupts are enabled first after GuC load if logging
is enabled. Earlier they were enabled only when submission was getting
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. 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: Updated guc_log_unregister again. Made all GuC log related functions
depend only guc_log_level. Updated uC init w.r.t enabling of GuC
interrupts.

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 | 12 ++++--------
 drivers/gpu/drm/i915/intel_uc.c      | 21 ++++++++++++---------
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 31f25e5..959057a 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -274,7 +274,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 (i915_modparams.guc_log_level >= 0)
+		intel_disable_guc_interrupts(guc);
 
 	ctx = dev_priv->kernel_context;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 200f0a1..f87e9f5 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -504,8 +504,7 @@ static void guc_log_capture_logs(struct intel_guc *guc)
 
 static void guc_flush_logs(struct intel_guc *guc)
 {
-	if (!i915_modparams.enable_guc_submission ||
-	    (i915_modparams.guc_log_level < 0))
+	if (i915_modparams.guc_log_level < 0)
 		return;
 
 	/* First disable the interrupts, will be renabled afterwards */
@@ -645,8 +644,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 (!i915_modparams.enable_guc_submission ||
-	    (i915_modparams.guc_log_level < 0))
+	if (i915_modparams.guc_log_level < 0)
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
@@ -656,12 +654,10 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
 
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 {
-	if (!i915_modparams.enable_guc_submission)
-		return;
-
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	/* GuC logging is currently the only user of Guc2Host interrupts */
-	intel_disable_guc_interrupts(&dev_priv->guc);
+	if (i915_modparams.guc_log_level >= 0)
+		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 8feefcd..95c5ec4 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -212,18 +212,18 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_log_capture;
 
+	if (i915_modparams.guc_log_level >= 0)
+		intel_enable_guc_interrupts(guc);
+
 	ret = guc_enable_communication(guc);
 	if (ret)
-		goto err_log_capture;
+		goto err_interrupts;
 
 	intel_huc_auth(&dev_priv->huc);
 	if (i915_modparams.enable_guc_submission) {
-		if (i915_modparams.guc_log_level >= 0)
-			intel_enable_guc_interrupts(guc);
-
 		ret = i915_guc_submission_enable(dev_priv);
 		if (ret)
-			goto err_interrupts;
+			goto err_comm;
 	}
 
 	dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version %u.%u])\n",
@@ -243,9 +243,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	 * nonfatal error (i.e. it doesn't prevent driver load, but
 	 * marks the GPU as wedged until reset).
 	 */
-err_interrupts:
+err_comm:
 	guc_disable_communication(guc);
-	intel_disable_guc_interrupts(guc);
+err_interrupts:
+	if (i915_modparams.guc_log_level >= 0)
+		intel_disable_guc_interrupts(guc);
 err_log_capture:
 	guc_capture_load_err_log(guc);
 err_submission:
@@ -285,10 +287,11 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 
 	guc_disable_communication(&dev_priv->guc);
 
-	if (i915_modparams.enable_guc_submission) {
+	if (i915_modparams.guc_log_level >= 0)
 		intel_disable_guc_interrupts(&dev_priv->guc);
+
+	if (i915_modparams.enable_guc_submission)
 		i915_guc_submission_fini(dev_priv);
-	}
 
 	i915_ggtt_disable_guc(dev_priv);
 }
-- 
1.9.1

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

[PATCH 06/11] drm/i915/guc: Only release GuC log object during submission_fini

[Sagar Arun Kamble]

GuC log runtime/relay channel data is released during i915 unregister,
So only GuC log vma needs to be released during submission_fini.

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_guc_submission.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index a2e8114..c360b37 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1021,7 +1021,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 
 	ida_destroy(&guc->stage_ids);
 	guc_ads_destroy(guc);
-	intel_guc_log_destroy(guc);
+	i915_vma_unpin_and_release(&guc->log.vma);
 	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
 	i915_vma_unpin_and_release(&guc->stage_desc_pool);
 }
-- 
1.9.1

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

[PATCH 07/11] 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

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 | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index f87e9f5..ed239cb 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -656,8 +656,17 @@ 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 */
-	if (i915_modparams.guc_log_level >= 0)
+	if (i915_modparams.guc_log_level >= 0) {
+		intel_runtime_pm_get(dev_priv);
 		intel_disable_guc_interrupts(&dev_priv->guc);
+		intel_runtime_pm_put(dev_priv);
+	}
+	/*
+	 * TODO: Need to synchronize access to relay channel from flush work
+	 * and release here if interrupt stays enabled from hereon.
+	 * Possibly with GuC CT recv. interrupts will stay enabled until GEM
+	 * suspend/unload.
+	 */
 	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 08/11] 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 others needs. 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)

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  |  8 ++++++++
 drivers/gpu/drm/i915/intel_guc.c     | 21 ++++++++++++++-------
 drivers/gpu/drm/i915/intel_guc.h     | 11 ++++++++---
 drivers/gpu/drm/i915/intel_guc_log.c |  6 +++---
 drivers/gpu/drm/i915/intel_uc.c      |  6 +++---
 5 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0bb6e01..bd421da 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2531,6 +2531,14 @@ static int i915_guc_info(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 	const struct intel_guc *guc = &dev_priv->guc;
 
+	seq_puts(m, "GuC Interrupt Clients: ");
+	spin_lock_irq(&dev_priv->irq_lock);
+	if (guc->interrupt_clients & BIT(GUC_INTR_CLIENT_LOG))
+		seq_puts(m, "GuC Logging\n");
+	else
+		seq_puts(m, "None\n");
+	spin_unlock_irq(&dev_priv->irq_lock);
+
 	if (!check_guc_submission(m))
 		return 0;
 
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 959057a..fbd27ea 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -275,7 +275,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 		return 0;
 
 	if (i915_modparams.guc_log_level >= 0)
-		intel_disable_guc_interrupts(guc);
+		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
 
 	ctx = dev_priv->kernel_context;
 
@@ -303,7 +303,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 		return 0;
 
 	if (i915_modparams.guc_log_level >= 0)
-		intel_enable_guc_interrupts(guc);
+		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
 
 	ctx = dev_priv->kernel_context;
 
@@ -378,26 +378,33 @@ 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)
+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) {
+	if (!guc->interrupt_clients) {
 		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);
 	}
+	set_bit(id, &guc->interrupt_clients);
+
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
-void intel_disable_guc_interrupts(struct intel_guc *guc)
+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);
-	dev_priv->guc.interrupts_enabled = false;
+
+	clear_bit(id, &guc->interrupt_clients);
+
+	if (guc->interrupt_clients) {
+		spin_unlock_irq(&dev_priv->irq_lock);
+		return;
+	}
 
 	gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
 
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index e89b4ae..4d58bf7 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -34,6 +34,11 @@
 #include "i915_guc_reg.h"
 #include "i915_vma.h"
 
+enum guc_intr_client {
+	GUC_INTR_CLIENT_LOG = 0,
+	GUC_INTR_CLIENT_COUNT
+};
+
 /*
  * Top level structure of GuC. It handles firmware loading and manages client
  * pool and doorbells. intel_guc owns a i915_guc_client to replace the legacy
@@ -48,7 +53,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;
@@ -117,8 +122,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 ed239cb..8c41d9a 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -508,7 +508,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.
@@ -626,7 +626,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
@@ -658,7 +658,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 	/* GuC logging is currently the only user of Guc2Host interrupts */
 	if (i915_modparams.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 95c5ec4..d96c847 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -213,7 +213,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		goto err_log_capture;
 
 	if (i915_modparams.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)
@@ -247,7 +247,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	guc_disable_communication(guc);
 err_interrupts:
 	if (i915_modparams.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_submission:
@@ -288,7 +288,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	guc_disable_communication(&dev_priv->guc);
 
 	if (i915_modparams.guc_log_level >= 0)
-		intel_disable_guc_interrupts(&dev_priv->guc);
+		intel_put_guc_interrupts(&dev_priv->guc, GUC_INTR_CLIENT_LOG);
 
 	if (i915_modparams.enable_guc_submission)
 		i915_guc_submission_fini(dev_priv);
-- 
1.9.1

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

[PATCH 09/11] 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 at a point
where CT mechanism ceases.

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 d96c847..9715eda 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -287,9 +287,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 
 	guc_disable_communication(&dev_priv->guc);
 
-	if (i915_modparams.guc_log_level >= 0)
-		intel_put_guc_interrupts(&dev_priv->guc, GUC_INTR_CLIENT_LOG);
-
 	if (i915_modparams.enable_guc_submission)
 		i915_guc_submission_fini(dev_priv);
 
-- 
1.9.1

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

[PATCH 10/11] drm/i915/guc: Skip interrupt enabling if logging is already enabled

[Sagar Arun Kamble]

To balance the GuC interrupt references we don't allow enabling interrupts
If logging was enabled earlier with different verbosity.
We allow request to change log parameters to be sent to GuC though as
user may want to just update the verbosity level at runtime.

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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 8c41d9a..90b8caf 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -613,6 +613,11 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 	}
 
 	if (log_param.logging_enabled) {
+		if (i915_modparams.guc_log_level >= 0) {
+			i915_modparams.guc_log_level = log_param.verbosity;
+			return 0;
+		}
+
 		i915_modparams.guc_log_level = log_param.verbosity;
 
 		/* If log_level was set as -1 at boot time, then the relay channel file
-- 
1.9.1

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

[PATCH 11/11] 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.

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     | 40 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_guc.h     |  2 ++
 3 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 897fe7e..742ab5e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3768,8 +3768,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 fbd27ea..1e5abf2 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -261,6 +261,25 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset)
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
+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;
+	}
+
+	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(guc);
+}
+
 /**
  * intel_guc_suspend() - notify GuC entering suspend state
  * @dev_priv:	i915 device private
@@ -274,8 +293,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	if (i915_modparams.guc_log_level >= 0)
-		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
+	intel_suspend_guc_interrupts(guc);
 
 	ctx = dev_priv->kernel_context;
 
@@ -289,6 +307,21 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
 
+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) {
+		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);
+	}
+
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
 /**
  * intel_guc_resume() - notify GuC resuming from suspend state
  * @dev_priv:	i915 device private
@@ -302,8 +335,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)
-		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
+	intel_restore_guc_interrupts(guc);
 
 	ctx = dev_priv->kernel_context;
 
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 4d58bf7..c55dcba 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -125,5 +125,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

✓ Fi.CI.BAT: success for GuC Interrupts/Log updates

[Patchwork]

== Series Details ==

Series: GuC Interrupts/Log updates
URL   : https://patchwork.freedesktop.org/series/32179/
State : success

== Summary ==

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

Test chamelium:
        Subgroup dp-edid-read:
                pass       -> FAIL       (fi-kbl-7500u) fdo#102672
Test gem_exec_reloc:
        Subgroup basic-write-gtt-active:
                pass       -> FAIL       (fi-gdg-551) fdo#102582
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-gdg-551) fdo#102618

fdo#102672 https://bugs.freedesktop.org/show_bug.cgi?id=102672
fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582
fdo#102618 https://bugs.freedesktop.org/show_bug.cgi?id=102618

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:438s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:453s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:372s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:531s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:263s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:499s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:496s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:490s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:482s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:560s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:417s
fi-gdg-551       total:289  pass:176  dwarn:1   dfail:0   fail:3   skip:109 time:259s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:579s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:423s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:431s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:490s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:454s
fi-kbl-7500u     total:289  pass:263  dwarn:1   dfail:0   fail:1   skip:24  time:483s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:570s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:473s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:585s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:541s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:449s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:646s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:516s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:496s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:457s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:561s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:417s

abb54f404cc5f5564dffcb1b5b300554bac9c32b drm-tip: 2017y-10m-18d-04h-09m-17s UTC integration manifest
a4281c6f2b4b drm/i915/guc: Restore GuC interrupts across suspend/reset if enabled
56efe9f91fe0 drm/i915/guc: Skip interrupt enabling if logging is already enabled
c3d90f11834a drm/i915/guc: Fix GuC interrupts disabling with Logging
1bd5755bbd5f drm/i915/guc: Add client support to enable/disable GuC interrupts
5ede352e9874 drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts
953349e30b15 drm/i915/guc: Only release GuC log object during submission_fini
e1860d1b90d5 drm/i915/guc: Make GuC log related functions depend only on log level
6bff6621ba22 drm/i915/guc: Sanitize module parameter guc_log_level
e0d358eec8e0 drm/i915/guc: Pass intel_guc struct parameter to GuC interrupts functions
988a47b21122 drm/i915/guc: Move GuC interrupts related functions from i915_irq.c to intel_guc.c
7a12b13c43dc drm/i915: Export low level PM IRQ functions to control from GuC functions

== Logs ==

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

Re: [PATCH 01/11] drm/i915: Export low level PM IRQ functions to control from GuC functions

[Tvrtko Ursulin]

On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> 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.
> 
> 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>
> ---
>   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 b1296a5..caa6283 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;
>   }

Can we pencil in for some future work to move this into the device 
private/info/... somewhere? It would be smaller on the aggregate, and 
more importantly more elegant that way I think.

> @@ -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);

I briefly considered, if now that these functions are exported, we would 
benefit from expressing the lock requirement in the functions name 
somehow, either with a double underscore prefix, or locked suffix. But 
since they have the asserts already I think it is fine.

>   
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d61985f..792d8ea 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1237,8 +1237,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);
> 

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

Regards,

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

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

[Tvrtko Ursulin]

On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> 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, i915 irq handling keeping them grouped in
> intel_guc.c instead of intel_uc.c.
> 
> 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>
> ---
>   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 caa6283..84a4bf3 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
> @@ -1474,7 +1441,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]);

Slight downside that it cannot be inlined any longer, should the 
compiler decide to do so. But it is probably not a relevant 
consideration for the frequency of these.

>   }
>   
>   static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> @@ -1751,41 +1718,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 792d8ea..39c76ba 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1269,9 +1269,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 10037c0..3a64ae1 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -274,7 +274,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);
>   
>   	ctx = dev_priv->kernel_context;
>   
> @@ -302,7 +302,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);
>   
>   	ctx = dev_priv->kernel_context;
>   
> @@ -367,3 +367,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 418450b..8b26505 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -116,5 +116,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 76d3eb1..8120208 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -510,7 +510,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.
> @@ -628,7 +628,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
> @@ -662,7 +662,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 25bd162..b9b9947a0 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -158,7 +158,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   		return 0;
>   
>   	guc_disable_communication(guc);
> -	gen9_reset_guc_interrupts(dev_priv);
> +	intel_reset_guc_interrupts(dev_priv);
>   
>   	/* We need to notify the guc whenever we change the GGTT */
>   	i915_ggtt_enable_guc(dev_priv);
> @@ -215,7 +215,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	intel_huc_auth(&dev_priv->huc);
>   	if (i915_modparams.enable_guc_submission) {
>   		if (i915_modparams.guc_log_level >= 0)
> -			gen9_enable_guc_interrupts(dev_priv);
> +			intel_enable_guc_interrupts(dev_priv);
>   
>   		ret = i915_guc_submission_enable(dev_priv);
>   		if (ret)
> @@ -241,7 +241,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	 */
>   err_interrupts:
>   	guc_disable_communication(guc);
> -	gen9_disable_guc_interrupts(dev_priv);
> +	intel_disable_guc_interrupts(dev_priv);
>   err_log_capture:
>   	guc_capture_load_err_log(guc);
>   err_submission:
> @@ -282,7 +282,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>   	guc_disable_communication(&dev_priv->guc);
>   
>   	if (i915_modparams.enable_guc_submission) {
> -		gen9_disable_guc_interrupts(dev_priv);
> +		intel_disable_guc_interrupts(dev_priv);
>   		i915_guc_submission_fini(dev_priv);
>   	}
>   
> 

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

Regards,

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

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

[Tvrtko Ursulin]

On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> GuC interrupts handling functions are GuC specific functions hence update
> the parameter from dev_priv to intel_guc struct.
> 
> 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_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 84a4bf3..1a5c026 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1441,7 +1441,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 3a64ae1..31f25e5 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -274,7 +274,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);
>   
>   	ctx = dev_priv->kernel_context;
>   
> @@ -302,7 +302,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);
>   
>   	ctx = dev_priv->kernel_context;
>   
> @@ -368,15 +368,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)) &
> @@ -387,8 +391,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;
>   
> @@ -397,11 +403,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 8b26505..e89b4ae 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -116,9 +116,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 8120208..f53c663 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -503,14 +503,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 (!i915_modparams.enable_guc_submission ||
>   	    (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.
> @@ -628,7 +626,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
> @@ -662,7 +660,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 b9b9947a0..62738ad 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -158,7 +158,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   		return 0;
>   
>   	guc_disable_communication(guc);
> -	intel_reset_guc_interrupts(dev_priv);
> +	intel_reset_guc_interrupts(guc);
>   
>   	/* We need to notify the guc whenever we change the GGTT */
>   	i915_ggtt_enable_guc(dev_priv);
> @@ -215,7 +215,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	intel_huc_auth(&dev_priv->huc);
>   	if (i915_modparams.enable_guc_submission) {
>   		if (i915_modparams.guc_log_level >= 0)
> -			intel_enable_guc_interrupts(dev_priv);
> +			intel_enable_guc_interrupts(guc);
>   
>   		ret = i915_guc_submission_enable(dev_priv);
>   		if (ret)
> @@ -241,7 +241,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	 */
>   err_interrupts:
>   	guc_disable_communication(guc);
> -	intel_disable_guc_interrupts(dev_priv);
> +	intel_disable_guc_interrupts(guc);
>   err_log_capture:
>   	guc_capture_load_err_log(guc);
>   err_submission:
> @@ -282,7 +282,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>   	guc_disable_communication(&dev_priv->guc);
>   
>   	if (i915_modparams.enable_guc_submission) {
> -		intel_disable_guc_interrupts(dev_priv);
> +		intel_disable_guc_interrupts(&dev_priv->guc);
>   		i915_guc_submission_fini(dev_priv);
>   	}
>   
> 

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

Regards,

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

Re: [PATCH 04/11] drm/i915/guc: Sanitize module parameter guc_log_level

[Tvrtko Ursulin]

On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> Parameter guc_log_level needs to be sanitized based on GuC support and
> enable_guc_loading parameter since it depends on them like
> enable_guc_submission. This will make GuC logging paths independent of
> enable_guc_submission parameter in further patches.
> 
> v2: Added documentation to intel_guc_log.c and param description about
> GuC loading dependency. (Michal Wajdeczko)
> 
> 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_params.c   |  4 +++-
>   drivers/gpu/drm/i915/intel_guc_log.c |  1 +
>   drivers/gpu/drm/i915/intel_uc.c      | 10 +++++++---
>   3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index b4faeb6..774c56e 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly = {
>   	"(-1=auto, 0=never [default], 1=if available, 2=required)");
>   
>   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_loading) (-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 f53c663..200f0a1 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_loading.
>    * 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.
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 62738ad..8feefcd 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>   {
>   	if (!HAS_GUC(dev_priv)) {
>   		if (i915_modparams.enable_guc_loading > 0 ||
> -		    i915_modparams.enable_guc_submission > 0)
> +		    i915_modparams.enable_guc_submission > 0 ||
> +		    i915_modparams.guc_log_level > 0)
>   			DRM_INFO("Ignoring GuC options, no hardware\n");

Hm, this won't fire on <=gen8 once enable_guc_submission starts to 
default to one? Or we can worry about that when we get there...

>   
>   		i915_modparams.enable_guc_loading = 0;
>   		i915_modparams.enable_guc_submission = 0;
> +		i915_modparams.guc_log_level = -1;
>   		return;
>   	}
>   
> @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>   			i915_modparams.enable_guc_loading = 0;
>   	}
>   
> -	/* Can't enable guc submission without guc loaded */
> -	if (!i915_modparams.enable_guc_loading)
> +	/* Can't enable guc submission and logging without guc loaded */
> +	if (!i915_modparams.enable_guc_loading) {
>   		i915_modparams.enable_guc_submission = 0;
> +		i915_modparams.guc_log_level = -1;

Hm2, how does this interact with the changes which are removing 
enable_guc_loading? Or it is a race?

I was just thinking that we should probably let the user know if they 
asked for logging, but it cannot happen due guc loading turned off, to 
have analogy with the same when GuC is not available in the first place.

Does that make sense?

> +	}
>   
>   	/* A negative value means "use platform default" */
>   	if (i915_modparams.enable_guc_submission < 0)
> 

But since it looks like a cleanup even without the above:

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

Regards,

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

Re: [PATCH 05/11] drm/i915/guc: Make GuC log related functions depend only on log level

[Tvrtko Ursulin]

On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> With guc_log_level parameter sanitized we can remove the GuC submission
> checks from flush_guc_logs and i915_guc_log_register/unregister and
> intel_uc_fini_hw. It is important to note that GuC log runtime/relay
> channel 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 and driver
> will try to destroy during cleanup/fini_hw where relay will be NULL
> already.
> 
> With this patch GuC interrupts are enabled first after GuC load if logging
> is enabled. Earlier they were enabled only when submission was getting
> 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. 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: Updated guc_log_unregister again. Made all GuC log related functions
> depend only guc_log_level. Updated uC init w.r.t enabling of GuC
> interrupts.
> 
> 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 | 12 ++++--------
>   drivers/gpu/drm/i915/intel_uc.c      | 21 ++++++++++++---------
>   3 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 31f25e5..959057a 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -274,7 +274,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 (i915_modparams.guc_log_level >= 0)
> +		intel_disable_guc_interrupts(guc);

I don't like this as much since we have a general direction of having 
less usage of modparams sprinkled around. Ideally we would take a 
snapshot after sanitizing so it remains immutable for the lifetime of 
the driver.

For this particular case it would be nice I think if in the guc object 
you would store something like bool guc->interrupts_needed.

Whether or not then to act on that from inside 
intel_(en|dis)ble_guc_interrupts would be open for discussion.

Your thoughts?

>   
>   	ctx = dev_priv->kernel_context;
>   
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 200f0a1..f87e9f5 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -504,8 +504,7 @@ static void guc_log_capture_logs(struct intel_guc *guc)
>   
>   static void guc_flush_logs(struct intel_guc *guc)
>   {
> -	if (!i915_modparams.enable_guc_submission ||
> -	    (i915_modparams.guc_log_level < 0))
> +	if (i915_modparams.guc_log_level < 0)

This would then become guc->log_level.

>   		return;
>   
>   	/* First disable the interrupts, will be renabled afterwards */
> @@ -645,8 +644,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 (!i915_modparams.enable_guc_submission ||
> -	    (i915_modparams.guc_log_level < 0))
> +	if (i915_modparams.guc_log_level < 0)
>   		return;
>   
>   	mutex_lock(&dev_priv->drm.struct_mutex);
> @@ -656,12 +654,10 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
>   
>   void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>   {
> -	if (!i915_modparams.enable_guc_submission)
> -		return;
> -
>   	mutex_lock(&dev_priv->drm.struct_mutex);
>   	/* GuC logging is currently the only user of Guc2Host interrupts */
> -	intel_disable_guc_interrupts(&dev_priv->guc);
> +	if (i915_modparams.guc_log_level >= 0)
> +		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 8feefcd..95c5ec4 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -212,18 +212,18 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	if (ret)
>   		goto err_log_capture;
>   
> +	if (i915_modparams.guc_log_level >= 0)
> +		intel_enable_guc_interrupts(guc);
> +
>   	ret = guc_enable_communication(guc);
>   	if (ret)
> -		goto err_log_capture;
> +		goto err_interrupts;
>   
>   	intel_huc_auth(&dev_priv->huc);
>   	if (i915_modparams.enable_guc_submission) {
> -		if (i915_modparams.guc_log_level >= 0)
> -			intel_enable_guc_interrupts(guc);
> -
>   		ret = i915_guc_submission_enable(dev_priv);
>   		if (ret)
> -			goto err_interrupts;
> +			goto err_comm;
>   	}
>   
>   	dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version %u.%u])\n",
> @@ -243,9 +243,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	 * nonfatal error (i.e. it doesn't prevent driver load, but
>   	 * marks the GPU as wedged until reset).
>   	 */
> -err_interrupts:
> +err_comm:
>   	guc_disable_communication(guc);
> -	intel_disable_guc_interrupts(guc);
> +err_interrupts:
> +	if (i915_modparams.guc_log_level >= 0)
> +		intel_disable_guc_interrupts(guc);
>   err_log_capture:
>   	guc_capture_load_err_log(guc);
>   err_submission:
> @@ -285,10 +287,11 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>   
>   	guc_disable_communication(&dev_priv->guc);
>   
> -	if (i915_modparams.enable_guc_submission) {
> +	if (i915_modparams.guc_log_level >= 0)
>   		intel_disable_guc_interrupts(&dev_priv->guc);
> +
> +	if (i915_modparams.enable_guc_submission)
>   		i915_guc_submission_fini(dev_priv);
> -	}
>   
>   	i915_ggtt_disable_guc(dev_priv);
>   }
> 

Open for discussion I think, but it feels even visually like to much 
modparams.

Regards,

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

Re: [PATCH 06/11] drm/i915/guc: Only release GuC log object during submission_fini

[Tvrtko Ursulin]

On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> GuC log runtime/relay channel data is released during i915 unregister,
> So only GuC log vma needs to be released during submission_fini.
> 
> 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_guc_submission.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index a2e8114..c360b37 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1021,7 +1021,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>   
>   	ida_destroy(&guc->stage_ids);
>   	guc_ads_destroy(guc);
> -	intel_guc_log_destroy(guc);
> +	i915_vma_unpin_and_release(&guc->log.vma);
>   	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>   	i915_vma_unpin_and_release(&guc->stage_desc_pool);
>   }
> 

Doesn't it make more sense to hide the logging implementation details 
from this call site?

And I can't find the remaining caller of the intel_guc_log_destroy in 
the current codebase? Unless it was added in one of the previous patches?

Regards,

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

✓ Fi.CI.IGT: success for GuC Interrupts/Log updates

[Patchwork]

== Series Details ==

Series: GuC Interrupts/Log updates
URL   : https://patchwork.freedesktop.org/series/32179/
State : success

== Summary ==

Test drv_suspend:
        Subgroup sysfs-reader-hibernate:
                dmesg-fail -> FAIL       (shard-hsw) k.org#196691

k.org#196691 https://bugzilla.kernel.org/show_bug.cgi?id=196691

shard-hsw        total:2551 pass:1441 dwarn:0   dfail:0   fail:9   skip:1101 time:9301s

== Logs ==

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

Re: [PATCH 01/11] drm/i915: Export low level PM IRQ functions to control from GuC functions

[Sagar Arun Kamble]

On 10/18/2017 6:12 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>> 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.
>>
>> 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>
>> ---
>>   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 b1296a5..caa6283 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;
>>   }
>
> Can we pencil in for some future work to move this into the device 
> private/info/... somewhere? It would be smaller on the aggregate, and 
> more importantly more elegant that way I think.
Yes. Agree. Will add comment about this.
Thanks for review.
>
>> @@ -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);
>
> I briefly considered, if now that these functions are exported, we 
> would benefit from expressing the lock requirement in the functions 
> name somehow, either with a double underscore prefix, or locked 
> suffix. But since they have the asserts already I think it is fine.
>
>>   diff --git a/drivers/gpu/drm/i915/intel_drv.h 
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index d61985f..792d8ea 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1237,8 +1237,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);
>>
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Regards,
>
> Tvrtko

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

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

[Sagar Arun Kamble]

On 10/18/2017 6:16 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>> 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, i915 irq handling keeping them grouped in
>> intel_guc.c instead of intel_uc.c.
>>
>> 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>
>> ---
>>   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 caa6283..84a4bf3 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
>> @@ -1474,7 +1441,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]);
>
> Slight downside that it cannot be inlined any longer, should the 
> compiler decide to do so. But it is probably not a relevant 
> consideration for the frequency of these.
Ok. If needed we can force by making these static inline from header.
Thanks for review.
>
>>   }
>>     static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
>> @@ -1751,41 +1718,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 792d8ea..39c76ba 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1269,9 +1269,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 10037c0..3a64ae1 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -274,7 +274,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);
>>         ctx = dev_priv->kernel_context;
>>   @@ -302,7 +302,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);
>>         ctx = dev_priv->kernel_context;
>>   @@ -367,3 +367,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 418450b..8b26505 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -116,5 +116,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 76d3eb1..8120208 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -510,7 +510,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.
>> @@ -628,7 +628,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
>> @@ -662,7 +662,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 25bd162..b9b9947a0 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -158,7 +158,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>           return 0;
>>         guc_disable_communication(guc);
>> -    gen9_reset_guc_interrupts(dev_priv);
>> +    intel_reset_guc_interrupts(dev_priv);
>>         /* We need to notify the guc whenever we change the GGTT */
>>       i915_ggtt_enable_guc(dev_priv);
>> @@ -215,7 +215,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>       intel_huc_auth(&dev_priv->huc);
>>       if (i915_modparams.enable_guc_submission) {
>>           if (i915_modparams.guc_log_level >= 0)
>> -            gen9_enable_guc_interrupts(dev_priv);
>> +            intel_enable_guc_interrupts(dev_priv);
>>             ret = i915_guc_submission_enable(dev_priv);
>>           if (ret)
>> @@ -241,7 +241,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>        */
>>   err_interrupts:
>>       guc_disable_communication(guc);
>> -    gen9_disable_guc_interrupts(dev_priv);
>> +    intel_disable_guc_interrupts(dev_priv);
>>   err_log_capture:
>>       guc_capture_load_err_log(guc);
>>   err_submission:
>> @@ -282,7 +282,7 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *dev_priv)
>>       guc_disable_communication(&dev_priv->guc);
>>         if (i915_modparams.enable_guc_submission) {
>> -        gen9_disable_guc_interrupts(dev_priv);
>> +        intel_disable_guc_interrupts(dev_priv);
>>           i915_guc_submission_fini(dev_priv);
>>       }
>>
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Regards,
>
> Tvrtko

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

Re: [PATCH 04/11] drm/i915/guc: Sanitize module parameter guc_log_level

[Sagar Arun Kamble]

On 10/18/2017 6:29 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>> Parameter guc_log_level needs to be sanitized based on GuC support and
>> enable_guc_loading parameter since it depends on them like
>> enable_guc_submission. This will make GuC logging paths independent of
>> enable_guc_submission parameter in further patches.
>>
>> v2: Added documentation to intel_guc_log.c and param description about
>> GuC loading dependency. (Michal Wajdeczko)
>>
>> 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_params.c   |  4 +++-
>>   drivers/gpu/drm/i915/intel_guc_log.c |  1 +
>>   drivers/gpu/drm/i915/intel_uc.c      | 10 +++++++---
>>   3 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>> b/drivers/gpu/drm/i915/i915_params.c
>> index b4faeb6..774c56e 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly = {
>>       "(-1=auto, 0=never [default], 1=if available, 2=required)");
>>     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_loading) (-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 f53c663..200f0a1 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_loading.
>>    * 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.
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 62738ad..8feefcd 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct 
>> drm_i915_private *dev_priv)
>>   {
>>       if (!HAS_GUC(dev_priv)) {
>>           if (i915_modparams.enable_guc_loading > 0 ||
>> -            i915_modparams.enable_guc_submission > 0)
>> +            i915_modparams.enable_guc_submission > 0 ||
>> +            i915_modparams.guc_log_level > 0)
>>               DRM_INFO("Ignoring GuC options, no hardware\n");
>
> Hm, this won't fire on <=gen8 once enable_guc_submission starts to 
> default to one? Or we can worry about that when we get there...
This will fire for <=gen8 for +ve values which is expected.
>
>> i915_modparams.enable_guc_loading = 0;
>>           i915_modparams.enable_guc_submission = 0;
>> +        i915_modparams.guc_log_level = -1;
>>           return;
>>       }
>>   @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct 
>> drm_i915_private *dev_priv)
>>               i915_modparams.enable_guc_loading = 0;
>>       }
>>   -    /* Can't enable guc submission without guc loaded */
>> -    if (!i915_modparams.enable_guc_loading)
>> +    /* Can't enable guc submission and logging without guc loaded */
>> +    if (!i915_modparams.enable_guc_loading) {
>>           i915_modparams.enable_guc_submission = 0;
>> +        i915_modparams.guc_log_level = -1;
>
> Hm2, how does this interact with the changes which are removing 
> enable_guc_loading? Or it is a race?
It interacts. Will race. I think I should pause this series till the 
other one gets merged.
>
> I was just thinking that we should probably let the user know if they 
> asked for logging, but it cannot happen due guc loading turned off, to 
> have analogy with the same when GuC is not available in the first place.
will change with enable_guc_loading change.
>
> Does that make sense?
Yes :)
>
>> +    }
>>         /* A negative value means "use platform default" */
>>       if (i915_modparams.enable_guc_submission < 0)
>>
>
> But since it looks like a cleanup even without the above:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Regards,
>
> Tvrtko

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

Re: [PATCH 05/11] drm/i915/guc: Make GuC log related functions depend only on log level

[Sagar Arun Kamble]

On 10/18/2017 6:37 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>> With guc_log_level parameter sanitized we can remove the GuC submission
>> checks from flush_guc_logs and i915_guc_log_register/unregister and
>> intel_uc_fini_hw. It is important to note that GuC log runtime/relay
>> channel 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 and driver
>> will try to destroy during cleanup/fini_hw where relay will be NULL
>> already.
>>
>> With this patch GuC interrupts are enabled first after GuC load if 
>> logging
>> is enabled. Earlier they were enabled only when submission was getting
>> 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. 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: Updated guc_log_unregister again. Made all GuC log related functions
>> depend only guc_log_level. Updated uC init w.r.t enabling of GuC
>> interrupts.
>>
>> 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 | 12 ++++--------
>>   drivers/gpu/drm/i915/intel_uc.c      | 21 ++++++++++++---------
>>   3 files changed, 18 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 31f25e5..959057a 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -274,7 +274,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 (i915_modparams.guc_log_level >= 0)
>> +        intel_disable_guc_interrupts(guc);
>
> I don't like this as much since we have a general direction of having 
> less usage of modparams sprinkled around. Ideally we would take a 
> snapshot after sanitizing so it remains immutable for the lifetime of 
> the driver.
Yes. Will update.
>
> For this particular case it would be nice I think if in the guc object 
> you would store something like bool guc->interrupts_needed.
>
> Whether or not then to act on that from inside 
> intel_(en|dis)ble_guc_interrupts would be open for discussion.
ok
>
> Your thoughts?
>
>>         ctx = dev_priv->kernel_context;
>>   diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 200f0a1..f87e9f5 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -504,8 +504,7 @@ static void guc_log_capture_logs(struct intel_guc 
>> *guc)
>>     static void guc_flush_logs(struct intel_guc *guc)
>>   {
>> -    if (!i915_modparams.enable_guc_submission ||
>> -        (i915_modparams.guc_log_level < 0))
>> +    if (i915_modparams.guc_log_level < 0)
>
> This would then become guc->log_level.
yes
>
>>           return;
>>         /* First disable the interrupts, will be renabled afterwards */
>> @@ -645,8 +644,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 (!i915_modparams.enable_guc_submission ||
>> -        (i915_modparams.guc_log_level < 0))
>> +    if (i915_modparams.guc_log_level < 0)
>>           return;
>>         mutex_lock(&dev_priv->drm.struct_mutex);
>> @@ -656,12 +654,10 @@ void i915_guc_log_register(struct 
>> drm_i915_private *dev_priv)
>>     void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>>   {
>> -    if (!i915_modparams.enable_guc_submission)
>> -        return;
>> -
>>       mutex_lock(&dev_priv->drm.struct_mutex);
>>       /* GuC logging is currently the only user of Guc2Host 
>> interrupts */
>> -    intel_disable_guc_interrupts(&dev_priv->guc);
>> +    if (i915_modparams.guc_log_level >= 0)
>> +        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 8feefcd..95c5ec4 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -212,18 +212,18 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>       if (ret)
>>           goto err_log_capture;
>>   +    if (i915_modparams.guc_log_level >= 0)
>> +        intel_enable_guc_interrupts(guc);
>> +
>>       ret = guc_enable_communication(guc);
>>       if (ret)
>> -        goto err_log_capture;
>> +        goto err_interrupts;
>>         intel_huc_auth(&dev_priv->huc);
>>       if (i915_modparams.enable_guc_submission) {
>> -        if (i915_modparams.guc_log_level >= 0)
>> -            intel_enable_guc_interrupts(guc);
>> -
>>           ret = i915_guc_submission_enable(dev_priv);
>>           if (ret)
>> -            goto err_interrupts;
>> +            goto err_comm;
>>       }
>>         dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version 
>> %u.%u])\n",
>> @@ -243,9 +243,11 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>        * nonfatal error (i.e. it doesn't prevent driver load, but
>>        * marks the GPU as wedged until reset).
>>        */
>> -err_interrupts:
>> +err_comm:
>>       guc_disable_communication(guc);
>> -    intel_disable_guc_interrupts(guc);
>> +err_interrupts:
>> +    if (i915_modparams.guc_log_level >= 0)
>> +        intel_disable_guc_interrupts(guc);
>>   err_log_capture:
>>       guc_capture_load_err_log(guc);
>>   err_submission:
>> @@ -285,10 +287,11 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *dev_priv)
>>         guc_disable_communication(&dev_priv->guc);
>>   -    if (i915_modparams.enable_guc_submission) {
>> +    if (i915_modparams.guc_log_level >= 0)
>>           intel_disable_guc_interrupts(&dev_priv->guc);
>> +
>> +    if (i915_modparams.enable_guc_submission)
>>           i915_guc_submission_fini(dev_priv);
>> -    }
>>         i915_ggtt_disable_guc(dev_priv);
>>   }
>>
>
> Open for discussion I think, but it feels even visually like to much 
> modparams.
yes. will update.
Thanks.
>
> Regards,
>
> Tvrtko

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

Re: [PATCH 06/11] drm/i915/guc: Only release GuC log object during submission_fini

[Sagar Arun Kamble]

On 10/18/2017 6:42 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>> GuC log runtime/relay channel data is released during i915 unregister,
>> So only GuC log vma needs to be released during submission_fini.
>>
>> 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_guc_submission.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index a2e8114..c360b37 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -1021,7 +1021,7 @@ void i915_guc_submission_fini(struct 
>> drm_i915_private *dev_priv)
>>         ida_destroy(&guc->stage_ids);
>>       guc_ads_destroy(guc);
>> -    intel_guc_log_destroy(guc);
>> +    i915_vma_unpin_and_release(&guc->log.vma);
>> i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>>       i915_vma_unpin_and_release(&guc->stage_desc_pool);
>>   }
>>
>
> Doesn't it make more sense to hide the logging implementation details 
> from this call site?
Yes right. Will need to separate logging from submission cleanup here.
>
> And I can't find the remaining caller of the intel_guc_log_destroy in 
> the current codebase? Unless it was added in one of the previous patches?
It is called during submission_init on failure and that too will need to 
be changed as we separate logging from submission.
Thanks
>
> Regards,
>
> Tvrtko

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

Re: [PATCH 06/11] drm/i915/guc: Only release GuC log object during submission_fini

[Tvrtko Ursulin]

On 18/10/2017 17:04, Sagar Arun Kamble wrote:
> On 10/18/2017 6:42 PM, Tvrtko Ursulin wrote:
>>
>> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>>> GuC log runtime/relay channel data is released during i915 unregister,
>>> So only GuC log vma needs to be released during submission_fini.
>>>
>>> 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_guc_submission.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index a2e8114..c360b37 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -1021,7 +1021,7 @@ void i915_guc_submission_fini(struct 
>>> drm_i915_private *dev_priv)
>>>         ida_destroy(&guc->stage_ids);
>>>       guc_ads_destroy(guc);
>>> -    intel_guc_log_destroy(guc);
>>> +    i915_vma_unpin_and_release(&guc->log.vma);
>>> i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>>>       i915_vma_unpin_and_release(&guc->stage_desc_pool);
>>>   }
>>>
>>
>> Doesn't it make more sense to hide the logging implementation details 
>> from this call site?
> Yes right. Will need to separate logging from submission cleanup here.
>>
>> And I can't find the remaining caller of the intel_guc_log_destroy in 
>> the current codebase? Unless it was added in one of the previous patches?
> It is called during submission_init on failure and that too will need to 
> be changed as we separate logging from submission.

But never gets to run on driver unload? By design or oversight?

Regards,

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

Re: [PATCH 04/11] drm/i915/guc: Sanitize module parameter guc_log_level

[Tvrtko Ursulin]

On 18/10/2017 16:50, Sagar Arun Kamble wrote:
> On 10/18/2017 6:29 PM, Tvrtko Ursulin wrote:
>>
>> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>>> Parameter guc_log_level needs to be sanitized based on GuC support and
>>> enable_guc_loading parameter since it depends on them like
>>> enable_guc_submission. This will make GuC logging paths independent of
>>> enable_guc_submission parameter in further patches.
>>>
>>> v2: Added documentation to intel_guc_log.c and param description about
>>> GuC loading dependency. (Michal Wajdeczko)
>>>
>>> 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_params.c   |  4 +++-
>>>   drivers/gpu/drm/i915/intel_guc_log.c |  1 +
>>>   drivers/gpu/drm/i915/intel_uc.c      | 10 +++++++---
>>>   3 files changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>>> b/drivers/gpu/drm/i915/i915_params.c
>>> index b4faeb6..774c56e 100644
>>> --- a/drivers/gpu/drm/i915/i915_params.c
>>> +++ b/drivers/gpu/drm/i915/i915_params.c
>>> @@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly = {
>>>       "(-1=auto, 0=never [default], 1=if available, 2=required)");
>>>     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_loading) (-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 f53c663..200f0a1 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_loading.
>>>    * 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.
>>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>>> b/drivers/gpu/drm/i915/intel_uc.c
>>> index 62738ad..8feefcd 100644
>>> --- a/drivers/gpu/drm/i915/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>>> @@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct 
>>> drm_i915_private *dev_priv)
>>>   {
>>>       if (!HAS_GUC(dev_priv)) {
>>>           if (i915_modparams.enable_guc_loading > 0 ||
>>> -            i915_modparams.enable_guc_submission > 0)
>>> +            i915_modparams.enable_guc_submission > 0 ||
>>> +            i915_modparams.guc_log_level > 0)
>>>               DRM_INFO("Ignoring GuC options, no hardware\n");
>>
>> Hm, this won't fire on <=gen8 once enable_guc_submission starts to 
>> default to one? Or we can worry about that when we get there...
> This will fire for <=gen8 for +ve values which is expected.
>>
>>> i915_modparams.enable_guc_loading = 0;
>>>           i915_modparams.enable_guc_submission = 0;
>>> +        i915_modparams.guc_log_level = -1;
>>>           return;
>>>       }
>>>   @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct 
>>> drm_i915_private *dev_priv)
>>>               i915_modparams.enable_guc_loading = 0;
>>>       }
>>>   -    /* Can't enable guc submission without guc loaded */
>>> -    if (!i915_modparams.enable_guc_loading)
>>> +    /* Can't enable guc submission and logging without guc loaded */
>>> +    if (!i915_modparams.enable_guc_loading) {
>>>           i915_modparams.enable_guc_submission = 0;
>>> +        i915_modparams.guc_log_level = -1;
>>
>> Hm2, how does this interact with the changes which are removing 
>> enable_guc_loading? Or it is a race?
> It interacts. Will race. I think I should pause this series till the 
> other one gets merged.

Okay, it's your call (as in you guys who are refactoring GuC heavily at 
the moment).

Should I continue to the end of this one then? It is not that big so I 
just as well can.

Regards,

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

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

[Tvrtko Ursulin]

On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> 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
> 
> 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 | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index f87e9f5..ed239cb 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -656,8 +656,17 @@ 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 */
> -	if (i915_modparams.guc_log_level >= 0)
> +	if (i915_modparams.guc_log_level >= 0) {
> +		intel_runtime_pm_get(dev_priv);
>   		intel_disable_guc_interrupts(&dev_priv->guc);
> +		intel_runtime_pm_put(dev_priv);

Is it possible to trigger the assert from I915_WRITE today and if so 
which test case?

> +	}
> +	/*
> +	 * TODO: Need to synchronize access to relay channel from flush work
> +	 * and release here if interrupt stays enabled from hereon.
> +	 * Possibly with GuC CT recv. interrupts will stay enabled until GEM
> +	 * suspend/unload.
> +	 */

I think we normally don't put such reminders in code. Regardless if it 
is going away in this patch series or not it looks equally pointless to me.

>   	guc_log_runtime_destroy(&dev_priv->guc);

Ha right, this is how the cleanup happens what I was wondering in the 
previous patch. So intel_guc_log_destroy is pretty pointless now since 
it effectively does only this. Or I missed something?

>   	mutex_unlock(&dev_priv->drm.struct_mutex);
>   }
> 

Regards,

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

Re: [PATCH 08/11] drm/i915/guc: Add client support to enable/disable GuC interrupts

[Tvrtko Ursulin]

On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> This patch adds support to enable/disable GuC interrupts for different
> features without impacting others needs. 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)
> 
> 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  |  8 ++++++++
>   drivers/gpu/drm/i915/intel_guc.c     | 21 ++++++++++++++-------
>   drivers/gpu/drm/i915/intel_guc.h     | 11 ++++++++---
>   drivers/gpu/drm/i915/intel_guc_log.c |  6 +++---
>   drivers/gpu/drm/i915/intel_uc.c      |  6 +++---
>   5 files changed, 36 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0bb6e01..bd421da 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2531,6 +2531,14 @@ static int i915_guc_info(struct seq_file *m, void *data)
>   	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>   	const struct intel_guc *guc = &dev_priv->guc;
>   
> +	seq_puts(m, "GuC Interrupt Clients: ");
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	if (guc->interrupt_clients & BIT(GUC_INTR_CLIENT_LOG))

Could use test_bit.

Also, spinlock not required here apart from documentation purposes?

> +		seq_puts(m, "GuC Logging\n");
> +	else
> +		seq_puts(m, "None\n");
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +
>   	if (!check_guc_submission(m))
>   		return 0;
>   
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 959057a..fbd27ea 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -275,7 +275,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
>   		return 0;
>   
>   	if (i915_modparams.guc_log_level >= 0)
> -		intel_disable_guc_interrupts(guc);
> +		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>   
>   	ctx = dev_priv->kernel_context;
>   
> @@ -303,7 +303,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
>   		return 0;
>   
>   	if (i915_modparams.guc_log_level >= 0)
> -		intel_enable_guc_interrupts(guc);
> +		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>   
>   	ctx = dev_priv->kernel_context;
>   
> @@ -378,26 +378,33 @@ 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)
> +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) {
> +	if (!guc->interrupt_clients) {
>   		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);
>   	}
> +	set_bit(id, &guc->interrupt_clients);

Can use __set_bit to save one atomic since it is already under the spinlock.

> +
>   	spin_unlock_irq(&dev_priv->irq_lock);
>   }
>   
> -void intel_disable_guc_interrupts(struct intel_guc *guc)
> +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);
> -	dev_priv->guc.interrupts_enabled = false;
> +
> +	clear_bit(id, &guc->interrupt_clients);

Equally as above __clear_bit would work.

> +
> +	if (guc->interrupt_clients) {
> +		spin_unlock_irq(&dev_priv->irq_lock);
> +		return;
> +	}
>   
>   	gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>   
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index e89b4ae..4d58bf7 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -34,6 +34,11 @@
>   #include "i915_guc_reg.h"
>   #include "i915_vma.h"
>   
> +enum guc_intr_client {
> +	GUC_INTR_CLIENT_LOG = 0,
> +	GUC_INTR_CLIENT_COUNT
> +};
> +
>   /*
>    * Top level structure of GuC. It handles firmware loading and manages client
>    * pool and doorbells. intel_guc owns a i915_guc_client to replace the legacy
> @@ -48,7 +53,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;
> @@ -117,8 +122,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 ed239cb..8c41d9a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -508,7 +508,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.
> @@ -626,7 +626,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
> @@ -658,7 +658,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>   	/* GuC logging is currently the only user of Guc2Host interrupts */
>   	if (i915_modparams.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 95c5ec4..d96c847 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -213,7 +213,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   		goto err_log_capture;
>   
>   	if (i915_modparams.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)
> @@ -247,7 +247,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	guc_disable_communication(guc);
>   err_interrupts:
>   	if (i915_modparams.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_submission:
> @@ -288,7 +288,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>   	guc_disable_communication(&dev_priv->guc);
>   
>   	if (i915_modparams.guc_log_level >= 0)
> -		intel_disable_guc_interrupts(&dev_priv->guc);
> +		intel_put_guc_interrupts(&dev_priv->guc, GUC_INTR_CLIENT_LOG);
>   
>   	if (i915_modparams.enable_guc_submission)
>   		i915_guc_submission_fini(dev_priv);
> 

With the bit ops tidy:

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

Regards,

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

Re: [PATCH 09/11] drm/i915/guc: Fix GuC interrupts disabling with Logging

[Tvrtko Ursulin]

On 18/10/2017 07:47, Sagar Arun Kamble wrote:
> 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 at a point
> where CT mechanism ceases.
> 
> 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 d96c847..9715eda 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -287,9 +287,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>   
>   	guc_disable_communication(&dev_priv->guc);
>   
> -	if (i915_modparams.guc_log_level >= 0)
> -		intel_put_guc_interrupts(&dev_priv->guc, GUC_INTR_CLIENT_LOG);
> -

And this hasn't became wrong in this series right? So normally I think 
you would remove the bug before refactoring patches, just so we don't 
have to read and review in a previous patch the lines which get removed 
in the next.

>   	if (i915_modparams.enable_guc_submission)
>   		i915_guc_submission_fini(dev_priv);
>   
> 

Regards,

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

Re: [PATCH 10/11] drm/i915/guc: Skip interrupt enabling if logging is already enabled

[Tvrtko Ursulin]

On 18/10/2017 07:47, Sagar Arun Kamble wrote:
> To balance the GuC interrupt references we don't allow enabling interrupts
> If logging was enabled earlier with different verbosity.

I've noticed in a couple of your previous commits words in the middle of 
sentences starting with upper case which is a tiny bit distracting when 
reading.

> We allow request to change log parameters to be sent to GuC though as
> user may want to just update the verbosity level at runtime.
> 
> 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 | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 8c41d9a..90b8caf 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -613,6 +613,11 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>   	}
>   
>   	if (log_param.logging_enabled) {
> +		if (i915_modparams.guc_log_level >= 0) {
> +			i915_modparams.guc_log_level = log_param.verbosity;
> +			return 0;
> +		}

Ok this will change if you go for the refactoring of how modparam is 
used mentioned earlier in the series.

> +
>   		i915_modparams.guc_log_level = log_param.verbosity;
>   
>   		/* If log_level was set as -1 at boot time, then the relay channel file
> 

Regards,

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

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

[Tvrtko Ursulin]

On 18/10/2017 07:47, Sagar Arun Kamble 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.
> 
> 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     | 40 ++++++++++++++++++++++++++++++++----
>   drivers/gpu/drm/i915/intel_guc.h     |  2 ++
>   3 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 897fe7e..742ab5e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3768,8 +3768,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 fbd27ea..1e5abf2 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -261,6 +261,25 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset)
>   	return intel_guc_send(guc, action, ARRAY_SIZE(action));
>   }
>   
> +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;
> +	}
> +
> +	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(guc);
> +}

Looks awfully similar to intel_put_guc_interrupts, minus the 
guc->interrupt_clients handling and single bit vs all. I am thinking if 
it could be consolidated somehow...

Maybe add __intel_put_guc_interrupts which does not touch 
guc->interrupt_clients and takes in a bitmask of all interrupt clients?

void __intel_put_guc_interrupts(..., unsigned long mask)
{
	assert_lock_held...

	if ((mask & guc->interrupt_clients) != guc->interrupt_clients)
		return;

	... do the irq disable stuff..
}

void intel_suspend_guc_interrupts(...)
{
	spin_lock...

	__intel_put_guc_interrupts(.., guc->interrupt_clients);

	spin_unlock...
}

void intel_put_guc_interrupts(..., id)
{
	spin_lock...

	__intel_put_guc_interrupts(.., BIT(id));
	__clear_bit(guc->interrupt_clients, id);

	spin_unlock...
}

Maybe some logic mistakes here, beware. :)

> +
>   /**
>    * intel_guc_suspend() - notify GuC entering suspend state
>    * @dev_priv:	i915 device private
> @@ -274,8 +293,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
>   	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>   		return 0;
>   
> -	if (i915_modparams.guc_log_level >= 0)
> -		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
> +	intel_suspend_guc_interrupts(guc);
>   
>   	ctx = dev_priv->kernel_context;
>   
> @@ -289,6 +307,21 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
>   	return intel_guc_send(guc, data, ARRAY_SIZE(data));
>   }
>   
> +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) {
> +		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);
> +	}
> +
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}

And ofc something similar like the above to consolidate the get/restore 
as well.

> +
>   /**
>    * intel_guc_resume() - notify GuC resuming from suspend state
>    * @dev_priv:	i915 device private
> @@ -302,8 +335,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)
> -		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
> +	intel_restore_guc_interrupts(guc);
>   
>   	ctx = dev_priv->kernel_context;
>   
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 4d58bf7..c55dcba 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -125,5 +125,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
> 

Regards,

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

Re: [PATCH 06/11] drm/i915/guc: Only release GuC log object during submission_fini

[Sagar Arun Kamble]

On 10/19/2017 12:48 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 17:04, Sagar Arun Kamble wrote:
>> On 10/18/2017 6:42 PM, Tvrtko Ursulin wrote:
>>>
>>> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>>>> GuC log runtime/relay channel data is released during i915 unregister,
>>>> So only GuC log vma needs to be released during submission_fini.
>>>>
>>>> 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_guc_submission.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
>>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> index a2e8114..c360b37 100644
>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> @@ -1021,7 +1021,7 @@ void i915_guc_submission_fini(struct 
>>>> drm_i915_private *dev_priv)
>>>>         ida_destroy(&guc->stage_ids);
>>>>       guc_ads_destroy(guc);
>>>> -    intel_guc_log_destroy(guc);
>>>> +    i915_vma_unpin_and_release(&guc->log.vma);
>>>> i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>>>> i915_vma_unpin_and_release(&guc->stage_desc_pool);
>>>>   }
>>>>
>>>
>>> Doesn't it make more sense to hide the logging implementation 
>>> details from this call site?
>> Yes right. Will need to separate logging from submission cleanup here.
>>>
>>> And I can't find the remaining caller of the intel_guc_log_destroy 
>>> in the current codebase? Unless it was added in one of the previous 
>>> patches?
>> It is called during submission_init on failure and that too will need 
>> to be changed as we separate logging from submission.
>
> But never gets to run on driver unload? By design or oversight?
Oversight. Never gets to run on driver unload.
>
> Regards,
>
> Tvrtko

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

Re: [PATCH 04/11] drm/i915/guc: Sanitize module parameter guc_log_level

[Sagar Arun Kamble]

On 10/19/2017 12:52 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 16:50, Sagar Arun Kamble wrote:
>> On 10/18/2017 6:29 PM, Tvrtko Ursulin wrote:
>>>
>>> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>>>> Parameter guc_log_level needs to be sanitized based on GuC support and
>>>> enable_guc_loading parameter since it depends on them like
>>>> enable_guc_submission. This will make GuC logging paths independent of
>>>> enable_guc_submission parameter in further patches.
>>>>
>>>> v2: Added documentation to intel_guc_log.c and param description about
>>>> GuC loading dependency. (Michal Wajdeczko)
>>>>
>>>> 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_params.c   |  4 +++-
>>>>   drivers/gpu/drm/i915/intel_guc_log.c |  1 +
>>>>   drivers/gpu/drm/i915/intel_uc.c      | 10 +++++++---
>>>>   3 files changed, 11 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>>>> b/drivers/gpu/drm/i915/i915_params.c
>>>> index b4faeb6..774c56e 100644
>>>> --- a/drivers/gpu/drm/i915/i915_params.c
>>>> +++ b/drivers/gpu/drm/i915/i915_params.c
>>>> @@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly 
>>>> = {
>>>>       "(-1=auto, 0=never [default], 1=if available, 2=required)");
>>>>     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_loading) (-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 f53c663..200f0a1 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_loading.
>>>>    * 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.
>>>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>>>> b/drivers/gpu/drm/i915/intel_uc.c
>>>> index 62738ad..8feefcd 100644
>>>> --- a/drivers/gpu/drm/i915/intel_uc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>>>> @@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct 
>>>> drm_i915_private *dev_priv)
>>>>   {
>>>>       if (!HAS_GUC(dev_priv)) {
>>>>           if (i915_modparams.enable_guc_loading > 0 ||
>>>> -            i915_modparams.enable_guc_submission > 0)
>>>> +            i915_modparams.enable_guc_submission > 0 ||
>>>> +            i915_modparams.guc_log_level > 0)
>>>>               DRM_INFO("Ignoring GuC options, no hardware\n");
>>>
>>> Hm, this won't fire on <=gen8 once enable_guc_submission starts to 
>>> default to one? Or we can worry about that when we get there...
>> This will fire for <=gen8 for +ve values which is expected.
>>>
>>>> i915_modparams.enable_guc_loading = 0;
>>>>           i915_modparams.enable_guc_submission = 0;
>>>> +        i915_modparams.guc_log_level = -1;
>>>>           return;
>>>>       }
>>>>   @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct 
>>>> drm_i915_private *dev_priv)
>>>>               i915_modparams.enable_guc_loading = 0;
>>>>       }
>>>>   -    /* Can't enable guc submission without guc loaded */
>>>> -    if (!i915_modparams.enable_guc_loading)
>>>> +    /* Can't enable guc submission and logging without guc loaded */
>>>> +    if (!i915_modparams.enable_guc_loading) {
>>>>           i915_modparams.enable_guc_submission = 0;
>>>> +        i915_modparams.guc_log_level = -1;
>>>
>>> Hm2, how does this interact with the changes which are removing 
>>> enable_guc_loading? Or it is a race?
>> It interacts. Will race. I think I should pause this series till the 
>> other one gets merged.
>
> Okay, it's your call (as in you guys who are refactoring GuC heavily 
> at the moment).
>
> Should I continue to the end of this one then? It is not that big so I 
> just as well can.
Thanks for the review. Will address all the inputs and post after 
Sujaritha's series as there will be some conflict.
I had thought of having this review in parallel as patches are largely 
independent but still there is some conflict so I
will defer next rev. for some more time.
>
> Regards,
>
> Tvrtko

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

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

[Sagar Arun Kamble]

On 10/19/2017 3:39 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>> 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
>>
>> 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 | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index f87e9f5..ed239cb 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -656,8 +656,17 @@ 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 */
>> -    if (i915_modparams.guc_log_level >= 0)
>> +    if (i915_modparams.guc_log_level >= 0) {
>> +        intel_runtime_pm_get(dev_priv);
>>           intel_disable_guc_interrupts(&dev_priv->guc);
>> +        intel_runtime_pm_put(dev_priv);
>
> Is it possible to trigger the assert from I915_WRITE today and if so 
> which test case?
drv_module_reload/basic-reload
>
>> +    }
>> +    /*
>> +     * TODO: Need to synchronize access to relay channel from flush 
>> work
>> +     * and release here if interrupt stays enabled from hereon.
>> +     * Possibly with GuC CT recv. interrupts will stay enabled until 
>> GEM
>> +     * suspend/unload.
>> +     */
>
> I think we normally don't put such reminders in code. Regardless if it 
> is going away in this patch series or not it looks equally pointless 
> to me.
Ok. Will remove this. Will need to be noted when we add CT buf recv 
support.
>
>> guc_log_runtime_destroy(&dev_priv->guc);
>
> Ha right, this is how the cleanup happens what I was wondering in the 
> previous patch. So intel_guc_log_destroy is pretty pointless now since 
> it effectively does only this. Or I missed something?
Yes. Part of intel_guc_log_destroy to unpin and release log vma is 
needed during uc_fini_hw. Will remove this function then as it is only 
used during init failure and we can open code there.
>
>> mutex_unlock(&dev_priv->drm.struct_mutex);
>>   }
>>
>
> Regards,
>
> Tvrtko

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

Re: [PATCH 08/11] drm/i915/guc: Add client support to enable/disable GuC interrupts

[Sagar Arun Kamble]

On 10/19/2017 3:49 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>> This patch adds support to enable/disable GuC interrupts for different
>> features without impacting others needs. 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)
>>
>> 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  |  8 ++++++++
>>   drivers/gpu/drm/i915/intel_guc.c     | 21 ++++++++++++++-------
>>   drivers/gpu/drm/i915/intel_guc.h     | 11 ++++++++---
>>   drivers/gpu/drm/i915/intel_guc_log.c |  6 +++---
>>   drivers/gpu/drm/i915/intel_uc.c      |  6 +++---
>>   5 files changed, 36 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 0bb6e01..bd421da 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2531,6 +2531,14 @@ static int i915_guc_info(struct seq_file *m, 
>> void *data)
>>       struct drm_i915_private *dev_priv = node_to_i915(m->private);
>>       const struct intel_guc *guc = &dev_priv->guc;
>>   +    seq_puts(m, "GuC Interrupt Clients: ");
>> +    spin_lock_irq(&dev_priv->irq_lock);
>> +    if (guc->interrupt_clients & BIT(GUC_INTR_CLIENT_LOG))
>
> Could use test_bit.
Yes. will update
>
> Also, spinlock not required here apart from documentation purposes?
Yes. Given the frequency of enable/disable of interrupts this spin_lock 
is not so important.
>
>> +        seq_puts(m, "GuC Logging\n");
>> +    else
>> +        seq_puts(m, "None\n");
>> +    spin_unlock_irq(&dev_priv->irq_lock);
>> +
>>       if (!check_guc_submission(m))
>>           return 0;
>>   diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 959057a..fbd27ea 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -275,7 +275,7 @@ int intel_guc_suspend(struct drm_i915_private 
>> *dev_priv)
>>           return 0;
>>         if (i915_modparams.guc_log_level >= 0)
>> -        intel_disable_guc_interrupts(guc);
>> +        intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>>         ctx = dev_priv->kernel_context;
>>   @@ -303,7 +303,7 @@ int intel_guc_resume(struct drm_i915_private 
>> *dev_priv)
>>           return 0;
>>         if (i915_modparams.guc_log_level >= 0)
>> -        intel_enable_guc_interrupts(guc);
>> +        intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>>         ctx = dev_priv->kernel_context;
>>   @@ -378,26 +378,33 @@ 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)
>> +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) {
>> +    if (!guc->interrupt_clients) {
>>           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);
>>       }
>> +    set_bit(id, &guc->interrupt_clients);
>
> Can use __set_bit to save one atomic since it is already under the 
> spinlock.
Yes.
>
>> +
>>       spin_unlock_irq(&dev_priv->irq_lock);
>>   }
>>   -void intel_disable_guc_interrupts(struct intel_guc *guc)
>> +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);
>> -    dev_priv->guc.interrupts_enabled = false;
>> +
>> +    clear_bit(id, &guc->interrupt_clients);
>
> Equally as above __clear_bit would work.
Yes.
>
>> +
>> +    if (guc->interrupt_clients) {
>> +        spin_unlock_irq(&dev_priv->irq_lock);
>> +        return;
>> +    }
>>         gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>>   diff --git a/drivers/gpu/drm/i915/intel_guc.h 
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index e89b4ae..4d58bf7 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -34,6 +34,11 @@
>>   #include "i915_guc_reg.h"
>>   #include "i915_vma.h"
>>   +enum guc_intr_client {
>> +    GUC_INTR_CLIENT_LOG = 0,
>> +    GUC_INTR_CLIENT_COUNT
>> +};
>> +
>>   /*
>>    * Top level structure of GuC. It handles firmware loading and 
>> manages client
>>    * pool and doorbells. intel_guc owns a i915_guc_client to replace 
>> the legacy
>> @@ -48,7 +53,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;
>> @@ -117,8 +122,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 ed239cb..8c41d9a 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -508,7 +508,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.
>> @@ -626,7 +626,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
>> @@ -658,7 +658,7 @@ void i915_guc_log_unregister(struct 
>> drm_i915_private *dev_priv)
>>       /* GuC logging is currently the only user of Guc2Host 
>> interrupts */
>>       if (i915_modparams.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 95c5ec4..d96c847 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -213,7 +213,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>           goto err_log_capture;
>>         if (i915_modparams.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)
>> @@ -247,7 +247,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>       guc_disable_communication(guc);
>>   err_interrupts:
>>       if (i915_modparams.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_submission:
>> @@ -288,7 +288,7 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *dev_priv)
>>       guc_disable_communication(&dev_priv->guc);
>>         if (i915_modparams.guc_log_level >= 0)
>> -        intel_disable_guc_interrupts(&dev_priv->guc);
>> +        intel_put_guc_interrupts(&dev_priv->guc, GUC_INTR_CLIENT_LOG);
>>         if (i915_modparams.enable_guc_submission)
>>           i915_guc_submission_fini(dev_priv);
>>
>
> With the bit ops tidy:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Regards,
>
> Tvrtko

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

Re: [PATCH 09/11] drm/i915/guc: Fix GuC interrupts disabling with Logging

[Sagar Arun Kamble]

On 10/19/2017 3:54 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:47, Sagar Arun Kamble wrote:
>> 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 at a 
>> point
>> where CT mechanism ceases.
>>
>> 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 d96c847..9715eda 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -287,9 +287,6 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *dev_priv)
>>         guc_disable_communication(&dev_priv->guc);
>>   -    if (i915_modparams.guc_log_level >= 0)
>> -        intel_put_guc_interrupts(&dev_priv->guc, GUC_INTR_CLIENT_LOG);
>> -
>
> And this hasn't became wrong in this series right? So normally I think 
> you would remove the bug before refactoring patches, just so we don't 
> have to read and review in a previous patch the lines which get 
> removed in the next.
Sure. Will remove this upfront.
>
>>       if (i915_modparams.enable_guc_submission)
>>           i915_guc_submission_fini(dev_priv);
>>
>
> Regards,
>
> Tvrtko

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

Re: [PATCH 10/11] drm/i915/guc: Skip interrupt enabling if logging is already enabled

[Sagar Arun Kamble]

On 10/19/2017 4:01 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:47, Sagar Arun Kamble wrote:
>> To balance the GuC interrupt references we don't allow enabling 
>> interrupts
>> If logging was enabled earlier with different verbosity.
>
> I've noticed in a couple of your previous commits words in the middle 
> of sentences starting with upper case which is a tiny bit distracting 
> when reading.
Sorry.  Will fix those. I hope it is okay to start words like 
"GuC"/"IRQ" or acronyms with upper case.
>
>> We allow request to change log parameters to be sent to GuC though as
>> user may want to just update the verbosity level at runtime.
>>
>> 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 | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 8c41d9a..90b8caf 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -613,6 +613,11 @@ int i915_guc_log_control(struct drm_i915_private 
>> *dev_priv, u64 control_val)
>>       }
>>         if (log_param.logging_enabled) {
>> +        if (i915_modparams.guc_log_level >= 0) {
>> +            i915_modparams.guc_log_level = log_param.verbosity;
>> +            return 0;
>> +        }
>
> Ok this will change if you go for the refactoring of how modparam is 
> used mentioned earlier in the series.
Yes. will update.
>
>> +
>>           i915_modparams.guc_log_level = log_param.verbosity;
>>             /* If log_level was set as -1 at boot time, then the 
>> relay channel file
>>
>
> Regards,
>
> Tvrtko

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

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

[Sagar Arun Kamble]

On 10/19/2017 4:33 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:47, Sagar Arun Kamble 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.
>>
>> 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     | 40 
>> ++++++++++++++++++++++++++++++++----
>>   drivers/gpu/drm/i915/intel_guc.h     |  2 ++
>>   3 files changed, 40 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c 
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 897fe7e..742ab5e 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3768,8 +3768,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 fbd27ea..1e5abf2 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -261,6 +261,25 @@ int intel_guc_auth_huc(struct intel_guc *guc, 
>> u32 rsa_offset)
>>       return intel_guc_send(guc, action, ARRAY_SIZE(action));
>>   }
>>   +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;
>> +    }
>> +
>> +    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(guc);
>> +}
>
> Looks awfully similar to intel_put_guc_interrupts, minus the 
> guc->interrupt_clients handling and single bit vs all. I am thinking 
> if it could be consolidated somehow...
>
> Maybe add __intel_put_guc_interrupts which does not touch 
> guc->interrupt_clients and takes in a bitmask of all interrupt clients?
>
> void __intel_put_guc_interrupts(..., unsigned long mask)
> {
>     assert_lock_held...
>
>     if ((mask & guc->interrupt_clients) != guc->interrupt_clients)
>         return;
>
>     ... do the irq disable stuff..
> }
>
> void intel_suspend_guc_interrupts(...)
> {
>     spin_lock...
>
>     __intel_put_guc_interrupts(.., guc->interrupt_clients);
>
>     spin_unlock...
> }
>
> void intel_put_guc_interrupts(..., id)
> {
>     spin_lock...
>
>     __intel_put_guc_interrupts(.., BIT(id));
>     __clear_bit(guc->interrupt_clients, id);
>
>     spin_unlock...
> }
>
> Maybe some logic mistakes here, beware. :)
Sure. will update.
>
>> +
>>   /**
>>    * intel_guc_suspend() - notify GuC entering suspend state
>>    * @dev_priv:    i915 device private
>> @@ -274,8 +293,7 @@ int intel_guc_suspend(struct drm_i915_private 
>> *dev_priv)
>>       if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>           return 0;
>>   -    if (i915_modparams.guc_log_level >= 0)
>> -        intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>> +    intel_suspend_guc_interrupts(guc);
>>         ctx = dev_priv->kernel_context;
>>   @@ -289,6 +307,21 @@ int intel_guc_suspend(struct drm_i915_private 
>> *dev_priv)
>>       return intel_guc_send(guc, data, ARRAY_SIZE(data));
>>   }
>>   +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) {
>> +        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);
>> +    }
>> +
>> +    spin_unlock_irq(&dev_priv->irq_lock);
>> +}
>
> And ofc something similar like the above to consolidate the 
> get/restore as well.
Yes.
>
>> +
>>   /**
>>    * intel_guc_resume() - notify GuC resuming from suspend state
>>    * @dev_priv:    i915 device private
>> @@ -302,8 +335,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)
>> -        intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>> +    intel_restore_guc_interrupts(guc);
>>         ctx = dev_priv->kernel_context;
>>   diff --git a/drivers/gpu/drm/i915/intel_guc.h 
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index 4d58bf7..c55dcba 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -125,5 +125,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
>>
>
> Regards,
>
> Tvrtko
Thanks again for review on the series.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx