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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Op 10-05-16 om 09:48 schreef Daniel Vetter:
> On Mon, May 09, 2016 at 03:54:15PM +0300, Ville Syrjälä wrote:
>> 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@xxxxxxxxxxxxxxx>
>>> Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
>>> Cc: drm-intel-fixes@xxxxxxxxxxxxxxxxxxxxx
>>> ---
>>>  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.
> We don't really need dev->mode_config.mutex for atomic commits, so taking
> that out and use drm_modeset_lock_all_ctx would give you that.
> -Daniel

---->8----

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.
Changes since v3:
- Rebase on top of dev_priv -> dev change.
- Use drm_modeset_lock_all_ctx instead of drm_modeset_lock_all.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
Cc: drm-intel-fixes@xxxxxxxxxxxxxxxxxxxxx
---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c | 164 +++++++++++++++++++++++------------
 2 files changed, 109 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4abd39f32c0f..422e551937f7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1835,6 +1835,7 @@ struct drm_i915_private {
 	enum modeset_restore modeset_restore;
 	struct mutex modeset_restore_lock;
 	struct drm_atomic_state *modeset_restore_state;
+	struct drm_modeset_acquire_ctx reset_ctx;
 
 	struct list_head vm_list; /* Global list of all address spaces */
 	struct i915_ggtt ggtt; /* VM representing the global address space */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 138ed1aa3938..e88a942787c5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3130,40 +3130,109 @@ 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;
+
+	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_i915_private *dev_priv)
 {
+	struct drm_atomic_state *state;
+	struct drm_modeset_acquire_ctx *ctx;
+	int ret;
+
 	/* no reset support for gen2 */
 	if (IS_GEN2(dev_priv))
 		return;
 
-	/* reset doesn't touch the display */
+	ctx = &dev_priv->reset_ctx;
+
+	/*
+	 * This is a cludge because with real atomic modeset mode_config.mutex
+	 * won't be taken. Unfortunately some probed state like
+	 * audio_codec_enable is still protected by mode_config.mutex, so lock
+	 * it here for now.
+	 */
+	mutex_lock(&dev_priv->dev->mode_config.mutex);
+	drm_modeset_acquire_init(ctx, 0);
+	while (1) {
+		ret = drm_modeset_lock_all_ctx(dev_priv->dev, ctx);
+		if (ret != -EDEADLK)
+			break;
+
+		drm_modeset_backoff(ctx);
+	}
+
+	/* reset doesn't touch the display, but flips might get nuked anyway, */
 	if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
 		return;
 
-	drm_modeset_lock_all(dev_priv->dev);
 	/*
 	 * Disabling the crtcs gracefully seems nicer. Also the
 	 * g33 docs say we should at least disable all the planes.
 	 */
-	intel_display_suspend(dev_priv->dev);
+	state = drm_atomic_helper_duplicate_state(dev_priv->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_priv->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_i915_private *dev_priv)
 {
+	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
+	struct drm_modeset_acquire_ctx *ctx = &dev_priv->reset_ctx;
+	int ret;
+
 	/*
 	 * Flips in the rings will be nuked by the reset,
 	 * so complete all pending flips so that user space
@@ -3175,6 +3244,8 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
 	if (IS_GEN2(dev_priv))
 		return;
 
+	dev_priv->modeset_restore_state = NULL;
+
 	/* reset doesn't touch the display */
 	if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv)) {
 		/*
@@ -3187,28 +3258,31 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
 		 * CS-based flips (which might get lost in gpu resets) any more.
 		 */
 		intel_update_primary_planes(dev_priv->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_priv->dev);
+		intel_modeset_init_hw(dev_priv->dev);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	if (dev_priv->display.hpd_irq_setup)
-		dev_priv->display.hpd_irq_setup(dev_priv);
-	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_priv);
+		spin_unlock_irq(&dev_priv->irq_lock);
 
-	intel_display_resume(dev_priv->dev);
+		ret = __intel_display_resume(dev_priv->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_priv->dev);
+	drm_modeset_drop_locks(ctx);
+	drm_modeset_acquire_fini(ctx);
+	mutex_unlock(&dev_priv->dev->mode_config.mutex);
 }
 
 static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
@@ -15957,9 +16031,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
@@ -15970,40 +16045,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


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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux