diff mbox

[2/7] drm/i915: Unbreak gpu reset vs. modeset locking

Message ID 20170715114006.6380-3-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter July 15, 2017, 11:40 a.m. UTC
Taking the modeset locks unconditionally isn't the greatest idea,
because atm that part is still broken and times out (and then atomic
keels over). And there's really no reason to do so, the old code
didn't do that either.

To make the patch a bit simpler let's also nuke 2 cases that are only
around for the old mmioflip paths. Atomic nonblocking workers will not
die (minus bugs) when a gpu reset happens.

And of course this doesn't fix any of the gpu reset vs. modeset
deadlock fun, but it at least stop modern CI machines from keeling
over all over the place for no reason at all.

And we still have the explicit testcases to run the fake gpu reset, so
coverage isn't that much worse.

v2: Split out additional changes on top, restrict this to purely reducing
the critical section of modeset locks.

Fixes: 739748939974 ("drm/i915: Fix modeset handling during gpu reset, v5.")
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 53 +++++++++---------------------------
 1 file changed, 13 insertions(+), 40 deletions(-)

Comments

Maarten Lankhorst July 17, 2017, 5:14 p.m. UTC | #1
Op 15-07-17 om 13:40 schreef Daniel Vetter:
> Taking the modeset locks unconditionally isn't the greatest idea,
> because atm that part is still broken and times out (and then atomic
> keels over). And there's really no reason to do so, the old code
> didn't do that either.
>
> To make the patch a bit simpler let's also nuke 2 cases that are only
> around for the old mmioflip paths. Atomic nonblocking workers will not
> die (minus bugs) when a gpu reset happens.
>
> And of course this doesn't fix any of the gpu reset vs. modeset
> deadlock fun, but it at least stop modern CI machines from keeling
> over all over the place for no reason at all.
>
> And we still have the explicit testcases to run the fake gpu reset, so
> coverage isn't that much worse.
>
> v2: Split out additional changes on top, restrict this to purely reducing
> the critical section of modeset locks.
>
> Fixes: 739748939974 ("drm/i915: Fix modeset handling during gpu reset, v5.")
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 53 +++++++++---------------------------
>  1 file changed, 13 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f69333b8995c..e3c55a996f6b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3413,26 +3413,6 @@ static void intel_complete_page_flips(struct drm_i915_private *dev_priv)
>  		intel_finish_page_flip_cs(dev_priv, crtc->pipe);
>  }
>  
> -static void intel_update_primary_planes(struct drm_device *dev)
> -{
> -	struct drm_crtc *crtc;
> -
> -	for_each_crtc(dev, crtc) {
> -		struct intel_plane *plane = to_intel_plane(crtc->primary);
> -		struct intel_plane_state *plane_state =
> -			to_intel_plane_state(plane->base.state);
> -
> -		if (plane_state->base.visible) {
> -			trace_intel_update_plane(&plane->base,
> -						 to_intel_crtc(crtc));
> -
> -			plane->update_plane(plane,
> -					    to_intel_crtc_state(crtc->state),
> -					    plane_state);
> -		}
> -	}
> -}
> -
>  static int
>  __intel_display_resume(struct drm_device *dev,
>  		       struct drm_atomic_state *state,
> @@ -3485,6 +3465,12 @@ void intel_prepare_reset(struct drm_i915_private *dev_priv)
>  	struct drm_atomic_state *state;
>  	int ret;
>  
> +
> +	/* reset doesn't touch the display, but flips might get nuked anyway, */
I think this comment was meant to address the reasoning for taking all locks,
so the part about flips being nuked no longer applies with mmio flips gone.
> +	if (!i915.force_reset_modeset_test &&
> +	    !gpu_reset_clobbers_display(dev_priv))
> +		return;
> +
>  	/*
>  	 * Need mode_config.mutex so that we don't
>  	 * trample ongoing ->detect() and whatnot.
> @@ -3498,12 +3484,6 @@ void intel_prepare_reset(struct drm_i915_private *dev_priv)
>  
>  		drm_modeset_backoff(ctx);
>  	}
> -
> -	/* reset doesn't touch the display, but flips might get nuked anyway, */
> -	if (!i915.force_reset_modeset_test &&
> -	    !gpu_reset_clobbers_display(dev_priv))
> -		return;
> -
>  	/*
>  	 * Disabling the crtcs gracefully seems nicer. Also the
>  	 * g33 docs say we should at least disable all the planes.
> @@ -3533,6 +3513,11 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
>  	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
>  	int ret;
>  
> +	/* reset doesn't touch the display, but flips might get nuked anyway, */
> +	if (!i915.force_reset_modeset_test &&
> +	    !gpu_reset_clobbers_display(dev_priv))
> +		return;
Same thing about comment here. Perhaps change the

if (!i915.force_reset_modeset_test && !gpu_reset_clobbers_display())

to

if (!state && !gpu_reset_clobbers_display())

so we don't run into a kernel panic if we change the parameter during reset?

Hm perhaps also either add if (state) to the __intel_display_resume call for <g4x,
or remove the one for drm_atomic_state_put.

With those changes I'm happy, and you can add
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f69333b8995c..e3c55a996f6b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3413,26 +3413,6 @@  static void intel_complete_page_flips(struct drm_i915_private *dev_priv)
 		intel_finish_page_flip_cs(dev_priv, crtc->pipe);
 }
 
