On 20/07/16 05:42, Goel, Akash wrote:
On 7/19/2016 4:54 PM, Tvrtko Ursulin wrote:
On 10/07/16 14:41, 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.
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 | 32 ++++++++++++++++-
drivers/gpu/drm/i915/i915_guc_submission.c | 57
++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_guc.h | 1 +
3 files changed, 89 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 5e35565..3c9c7f7 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2644,6 +2644,35 @@ static int i915_guc_log_dump(struct seq_file
*m, void *data)
return 0;
}
+static int
+i915_guc_log_control_set(void *data, u64 val)
+{
+ struct drm_device *dev = data;
+ struct drm_i915_private *dev_priv = dev->dev_private;
to_i915 should be used.
Sorry for missing this, need to use this at other places also.
+ int ret;
+
+ ret = mutex_lock_interruptible(&dev->struct_mutex);
+ if (ret)
+ return ret;
+
+ if (!i915.enable_guc_submission || !dev_priv->guc.log.obj) {
Wouldn't guc.log.obj be enough?
Actually failure in allocation of log buffer, at boot time, is not
considered fatal and submission through GuC is still done.
So i915.enable_guc_submission could be 1 with guc.log.obj as NULL.
If guc.log.obj is NULL it will return -EINVAL without trying to create
it here. If you intended for this function to try and create the log
object if not already present, via i915_guc_log_control, in that case
the condition above should only be if (!i915.enable_guc_submisison), no?
+ ret = -EINVAL;
+ goto end;
+ }
+
+ intel_runtime_pm_get(dev_priv);
+ ret = i915_guc_log_control(dev, val);
+ intel_runtime_pm_put(dev_priv);
+
+end:
+ mutex_unlock(&dev->struct_mutex);
+ return ret;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
+ NULL, i915_guc_log_control_set,
+ "0x%08llx\n");
Does the readback still work with no get method?
readback will give a 'Permission denied' error
Is that what we want? I think it would be nice to allow read-back unless
there is a specific reason why it shouldn't be allowed.
+
static int i915_edp_psr_status(struct seq_file *m, void *data)
{
struct drm_info_node *node = m->private;
@@ -5464,7 +5493,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 8cc31c6..2e3b723 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -193,6 +193,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
*
@@ -1455,3 +1465,50 @@ void i915_guc_register(struct drm_device *dev)
guc_log_late_setup(dev);
mutex_unlock(&dev->struct_mutex);
}
+
+int i915_guc_log_control(struct drm_device *dev, uint64_t control_val)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
to_i915
Actually, function should take dev_priv if not even guc depending on the
established convention in the file.
Ok for all the new logging related exported functions, will use dev_priv.
Or intel_guc where applicable, please look in guc code to see what is
mostly used. There is also guc_to_i915 helper or something.
+ union guc_log_control log_param;
+ int ret;
+
+ log_param.logging_enabled = control_val & 0x1;
+ log_param.verbosity = (control_val >> 4) & 0xF;
+
+ 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 -EINVAL;
Hm, disabling while already disabled - why should that return an error?
Might be annoying in scripts.
Just to make the User aware. Ok will suppress this and return 0.
Good, because it would be really annoying since you don't implement
readback as well. For example:
echo 0x0 > guc_logging_control
= -EINVAL
"What's wrong? What's the current status?"
cat guc_logging_control
= -EACESS (or whatever)
"What?!?"
:)
+
+ ret = host2guc_logging_control(&dev_priv->guc, log_param.value);
+ if (ret < 0) {
+ DRM_DEBUG_DRIVER("host2guc action failed\n");
Add ret to the log since it is easy?
fine will do that.
+ return ret;
+ }
+
+ i915.guc_log_level = log_param.verbosity;
+
+ /* 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);
+ if (!ret)
+ gen9_enable_guc_interrupts(dev_priv);
Hm, look at the above and below, do we need to create the relay channel
if logging_enabled == false ?
Can come here only if logging is enabled, by the virtue of above check,
/* This combination doesn't make sense & won't have any effect */
if (!log_param.logging_enabled && (i915.guc_log_level < 0))
return -EINVAL;
When guc_log_level < 0, first write on this file by User should be to
enable logging.
Okay just make sure that the relay channel is not created on repeated
disabling.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
