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

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

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@intel.com wrote:
>>>>> From: Sagar Arun Kamble <sagar.a.kamble@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@intel.com>
>>>>> Signed-off-by: Akash Goel <akash.goel@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?

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

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?

Is it too late to change that? :)

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx