On Wed, Jul 20, 2016 at 09:51:45AM +0530, Goel, Akash wrote:
>
>
> On 7/19/2016 4:51 PM, Chris Wilson wrote:
> >On Tue, Jul 19, 2016 at 12:12:20PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 10/07/16 14:41, akash.goel@xxxxxxxxx wrote:
> >>>From: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> >>>
> >>>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@xxxxxxxxx>
> >>>Signed-off-by: Akash Goel <akash.goel@xxxxxxxxx>
> >>>---
> >>> 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)?
> >
>
> Took the struct_mutex, just to avoid a very remote possibility where
> i915_guc_capture_logs_on_reset & debugfs function
> i915_guc_log_control executes concurrently.
>
> >Flat out invalid to take struct_mutex on the error capture path, or any
> >lock at all really (just in case of driver bugs). Consider it to be an
> >atomic context that may preempt the driver at any point.
>
> Actually I see that i915_reset() too takes the struct_mutex right at
> the beginning and I have plugged the call to
> i915_guc_capture_logs_on_reset() just before that.
Postmortem state is captured from i915_capture_error_state(), and as I
recall one of the raison d'etre for this facility was to include the guc
log in the error state.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
