On 7/20/2016 4:10 PM, Tvrtko Ursulin wrote:
On 20/07/16 11:12, Goel, Akash wrote:
On 7/20/2016 3:17 PM, Tvrtko Ursulin wrote:
+ 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 ?
The verbosity value will be same as guc_log_level. But whether logging
on GuC side is currently enabled or disabled can't be inferred (it could
have been disabled at run time).
So will have to store the exact value written by User.
That's what I meant. Code currently seem to decompose the value written
via debugfs and store it in i915.guc_log_level:
0x00 = -1
0x10 = -1
...
0x01 = 0
0x11 = 1
0x21 = 2
0x31 = 3
...
So for readback you could translate back from i915.guc_log_level to the
debugfs format.
Sorry for all the mess.
Should I add a new field 'debugfs_ctrl_val' in guc structure, to store
the value previously written to debugfs file, considering guc_log_level
only gives an indication of the verbosity level ?
Actually in future there may be other additions also to the value
written to guc_log_control debugfs, have right now exposed only logging
& verbosity level controls to User, as they are deemed most useful right
now.
But there are some other controls also which can be passed to GuC
firmware through UK_LOG_ENABLE_LOGGING host2guc action.
Best regards
Akash
Although I have suggested below even more...
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?
... that why do we have to have two formats? Isn't that a bit confusing?
Why couldn't we use the same integer values from i915.guc_log_level for
debugfs control ?
And I forgot, i915.guc_log_level == 0 is logging enabled with minimum
verbosity?
i915.guc_log_level == 0 just indicates the minimum verbosity. But
logging could still be disabled on GuC side.
Yes, I can't remember any precedent where zero means enabled so it is
just weird. But it is too late to change it now. :(
For example, Driver boots with 'i915.guc_log_level = 0' so logging is
enabled, later User disables the logging by echoing 0x0 on the
guc_log_control debugfs file.
That's fine
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
