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

[Tvrtko Ursulin]

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.

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

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

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

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

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

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

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