On 8/12/2016 9:27 PM, Tvrtko Ursulin wrote:
On 12/08/16 07:25, akash.goel@xxxxxxxxx wrote:
From: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
This patch provides debugfs interface i915_guc_output_control for
on the fly enabling/disabling of logging in GuC firmware and controlling
the verbosity level of logs.
The value written to the file, should have bit 0 set to enable logging
and
bits 4-7 should contain the verbosity info.
v2: Add a forceful flush, to collect left over logs, on disabling
logging.
Useful for Validation.
v3: Besides minor cleanup, implement read method for the debugfs file and
set the guc_log_level to -1 when logging is disabled. (Tvrtko)
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
Signed-off-by: Akash Goel <akash.goel@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_debugfs.c | 44 ++++++++++++++++++++-
drivers/gpu/drm/i915/i915_guc_submission.c | 63
++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_guc.h | 1 +
3 files changed, 107 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 14e0dcf..f472fbcd3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2674,6 +2674,47 @@ 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)
+{
+ struct drm_device *dev = data;
+ struct drm_i915_private *dev_priv = to_i915(dev);
+
+ if (!dev_priv->guc.log.obj)
+ return -EINVAL;
+
+ *val = i915.guc_log_level;
+
+ return 0;
+}
+
+static int i915_guc_log_control_set(void *data, u64 val)
+{
+ struct drm_device *dev = data;
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ int ret;
+
+ ret = mutex_lock_interruptible(&dev->struct_mutex);
+ if (ret)
+ return ret;
+
+ if (!dev_priv->guc.log.obj) {
+ ret = -EINVAL;
+ goto end;
+ }
+
+ intel_runtime_pm_get(dev_priv);
+ ret = i915_guc_log_control(dev_priv, val);
+ intel_runtime_pm_put(dev_priv);
+
+end:
+ mutex_unlock(&dev->struct_mutex);
+ return ret;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
+ i915_guc_log_control_get, i915_guc_log_control_set,
+ "%lld\n");
+
static int i915_edp_psr_status(struct seq_file *m, void *data)
{
struct drm_info_node *node = m->private;
@@ -5477,7 +5518,8 @@ static const struct i915_debugfs_files {
{"i915_fbc_false_color", &i915_fbc_fc_fops},
{"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_dp_test_active", &i915_displayport_test_active_fops},
+ {"i915_guc_log_control", &i915_guc_log_control_fops}
};
void intel_display_crc_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 4a75c16..041cf68 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -195,6 +195,16 @@ static int host2guc_force_logbuffer_flush(struct
intel_guc *guc)
return host2guc_action(guc, data, 2);
}
+static int host2guc_logging_control(struct intel_guc *guc, u32
control_val)
+{
+ u32 data[2];
+
+ data[0] = HOST2GUC_ACTION_UK_LOG_ENABLE_LOGGING;
+ data[1] = control_val;
+
+ return host2guc_action(guc, data, 2);
+}
+
/*
* Initialise, update, or clear doorbell data shared with the GuC
*
@@ -1538,3 +1548,56 @@ void i915_guc_register(struct drm_i915_private
*dev_priv)
guc_log_late_setup(&dev_priv->guc);
mutex_unlock(&dev_priv->drm.struct_mutex);
}
+
+int i915_guc_log_control(struct drm_i915_private *dev_priv, u64
control_val)
+{
+ union guc_log_control log_param;
+ int ret;
+
+ log_param.logging_enabled = control_val & 0x1;
+ log_param.verbosity = (control_val >> 4) & 0xF;
Maybe "log_param.value = control_val" would also work since
guc_log_control is conveniently defined as an union. Doesn't matter though.
+
+ if (log_param.verbosity < GUC_LOG_VERBOSITY_MIN ||
+ log_param.verbosity > GUC_LOG_VERBOSITY_MAX)
+ return -EINVAL;
+
+ /* This combination doesn't make sense & won't have any effect */
+ if (!log_param.logging_enabled && (i915.guc_log_level < 0))
+ return 0;
I wonder if it would work and maybe look nicer to generalize as:
int guc_log_level;
guc_log_level = log_param.logging_enabled ? log_param.verbosity : -1;
if (i915.guc_log_level == guc_log_level)
return 0;
Fine, will try to refactor the code as per your suggestions.
Thanks for the suggestions.
+
+ ret = host2guc_logging_control(&dev_priv->guc, log_param.value);
+ if (ret < 0) {
+ DRM_DEBUG_DRIVER("host2guc action failed %d\n", ret);
+ return ret;
+ }
+
+ i915.guc_log_level = log_param.verbosity;
This would then become i915.guc_log_level = guc_log_level.
+
+ /* If log_level was set as -1 at boot time, then the relay
channel file
+ * wouldn't have been created by now and interrupts also would
not have
+ * been enabled.
+ */
+ if (!dev_priv->guc.log.relay_chan) {
+ ret = guc_log_late_setup(&dev_priv->guc);
+ if (!ret)
+ gen9_enable_guc_interrupts(dev_priv);
+ } else if (!log_param.logging_enabled) {
+ /* 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.
+ */
+ i915_guc_flush_logs(dev_priv);
+
+ /* GuC would have updated the log buffer by now, so capture
it */
+ i915_guc_capture_logs(dev_priv);
+
+ /* As logging is disabled, update the log level to reflect
that */
+ i915.guc_log_level = -1;
+ } else {
+ /* In case interrupts were disabled, enable them now */
+ gen9_enable_guc_interrupts(dev_priv);
+ }
And this block would need some adjustments with my guc_log_level idea.
Well not sure, see what you think. I am just attracted to the idea of
operating in the same value domain as much as possible for readability
and simplicity. Maybe it would not improve anything, I did not bother
with typing it all to check.
+
+ return ret;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h
b/drivers/gpu/drm/i915/intel_guc.h
index d3a5447..2f8c408 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -186,5 +186,6 @@ void i915_guc_capture_logs(struct drm_i915_private
*dev_priv);
void i915_guc_flush_logs(struct drm_i915_private *dev_priv);
void i915_guc_register(struct drm_i915_private *dev_priv);
void i915_guc_unregister(struct drm_i915_private *dev_priv);
+int i915_guc_log_control(struct drm_i915_private *dev_priv, u64
control_val);
#endif
Patch looks correct as is, so:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Although I would be happier though if my suggestion to use the same
value domain as for the module parameter was used. In other words:
{"i915_guc_log_level", &i915_guc_log_control_fops}
...
int i915_guc_log_control(struct drm_i915_private *dev_priv, u64
control_val)
...
int guc_log_level = (int)control_val;
...
log_param.logging_enabled = guc_log_level > -1;
log_param.verbosity = guc_log_level > -1 ? guc_log_level : 0;
...
It think it would be simpler for the user and developer to only have to
think about one set of values when dealing with guc logging.
Really nice suggestion, but as you mentioned below this log control
interface is most likely to get extended in near future.
Best regards
Akash
But maybe extensions to guc_log_control are imminent and expected so it
would not be worth it in the long run. No idea.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
