[08/15] drm/i915/guc: Split relay control and GuC log level

Commit Message

Michał Winiarski Feb. 27, 2018, 12:52 p.m. UTC

Those two concepts are really separate. Since GuC is writing data into
its own buffer and we even provide a way for userspace to read directly
from it using i915_guc_log_dump debugfs, there's no real reason to tie
log level with relay creation.
Let's create a separate debugfs, giving userspace a way to create a
relay on demand, when it wants to read a continuous log rather than a
snapshot.

Signed-off-by: Michał Winiarski <[e-mail]>
Cc: Chris Wilson <[e-mail]>
Cc: Daniele Ceraolo Spurio <[e-mail]>
Cc: Sagar Arun Kamble <[e-mail]>
Cc: Michal Wajdeczko <[e-mail]>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 56 ++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_drv.c      |  2 -
 drivers/gpu/drm/i915/intel_guc_log.c | 76 +++++++++++++++---------------------
 drivers/gpu/drm/i915/intel_guc_log.h |  9 +++--
 drivers/gpu/drm/i915/intel_uc.c      | 22 -----------
 drivers/gpu/drm/i915/intel_uc.h      |  2 -
 6 files changed, 86 insertions(+), 81 deletions(-)

Comments

[e-mail] March 6, 2018, 7:16 a.m. UTC | #1

On 2/27/2018 6:22 PM, Michał Winiarski wrote:
> Those two concepts are really separate. Since GuC is writing data into
> its own buffer and we even provide a way for userspace to read directly
> from it using i915_guc_log_dump debugfs, there's no real reason to tie
> log level with relay creation.
> Let's create a separate debugfs, giving userspace a way to create a
> relay on demand, when it wants to read a continuous log rather than a
> snapshot.
>
> Signed-off-by: Michał Winiarski <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> Cc: Daniele Ceraolo Spurio <[e-mail]>
> Cc: Sagar Arun Kamble <[e-mail]>
> Cc: Michal Wajdeczko <[e-mail]>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c  | 56 ++++++++++++++++++++++----
>   drivers/gpu/drm/i915/i915_drv.c      |  2 -
>   drivers/gpu/drm/i915/intel_guc_log.c | 76 +++++++++++++++---------------------
>   drivers/gpu/drm/i915/intel_guc_log.h |  9 +++--
>   drivers/gpu/drm/i915/intel_uc.c      | 22 -----------
>   drivers/gpu/drm/i915/intel_uc.h      |  2 -
>   6 files changed, 86 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 58983cafaece..4bd24bbe7966 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2496,32 +2496,73 @@ static int i915_guc_log_dump(struct seq_file *m, void *data)
>   	return 0;
>   }
>   
> -static int i915_guc_log_control_get(void *data, u64 *val)
> +static int i915_guc_log_level_get(void *data, u64 *val)
>   {
>   	struct drm_i915_private *dev_priv = data;
s/dev_priv/i915 here and at other places
>   
>   	if (!USES_GUC(dev_priv))
>   		return -ENODEV;
>   
> -	*val = intel_guc_log_control_get(&dev_priv->guc);
> +	*val = intel_guc_log_level_get(&dev_priv->guc);
>   
>   	return 0;
>   }
>   
> -static int i915_guc_log_control_set(void *data, u64 val)
> +static int i915_guc_log_level_set(void *data, u64 val)
>   {
>   	struct drm_i915_private *dev_priv = data;
>   
>   	if (!USES_GUC(dev_priv))
>   		return -ENODEV;
>   
> -	return intel_guc_log_control_set(&dev_priv->guc, val);
> +	return intel_guc_log_level_set(&dev_priv->guc, val);
>   }
>   
> -DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
> -			i915_guc_log_control_get, i915_guc_log_control_set,
> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_level_fops,
> +			i915_guc_log_level_get, i915_guc_log_level_set,
>   			"%lld\n");
>   
> +static int i915_guc_log_relay_open(struct inode *inode, struct file *file)
> +{
> +	struct drm_i915_private *dev_priv = inode->i_private;
> +
> +	if (!USES_GUC(dev_priv))
> +		return -ENODEV;
> +
> +	file->private_data = dev_priv;
> +
write is passing guc struct so should we just set private_data to 
&dev_priv->guc?
> +	return intel_guc_log_relay_open(&dev_priv->guc);
> +}
> +
> +static ssize_t
> +i915_guc_log_relay_write(struct file *filp,
> +			 const char __user *ubuf,
> +			 size_t cnt,
> +			 loff_t *ppos)
> +{
> +	struct drm_i915_private *dev_priv = filp->private_data;
> +
> +	intel_guc_log_relay_flush(&dev_priv->guc);
> +
> +	return cnt;
> +}
<snip>
> +void intel_guc_log_relay_close(struct intel_guc *guc)
> +{
>   	GEM_BUG_ON(!guc_log_has_runtime(guc));
>   
guc_log_has_runtime() check has to be with runtime.lock mutex locked.

With comments addressed:
Reviewed-by: Sagar Arun Kamble <[e-mail]>
> +	guc_log_flush_irq_disable(guc);
> +	flush_work(&guc->log.runtime.flush_work);
> +
> +	mutex_lock(&guc->log.runtime.lock);
>   	guc_log_unmap(guc);
>   	guc_log_relay_destroy(guc);
> -
>   	mutex_unlock(&guc->log.runtime.lock);
>   }
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> index 8c26cce77a98..df91f12a36ed 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -61,9 +61,10 @@ 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_control_get(struct intel_guc *guc);
> -int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val);
> -int intel_guc_log_register(struct intel_guc *guc);
> -void intel_guc_log_unregister(struct intel_guc *guc);
> +int intel_guc_log_level_get(struct intel_guc *guc);
> +int intel_guc_log_level_set(struct intel_guc *guc, u64 control_val);
> +int intel_guc_log_relay_open(struct intel_guc *guc);
> +void intel_guc_log_relay_close(struct intel_guc *guc);
> +void intel_guc_log_relay_flush(struct intel_guc *guc);
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 55a9b5b673e0..27e1f4c43b7b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -219,28 +219,6 @@ static void guc_free_load_err_log(struct intel_guc *guc)
>   		i915_gem_object_put(guc->load_err_log);
>   }
>   
> -int intel_uc_log_register(struct drm_i915_private *dev_priv)
> -{
> -	int ret = 0;
> -
> -	if (!USES_GUC(dev_priv))
> -		return 0;
> -
> -	if (i915_modparams.guc_log_level)
> -		ret = intel_guc_log_register(&dev_priv->guc);
> -
> -	return ret;
> -}
> -
> -void intel_uc_log_unregister(struct drm_i915_private *dev_priv)
> -{
> -	if (!USES_GUC(dev_priv))
> -		return;
> -
> -	if (i915_modparams.guc_log_level)
> -		intel_guc_log_unregister(&dev_priv->guc);
> -}
> -
>   static int guc_enable_communication(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 9bc22103396d..8a7249722ef1 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -31,8 +31,6 @@
>   void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
>   void intel_uc_init_early(struct drm_i915_private *dev_priv);
>   void intel_uc_init_mmio(struct drm_i915_private *dev_priv);
> -int intel_uc_log_register(struct drm_i915_private *dev_priv);
> -void intel_uc_log_unregister(struct drm_i915_private *dev_priv);
>   void intel_uc_init_fw(struct drm_i915_private *dev_priv);
>   void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
>   int intel_uc_init_wq(struct drm_i915_private *dev_priv);

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 58983cafaece..4bd24bbe7966 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2496,32 +2496,73 @@  static int i915_guc_log_dump(struct seq_file *m, void *data)
 	return 0;
 }
 
