All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/3] drm/i915: Use atomic helpers for suspend.
Date: Tue, 9 Feb 2016 15:37:47 +0200	[thread overview]
Message-ID: <20160209133747.GN23290@intel.com> (raw)
In-Reply-To: <1455022343-15222-2-git-send-email-maarten.lankhorst@linux.intel.com>

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.lankhorst@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?

> +	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?

>  
> -	for_each_intel_plane(dev, plane) {
> -		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state, &plane->base));
> -		if (ret)
> -			goto err;
> -	}
> +retry:
> +	ret = drm_modeset_lock_all_ctx(dev, &ctx);
> +	if (ret == 0)
> +		ret = drm_atomic_commit(state);
>  
> -	for_each_intel_connector(dev, conn) {
> -		ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(state, &conn->base));
> -		if (ret)
> -			goto err;
> +	if (ret == -EDEADLK) {
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
>  	}
>  
> -	intel_modeset_setup_hw_state(dev);
> -
> -	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)
> -- 
> 2.1.0

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

  reply	other threads:[~2016-02-09 13:37 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-01 13:43 [PATCH 0/6] Use more atomic state in i915 Maarten Lankhorst
2016-02-01 13:43 ` [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc Maarten Lankhorst
2016-02-01 16:08   ` Ville Syrjälä
2016-02-02 10:41     ` Maarten Lankhorst
2016-02-01 17:09   ` Ville Syrjälä
2016-02-02 12:46     ` Maarten Lankhorst
2016-02-02 12:48   ` [PATCH v2 1/6] drm/i915: Use atomic state to obtain load detection crtc, v2 Maarten Lankhorst
2016-02-02 17:32     ` Ville Syrjälä
2016-02-03  8:57       ` Maarten Lankhorst
2016-02-03 16:34         ` Ville Syrjälä
2016-02-09  8:57           ` Maarten Lankhorst
2016-02-09 12:52           ` [PATCH 1/3] drm/i915: Clear shared dpll based on old state Maarten Lankhorst
2016-02-09 12:52             ` [PATCH 2/3] drm/i915: Use atomic helpers for suspend Maarten Lankhorst
2016-02-09 13:37               ` Ville Syrjälä [this message]
2016-02-09 14:05                 ` Maarten Lankhorst
2016-02-09 14:58                   ` Ville Syrjälä
2016-02-15 12:34                     ` Maarten Lankhorst
2016-02-16  9:06                     ` [PATCH v2 2/3] drm/i915: Use atomic helpers for suspend, v2 Maarten Lankhorst
2016-02-16  9:55                       ` Ville Syrjälä
2016-02-09 12:52             ` [PATCH 3/3] drm/i915: Use atomic state to obtain load detection crtc, v3 Maarten Lankhorst
2016-02-16 11:13               ` Ville Syrjälä
2016-02-09 13:25             ` [PATCH 1/3] drm/i915: Clear shared dpll based on old state Ville Syrjälä
2016-02-11  8:56   ` [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc Daniel Vetter
2016-02-01 13:43 ` [PATCH 2/6] drm/i915: Use atomic state in crt load detection Maarten Lankhorst
2016-02-01 13:43 ` [PATCH 3/6] drm/i915: Use atomic state in tv " Maarten Lankhorst
2016-02-01 13:44 ` [PATCH 4/6] drm/i915: Use correct dpms for intel_enable_crt Maarten Lankhorst
2016-02-01 13:44 ` [IGT PATCH 5/6] kms_force_connector_basic: Add force-load-detect test Maarten Lankhorst
2016-02-11  8:59   ` Daniel Vetter
2016-02-11 11:50     ` Maarten Lankhorst
2016-02-01 13:44 ` [PATCH 6/6] drm/i915: Use atomic state in intel_fb_initial_config Maarten Lankhorst
2016-02-11 14:38   ` Ville Syrjälä

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160209133747.GN23290@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=maarten.lankhorst@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.