[Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control

[Goel, Akash]

On 7/20/2016 5:20 PM, Tvrtko Ursulin wrote:
>
> On 20/07/16 12:29, Goel, Akash wrote:
>> 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:

>>>>>>>>>> +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.
>
> I see.
>
> Would it work, for time being at least, to set i915.guc_log_level to -1
> when logging is disabled via debugfs?
>
Actually had thought about this, but didn't pursue since on doing so 
will have to adjust some of the guc_log_level related asserts/ conditions.
Will do it now as currently this looks to be the best alternative.
Thanks much for the inputs.

Best regards
Akash

> It think that also has the advantage of making the current guc logging
> state consistent when observed from the outside. Otherwise the debugfs
> value and module parameter may disagree on it, as you said before. Which
> is not that great I think.
>
> Apart from making the reported stated consistent, that way you could, at
> least for the time being, get away without storing a copy of
> guc_log_control but reconstruct it from the module parameter on read-back.
>
> Regards,
>
> Tvrtko
>
>
>
> You could avoid storing a copy of guc_log_control like that.
>
>
>