-static int i915_guc_log_control_get(void *data, u64 *val)
+static int i915_guc_log_level_get(void *data, u64 *val)
 {
 	struct drm_i915_private *dev_priv = data;
 
 	if (!USES_GUC(dev_priv))
 		return -ENODEV;
 
-	*val = intel_guc_log_control_get(&dev_priv->guc);
+	*val = intel_guc_log_level_get(&dev_priv->guc);
 
 	return 0;
 }
 
-static int i915_guc_log_control_set(void *data, u64 val)
+static int i915_guc_log_level_set(void *data, u64 val)
 {
 	struct drm_i915_private *dev_priv = data;
 
 	if (!USES_GUC(dev_priv))
 		return -ENODEV;
 
-	return intel_guc_log_control_set(&dev_priv->guc, val);
+	return intel_guc_log_level_set(&dev_priv->guc, val);
 }
 
-DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
-			i915_guc_log_control_get, i915_guc_log_control_set,
+DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_level_fops,
+			i915_guc_log_level_get, i915_guc_log_level_set,
 			"%lld\n");
 
+static int i915_guc_log_relay_open(struct inode *inode, struct file *file)
+{
+	struct drm_i915_private *dev_priv = inode->i_private;
+
+	if (!USES_GUC(dev_priv))
+		return -ENODEV;
+
+	file->private_data = dev_priv;
+
+	return intel_guc_log_relay_open(&dev_priv->guc);
+}
+
+static ssize_t
+i915_guc_log_relay_write(struct file *filp,
+			 const char __user *ubuf,
+			 size_t cnt,
+			 loff_t *ppos)
+{
+	struct drm_i915_private *dev_priv = filp->private_data;
+
+	intel_guc_log_relay_flush(&dev_priv->guc);
+
+	return cnt;
+}
+
+static int i915_guc_log_relay_release(struct inode *inode, struct file *file)
+{
+	struct drm_i915_private *dev_priv = inode->i_private;
+
+	intel_guc_log_relay_close(&dev_priv->guc);
+
+	return 0;
+}
+
+static const struct file_operations i915_guc_log_relay_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_guc_log_relay_open,
+	.write = i915_guc_log_relay_write,
+	.release = i915_guc_log_relay_release,
+};
+
 static const char *psr2_live_status(u32 val)
 {
 	static const char * const live_status[] = {
@@ -4761,7 +4802,8 @@  static const struct i915_debugfs_files {
 	{"i915_dp_test_data", &i915_displayport_test_data_fops},
 	{"i915_dp_test_type", &i915_displayport_test_type_fops},
 	{"i915_dp_test_active", &i915_displayport_test_active_fops},
-	{"i915_guc_log_control", &i915_guc_log_control_fops},
+	{"i915_guc_log_level", &i915_guc_log_level_fops},
+	{"i915_guc_log_relay", &i915_guc_log_relay_fops},
 	{"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops},
 	{"i915_ipc_status", &i915_ipc_status_fops},
 	{"i915_drrs_ctl", &i915_drrs_ctl_fops}
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 719b2be73292..7ebfe7525b65 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1237,7 +1237,6 @@  static void i915_driver_register(struct drm_i915_private *dev_priv)
 	/* Reveal our presence to userspace */
 	if (drm_dev_register(dev, 0) == 0) {
 		i915_debugfs_register(dev_priv);
-		intel_uc_log_register(dev_priv);
 		i915_setup_sysfs(dev_priv);
 
 		/* Depends on sysfs having been initialized */
@@ -1297,7 +1296,6 @@  static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 	i915_pmu_unregister(dev_priv);
 
 	i915_teardown_sysfs(dev_priv);
-	intel_uc_log_unregister(dev_priv);
 	drm_dev_unregister(&dev_priv->drm);
 
 	i915_gem_shrinker_unregister(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 0800c5317510..4dee65692f5f 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -508,7 +508,7 @@  void intel_guc_log_destroy(struct intel_guc *guc)
 	i915_vma_unpin_and_release(&guc->log.vma);
 }
 
-int intel_guc_log_control_get(struct intel_guc *guc)
+int intel_guc_log_level_get(struct intel_guc *guc)
 {
 	GEM_BUG_ON(!guc->log.vma);
 	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
@@ -518,7 +518,7 @@  int intel_guc_log_control_get(struct intel_guc *guc)
 
 #define GUC_LOG_IS_ENABLED(x)		(x > 0)
 #define GUC_LOG_LEVEL_TO_VERBOSITY(x)	(GUC_LOG_IS_ENABLED(x) ? x - 1 : 0)
-int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
+int intel_guc_log_level_set(struct intel_guc *guc, u64 val)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	int ret;
@@ -535,7 +535,7 @@  int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
 	    GUC_LOG_LEVEL_TO_VERBOSITY(val) > GUC_LOG_VERBOSITY_MAX)
 		return -EINVAL;
 
-	mutex_lock(&dev_priv->drm.struct_mutex);
+	mutex_lock(&guc->log.runtime.lock);
 
 	if (i915_modparams.guc_log_level == val) {
 		ret = 0;
@@ -551,93 +551,81 @@  int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
 
 	i915_modparams.guc_log_level = val;
 
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-
-	if (GUC_LOG_IS_ENABLED(val) && !guc_log_has_runtime(guc)) {
-		ret = intel_guc_log_register(guc);
-		if (ret) {
-			/* logging will remain off */
-			i915_modparams.guc_log_level = 0;
-			goto out;
-		}
-	} else if (!GUC_LOG_IS_ENABLED(val) && guc_log_has_runtime(guc)) {
-		intel_guc_log_unregister(guc);
-	}
-
-	return 0;
-
 out_unlock:
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-out:
+	mutex_unlock(&guc->log.runtime.lock);
+
 	return ret;
 }
 
-int intel_guc_log_register(struct intel_guc *guc)
+int intel_guc_log_relay_open(struct intel_guc *guc)
 {
 	int ret;
 
 	mutex_lock(&guc->log.runtime.lock);
 
-	GEM_BUG_ON(guc_log_has_runtime(guc));
+	if (guc_log_has_runtime(guc)) {
+		ret = -EEXIST;
+		goto out_unlock;
+	}
 
-	/*
-	 * 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 = guc_log_relay_create(guc);
 	if (ret)
-		goto err;
+		goto out_unlock;
 
 	ret = guc_log_map(guc);
 	if (ret)
-		goto err_relay;
+		goto out_relay;
+
+	mutex_unlock(&guc->log.runtime.lock);
 
 	guc_log_flush_irq_enable(guc);
 
-	mutex_unlock(&guc->log.runtime.lock);
+	/*
+	 * When GuC is logging without us relaying to userspace, we're ignoring
+	 * the flush notification. This means that we need to unconditionally
+	 * flush on relay enabling, since GuC only notifies us once.
+	 */
+	queue_work(guc->log.runtime.flush_wq, &guc->log.runtime.flush_work);
 
 	return 0;
 
-err_relay:
+out_relay:
 	guc_log_relay_destroy(guc);
-err:
+out_unlock:
 	mutex_unlock(&guc->log.runtime.lock);
 
 	return ret;
 }
 
-void intel_guc_log_unregister(struct intel_guc *guc)
+void intel_guc_log_relay_flush(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
-	guc_log_flush_irq_disable(guc);
-
 	/*
 	 * Before initiating the forceful flush, wait for any pending/ongoing
 	 * flush to complete otherwise forceful flush may not actually happen.
 	 */
 	flush_work(&guc->log.runtime.flush_work);
 
-	/*
-	 * Once logging is disabled, GuC won't generate logs & send an
-	 * interrupt. But there could be some data in the log buffer
-	 * which is yet to be captured. So request GuC to update the log
-	 * buffer state and then collect the left over logs.
-	 */
 	intel_runtime_pm_get(dev_priv);
 	guc_log_flush(guc);
 	intel_runtime_pm_put(dev_priv);
 
+	mutex_lock(&guc->log.runtime.lock);
 	/* GuC would have updated log buffer by now, so capture it */
 	guc_log_capture_logs(guc);
+	mutex_unlock(&guc->log.runtime.lock);
+}
 
-	mutex_lock(&guc->log.runtime.lock);
-
+void intel_guc_log_relay_close(struct intel_guc *guc)
+{
 	GEM_BUG_ON(!guc_log_has_runtime(guc));
 
+	guc_log_flush_irq_disable(guc);
+	flush_work(&guc->log.runtime.flush_work);
+
+	mutex_lock(&guc->log.runtime.lock);
 	guc_log_unmap(guc);
 	guc_log_relay_destroy(guc);
-
 	mutex_unlock(&guc->log.runtime.lock);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index 8c26cce77a98..df91f12a36ed 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -61,9 +61,10 @@  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_control_get(struct intel_guc *guc);
-int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val);
-int intel_guc_log_register(struct intel_guc *guc);
-void intel_guc_log_unregister(struct intel_guc *guc);
+int intel_guc_log_level_get(struct intel_guc *guc);
+int intel_guc_log_level_set(struct intel_guc *guc, u64 control_val);
+int intel_guc_log_relay_open(struct intel_guc *guc);
+void intel_guc_log_relay_close(struct intel_guc *guc);
+void intel_guc_log_relay_flush(struct intel_guc *guc);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 55a9b5b673e0..27e1f4c43b7b 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -219,28 +219,6 @@  static void guc_free_load_err_log(struct intel_guc *guc)
 		i915_gem_object_put(guc->load_err_log);
 }
 
-int intel_uc_log_register(struct drm_i915_private *dev_priv)
-{
-	int ret = 0;
-
-	if (!USES_GUC(dev_priv))
-		return 0;
-
-	if (i915_modparams.guc_log_level)
-		ret = intel_guc_log_register(&dev_priv->guc);
-
-	return ret;
-}
-
-void intel_uc_log_unregister(struct drm_i915_private *dev_priv)
-{
-	if (!USES_GUC(dev_priv))
-		return;
-
-	if (i915_modparams.guc_log_level)
-		intel_guc_log_unregister(&dev_priv->guc);
-}
-
 static int guc_enable_communication(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 9bc22103396d..8a7249722ef1 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -31,8 +31,6 @@ 
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_uc_init_mmio(struct drm_i915_private *dev_priv);
-int intel_uc_log_register(struct drm_i915_private *dev_priv);
-void intel_uc_log_unregister(struct drm_i915_private *dev_priv);
 void intel_uc_init_fw(struct drm_i915_private *dev_priv);
 void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
 int intel_uc_init_wq(struct drm_i915_private *dev_priv);