[2/3] drm/i915/guc: Handle interrupt based logging with lack of SSE4.1, relay and other setup
diff mbox

Message ID 1517379279-12967-2-git-send-email-sagar.a.kamble@intel.com
State New
Headers show

Commit Message

sagar.a.kamble@intel.com Jan. 31, 2018, 6:14 a.m. UTC
On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available.
movntdqa is needed for efficient capture of the logs from uncached GuC
log buffer. GuC init was tied with this support and other setup needed
for interrupt based GuC log capture like relay channel/file support and
uncached mapping support. With this patch, GuC init is now unblocked from
lack of this support.
SSE and relay support init/fini is now being done by new functions
intel_guc_log_init|fini_runtime() which makes relay functions static.
We have introduced two states "supported" and "enabled". Supported is set
when we have SSE4.1 support and have relay, GuC log, WC mapping available.
Enabled is set when support is present and user has requested logging
through i915_modparams.guc_log_level.
While at this change, fixed unwind order in intel_uc_fini_misc.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c     |  2 +-
 drivers/gpu/drm/i915/intel_guc_log.c | 97 ++++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_guc_log.h | 16 +++++-
 drivers/gpu/drm/i915/intel_uc.c      | 14 ++----
 4 files changed, 90 insertions(+), 39 deletions(-)

Comments

Chris Wilson Jan. 31, 2018, 9:38 a.m. UTC | #1
Quoting Sagar Arun Kamble (2018-01-31 06:14:38)
> On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available.
> movntdqa is needed for efficient capture of the logs from uncached GuC
> log buffer. GuC init was tied with this support and other setup needed
> for interrupt based GuC log capture like relay channel/file support and
> uncached mapping support. With this patch, GuC init is now unblocked from
> lack of this support.
> SSE and relay support init/fini is now being done by new functions
> intel_guc_log_init|fini_runtime() which makes relay functions static.
> We have introduced two states "supported" and "enabled". Supported is set
> when we have SSE4.1 support and have relay, GuC log, WC mapping available.
> Enabled is set when support is present and user has requested logging
> through i915_modparams.guc_log_level.
> While at this change, fixed unwind order in intel_uc_fini_misc.

Downside would appear to be the loss of feedback in i915.guc_log_level
when it fail? Otherwise, looks tidy enough.
-Chris
sagar.a.kamble@intel.com Jan. 31, 2018, 9:54 a.m. UTC | #2
On 1/31/2018 3:08 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2018-01-31 06:14:38)
>> On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available.
>> movntdqa is needed for efficient capture of the logs from uncached GuC
>> log buffer. GuC init was tied with this support and other setup needed
>> for interrupt based GuC log capture like relay channel/file support and
>> uncached mapping support. With this patch, GuC init is now unblocked from
>> lack of this support.
>> SSE and relay support init/fini is now being done by new functions
>> intel_guc_log_init|fini_runtime() which makes relay functions static.
>> We have introduced two states "supported" and "enabled". Supported is set
>> when we have SSE4.1 support and have relay, GuC log, WC mapping available.
>> Enabled is set when support is present and user has requested logging
>> through i915_modparams.guc_log_level.
>> While at this change, fixed unwind order in intel_uc_fini_misc.
> Downside would appear to be the loss of feedback in i915.guc_log_level
> when it fail?
This patch decouples the guc_log_level from only interrupt based log 
support.
We can continue to use/know guc_log_level to send the verbosity to GuC 
still.
Have to rely only on static dumps like i915_guc_log_dump or 
error_uc->guc_log wherever runtime logging is off.
>   Otherwise, looks tidy enough.
> -Chris
Michał Winiarski Jan. 31, 2018, 10:37 a.m. UTC | #3
On Wed, Jan 31, 2018 at 11:44:38AM +0530, Sagar Arun Kamble wrote:
> On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available.
> movntdqa is needed for efficient capture of the logs from uncached GuC
> log buffer. GuC init was tied with this support and other setup needed
> for interrupt based GuC log capture like relay channel/file support and
> uncached mapping support. With this patch, GuC init is now unblocked from
> lack of this support.
> SSE and relay support init/fini is now being done by new functions
> intel_guc_log_init|fini_runtime() which makes relay functions static.
> We have introduced two states "supported" and "enabled". Supported is set
> when we have SSE4.1 support and have relay, GuC log, WC mapping available.
> Enabled is set when support is present and user has requested logging
> through i915_modparams.guc_log_level.
> While at this change, fixed unwind order in intel_uc_fini_misc.

