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

[Goel, Akash]

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.

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


>>>
>>>> +
>>>>   static int i915_edp_psr_status(struct seq_file *m, void *data)
>>>>   {
>>>>       struct drm_info_node *node = m->private;
>>>> @@ -5464,7 +5493,8 @@ static const struct i915_debugfs_files {
>>>>       {"i915_fbc_false_color", &i915_fbc_fc_fops},
>>>>       {"i915_dp_test_data", &i915_displayport_test_data_fops},
>>>>       {"i915_dp_test_type", &i915_displayport_test_type_fops},
>>>> -    {"i915_dp_test_active", &i915_displayport_test_active_fops}
>>>> +    {"i915_dp_test_active", &i915_displayport_test_active_fops},
>>>> +    {"i915_guc_log_control", &i915_guc_log_control_fops}
>>>>   };
>>>>
>>>>   void intel_display_crc_init(struct drm_device *dev)
>>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> index 8cc31c6..2e3b723 100644
>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> @@ -193,6 +193,16 @@ static int host2guc_force_logbuffer_flush(struct
>>>> intel_guc *guc)
>>>>       return host2guc_action(guc, data, 2);
>>>>   }
>>>>
>>>> +static int host2guc_logging_control(struct intel_guc *guc, u32
>>>> control_val)
>>>> +{
>>>> +    u32 data[2];
>>>> +
>>>> +    data[0] = HOST2GUC_ACTION_UK_LOG_ENABLE_LOGGING;
>>>> +    data[1] = control_val;
>>>> +
>>>> +    return host2guc_action(guc, data, 2);
>>>> +}
>>>> +
>>>>   /*
>>>>    * Initialise, update, or clear doorbell data shared with the GuC
>>>>    *
>>>> @@ -1455,3 +1465,50 @@ void i915_guc_register(struct drm_device *dev)
>>>>       guc_log_late_setup(dev);
>>>>       mutex_unlock(&dev->struct_mutex);
>>>>   }
>>>> +
>>>> +int i915_guc_log_control(struct drm_device *dev, uint64_t control_val)
>>>> +{
>>>> +    struct drm_i915_private *dev_priv = dev->dev_private;
>>>
>>> to_i915
>>>
>>> Actually, function should take dev_priv if not even guc depending on the
>>> established convention in the file.
>>>
>> Ok for all the new logging related exported functions, will use dev_priv.
>
> Or intel_guc where applicable, please look in guc code to see what is
> mostly used. There is also guc_to_i915 helper or something.
>
for functions exported from i915_guc_submission.c file, dev_priv has 
been used.

>>
>>>> +    union guc_log_control log_param;
>>>> +    int ret;
>>>> +
>>>> +    log_param.logging_enabled = control_val & 0x1;
>>>> +    log_param.verbosity = (control_val >> 4) & 0xF;
>>>> +
>>>> +    if (log_param.verbosity < GUC_LOG_VERBOSITY_MIN ||
>>>> +        log_param.verbosity > GUC_LOG_VERBOSITY_MAX)
>>>> +        return -EINVAL;
>>>> +
>>>> +    /* This combination doesn't make sense & won't have any effect */
>>>> +    if (!log_param.logging_enabled && (i915.guc_log_level < 0))
>>>> +        return -EINVAL;
>>>
>>> Hm, disabling while already disabled - why should that return an error?
>>> Might be annoying in scripts.
>>
>> Just to make the User aware. Ok will suppress this and return 0.
>
> Good, because it would be really annoying since you don't implement
> readback as well. For example:
>
> echo 0x0 > guc_logging_control
>
> = -EINVAL
>
> "What's wrong? What's the current status?"
>
> cat guc_logging_control
>
> = -EACESS (or whatever)
>
> "What?!?"
>
> :)
>
>>>
>>>> +
>>>> +    ret = host2guc_logging_control(&dev_priv->guc, log_param.value);
>>>> +    if (ret < 0) {
>>>> +        DRM_DEBUG_DRIVER("host2guc action failed\n");
>>>
>>> Add ret to the log since it is easy?
>>>
>> fine will do that.
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    i915.guc_log_level = log_param.verbosity;
>>>> +
>>>> +    /* If log_level was set as -1 at boot time, then the relay
>>>> channel file
>>>> +     * wouldn't have been created by now and interrupts also would
>>>> not have
>>>> +     * been enabled.
>>>> +     */
>>>> +    if (!dev_priv->guc.log.relay_chan) {
>>>> +        ret = guc_log_late_setup(dev);
>>>> +        if (!ret)
>>>> +            gen9_enable_guc_interrupts(dev_priv);
>>>
>>> Hm, look at the above and below, do we need to create the relay channel
>>> if logging_enabled == false ?
>>
>> Can come here only if logging is enabled, by the virtue of above check,
>>         /* This combination doesn't make sense & won't have any effect */
>>         if (!log_param.logging_enabled && (i915.guc_log_level < 0))
>>         return -EINVAL;
>>
>> When guc_log_level < 0, first write on this file by User should be to
>> enable logging.
>
> Okay just make sure that the relay channel is not created on repeated
> disabling.
>
Yes channel will be created only once, when logging is enabled for first 
time.

Best regards
Akash
> Regards,
>
> Tvrtko