drm/i915/guc: Tidy guc_log_control

We plan to decouple log runtime (mapping + relay) from verbosity control.
Let's tidy the code now to reduce the churn in the following patches.

v2: Tidy macros, keep debug messages, use helper var for enable,
    correct typo (Michał)
    Fix incorrect input validaction (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: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@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/20180308154707.21716-1-michal.winiarski@intel.com

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 34d1252..c4cc8fe 100644 (file)
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2499,13 +2499,10 @@ static int i915_guc_log_control_get(void *data, u64 *val)
 {
        struct drm_i915_private *dev_priv = data;
 
-       if (!HAS_GUC(dev_priv))
+       if (!USES_GUC(dev_priv))
                return -ENODEV;
 
-       if (!dev_priv->guc.log.vma)
-               return -EINVAL;
-
-       *val = i915_modparams.guc_log_level;
+       *val = intel_guc_log_control_get(&dev_priv->guc);
 
        return 0;
 }
@@ -2514,10 +2511,10 @@ static int i915_guc_log_control_set(void *data, u64 val)
 {
        struct drm_i915_private *dev_priv = data;
 
-       if (!HAS_GUC(dev_priv))
+       if (!USES_GUC(dev_priv))
                return -ENODEV;
 
-       return intel_guc_log_control(&dev_priv->guc, val);
+       return intel_guc_log_control_set(&dev_priv->guc, val);
 }
 
 DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index c0c2e7d..7e59fb0 100644 (file)
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -659,51 +659,63 @@ void intel_guc_log_destroy(struct intel_guc *guc)
        i915_vma_unpin_and_release(&guc->log.vma);
 }
 
-int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
+int intel_guc_log_control_get(struct intel_guc *guc)
+{
+       GEM_BUG_ON(!guc->log.vma);
+       GEM_BUG_ON(i915_modparams.guc_log_level < 0);
+
+       return i915_modparams.guc_log_level;
+}
+
+#define GUC_LOG_LEVEL_DISABLED         0
+#define LOG_LEVEL_TO_ENABLED(x)                ((x) > 0)
+#define LOG_LEVEL_TO_VERBOSITY(x) ({           \
+       typeof(x) _x = (x);                     \
+       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 *guc, u64 val)
 {
        struct drm_i915_private *dev_priv = guc_to_i915(guc);
-       bool enable_logging = control_val > 0;
-       u32 verbosity;
+       bool enabled = LOG_LEVEL_TO_ENABLED(val);
        int ret;
 
-       if (!guc->log.vma)
-               return -ENODEV;
+       BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0);
+       GEM_BUG_ON(!guc->log.vma);
+       GEM_BUG_ON(i915_modparams.guc_log_level < 0);
 
-       BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN);
-       if (control_val > 1 + GUC_LOG_VERBOSITY_MAX)
+       /*
+        * GuC is recognizing log levels starting from 0 to max, we're using 0
+        * as indication that logging should be disabled.
+        */
+       if (val < GUC_LOG_LEVEL_DISABLED ||
+           val > VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX))
                return -EINVAL;
 
-       /* This combination doesn't make sense & won't have any effect */
-       if (!enable_logging && !i915_modparams.guc_log_level)
-               return 0;
+       mutex_lock(&dev_priv->drm.struct_mutex);
 
-       verbosity = enable_logging ? control_val - 1 : 0;
+       if (i915_modparams.guc_log_level == val) {
+               ret = 0;
+               goto out_unlock;
+       }
 
-       ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
-       if (ret)
-               return ret;
        intel_runtime_pm_get(dev_priv);
-       ret = guc_log_control(guc, enable_logging, verbosity);
+       ret = guc_log_control(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val));
        intel_runtime_pm_put(dev_priv);
-       mutex_unlock(&dev_priv->drm.struct_mutex);
-
-       if (ret < 0) {
-               DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
-               return ret;
+       if (ret) {
+               DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret);
+               goto out_unlock;
        }
 
-       if (enable_logging) {
-               i915_modparams.guc_log_level = 1 + verbosity;
+       i915_modparams.guc_log_level = val;
 
-               /*
-                * If log was disabled at boot time, then the relay channel file
-                * wouldn't have been created by now and interrupts also would
-                * not have been enabled. Try again now, just in case.
-                */
+       mutex_unlock(&dev_priv->drm.struct_mutex);
+
+       if (enabled && !guc_log_has_runtime(guc)) {
                ret = guc_log_late_setup(guc);
-               if (ret < 0) {
+               if (ret) {
                        DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
-                       return ret;
+                       goto out;
                }
 
                /* GuC logging is currently the only user of Guc2Host interrupts */
@@ -712,7 +724,7 @@ int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
                gen9_enable_guc_interrupts(dev_priv);
                intel_runtime_pm_put(dev_priv);
                mutex_unlock(&dev_priv->drm.struct_mutex);
-       } else {
+       } else if (!enabled && guc_log_has_runtime(guc)) {
                /*
                 * Once logging is disabled, GuC won't generate logs & send an
                 * interrupt. But there could be some data in the log buffer
@@ -720,11 +732,13 @@ int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
                 * buffer state and then collect the left over logs.
                 */
                guc_flush_logs(guc);
-
-               /* As logging is disabled, update log level to reflect that */
-               i915_modparams.guc_log_level = 0;
        }
 
+       return 0;
+
+out_unlock:
+       mutex_unlock(&dev_priv->drm.struct_mutex);
+out:
        return ret;
 }
 

diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index dab0e94..141ce9c 100644 (file)
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -64,7 +64,8 @@ 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);
-int intel_guc_log_control(struct intel_guc *guc, u64 control_val);
+int intel_guc_log_control_get(struct intel_guc *guc);
+int intel_guc_log_control_set(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);