Or, we could rework GuC log a bit.
We could change the meaning of guc_log_level - it would only affect the
"internal" GuC state (verbosity - in other words, the content of buffer used by
GuC). This allows userspace to inspect the content of GuC buffer
(i915_guc_log_dump), but i915 is not copying the data to the relay (we're
ignoring the interrupts recieved from GuC).

This means that we need to introduce alternative way of controling the relay,
let's say another file called "i915_guc_log_relay". Opening this file causes the
relay to be created, we can throw an error if we don't have SSE4.1 there.
(well - the whole runtime is created actually, things are quite badly named
today, runtime means mapping of buffer shared with GuC, relay is the buffer
"shared" with userspace, functions operating on "runtime" should probably setup
both of those rather than just the mapping).

We're also solving the problem of overflows if GuC is loaded with
"guc_log_level > 0" while nobody is consuming the data from the relay.
And we don't really need to handle the "runtime" at module load time at all,
which simplifies things.

I'm working on a series that does all of the above.

-Michał

> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc.c     |  2 +-
>  drivers/gpu/drm/i915/intel_guc_log.c | 97 ++++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_guc_log.h | 16 +++++-
>  drivers/gpu/drm/i915/intel_uc.c      | 14 ++----
>  4 files changed, 90 insertions(+), 39 deletions(-)
Chris Wilson Jan. 31, 2018, 10:45 a.m. UTC | #4
Quoting Michał Winiarski (2018-01-31 10:37:20)
> On Wed, Jan 31, 2018 at 11:44:38AM +0530, Sagar Arun Kamble wrote:
> > On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available.
> > movntdqa is needed for efficient capture of the logs from uncached GuC
> > log buffer. GuC init was tied with this support and other setup needed
> > for interrupt based GuC log capture like relay channel/file support and
> > uncached mapping support. With this patch, GuC init is now unblocked from
> > lack of this support.
> > SSE and relay support init/fini is now being done by new functions
> > intel_guc_log_init|fini_runtime() which makes relay functions static.
> > We have introduced two states "supported" and "enabled". Supported is set
> > when we have SSE4.1 support and have relay, GuC log, WC mapping available.
> > Enabled is set when support is present and user has requested logging
> > through i915_modparams.guc_log_level.
> > While at this change, fixed unwind order in intel_uc_fini_misc.
> 
> Or, we could rework GuC log a bit.
> We could change the meaning of guc_log_level - it would only affect the
> "internal" GuC state (verbosity - in other words, the content of buffer used by
> GuC). This allows userspace to inspect the content of GuC buffer
> (i915_guc_log_dump), but i915 is not copying the data to the relay (we're
> ignoring the interrupts recieved from GuC).
> 
> This means that we need to introduce alternative way of controling the relay,
> let's say another file called "i915_guc_log_relay". Opening this file causes the
> relay to be created, we can throw an error if we don't have SSE4.1 there.
> (well - the whole runtime is created actually, things are quite badly named
> today, runtime means mapping of buffer shared with GuC, relay is the buffer
> "shared" with userspace, functions operating on "runtime" should probably setup
> both of those rather than just the mapping).
> 
> We're also solving the problem of overflows if GuC is loaded with
> "guc_log_level > 0" while nobody is consuming the data from the relay.
> And we don't really need to handle the "runtime" at module load time at all,
> which simplifies things.
> 
> I'm working on a series that does all of the above.

