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

["Goel, Akash" <akash.goel@intel.com>]

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.

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

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

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

Best regards
Akash

>
>> +    } else if (!log_param.logging_enabled) {
>> +        /* Once logging is disabled, GuC won't generate logs & send an
>> +         * interrupt. But there could be some data in the log buffer
>> +         * which is yet to be captured. So request GuC to update the log
>> +         * buffer state and send the flush interrupt so that Host can
>> +         * collect the left over logs also.
>> +         */
>> +        flush_work(&dev_priv->guc.events_work);
>> +        host2guc_force_logbuffer_flush(&dev_priv->guc);
>> +    }
>> +
>> +    return ret;
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index ed773b5..d56bde6 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -178,5 +178,6 @@ void i915_guc_capture_logs(struct drm_device *dev);
>>   void i915_guc_capture_logs_on_reset(struct drm_device *dev);
>>   void i915_guc_register(struct drm_device *dev);
>>   void i915_guc_unregister(struct drm_device *dev);
>> +int i915_guc_log_control(struct drm_device *dev, uint64_t control_val);
>>
>>   #endif
>>
>
> Regards,
>
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx