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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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>

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