I'll push the 1st patch (the -EEXISTS fix) and we can come back for the
rest.
-Chris
sagar.a.kamble@intel.com Jan. 31, 2018, 11:20 a.m. UTC | #5
On 1/31/2018 4:07 PM, Michał Winiarski wrote:
> On Wed, Jan 31, 2018 at 11:44:38AM +0530, Sagar Arun Kamble wrote:
>> On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available.
>> movntdqa is needed for efficient capture of the logs from uncached GuC
>> log buffer. GuC init was tied with this support and other setup needed
>> for interrupt based GuC log capture like relay channel/file support and
>> uncached mapping support. With this patch, GuC init is now unblocked from
>> lack of this support.
>> SSE and relay support init/fini is now being done by new functions
>> intel_guc_log_init|fini_runtime() which makes relay functions static.
>> We have introduced two states "supported" and "enabled". Supported is set
>> when we have SSE4.1 support and have relay, GuC log, WC mapping available.
>> Enabled is set when support is present and user has requested logging
>> through i915_modparams.guc_log_level.
>> While at this change, fixed unwind order in intel_uc_fini_misc.
> Or, we could rework GuC log a bit.
> We could change the meaning of guc_log_level - it would only affect the
> "internal" GuC state (verbosity - in other words, the content of buffer used by
> GuC). This allows userspace to inspect the content of GuC buffer
> (i915_guc_log_dump), but i915 is not copying the data to the relay (we're
> ignoring the interrupts recieved from GuC).
>
> This means that we need to introduce alternative way of controling the relay,
> let's say another file called "i915_guc_log_relay". Opening this file causes the
> relay to be created, we can throw an error if we don't have SSE4.1 there.
> (well - the whole runtime is created actually, things are quite badly named
> today, runtime means mapping of buffer shared with GuC, relay is the buffer
> "shared" with userspace, functions operating on "runtime" should probably setup
> both of those rather than just the mapping).
True. Setup spread over different stages of load made it little 
complicated to change and your change will help.
> We're also solving the problem of overflows if GuC is loaded with
> "guc_log_level > 0" while nobody is consuming the data from the relay.
> And we don't really need to handle the "runtime" at module load time at all,
> which simplifies things.
Yes. Agree on this approach. Thanks.
> I'm working on a series that does all of the above.
>
> -Michał
>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michal Winiarski <michal.winiarski@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_guc.c     |  2 +-
>>   drivers/gpu/drm/i915/intel_guc_log.c | 97 ++++++++++++++++++++++++++----------
>>   drivers/gpu/drm/i915/intel_guc_log.h | 16 +++++-
>>   drivers/gpu/drm/i915/intel_uc.c      | 14 ++----
>>   4 files changed, 90 insertions(+), 39 deletions(-)

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 21140cc..a9e5848 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -458,7 +458,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)
+	if (guc->log.runtime.enabled)
 		gen9_enable_guc_interrupts(dev_priv);
 
 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 3fbe93a..3454e0c 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -438,7 +438,7 @@  void intel_guc_log_init_early(struct intel_guc *guc)
 	INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
 }
 
-int intel_guc_log_relay_create(struct intel_guc *guc)
+static int guc_log_relay_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct rchan *guc_log_relay_chan;
@@ -486,12 +486,10 @@  int intel_guc_log_relay_create(struct intel_guc *guc)
 
 err:
 	mutex_unlock(&guc->log.runtime.relay_lock);
-	/* logging will be off */
-	i915_modparams.guc_log_level = 0;
 	return ret;
 }
 
-void intel_guc_log_relay_destroy(struct intel_guc *guc)
+static void guc_log_relay_destroy(struct intel_guc *guc)
 {
 	mutex_lock(&guc->log.runtime.relay_lock);
 
@@ -509,18 +507,51 @@  void intel_guc_log_relay_destroy(struct intel_guc *guc)
 	mutex_unlock(&guc->log.runtime.relay_lock);
 }
 
