Re: [PATCH 08/17] drm/i915: Forcefully flush GuC log buffer on reset

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

On 10/07/16 14:41, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> If GuC logs are being captured, there should be a force log buffer flush
> action sent to GuC before proceeding with GPU reset and re-initializing
> GUC. Those logs would be useful to understand why the GPU reset was
> initiated.
>
> v2: Rebase.
>
> 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_guc_submission.c | 32 ++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_irq.c            |  2 ++
>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>   3 files changed, 35 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 9b436fa..8cc31c6 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -183,6 +183,16 @@ static int host2guc_logbuffer_flush_complete(struct intel_guc *guc)
>   	return host2guc_action(guc, data, 1);
>   }
>
> +static int host2guc_force_logbuffer_flush(struct intel_guc *guc)
> +{
> +	u32 data[2];
> +
> +	data[0] = HOST2GUC_ACTION_FORCE_LOG_BUFFER_FLUSH;
> +	data[1] = 0;
> +
> +	return host2guc_action(guc, data, 2);
> +}
> +
>   /*
>    * Initialise, update, or clear doorbell data shared with the GuC
>    *
> @@ -1404,6 +1414,28 @@ void i915_guc_capture_logs(struct drm_device *dev)
>   	intel_runtime_pm_put(dev_priv);
>   }
>
> +void i915_guc_capture_logs_on_reset(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	mutex_lock(&dev->struct_mutex);

Not sure what are the repercussion of taking the mutex on the 
i915_reset_and_wakeup and path (error capture, hangcheck, dont' know 
this area well). Check with Chris and Mika I suppose (cc-ed)?

> +
> +	if (!i915.enable_guc_submission || (i915.guc_log_level < 0))

It would be cool if guc_log_level could be guaranteed to be disabled 
when guc submission is not on. I think it would be more maintainable.

> +		goto end;
> +
> +	/* First disable the interrupts, will be renabled after reset */
> +	gen9_disable_guc_interrupts(dev_priv);
> +
> +	/* Ask GuC to update the log buffer state */
> +	host2guc_force_logbuffer_flush(&dev_priv->guc);
> +
> +	/* GuC would have updated the log buffer by now, so capture it */
> +	i915_guc_capture_logs(dev);
> +
> +end:
> +	mutex_unlock(&dev->struct_mutex);
> +}
> +
>   void i915_guc_unregister(struct drm_device *dev)
>   {
>   	if (!i915.enable_guc_submission)
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index f90d3c6..bdd7a67 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2640,6 +2640,8 @@ static void i915_reset_and_wakeup(struct drm_i915_private *dev_priv)
>   		 */
>   		intel_runtime_pm_get(dev_priv);
>
> +		i915_guc_capture_logs_on_reset(&dev_priv->drm);
> +
>   		intel_prepare_reset(dev_priv);
>
>   		/*
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index a784cf8..ed773b5 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -175,6 +175,7 @@ int i915_guc_submit(struct drm_i915_gem_request *rq);
>   void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
>   void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
>   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);
>
>

Regards,

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