Op 09-02-16 om 15:58 schreef Ville Syrjälä:
> On Tue, Feb 09, 2016 at 03:05:51PM +0100, Maarten Lankhorst wrote:
>> Op 09-02-16 om 14:37 schreef Ville Syrjälä:
>>> On Tue, Feb 09, 2016 at 01:52:22PM +0100, Maarten Lankhorst wrote:
>>>> Instead of duplicating the functionality now that we no longer need
>>>> to preserve dpll state we can move to using the upstream suspend helper.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/drm_atomic_helper.c | 3 +
>>>> drivers/gpu/drm/i915/i915_drv.c | 8 ---
>>>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>>>> drivers/gpu/drm/i915/intel_display.c | 117
>>>> ++++++++++++-----------------------
>>>> 4 files changed, 42 insertions(+), 87 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>>>> b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index 2b430b05f35d..a2d3b094f27c 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -2102,6 +2102,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>>>> err = drm_atomic_commit(state);
>>>> drm_modeset_unlock_all(dev);
>>>>
>>>> + if (err)
>>>> + drm_atomic_state_free(state);
>>>> +
>>>> return err;
>>>> }
>>>> EXPORT_SYMBOL(drm_atomic_helper_resume);
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>>>> b/drivers/gpu/drm/i915/i915_drv.c
>>>> index 11d8414edbbe..fc9b552bfcd1 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -600,13 +600,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>>>>
>>>> intel_suspend_gt_powersave(dev);
>>>>
>>>> - /*
>>>> - * Disable CRTCs directly since we want to preserve sw state
>>>> - * for _thaw. Also, power gate the CRTC power wells.
>>>> - */
>>>> - drm_modeset_lock_all(dev);
>>>> intel_display_suspend(dev);
>>>> - drm_modeset_unlock_all(dev);
>>>>
>>>> intel_dp_mst_suspend(dev);
>>>>
>>>> @@ -761,9 +755,7 @@ static int i915_drm_resume(struct drm_device *dev)
>>>> dev_priv->display.hpd_irq_setup(dev);
>>>> spin_unlock_irq(&dev_priv->irq_lock);
>>>>
>>>> - drm_modeset_lock_all(dev);
>>>> intel_display_resume(dev);
>>>> - drm_modeset_unlock_all(dev);
>>>>
>>>> intel_dp_mst_resume(dev);
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 8216665405eb..ef289514b97e 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1848,6 +1848,7 @@ struct drm_i915_private {
>>>>
>>>> enum modeset_restore modeset_restore;
>>>> struct mutex modeset_restore_lock;
>>>> + struct drm_atomic_state *modeset_restore_state;
>>>>
>>>> struct list_head vm_list; /* Global list of all address spaces */
>>>> struct i915_gtt gtt; /* 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 e496c130364d..4c91fd1c5222 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -6393,58 +6393,19 @@ static void intel_crtc_disable_noatomic(struct
>>>> drm_crtc *crtc)
>>>> */
>>>> int intel_display_suspend(struct drm_device *dev)
>>>> {
>>>> - struct drm_mode_config *config = &dev->mode_config;
>>>> - struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
>>>> - struct drm_atomic_state *state;
>>>> - struct drm_crtc *crtc;
>>>> - unsigned crtc_mask = 0;
>>>> - int ret = 0;
>>>> -
>>>> - if (WARN_ON(!ctx))
>>>> - return 0;
>>>> -
>>>> - lockdep_assert_held(&ctx->ww_ctx);
>>>> - state = drm_atomic_state_alloc(dev);
>>>> - if (WARN_ON(!state))
>>>> - return -ENOMEM;
>>>> -
>>>> - state->acquire_ctx = ctx;
>>>> - state->allow_modeset = true;
>>>> -
>>>> - for_each_crtc(dev, crtc) {
>>>> - struct drm_crtc_state *crtc_state =
>>>> - drm_atomic_get_crtc_state(state, crtc);
>>>> -
>>>> - ret = PTR_ERR_OR_ZERO(crtc_state);
>>>> - if (ret)
>>>> - goto free;
>>>> -
>>>> - if (!crtc_state->active)
>>>> - continue;
>>>> -
>>>> - crtc_state->active = false;
>>>> - crtc_mask |= 1 << drm_crtc_index(crtc);
>>>> - }
>>>> -
>>>> - if (crtc_mask) {
>>>> - ret = drm_atomic_commit(state);
>>>> -
>>>> - if (!ret) {
>>>> - for_each_crtc(dev, crtc)
>>>> - if (crtc_mask & (1 << drm_crtc_index(crtc)))
>>>> - crtc->state->active = true;
>>>> -
>>>> - return ret;
>>>> - }
>>>> - }
>>>> + struct drm_i915_private *dev_priv = to_i915(dev);
>>>> + int ret;
>>>>
>>>> -free:
>>>> - if (ret)
>>>> + dev_priv->modeset_restore_state = drm_atomic_helper_suspend(dev);
>>>> + ret = PTR_ERR_OR_ZERO(dev_priv->modeset_restore_state);
>>>> + if (ret) {
>>>> DRM_ERROR("Suspending crtc's failed with %i\n", ret);
>>>> - drm_atomic_state_free(state);
>>>> + dev_priv->modeset_restore_state = NULL;
>>>> + }
>>> I would use a local state pointer and only assign it to
>>> modeset_restore_state once we know it's good.
>>>
>>>> return ret;
>>>> }
>>>>
>>>> +
>>> Spurious newline.
>>>
>>>> void intel_encoder_destroy(struct drm_encoder *encoder)
>>>> {
>>>> struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
>>>> @@ -15909,51 +15870,49 @@ intel_modeset_setup_hw_state(struct drm_device
>>>> *dev)
>>>>
>>>> void intel_display_resume(struct drm_device *dev)
>>>> {
>>>> - struct drm_atomic_state *state = drm_atomic_state_alloc(dev);
>>>> - struct intel_connector *conn;
>>>> - struct intel_plane *plane;
>>>> + struct drm_i915_private *dev_priv = to_i915(dev);
>>>> + struct drm_atomic_state *state = dev_priv->modeset_restore_state;
>>>> + struct drm_modeset_acquire_ctx ctx;
>>>> struct drm_crtc *crtc;
>>>> - int ret;
>>>> + struct drm_crtc_state *crtc_state;
>>>> + int ret, i;
>>>> +
>>>> + intel_modeset_setup_hw_state(dev);
>>> Isn't something in there going to scream about locking?
>> I was hoping to find out by running it through CI..
>>
>> I don't see what possibly could scream however, from a glance it seems
>> nothing depends on it.
>>>> + i915_redisable_vga(dev);
>>>> + dev_priv->modeset_restore_state = NULL;
>>>>
>>>> if (!state)
>>>> return;
>>>>
>>>> - state->acquire_ctx = dev->mode_config.acquire_ctx;
>>>> -
>>>> - for_each_crtc(dev, crtc) {
>>>> - struct drm_crtc_state *crtc_state =
>>>> - drm_atomic_get_crtc_state(state, crtc);
>>>> -
>>>> - ret = PTR_ERR_OR_ZERO(crtc_state);
>>>> - if (ret)
>>>> - goto err;
>>>> + drm_modeset_acquire_init(&ctx, 0);
>>>> + state->acquire_ctx = &ctx;
>>>>
>>>> - /* force a restore */
>>>> + 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;
>>>> }
>>> Hmm. Why do we need to set that exactly? If the current state doesn't
>>> match the "to be restored" state, shouldn't something somewhere figure
>>> out that we really need a modeset?
>> We have a whole bunch of hw state from suspend that may be different from
>> the hw readout in resume. This is not handled by the core because it doesn't
>> know about driver specific pll's etc. So that's what the mode_changed = true
>> is for. It's a hint to intel_atomic_check that might get downgraded to
>> update_pipe if i915.fastboot is true and the hw readout and resume states
>> are compatible.
> Hmm. So it's a bit of a workaround due to the fact that
> intel_atomic_check() doesn't actually check/update everything, and
> instead tries to skip a bunch of stuff if the core didn't detect any
> meaningful changes? I guess that makes some sense as an optimization.
> That is, there's no need to check stuff if nothing user visible
> changed anyway because we should anyway end up with the same state
> that we had should we do a full recompute.
>
How about this? with --scissors
--->8----
Instead of duplicating the functionality now that we no longer need
to preserve dpll state we can move to using the upstream suspend helper.
Changes since v1:
- Call hw readout with all mutexes held.
- Rework intel_display_suspend to only assign modeset_restore_state
on success.
Signed-off-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 44912ecebc1a..20e82008b8b6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -603,13 +603,7 @@ static int i915_drm_suspend(struct drm_device *dev)
intel_suspend_gt_powersave(dev);
- /*
- * Disable CRTCs directly since we want to preserve sw state
- * for _thaw. Also, power gate the CRTC power wells.
- */
- drm_modeset_lock_all(dev);
intel_display_suspend(dev);
- drm_modeset_unlock_all(dev);
intel_dp_mst_suspend(dev);
@@ -764,9 +758,7 @@ static int i915_drm_resume(struct drm_device *dev)
dev_priv->display.hpd_irq_setup(dev);
spin_unlock_irq(&dev_priv->irq_lock);
- drm_modeset_lock_all(dev);
intel_display_resume(dev);
- drm_modeset_unlock_all(dev);
intel_dp_mst_resume(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20d9dbd9f9cf..6644c2e354c1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1848,6 +1848,7 @@ struct drm_i915_private {
enum modeset_restore modeset_restore;
struct mutex modeset_restore_lock;
+ struct drm_atomic_state *modeset_restore_state;
struct list_head vm_list; /* Global list of all address spaces */
struct i915_gtt gtt; /* 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 a18bd1296ce8..eaff32f59953 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6393,55 +6393,16 @@ static void intel_crtc_disable_noatomic(struct drm_crtc
*crtc)
*/
int intel_display_suspend(struct drm_device *dev)
{
- struct drm_mode_config *config = &dev->mode_config;
- struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
+ struct drm_i915_private *dev_priv = to_i915(dev);
struct drm_atomic_state *state;
- struct drm_crtc *crtc;
- unsigned crtc_mask = 0;
- int ret = 0;
-
- if (WARN_ON(!ctx))
- return 0;
-
- lockdep_assert_held(&ctx->ww_ctx);
- state = drm_atomic_state_alloc(dev);
- if (WARN_ON(!state))
- return -ENOMEM;
-
- state->acquire_ctx = ctx;
- state->allow_modeset = true;
-
- for_each_crtc(dev, crtc) {
- struct drm_crtc_state *crtc_state =
- drm_atomic_get_crtc_state(state, crtc);
-
- ret = PTR_ERR_OR_ZERO(crtc_state);
- if (ret)
- goto free;
-
- if (!crtc_state->active)
- continue;
-
- crtc_state->active = false;
- crtc_mask |= 1 << drm_crtc_index(crtc);
- }
-
- if (crtc_mask) {
- ret = drm_atomic_commit(state);
-
- if (!ret) {
- for_each_crtc(dev, crtc)
- if (crtc_mask & (1 << drm_crtc_index(crtc)))
- crtc->state->active = true;
-
- return ret;
- }
- }
+ int ret;
-free:
+ state = drm_atomic_helper_suspend(dev);
+ ret = PTR_ERR_OR_ZERO(state);
if (ret)
DRM_ERROR("Suspending crtc's failed with %i\n", ret);
- drm_atomic_state_free(state);
+ else
+ dev_priv->modeset_restore_state = state;
return ret;
}
@@ -15914,51 +15875,57 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
void intel_display_resume(struct drm_device *dev)
{
- struct drm_atomic_state *state = drm_atomic_state_alloc(dev);
- struct intel_connector *conn;
- struct intel_plane *plane;
- struct drm_crtc *crtc;
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ struct drm_atomic_state *state = dev_priv->modeset_restore_state;
+ struct drm_modeset_acquire_ctx ctx;
int ret;
+ bool setup = false;
- if (!state)
- return;
+ dev_priv->modeset_restore_state = NULL;
- state->acquire_ctx = dev->mode_config.acquire_ctx;
+ drm_modeset_acquire_init(&ctx, 0);
- for_each_crtc(dev, crtc) {
- struct drm_crtc_state *crtc_state =
- drm_atomic_get_crtc_state(state, crtc);
+retry:
+ ret = drm_modeset_lock_all_ctx(dev, &ctx);
- ret = PTR_ERR_OR_ZERO(crtc_state);
- if (ret)
- goto err;
+ if (ret == 0 && !setup) {
+ setup = true;
- /* force a restore */
- crtc_state->mode_changed = true;
+ intel_modeset_setup_hw_state(dev);
+ i915_redisable_vga(dev);
}
- for_each_intel_plane(dev, plane) {
- ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state,
&plane->base));
- if (ret)
- goto err;
- }
+ if (ret == 0 && state) {
+ struct drm_crtc_state *crtc_state;
+ struct drm_crtc *crtc;
+ int i;
- for_each_intel_connector(dev, conn) {
- ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(state,
&conn->base));
- if (ret)
- goto err;
+ 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);
}
- intel_modeset_setup_hw_state(dev);
+ if (ret == -EDEADLK) {
+ drm_modeset_backoff(&ctx);
+ goto retry;
+ }
- i915_redisable_vga(dev);
- ret = drm_atomic_commit(state);
- if (!ret)
- return;
+ drm_modeset_drop_locks(&ctx);
+ drm_modeset_acquire_fini(&ctx);
-err:
- DRM_ERROR("Restoring old state failed with %i\n", ret);
- drm_atomic_state_free(state);
+ if (ret) {
+ DRM_ERROR("Restoring old state failed with %i\n", ret);
+ drm_atomic_state_free(state);
+ }
}
void intel_modeset_gem_init(struct drm_device *dev)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
