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.
Without the hint that there is a change it can't even do the checking since all
connectors and planes aren't added.
But indeed, nothing user visible would have changed.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
