Re: [PATCH 09/17] drm/i915: Debugfs support for GuC logging control

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx