From patchwork Tue Feb 16 09:06:14 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Maarten Lankhorst X-Patchwork-Id: 8323061 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 6BC72C02AA for ; Tue, 16 Feb 2016 09:06:22 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0CAD7202F0 for ; Tue, 16 Feb 2016 09:06:21 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 9C561202EC for ; Tue, 16 Feb 2016 09:06:19 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 450066E76B; Tue, 16 Feb 2016 09:06:19 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 3310E6E76B for ; Tue, 16 Feb 2016 09:06:18 +0000 (UTC) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP; 16 Feb 2016 01:06:18 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,454,1449561600"; d="scan'208";a="903463514" Received: from cclerenc-mobl6.ger.corp.intel.com (HELO patser.lan) ([10.252.10.92]) by fmsmga001.fm.intel.com with ESMTP; 16 Feb 2016 01:06:16 -0800 To: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= References: <20160203163408.GF23290@intel.com> <1455022343-15222-1-git-send-email-maarten.lankhorst@linux.intel.com> <1455022343-15222-2-git-send-email-maarten.lankhorst@linux.intel.com> <20160209133747.GN23290@intel.com> <56B9F23F.9080502@linux.intel.com> <20160209145834.GQ23290@intel.com> From: Maarten Lankhorst Message-ID: <56C2E686.5060803@linux.intel.com> Date: Tue, 16 Feb 2016 10:06:14 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20160209145834.GQ23290@intel.com> Cc: intel-gfx@lists.freedesktop.org Subject: [Intel-gfx] [PATCH v2 2/3] drm/i915: Use atomic helpers for suspend, v2. X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 >>>> --- >>>> 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 Reviewed-by: Ville Syrjälä 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)