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

[Tvrtko Ursulin]

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:
>>>>>>>>> +        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.

I see.

Would it work, for time being at least, to set i915.guc_log_level to -1 
when logging is disabled via debugfs?

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.