On Tue, 27 Feb 2018 13:52:16 +0100, Michał Winiarski
<michal.winiarski@xxxxxxxxx> wrote:
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.
Signed-off-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
---
/snip/
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -657,52 +657,55 @@ 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)
Can we add kernel-doc for this new function?
+{
+ GEM_BUG_ON(!guc->log.vma);
+ GEM_BUG_ON(i915_modparams.guc_log_level < 0);
+
+ return i915_modparams.guc_log_level;
+}
+
+#define GUC_LOG_IS_ENABLED(x) (x > 0)
1) x must be wrapped into (x)
2) GUC_LOG_ prefix is misleading, maybe LOG_LEVEL_TO_ENABLED()
+#define GUC_LOG_LEVEL_TO_VERBOSITY(x) (GUC_LOG_IS_ENABLED(x) ? x - 1 :
0)
1) x must be wrapped into (x)
2) try to avoid double evaluation of x
+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;
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 disablded.
typo
+ */
+ if (GUC_LOG_LEVEL_TO_VERBOSITY(val) < GUC_LOG_VERBOSITY_MIN ||
+ GUC_LOG_LEVEL_TO_VERBOSITY(val) > GUC_LOG_VERBOSITY_MAX)
try to use helper variable easier read:
int verbosity = LOG_LEVEL_TO_VERBOSITY(val);
if (verbosity < GUC_LOG_VERBOSITY_MIN || verbosity > 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, GUC_LOG_IS_ENABLED(val),
+ GUC_LOG_LEVEL_TO_VERBOSITY(val));
intel_runtime_pm_put(dev_priv);
- mutex_unlock(&dev_priv->drm.struct_mutex);
+ if (ret)
+ goto out_unlock;
- if (ret < 0) {
- DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
- return ret;
- }
+ i915_modparams.guc_log_level = val;
- if (enable_logging) {
- i915_modparams.guc_log_level = 1 + verbosity;
+ mutex_unlock(&dev_priv->drm.struct_mutex);
- /*
- * 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.
- */
+ if (GUC_LOG_IS_ENABLED(val) && !guc_log_has_runtime(guc)) {
ret = guc_log_late_setup(guc);
- if (ret < 0) {
- DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
why do you want to remove all these diagnostic messages?
- return ret;
- }
+ if (ret)
+ goto out;
/* GuC logging is currently the only user of Guc2Host interrupts */
mutex_lock(&dev_priv->drm.struct_mutex);
@@ -710,7 +713,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 (!GUC_LOG_IS_ENABLED(val) && 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
@@ -718,11 +721,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 dab0e949567a..141ce9ca22ce 100644
--- 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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