+void intel_guc_log_init_runtime(struct intel_guc *guc)
+{
+	/*
+	 * We require SSE 4.1 for fast reads from the GuC log buffer and
+	 * it should likely be present on the chipsets supporting GuC based
+	 * submisssions. Runtime logging is to be supported if SSE 4.1 support
+	 * is present and relay & uncached buffer can be setup.
+	 */
+	guc->log.runtime.supported = i915_has_memcpy_from_wc();
+	if (!guc->log.runtime.supported) {
+		DRM_DEBUG_DRIVER("SSE4.1 support not present. Runtime logging "
+				 "disabled\n");
+		return;
+	}
+
+	if (guc_log_relay_create(guc)) {
+		DRM_ERROR("Couldn't allocate relay for GuC log. Runtime logging"
+			  " disabled\n");
+		guc->log.runtime.supported = false;
+	}
+}
+
+void intel_guc_log_fini_runtime(struct intel_guc *guc)
+{
+	guc_log_relay_destroy(guc);
+	guc->log.runtime.supported = false;
+}
+
 static int guc_log_late_setup(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	int ret;
 
+	if (!guc->log.runtime.supported) {
+		ret = -ENODEV;
+		goto err;
+	}
+
 	if (!guc_log_has_runtime(guc)) {
 		/*
 		 * If log was disabled at boot time, then setup needed to handle
 		 * log buffer flush interrupts would not have been done yet, so
 		 * do that now.
 		 */
-		ret = intel_guc_log_relay_create(guc);
+		ret = guc_log_relay_create(guc);
 		if (ret)
 			goto err;
 
@@ -545,10 +576,18 @@  static int guc_log_late_setup(struct intel_guc *guc)
 	guc_log_runtime_destroy(guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 err_relay:
-	intel_guc_log_relay_destroy(guc);
+	guc_log_relay_destroy(guc);
 err:
-	/* logging will remain off */
-	i915_modparams.guc_log_level = 0;
+	mutex_lock(&dev_priv->drm.struct_mutex);
+	intel_runtime_pm_get(dev_priv);
+	gen9_disable_guc_interrupts(dev_priv);
+	guc->log.runtime.enabled = false;
+	intel_runtime_pm_put(dev_priv);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+
+	/* runtime logging will remain off */
+	guc->log.runtime.supported = false;
+
 	return ret;
 }
 
@@ -571,7 +610,7 @@  static void guc_flush_logs(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
-	if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level)
+	if (!USES_GUC_SUBMISSION(dev_priv) || !guc->log.runtime.enabled)
 		return;
 
 	/* First disable the interrupts, will be renabled afterwards */
@@ -605,16 +644,6 @@  int intel_guc_log_create(struct intel_guc *guc)
 
 	GEM_BUG_ON(guc->log.vma);
 
-	/*
-	 * We require SSE 4.1 for fast reads from the GuC log buffer and
-	 * it should be present on the chipsets supporting GuC based
-	 * submisssions.
-	 */
-	if (WARN_ON(!i915_has_memcpy_from_wc())) {
-		ret = -EINVAL;
-		goto err;
-	}
-
 	vma = intel_guc_allocate_vma(guc, GUC_LOG_SIZE);
 	if (IS_ERR(vma)) {
 		ret = PTR_ERR(vma);
@@ -623,28 +652,41 @@  int intel_guc_log_create(struct intel_guc *guc)
 
 	guc->log.vma = vma;
 
-	if (i915_modparams.guc_log_level) {
+	/*
+	 * We are making runtime creation optional based on support as
+	 * we can continue to use GuC and base log support.
+	 */
+	if (i915_modparams.guc_log_level && guc->log.runtime.supported) {
 		ret = guc_log_runtime_create(guc);
-		if (ret < 0)
-			goto err_vma;
+		if (ret < 0) {
+			DRM_ERROR("GuC log runtime create failed. Runtime "
+				  "logging disabled\n");
+			guc->log.runtime.supported = false;
+		}
 	}
 
 	/* each allocated unit is a page */
-	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
+	flags = GUC_LOG_VALID |
 		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
 		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
 		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
 
+	if (guc->log.runtime.supported) {
+		flags |= GUC_LOG_NOTIFY_ON_HALF_FULL;
+		guc->log.runtime.enabled = true;
+	}
+
 	offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
 	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
 
 	return 0;
 
-err_vma:
-	i915_vma_unpin_and_release(&guc->log.vma);
 err:
 	/* logging will be off */
 	i915_modparams.guc_log_level = 0;
+	/* runtime logging will remain off */
+	guc->log.runtime.supported = false;
+
 	return ret;
 }
 
@@ -720,6 +762,8 @@  int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
 		i915_modparams.guc_log_level = 0;
 	}
 
+	guc->log.runtime.enabled = enable_logging;
+
 	return ret;
 }
 
@@ -742,10 +786,11 @@  void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 	/* GuC logging is currently the only user of Guc2Host interrupts */
 	intel_runtime_pm_get(dev_priv);
 	gen9_disable_guc_interrupts(dev_priv);
+	guc->log.runtime.enabled = false;
 	intel_runtime_pm_put(dev_priv);
 
 	guc_log_runtime_destroy(guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 
-	intel_guc_log_relay_destroy(guc);
+	guc_log_relay_destroy(guc);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index dab0e94..c89c331 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -44,6 +44,18 @@  struct intel_guc_log {
 	struct i915_vma *vma;
 	/* The runtime stuff gets created only when GuC logging gets enabled */
 	struct {
+		/*
+		 * Support based on movntdqa availability, GuC log, relay and
+		 * runtime creation status.
+		 */
+		bool supported;
+
+		/*
+		 * Status based on "supported" and user request for logging
+		 * through i915_modparams.guc_log_level.
+		 */
+		bool enabled;
+
 		void *buf_addr;
 		struct workqueue_struct *flush_wq;
 		struct work_struct flush_work;
@@ -62,8 +74,8 @@  struct intel_guc_log {
 int intel_guc_log_create(struct intel_guc *guc);
 void intel_guc_log_destroy(struct intel_guc *guc);
 void intel_guc_log_init_early(struct intel_guc *guc);
-int intel_guc_log_relay_create(struct intel_guc *guc);
-void intel_guc_log_relay_destroy(struct intel_guc *guc);
+void intel_guc_log_init_runtime(struct intel_guc *guc);
+void intel_guc_log_fini_runtime(struct intel_guc *guc);
 int intel_guc_log_control(struct intel_guc *guc, u64 control_val);
 void i915_guc_log_register(struct drm_i915_private *dev_priv);
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index e3f3509..1cc0d86 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -250,16 +250,10 @@  int intel_uc_init_misc(struct drm_i915_private *dev_priv)
 		goto err;
 	}
 
-	ret = intel_guc_log_relay_create(guc);
-	if (ret) {
-		DRM_ERROR("Couldn't allocate relay for GuC log\n");
-		goto err_relay;
-	}
+	intel_guc_log_init_runtime(guc);
 
 	return 0;
 
-err_relay:
-	intel_guc_fini_wq(guc);
 err:
 	return ret;
 }
@@ -271,9 +265,9 @@  void intel_uc_fini_misc(struct drm_i915_private *dev_priv)
 	if (!USES_GUC(dev_priv))
 		return;
 
-	intel_guc_fini_wq(guc);
+	intel_guc_log_fini_runtime(guc);
 
-	intel_guc_log_relay_destroy(guc);
+	intel_guc_fini_wq(guc);
 }
 
 int intel_uc_init(struct drm_i915_private *dev_priv)
@@ -386,7 +380,7 @@  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	}
 
 	if (USES_GUC_SUBMISSION(dev_priv)) {
-		if (i915_modparams.guc_log_level)
+		if (guc->log.runtime.enabled)
 			gen9_enable_guc_interrupts(dev_priv);
 
 		ret = intel_guc_submission_enable(guc);