Re: [PATCH v3 1/2] drm/i915: Fix modeset handling during gpu reset, v3.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, May 09, 2016 at 01:04:21PM +0200, Maarten Lankhorst wrote:
> This function would call drm_modeset_lock_all, while the suspend/resume
> functions already have their own locking. Fix this by factoring out
> __intel_display_resume, and calling the atomic helpers for duplicating
> atomic state and disabling all crtc's during suspend.
> 
> Changes since v1:
> - Deal with -EDEADLK right after lock_all and clean up calls
>   to hw readout.
> - Always take all modeset locks so updates during gpu reset are blocked.
> Changes since v2:
> - Fix deadlock in intel_update_primary_planes.
> - Move WARN_ON(EDEADLK) to __intel_display_resume.
> - pctx -> ctx
> - only call __intel_display_resume on success in intel_display_resume.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> Cc: drm-intel-fixes@lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/intel_display.c | 150 ++++++++++++++++++++++-------------
>  1 file changed, 93 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 14e05091c397..6faa529f35df 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3130,41 +3130,96 @@ static void intel_update_primary_planes(struct drm_device *dev)
>  
>  	for_each_crtc(dev, crtc) {
>  		struct intel_plane *plane = to_intel_plane(crtc->primary);
> -		struct intel_plane_state *plane_state;
> -
> -		drm_modeset_lock_crtc(crtc, &plane->base);
> -		plane_state = to_intel_plane_state(plane->base.state);
> +		struct intel_plane_state *plane_state =
> +			to_intel_plane_state(plane->base.state);
>  
>  		if (plane_state->visible)
>  			plane->update_plane(&plane->base,
>  					    to_intel_crtc_state(crtc->state),
>  					    plane_state);
> +	}
> +}
> +
> +static int
> +__intel_display_resume(struct drm_device *dev,
> +		       struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int i, ret;
> +
> +	intel_modeset_setup_hw_state(dev);
> +	i915_redisable_vga(dev);
>  
> -		drm_modeset_unlock_crtc(crtc);
> +	if (!state)
> +		return 0;

Thank you diff for making things illegible.

I'm still not convinced we should be changing the locking scheme
here, especially in what's supposed to be just a fix.

In general, I'm not sure we want ->detect() and other irrelevant
stuff to block the GPU reset. The nice thing about g4x+ reset so
far has been that it's almost unnoticeable. Of course if it's a
genuine GPU hang the screen will probably have been frozen for
quite a while when we initiate the reset, so perhaps that's not
a huge real world issue.

Anyways, I kinda just want this fixed now, so I'll just toss in my
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

BTW as a followup I think we would actually want gen2 to take the
g4x+ path. There won't have been a reset, but the flips in the
ring will still never complete so our idea of frontbuffer may be
out of sync with the hardware, and so we should fix it up.

> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		/*
> +		 * Force recalculation even if we restore
> +		 * current state. With fast modeset this may not result
> +		 * in a modeset when the state is compatible.
> +		 */
> +		crtc_state->mode_changed = true;
>  	}
> +
> +	ret = drm_atomic_commit(state);
> +
> +	WARN_ON(ret == -EDEADLK);
> +	return ret;
>  }
>  
>  void intel_prepare_reset(struct drm_device *dev)
>  {
> +	struct drm_atomic_state *state;
> +	struct drm_modeset_acquire_ctx *ctx;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	int ret;
> +
>  	/* no reset support for gen2 */
> -	if (IS_GEN2(dev))
> +	if (IS_GEN2(dev_priv))
>  		return;
>  
> -	/* reset doesn't touch the display */
> -	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
> +	drm_modeset_lock_all(dev);


> +
> +	/* reset doesn't touch the display, but flips might get nuked anyway, */
> +	if (INTEL_INFO(dev_priv)->gen >= 5 || IS_G4X(dev_priv))
>  		return;
>  
> -	drm_modeset_lock_all(dev);
> +	ctx = dev->mode_config.acquire_ctx;
> +
>  	/*
>  	 * Disabling the crtcs gracefully seems nicer. Also the
>  	 * g33 docs say we should at least disable all the planes.
>  	 */
> -	intel_display_suspend(dev);
> +
> +	state = drm_atomic_helper_duplicate_state(dev, ctx);
> +	if (IS_ERR(state)) {
> +		ret = PTR_ERR(state);
> +		state = NULL;
> +		DRM_ERROR("Duplicating state failed with %i\n", ret);
> +		goto err;
> +	}
> +
> +	ret = drm_atomic_helper_disable_all(dev, ctx);
> +	if (ret) {
> +		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
> +		goto err;
> +	}
> +
> +	dev_priv->modeset_restore_state = state;
> +	state->acquire_ctx = ctx;
> +	return;
> +
> +err:
> +	drm_atomic_state_free(state);
>  }
>  
>  void intel_finish_reset(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
> +	int ret;
>  
>  	/*
>  	 * Flips in the rings will be nuked by the reset,
> @@ -3177,6 +3232,8 @@ void intel_finish_reset(struct drm_device *dev)
>  	if (IS_GEN2(dev))
>  		return;
>  
> +	dev_priv->modeset_restore_state = NULL;
> +
>  	/* reset doesn't touch the display */
>  	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev)) {
>  		/*
> @@ -3189,26 +3246,27 @@ void intel_finish_reset(struct drm_device *dev)
>  		 * CS-based flips (which might get lost in gpu resets) any more.
>  		 */
>  		intel_update_primary_planes(dev);
> -		return;
> -	}
> -
> -	/*
> -	 * The display has been reset as well,
> -	 * so need a full re-initialization.
> -	 */
> -	intel_runtime_pm_disable_interrupts(dev_priv);
> -	intel_runtime_pm_enable_interrupts(dev_priv);
> +	} else {
> +		/*
> +		 * The display has been reset as well,
> +		 * so need a full re-initialization.
> +		 */
> +		intel_runtime_pm_disable_interrupts(dev_priv);
> +		intel_runtime_pm_enable_interrupts(dev_priv);
>  
> -	intel_modeset_init_hw(dev);
> +		intel_modeset_init_hw(dev);
>  
> -	spin_lock_irq(&dev_priv->irq_lock);
> -	if (dev_priv->display.hpd_irq_setup)
> -		dev_priv->display.hpd_irq_setup(dev);
> -	spin_unlock_irq(&dev_priv->irq_lock);
> +		spin_lock_irq(&dev_priv->irq_lock);
> +		if (dev_priv->display.hpd_irq_setup)
> +			dev_priv->display.hpd_irq_setup(dev);
> +		spin_unlock_irq(&dev_priv->irq_lock);
>  
> -	intel_display_resume(dev);
> +		ret = __intel_display_resume(dev, state);
> +		if (ret)
> +			DRM_ERROR("Restoring old state failed with %i\n", ret);
>  
> -	intel_hpd_init(dev_priv);
> +		intel_hpd_init(dev_priv);
> +	}
>  
>  	drm_modeset_unlock_all(dev);
>  }
> @@ -15955,9 +16013,10 @@ void intel_display_resume(struct drm_device *dev)
>  	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
>  	struct drm_modeset_acquire_ctx ctx;
>  	int ret;
> -	bool setup = false;
>  
>  	dev_priv->modeset_restore_state = NULL;
> +	if (state)
> +		state->acquire_ctx = &ctx;
>  
>  	/*
>  	 * This is a cludge because with real atomic modeset mode_config.mutex
> @@ -15968,40 +16027,17 @@ void intel_display_resume(struct drm_device *dev)
>  	mutex_lock(&dev->mode_config.mutex);
>  	drm_modeset_acquire_init(&ctx, 0);
>  
> -retry:
> -	ret = drm_modeset_lock_all_ctx(dev, &ctx);
> -
> -	if (ret == 0 && !setup) {
> -		setup = true;
> -
> -		intel_modeset_setup_hw_state(dev);
> -		i915_redisable_vga(dev);
> -	}
> -
> -	if (ret == 0 && state) {
> -		struct drm_crtc_state *crtc_state;
> -		struct drm_crtc *crtc;
> -		int i;
> -
> -		state->acquire_ctx = &ctx;
> -
> -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -			/*
> -			 * Force recalculation even if we restore
> -			 * current state. With fast modeset this may not result
> -			 * in a modeset when the state is compatible.
> -			 */
> -			crtc_state->mode_changed = true;
> -		}
> -
> -		ret = drm_atomic_commit(state);
> -	}
> +	while (1) {
> +		ret = drm_modeset_lock_all_ctx(dev, &ctx);
> +		if (ret != -EDEADLK)
> +			break;
>  
> -	if (ret == -EDEADLK) {
>  		drm_modeset_backoff(&ctx);
> -		goto retry;
>  	}
>  
> +	if (!ret)
> +		ret = __intel_display_resume(dev, state);
> +
>  	drm_modeset_drop_locks(&ctx);
>  	drm_modeset_acquire_fini(&ctx);
>  	mutex_unlock(&dev->mode_config.mutex);
> -- 
> 2.5.5
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx