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

[Goel, Akash]

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