drm/i915/guc: Split relay control and GuC log level

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.

v2: Don't touch guc_log_level on relay creation error, adjust locking
    after rebase, s/dev_priv/i915, pass guc to file->private_data (Sagar)
    Use struct_mutex rather than runtime.lock for set_log_level
v3: Tidy ordering of definitions (Sagar)

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Link: https://patchwork.freedesktop.org/patch/msgid/20180319095348.9716-5-michal.winiarski@intel.com

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5378863e32381ec873f7d5ae3354c2c7be84d0cf..e857a9493b6f67af9b3d06f8cca079a5a7812233 100644 (file)
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2495,32 +2495,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.log);
+       *val = intel_guc_log_level_get(&dev_priv->guc.log);
 
        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.log, val);
+       return intel_guc_log_level_set(&dev_priv->guc.log, 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->guc.log;
+
+       return intel_guc_log_relay_open(&dev_priv->guc.log);
+}
+
+static ssize_t
+i915_guc_log_relay_write(struct file *filp,
+                        const char __user *ubuf,
+                        size_t cnt,
+                        loff_t *ppos)
+{
+       struct intel_guc_log *log = filp->private_data;
+
+       intel_guc_log_relay_flush(log);
+
+       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.log);
+
+       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[] = {
@@ -4748,7 +4789,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 3df5193487f3f22ae085856fc4bb9e4dc6afb9aa..1021bf40e236c954281c294c28a6a428597a46f4 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1239,9 +1239,6 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
                i915_debugfs_register(dev_priv);
                i915_setup_sysfs(dev_priv);
 
-               /* Depends on debugfs having been initialized */
-               intel_uc_register(dev_priv);
-
                /* Depends on sysfs having been initialized */
                i915_perf_register(dev_priv);
        } else
@@ -1299,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_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 72a71bc94adf74ad90edb359c9b6c9108dcde5f3..20254dde172cc5f3c86b3a38afe1d26c9766d37c 100644 (file)
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -423,18 +423,13 @@ static int guc_log_relay_create(struct intel_guc_log *log)
                DRM_ERROR("Couldn't create relay chan for GuC logging\n");
 
                ret = -ENOMEM;
-               goto err;
+               return ret;
        }
 
        GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
        log->runtime.relay_chan = guc_log_relay_chan;
 
        return 0;
-
-err:
-       /* logging will be off */
-       i915_modparams.guc_log_level = 0;
-       return ret;
 }
 
 static void guc_log_relay_destroy(struct intel_guc_log *log)
@@ -511,7 +506,7 @@ void intel_guc_log_destroy(struct intel_guc_log *log)
        i915_vma_unpin_and_release(&log->vma);
 }
 
-int intel_guc_log_control_get(struct intel_guc_log *log)
+int intel_guc_log_level_get(struct intel_guc_log *log)
 {
        GEM_BUG_ON(!log->vma);
        GEM_BUG_ON(i915_modparams.guc_log_level < 0);
@@ -526,11 +521,10 @@ int intel_guc_log_control_get(struct intel_guc_log *log)
        LOG_LEVEL_TO_ENABLED(_x) ? _x - 1 : 0;  \
 })
 #define VERBOSITY_TO_LOG_LEVEL(x)  ((x) + 1)
-int intel_guc_log_control_set(struct intel_guc_log *log, u64 val)
+int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
 {
        struct intel_guc *guc = log_to_guc(log);
        struct drm_i915_private *dev_priv = guc_to_i915(guc);
-       bool enabled = LOG_LEVEL_TO_ENABLED(val);
        int ret;
 
        BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0);
@@ -553,7 +547,8 @@ int intel_guc_log_control_set(struct intel_guc_log *log, u64 val)
        }
 
        intel_runtime_pm_get(dev_priv);
-       ret = guc_log_control(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val));
+       ret = guc_log_control(guc, LOG_LEVEL_TO_ENABLED(val),
+                             LOG_LEVEL_TO_VERBOSITY(val));
        intel_runtime_pm_put(dev_priv);
        if (ret) {
                DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret);
@@ -562,89 +557,79 @@ int intel_guc_log_control_set(struct intel_guc_log *log, u64 val)
 
        i915_modparams.guc_log_level = val;
 
-       mutex_unlock(&dev_priv->drm.struct_mutex);
-
-       if (enabled && !guc_log_has_runtime(log)) {
-               ret = intel_guc_log_register(log);
-               if (ret) {
-                       /* logging will remain off */
-                       i915_modparams.guc_log_level = 0;
-                       goto out;
-               }
-       } else if (!enabled && guc_log_has_runtime(log)) {
-               intel_guc_log_unregister(log);
-       }
-
-       return 0;
-
 out_unlock:
        mutex_unlock(&dev_priv->drm.struct_mutex);
