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, [email protected] wrote:From: Sagar Arun Kamble <[email protected]> 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 <[email protected]> Signed-off-by: Akash Goel <[email protected]> --- 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' errorIs 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