-static void intel_update_primary_planes(struct drm_device *dev)
-{
-	struct drm_crtc *crtc;
-
-	for_each_crtc(dev, crtc) {
-		struct intel_plane *plane = to_intel_plane(crtc->primary);
-		struct intel_plane_state *plane_state =
-			to_intel_plane_state(plane->base.state);
-
-		if (plane_state->base.visible) {
-			trace_intel_update_plane(&plane->base,
-						 to_intel_crtc(crtc));
-
-			plane->update_plane(plane,
-					    to_intel_crtc_state(crtc->state),
-					    plane_state);
-		}
-	}
-}
-
 static int
 __intel_display_resume(struct drm_device *dev,
 		       struct drm_atomic_state *state,
@@ -3485,6 +3465,12 @@  void intel_prepare_reset(struct drm_i915_private *dev_priv)
 	struct drm_atomic_state *state;
 	int ret;
 
+
+	/* reset doesn't touch the display, but flips might get nuked anyway, */
+	if (!i915.force_reset_modeset_test &&
+	    !gpu_reset_clobbers_display(dev_priv))
+		return;
+
 	/*
 	 * Need mode_config.mutex so that we don't
 	 * trample ongoing ->detect() and whatnot.
@@ -3498,12 +3484,6 @@  void intel_prepare_reset(struct drm_i915_private *dev_priv)
 
 		drm_modeset_backoff(ctx);
 	}
-
-	/* reset doesn't touch the display, but flips might get nuked anyway, */
-	if (!i915.force_reset_modeset_test &&
-	    !gpu_reset_clobbers_display(dev_priv))
-		return;
-
 	/*
 	 * Disabling the crtcs gracefully seems nicer. Also the
 	 * g33 docs say we should at least disable all the planes.
@@ -3533,6 +3513,11 @@  void intel_finish_reset(struct drm_i915_private *dev_priv)
 	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
 	int ret;
 
+	/* reset doesn't touch the display, but flips might get nuked anyway, */
+	if (!i915.force_reset_modeset_test &&
+	    !gpu_reset_clobbers_display(dev_priv))
+		return;
+
 	/*
 	 * Flips in the rings will be nuked by the reset,
 	 * so complete all pending flips so that user space
@@ -3544,22 +3529,10 @@  void intel_finish_reset(struct drm_i915_private *dev_priv)
 
 	/* reset doesn't touch the display */
 	if (!gpu_reset_clobbers_display(dev_priv)) {
-		if (!state) {
-			/*
-			 * Flips in the rings have been nuked by the reset,
-			 * so update the base address of all primary
-			 * planes to the the last fb to make sure we're
-			 * showing the correct fb after a reset.
-			 *
-			 * FIXME: Atomic will make this obsolete since we won't schedule
-			 * CS-based flips (which might get lost in gpu resets) any more.
-			 */
-			intel_update_primary_planes(dev);
-		} else {
-			ret = __intel_display_resume(dev, state, ctx);
+		/* for testing only restore the display */
+		ret = __intel_display_resume(dev, state, ctx);
 			if (ret)
 				DRM_ERROR("Restoring old state failed with %i\n", ret);
-		}
 	} else {
 		/*
 		 * The display has been reset as well,