Re: [Intel-gfx] [PATCH 08/15] drm/i915/guc: Split relay control and GuC log level

2018-03-05 Thread Sagar Arun Kamble 



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 
Cc: Chris Wilson 
Cc: Daniele Ceraolo Spurio 
Cc: Sagar Arun Kamble 
Cc: Michal Wajdeczko 
---
  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(_priv->guc);

+   *val = intel_guc_log_level_get(_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(_priv->guc, val);

+   return intel_guc_log_level_set(_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 
_priv->guc?

+   return intel_guc_log_relay_open(_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(_priv->guc);
+
+   return cnt;
+}



+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 

+   guc_log_flush_irq_disable(guc);
+   flush_work(>log.runtime.flush_work);
+
+   mutex_lock(>log.runtime.lock);
guc_log_unmap(guc);
guc_log_relay_destroy(guc);
-
mutex_unlock(>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);
  }

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

2018-02-27 Thread Michał Winiarski 
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 
Cc: Chris Wilson 
Cc: Daniele Ceraolo Spurio 
Cc: Sagar Arun Kamble 
Cc: Michal Wajdeczko 
---
 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;
 
if (!USES_GUC(dev_priv))
return -ENODEV;
 
-   *val = intel_guc_log_control_get(_priv->guc);
+   *val = intel_guc_log_level_get(_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(_priv->guc, val);
+   return intel_guc_log_level_set(_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(_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(_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(_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", _displayport_test_data_fops},
{"i915_dp_test_type", _displayport_test_type_fops},
{"i915_dp_test_active", _displayport_test_active_fops},
-   {"i915_guc_log_control", _guc_log_control_fops},
+   {"i915_guc_log_level", _guc_log_level_fops},
+   {"i915_guc_log_relay", _guc_log_relay_fops},
{"i915_hpd_storm_ctl", _hpd_storm_ctl_fops},
{"i915_ipc_status", _ipc_status_fops},
{"i915_drrs_ctl", _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(_priv->drm);