-out:
+
        return ret;
 }
 
-int intel_guc_log_register(struct intel_guc_log *log)
+int intel_guc_log_relay_open(struct intel_guc_log *log)
 {
        int ret;
 
        mutex_lock(&log->runtime.lock);
 
-       GEM_BUG_ON(guc_log_has_runtime(log));
+       if (guc_log_has_runtime(log)) {
+               ret = -EEXIST;
+               goto out_unlock;
+       }
 
        ret = guc_log_relay_create(log);
        if (ret)
-               goto err;
+               goto out_unlock;
 
        ret = guc_log_map(log);
        if (ret)
-               goto err_relay;
+               goto out_relay;
+
+       mutex_unlock(&log->runtime.lock);
 
        guc_flush_log_msg_enable(log_to_guc(log));
 
-       mutex_unlock(&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(log->runtime.flush_wq, &log->runtime.flush_work);
 
        return 0;
 
-err_relay:
+out_relay:
        guc_log_relay_destroy(log);
-err:
+out_unlock:
        mutex_unlock(&log->runtime.lock);
 
        return ret;
 }
 
-void intel_guc_log_unregister(struct intel_guc_log *log)
+void intel_guc_log_relay_flush(struct intel_guc_log *log)
 {
        struct intel_guc *guc = log_to_guc(log);
        struct drm_i915_private *i915 = guc_to_i915(guc);
 
-       guc_flush_log_msg_disable(guc);
-
        /*
         * Before initiating the forceful flush, wait for any pending/ongoing
         * flush to complete otherwise forceful flush may not actually happen.
         */
        flush_work(&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(i915);
        guc_log_flush(guc);
        intel_runtime_pm_put(i915);
 
        /* GuC would have updated log buffer by now, so capture it */
        guc_log_capture_logs(log);
+}
 
-       mutex_lock(&log->runtime.lock);
+void intel_guc_log_relay_close(struct intel_guc_log *log)
+{
+       guc_flush_log_msg_disable(log_to_guc(log));
+       flush_work(&log->runtime.flush_work);
 
+       mutex_lock(&log->runtime.lock);
        GEM_BUG_ON(!guc_log_has_runtime(log));
-
        guc_log_unmap(log);
        guc_log_relay_destroy(log);
-
        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 e0ea625032fbd27ca98bb40c456e5113d2ada0f7..3cf911eef3a8ddda35aa3f662b3e0dfaa97d9720 100644 (file)
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -61,11 +61,12 @@ struct intel_guc_log {
 
 void intel_guc_log_init_early(struct intel_guc_log *log);
 int intel_guc_log_create(struct intel_guc_log *log);
-int intel_guc_log_register(struct intel_guc_log *log);
-void intel_guc_log_unregister(struct intel_guc_log *log);
 void intel_guc_log_destroy(struct intel_guc_log *log);
 
-int intel_guc_log_control_get(struct intel_guc_log *log);
-int intel_guc_log_control_set(struct intel_guc_log *log, u64 control);
+int intel_guc_log_level_get(struct intel_guc_log *log);
+int intel_guc_log_level_set(struct intel_guc_log *log, u64 control_val);
+int intel_guc_log_relay_open(struct intel_guc_log *log);
+void intel_guc_log_relay_flush(struct intel_guc_log *log);
+void intel_guc_log_relay_close(struct intel_guc_log *log);
 
 #endif

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 765b86a53f19bb002d411c95dac3f67b3fbcda37..9bb40cd047a001d0668e6d68211ac0635d4668f5 100644 (file)
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -221,28 +221,6 @@ static void guc_free_load_err_log(struct intel_guc *guc)
                i915_gem_object_put(guc->load_err_log);
 }
 
-int intel_uc_register(struct drm_i915_private *i915)
-{
-       int ret = 0;
-
-       if (!USES_GUC(i915))
-               return 0;
-
-       if (i915_modparams.guc_log_level)
-               ret = intel_guc_log_register(&i915->guc.log);
-
-       return ret;
-}
-
-void intel_uc_unregister(struct drm_i915_private *i915)
-{
-       if (!USES_GUC(i915))
-               return;
-
-       if (i915_modparams.guc_log_level)
-               intel_guc_log_unregister(&i915->guc.log);
-}
-
 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 0a2b413e9cd087ffd7868f509670fb3223eb3a58..937e6117525884bc229015503371578e6a123713 100644 (file)
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -30,8 +30,6 @@
 
 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_register(struct drm_i915_private *dev_priv);
-void intel_uc_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_misc(struct drm_i915_private *dev_priv);