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

[Goel, Akash]

On 7/20/2016 3:17 PM, Tvrtko Ursulin wrote:
>
> On 20/07/16 10:32, Goel, Akash wrote:
>>
>>
>> On 7/20/2016 2:38 PM, Tvrtko Ursulin wrote:
>>>
>>> On 20/07/16 05:42, Goel, Akash wrote:
>>>> On 7/19/2016 4:54 PM, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 10/07/16 14:41, akash.goel at intel.com wrote:
>>>>>> From: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>>>>>>
>>>>>> This patch provides debugfs interface i915_guc_output_control for
>>>>>> on the fly enabling/disabling of logging in GuC firmware and
>>>>>> controlling
>>>>>> the verbosity level of logs.
>>>>>> The value written to the file, should have bit 0 set to enable
>>>>>> logging
>>>>>> and
>>>>>> bits 4-7 should contain the verbosity info.
>>>>>>
>>>>>> v2: Add a forceful flush, to collect left over logs, on disabling
>>>>>> logging.
>>>>>>      Useful for Validation.
>>>>>>
>>>>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>>>>>> Signed-off-by: Akash Goel <akash.goel at intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/i915_debugfs.c        | 32 ++++++++++++++++-
>>>>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 57
>>>>>> ++++++++++++++++++++++++++++++
>>>>>>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>>>>>>   3 files changed, 89 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>>> index 5e35565..3c9c7f7 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>>> @@ -2644,6 +2644,35 @@ static int i915_guc_log_dump(struct seq_file
>>>>>> *m, void *data)
>>>>>>       return 0;
>>>>>>   }
>>>>>>
>>>>>> +static int
>>>>>> +i915_guc_log_control_set(void *data, u64 val)
>>>>>> +{
>>>>>> +    struct drm_device *dev = data;
>>>>>> +    struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>
>>>>> to_i915 should be used.
>>>> Sorry for missing this, need to use this at other places also.
>>>>
>>>>>
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = mutex_lock_interruptible(&dev->struct_mutex);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    if (!i915.enable_guc_submission || !dev_priv->guc.log.obj) {
>>>>>
>>>>> Wouldn't guc.log.obj be enough?
>>>>
>>>> Actually failure in allocation of log buffer, at boot time, is not
>>>> considered fatal and submission through GuC is still done.
>>>> So i915.enable_guc_submission could be 1 with guc.log.obj as NULL.
>>>
>>> If guc.log.obj is NULL it will return -EINVAL without trying to create
>>> it here. If you intended for this function to try and create the log
>>> object if not already present, via i915_guc_log_control, in that case
>>> the condition above should only be if (!i915.enable_guc_submisison), no?
>>>
>> If guc.log.obj is found to be NULL, we consider logging can't be enabled
>> at run time. Allocation of log buffer is supposed to done
>> at boot time only, otherwise GuC would have to be reset & firmware to be
>> reloaded to pass the log buffer address at run time, which is probably
>> not desirable. That's why in the first patch decoupled the allocation of
>> log buffer from log_level value.
>
> Okay so why then the check above shouldn't just be;
>
>     if (!dev_priv->guc.log.obj)
>
> as I originally suggested?

Right, so sorry got confused, I misread & interpreted that you are 
suggesting to have !i915.enable_guc_submission check instead.

(!dev_priv->guc.log.obj) check should suffice.

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

> 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?
>
> 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.

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.

Best regards
Akash

> Is it too late to change that? :)
>
> Regards,
>
> Tvrtko
>