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

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

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?

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

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

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

Regards,

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