On 20/07/16 10:32, Goel, Akash wrote:
On 7/20/2016 2:38 PM, Tvrtko Ursulin wrote:
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?
If guc.log.obj is found to be NULL, we consider logging can't be enabled
at run time. Allocation of log buffer is supposed to done
at boot time only, otherwise GuC would have to be reset & firmware to be
reloaded to pass the log buffer address at run time, which is probably
not desirable. That's why in the first patch decoupled the allocation of
log buffer from log_level value.
Okay so why then the check above shouldn't just be;
if (!dev_priv->guc.log.obj)
as I originally suggested?
+ 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.
Ok can implement a dummy read back function but what should be
shown/returned on read.
Should I show/return the guc_log_level value (which is also available
from /sys/module/i915/parameters/) ?
I would return the same value that was written in. Is the problem that
it is not stored anywhere? Maybe reconstruct it from
i915.guc_log_level ?
Although it is not ideal that we got two formats for the same thing.
Thinking about that, why not use the same format in debugfs as for the
module param?
And I forgot, i915.guc_log_level == 0 is logging enabled with minimum
verbosity?
Is it too late to change that? :)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
