[PATCH v2 1/3] drm/atomic-helper: Implement drm_atomic_helper_duplicate_state()

[PATCH v2 1/3] drm/atomic-helper: Implement drm_atomic_helper_duplicate_state()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This function can be used to duplicate an atomic state object. This is
useful for example to implement suspend/resume, where the state before
suspend can be saved and restored upon resume.

v2: move locking to caller, be more explicit about prerequisites

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 80 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h     |  2 +
 2 files changed, 82 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index fd06595c2ee5..1f84df3b0a5d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2358,6 +2358,86 @@ drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector)
 EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state);
 
 /**
+ * drm_atomic_helper_duplicate_state - duplicate an atomic state object
+ * @dev: DRM device
+ *
+ * Makes a copy of the current atomic state by looping over all objects and
+ * duplicating their respective states. This is used for example by suspend/
+ * resume support code to save the state prior to suspend such that it can
+ * be restored upon resume.
+ *
+ * Note that this treats atomic state as persistent between save and restore.
+ * Drivers must make sure that this is guaranteed.
+ *
+ * Calls to this function must be guarded by drm_modeset_lock_all() and
+ * drm_modeset_unlock_all().
+ *
+ * Returns:
+ * A pointer to the copy of the atomic state object on success or an
+ * ERR_PTR()-encoded error code on failure.
+ */
+struct drm_atomic_state *
+drm_atomic_helper_duplicate_state(struct drm_device *dev)
+{
+	struct drm_atomic_state *state;
+	struct drm_connector *conn;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+	int err = 0;
+
+	if (WARN_ON(!dev->mode_config.acquire_ctx))
+		return ERR_PTR(-EINVAL);
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+
+	state->acquire_ctx = dev->mode_config.acquire_ctx;
+
+	drm_for_each_crtc(crtc, dev) {
+		struct drm_crtc_state *crtc_state;
+
+		crtc_state = drm_atomic_get_crtc_state(state, crtc);
+		if (IS_ERR(crtc_state)) {
+			err = PTR_ERR(crtc_state);
+			goto free;
+		}
+	}
+
+	drm_for_each_plane(plane, dev) {
+		struct drm_plane_state *plane_state;
+
+		plane_state = drm_atomic_get_plane_state(state, plane);
+		if (IS_ERR(plane_state)) {
+			err = PTR_ERR(plane_state);
+			goto free;
+		}
+	}
+
+	drm_for_each_connector(conn, dev) {
+		struct drm_connector_state *conn_state;
+
+		conn_state = drm_atomic_get_connector_state(state, conn);
+		if (IS_ERR(conn_state)) {
+			err = PTR_ERR(conn_state);
+			goto free;
+		}
+	}
+
+	/* clear the acquire context so that it isn't accidentally reused */
+	state->acquire_ctx = NULL;
+
+free:
+	if (err < 0) {
+		drm_atomic_state_free(state);
+		state = ERR_PTR(err);
+	}
+
+	return state;
+}
+EXPORT_SYMBOL(drm_atomic_helper_duplicate_state);
+
+/**
  * __drm_atomic_helper_connector_destroy_state - release connector state
  * @connector: connector object
  * @state: connector state object to release
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 4ffe9dca07c4..5d27612997ce 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -118,6 +118,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
 					   struct drm_connector_state *state);
 struct drm_connector_state *
 drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector);
+struct drm_atomic_state *
+drm_atomic_helper_duplicate_state(struct drm_device *dev);
 void
 __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
 					    struct drm_connector_state *state);
-- 
2.4.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/3] drm/atomic-helper: Implement subsystem-level suspend/resume

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Provide subsystem-level suspend and resume helpers that can be used to
implement suspend/resume on atomic mode-setting enabled drivers.

v2: simplify locking, enhance kerneldoc comments

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 141 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h     |   5 ++
 2 files changed, 146 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 1f84df3b0a5d..0eb45a7a760e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1751,6 +1751,147 @@ backoff:
 EXPORT_SYMBOL(drm_atomic_helper_set_config);
 
 /**
+ * drm_atomic_helper_disable_all - disable all currently active outputs
+ * @dev: DRM device
+ *
+ * Loops through all connectors, finding those that aren't turned off and then
+ * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
+ * that they are connected to.
+ *
+ * This is used for example in suspend/resume to disable all currently active
+ * functions when suspending. See drm_atomic_helper_suspend().
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_atomic_helper_disable_all(struct drm_device *dev)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_connector_state *conn_state;
+	struct drm_atomic_state *state;
+	struct drm_connector *conn;
+	unsigned int i;
+	int err;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return -ENOMEM;
+
+	state->acquire_ctx = config->acquire_ctx;
+
+	drm_for_each_connector(conn, dev) {
+		struct drm_crtc *crtc = conn->state->crtc;
+		struct drm_crtc_state *crtc_state;
+
+		if (!crtc || conn->dpms != DRM_MODE_DPMS_ON)
+			continue;
+
+		crtc_state = drm_atomic_get_crtc_state(state, crtc);
+		if (IS_ERR(crtc_state)) {
+			err = PTR_ERR(crtc_state);
+			goto free;
+		}
+
+		err = drm_atomic_add_affected_connectors(state, crtc);
+		if (err < 0)
+			goto free;
+
+		conn->dpms = DRM_MODE_DPMS_OFF;
+		crtc_state->active = false;
+	}
+
+	err = drm_atomic_commit(state);
+	if (err < 0) {
+		/* restore DPMS mode */
+		for_each_connector_in_state(state, conn, conn_state, i)
+			conn->dpms = DRM_MODE_DPMS_ON;
+
+		goto free;
+	}
+
+free:
+	if (err < 0)
+		drm_atomic_state_free(state);
+
+	return err;
+}
+EXPORT_SYMBOL(drm_atomic_helper_disable_all);
+
+/**
+ * drm_atomic_helper_suspend - subsystem-level suspend helper
+ * @dev: DRM device
+ *
+ * Duplicates the current atomic state, disables all active outputs and then
+ * returns a pointer to the original atomic state to the caller. Drivers can
+ * pass this pointer to the drm_atomic_helper_resume() helper upon resume to
+ * restore the output configuration that was active at the time the system
+ * entered suspend.
+ *
+ * Note that it is potentially unsafe to use this. The atomic state object
+ * returned by this function is assumed to be persistent. Drivers must ensure
+ * that this holds true. Before calling this function, drivers must make sure
+ * to suspend fbdev emulation so that nothing can be using the device.
+ *
+ * Returns:
+ * A pointer to a copy of the state before suspend on success or an ERR_PTR()-
+ * encoded error code on failure. Drivers should store the returned atomic
+ * state object and pass it to the drm_atomic_helper_resume() helper upon
+ * resume.
+ */
+struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
+{
+	struct drm_atomic_state *state;
+	int err;
+
+	drm_modeset_lock_all(dev);
+
+	state = drm_atomic_helper_duplicate_state(dev);
+	if (IS_ERR(state))
+		goto unlock;
+
+	err = drm_atomic_helper_disable_all(dev);
+	if (err < 0) {
+		drm_atomic_state_free(state);
+		state = ERR_PTR(err);
+		goto unlock;
+	}
+
+unlock:
+	drm_modeset_unlock_all(dev);
+	return state;
+}
+EXPORT_SYMBOL(drm_atomic_helper_suspend);
+
+/**
+ * drm_atomic_helper_resume - subsystem-level resume helper
+ * @dev: DRM device
+ * @state: atomic state to resume to
+ *
+ * Calls drm_mode_config_reset() to synchronize hardware and software states,
+ * grabs all modeset locks and commits the atomic state object. This can be
+ * used in conjunction with the drm_atomic_helper_suspend() helper to
+ * implement suspend/resume for drivers that support atomic mode-setting.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_atomic_helper_resume(struct drm_device *dev,
+			     struct drm_atomic_state *state)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	int err;
+
+	drm_mode_config_reset(dev);
+	drm_modeset_lock_all(dev);
+	state->acquire_ctx = config->acquire_ctx;
+	err = drm_atomic_commit(state);
+	drm_modeset_unlock_all(dev);
+
+	return err;
+}
+EXPORT_SYMBOL(drm_atomic_helper_resume);
+
+/**
  * drm_atomic_helper_crtc_set_property - helper for crtc properties
  * @crtc: DRM crtc
  * @property: DRM property
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 5d27612997ce..080f131c9f2e 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -75,6 +75,11 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane,
 int drm_atomic_helper_disable_plane(struct drm_plane *plane);
 int drm_atomic_helper_set_config(struct drm_mode_set *set);
 
+int drm_atomic_helper_disable_all(struct drm_device *dev);
+struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);
+int drm_atomic_helper_resume(struct drm_device *dev,
+			     struct drm_atomic_state *state);
+
 int drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
 					struct drm_property *property,
 					uint64_t val);
-- 
2.4.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/3] drm/tegra: Implement subsystem-level suspend/resume

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Use the drm_atomic_helper_suspend() and drm_atomic_helper_resume()
helpers to implement subsystem-level suspend/resume.

v2: suspend framebuffer device to avoid concurrency issues

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/drm.c | 11 +++++++++++
 drivers/gpu/drm/tegra/drm.h |  4 ++++
 drivers/gpu/drm/tegra/fb.c  | 24 ++++++++++++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index b5f24c0f93dc..10d62e3077de 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1022,8 +1022,16 @@ static int host1x_drm_remove(struct host1x_device *dev)
 static int host1x_drm_suspend(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
+	struct tegra_drm *tegra = drm->dev_private;
 
 	drm_kms_helper_poll_disable(drm);
+	tegra_drm_fb_suspend(drm);
+
+	tegra->state = drm_atomic_helper_suspend(drm);
+	if (IS_ERR(tegra->state)) {
+		drm_kms_helper_poll_enable(drm);
+		return PTR_ERR(tegra->state);
+	}
 
 	return 0;
 }
@@ -1031,7 +1039,10 @@ static int host1x_drm_suspend(struct device *dev)
 static int host1x_drm_resume(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
+	struct tegra_drm *tegra = drm->dev_private;
 
+	drm_atomic_helper_resume(drm, tegra->state);
+	tegra_drm_fb_resume(drm);
 	drm_kms_helper_poll_enable(drm);
 
 	return 0;
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 567831191f8b..112e07d81557 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -57,6 +57,8 @@ struct tegra_drm {
 		struct work_struct work;
 		struct mutex lock;
 	} commit;
+
+	struct drm_atomic_state *state;
 };
 
 struct tegra_drm_client;
@@ -267,6 +269,8 @@ int tegra_drm_fb_prepare(struct drm_device *drm);
 void tegra_drm_fb_free(struct drm_device *drm);
 int tegra_drm_fb_init(struct drm_device *drm);
 void tegra_drm_fb_exit(struct drm_device *drm);
+void tegra_drm_fb_suspend(struct drm_device *drm);
+void tegra_drm_fb_resume(struct drm_device *drm);
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 void tegra_fbdev_restore_mode(struct tegra_fbdev *fbdev);
 void tegra_fb_output_poll_changed(struct drm_device *drm);
diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
index 9b8aa967fa71..6c60795a2622 100644
--- a/drivers/gpu/drm/tegra/fb.c
+++ b/drivers/gpu/drm/tegra/fb.c
@@ -10,6 +10,8 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/console.h>
+
 #include "drm.h"
 #include "gem.h"
 
@@ -414,3 +416,25 @@ void tegra_drm_fb_exit(struct drm_device *drm)
 	tegra_fbdev_exit(tegra->fbdev);
 #endif
 }
+
+void tegra_drm_fb_suspend(struct drm_device *drm)
+{
+#ifdef CONFIG_DRM_FBDEV_EMULATION
+	struct tegra_drm *tegra = drm->dev_private;
+
+	console_lock();
+	drm_fb_helper_set_suspend(&tegra->fbdev->base, 1);
+	console_unlock();
+#endif
+}
+
+void tegra_drm_fb_resume(struct drm_device *drm)
+{
+#ifdef CONFIG_DRM_FBDEV_EMULATION
+	struct tegra_drm *tegra = drm->dev_private;
+
+	console_lock();
+	drm_fb_helper_set_suspend(&tegra->fbdev->base, 0);
+	console_unlock();
+#endif
+}
-- 
2.4.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm/atomic-helper: Implement drm_atomic_helper_duplicate_state()

[Daniel Vetter]

On Wed, Aug 12, 2015 at 04:37:28PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This function can be used to duplicate an atomic state object. This is
> useful for example to implement suspend/resume, where the state before
> suspend can be saved and restored upon resume.
> 
> v2: move locking to caller, be more explicit about prerequisites
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 80 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  2 +
>  2 files changed, 82 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index fd06595c2ee5..1f84df3b0a5d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2358,6 +2358,86 @@ drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector)
>  EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state);
>  
>  /**
> + * drm_atomic_helper_duplicate_state - duplicate an atomic state object
> + * @dev: DRM device
> + *
> + * Makes a copy of the current atomic state by looping over all objects and
> + * duplicating their respective states. This is used for example by suspend/
> + * resume support code to save the state prior to suspend such that it can
> + * be restored upon resume.
> + *
> + * Note that this treats atomic state as persistent between save and restore.
> + * Drivers must make sure that this is guaranteed.

s/guaranteed/is possible and won't result in confusion/ maybe?

> + *
> + * Calls to this function must be guarded by drm_modeset_lock_all() and
> + * drm_modeset_unlock_all().

If you pass in the acquire ctx explicitly this isn't strictly true since
you're using the get_*_state functions, which might return -EDEADLK. So
maybe instead:

"Note that if callers haven't already acquired all modeset locks this
might return -EDEADLK, which must be handled by the caller using
rm_modeset_backoff()."

> + *
> + * Returns:
> + * A pointer to the copy of the atomic state object on success or an
> + * ERR_PTR()-encoded error code on failure.
> + */
> +struct drm_atomic_state *
> +drm_atomic_helper_duplicate_state(struct drm_device *dev)
> +{
> +	struct drm_atomic_state *state;
> +	struct drm_connector *conn;
> +	struct drm_plane *plane;
> +	struct drm_crtc *crtc;
> +	int err = 0;
> +
> +	if (WARN_ON(!dev->mode_config.acquire_ctx))
> +		return ERR_PTR(-EINVAL);
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	state->acquire_ctx = dev->mode_config.acquire_ctx;

The hidden acquire_ctx was just a hack to avoid needing to redo _all_
driver entry points by adding a ctx parameter. I want to remove this
long-term, so please don't spread it and instead just add a ctx parameter
to this function.

Didn't spot anything else.
-Daniel

> +
> +	drm_for_each_crtc(crtc, dev) {
> +		struct drm_crtc_state *crtc_state;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(crtc_state)) {
> +			err = PTR_ERR(crtc_state);
> +			goto free;
> +		}
> +	}
> +
> +	drm_for_each_plane(plane, dev) {
> +		struct drm_plane_state *plane_state;
> +
> +		plane_state = drm_atomic_get_plane_state(state, plane);
> +		if (IS_ERR(plane_state)) {
> +			err = PTR_ERR(plane_state);
> +			goto free;
> +		}
> +	}
> +
> +	drm_for_each_connector(conn, dev) {
> +		struct drm_connector_state *conn_state;
> +
> +		conn_state = drm_atomic_get_connector_state(state, conn);
> +		if (IS_ERR(conn_state)) {
> +			err = PTR_ERR(conn_state);
> +			goto free;
> +		}
> +	}
> +
> +	/* clear the acquire context so that it isn't accidentally reused */
> +	state->acquire_ctx = NULL;
> +
> +free:
> +	if (err < 0) {
> +		drm_atomic_state_free(state);
> +		state = ERR_PTR(err);
> +	}
> +
> +	return state;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_duplicate_state);
> +
> +/**
>   * __drm_atomic_helper_connector_destroy_state - release connector state
>   * @connector: connector object
>   * @state: connector state object to release
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 4ffe9dca07c4..5d27612997ce 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -118,6 +118,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
>  					   struct drm_connector_state *state);
>  struct drm_connector_state *
>  drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector);
> +struct drm_atomic_state *
> +drm_atomic_helper_duplicate_state(struct drm_device *dev);
>  void
>  __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
>  					    struct drm_connector_state *state);
> -- 
> 2.4.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/atomic-helper: Implement subsystem-level suspend/resume

[Daniel Vetter]

On Wed, Aug 12, 2015 at 04:37:29PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Provide subsystem-level suspend and resume helpers that can be used to
> implement suspend/resume on atomic mode-setting enabled drivers.
> 
> v2: simplify locking, enhance kerneldoc comments
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 141 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |   5 ++
>  2 files changed, 146 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 1f84df3b0a5d..0eb45a7a760e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1751,6 +1751,147 @@ backoff:
>  EXPORT_SYMBOL(drm_atomic_helper_set_config);
>  
>  /**
> + * drm_atomic_helper_disable_all - disable all currently active outputs
> + * @dev: DRM device
> + *
> + * Loops through all connectors, finding those that aren't turned off and then
> + * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
> + * that they are connected to.
> + *
> + * This is used for example in suspend/resume to disable all currently active
> + * functions when suspending. See drm_atomic_helper_suspend().

I think an analog paragraph should be added to duplicate_state(), maybe in
this patch?

> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.

Please add the same caveat about locking like for duplicate_state here.

> + */
> +int drm_atomic_helper_disable_all(struct drm_device *dev)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_connector_state *conn_state;
> +	struct drm_atomic_state *state;
> +	struct drm_connector *conn;
> +	unsigned int i;
> +	int err;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	state->acquire_ctx = config->acquire_ctx;

Same comment about reusing the hidden acquire_ctx: Please don't and
instead make it an explicit paramter.

> +
> +	drm_for_each_connector(conn, dev) {
> +		struct drm_crtc *crtc = conn->state->crtc;
> +		struct drm_crtc_state *crtc_state;
> +
> +		if (!crtc || conn->dpms != DRM_MODE_DPMS_ON)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(crtc_state)) {
> +			err = PTR_ERR(crtc_state);
> +			goto free;
> +		}
> +
> +		err = drm_atomic_add_affected_connectors(state, crtc);
> +		if (err < 0)
> +			goto free;
> +
> +		conn->dpms = DRM_MODE_DPMS_OFF;
> +		crtc_state->active = false;
> +	}
> +
> +	err = drm_atomic_commit(state);
> +	if (err < 0) {
> +		/* restore DPMS mode */
> +		for_each_connector_in_state(state, conn, conn_state, i)
> +			conn->dpms = DRM_MODE_DPMS_ON;
> +
> +		goto free;
> +	}
> +
> +free:
> +	if (err < 0)
> +		drm_atomic_state_free(state);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_disable_all);
> +
> +/**
> + * drm_atomic_helper_suspend - subsystem-level suspend helper
> + * @dev: DRM device
> + *
> + * Duplicates the current atomic state, disables all active outputs and then
> + * returns a pointer to the original atomic state to the caller. Drivers can
> + * pass this pointer to the drm_atomic_helper_resume() helper upon resume to
> + * restore the output configuration that was active at the time the system
> + * entered suspend.

I'd mention the two helpers explicitly so that they all get linked up here
in the kerneldoc (i.e. duplicate_state() and disable_all()).

lgtm otherwise.
-Daniel

> + *
> + * Note that it is potentially unsafe to use this. The atomic state object
> + * returned by this function is assumed to be persistent. Drivers must ensure
> + * that this holds true. Before calling this function, drivers must make sure
> + * to suspend fbdev emulation so that nothing can be using the device.
> + *
> + * Returns:
> + * A pointer to a copy of the state before suspend on success or an ERR_PTR()-
> + * encoded error code on failure. Drivers should store the returned atomic
> + * state object and pass it to the drm_atomic_helper_resume() helper upon
> + * resume.
> + */
> +struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
> +{
> +	struct drm_atomic_state *state;
> +	int err;
> +
> +	drm_modeset_lock_all(dev);
> +
> +	state = drm_atomic_helper_duplicate_state(dev);
> +	if (IS_ERR(state))
> +		goto unlock;
> +
> +	err = drm_atomic_helper_disable_all(dev);
> +	if (err < 0) {
> +		drm_atomic_state_free(state);
> +		state = ERR_PTR(err);
> +		goto unlock;
> +	}
> +
> +unlock:
> +	drm_modeset_unlock_all(dev);
> +	return state;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_suspend);
> +
> +/**
> + * drm_atomic_helper_resume - subsystem-level resume helper
> + * @dev: DRM device
> + * @state: atomic state to resume to
> + *
> + * Calls drm_mode_config_reset() to synchronize hardware and software states,
> + * grabs all modeset locks and commits the atomic state object. This can be
> + * used in conjunction with the drm_atomic_helper_suspend() helper to
> + * implement suspend/resume for drivers that support atomic mode-setting.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_atomic_helper_resume(struct drm_device *dev,
> +			     struct drm_atomic_state *state)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	int err;
> +
> +	drm_mode_config_reset(dev);
> +	drm_modeset_lock_all(dev);
> +	state->acquire_ctx = config->acquire_ctx;
> +	err = drm_atomic_commit(state);
> +	drm_modeset_unlock_all(dev);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_resume);
> +
> +/**
>   * drm_atomic_helper_crtc_set_property - helper for crtc properties
>   * @crtc: DRM crtc
>   * @property: DRM property
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 5d27612997ce..080f131c9f2e 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -75,6 +75,11 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane,
>  int drm_atomic_helper_disable_plane(struct drm_plane *plane);
>  int drm_atomic_helper_set_config(struct drm_mode_set *set);
>  
> +int drm_atomic_helper_disable_all(struct drm_device *dev);
> +struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);
> +int drm_atomic_helper_resume(struct drm_device *dev,
> +			     struct drm_atomic_state *state);
> +
>  int drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>  					struct drm_property *property,
>  					uint64_t val);
> -- 
> 2.4.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm/tegra: Implement subsystem-level suspend/resume

[Daniel Vetter]

On Wed, Aug 12, 2015 at 04:37:30PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Use the drm_atomic_helper_suspend() and drm_atomic_helper_resume()
> helpers to implement subsystem-level suspend/resume.
> 
> v2: suspend framebuffer device to avoid concurrency issues
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/drm.c | 11 +++++++++++
>  drivers/gpu/drm/tegra/drm.h |  4 ++++
>  drivers/gpu/drm/tegra/fb.c  | 24 ++++++++++++++++++++++++
>  3 files changed, 39 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index b5f24c0f93dc..10d62e3077de 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1022,8 +1022,16 @@ static int host1x_drm_remove(struct host1x_device *dev)
>  static int host1x_drm_suspend(struct device *dev)
>  {
>  	struct drm_device *drm = dev_get_drvdata(dev);
> +	struct tegra_drm *tegra = drm->dev_private;
>  
>  	drm_kms_helper_poll_disable(drm);
> +	tegra_drm_fb_suspend(drm);
> +
> +	tegra->state = drm_atomic_helper_suspend(drm);
> +	if (IS_ERR(tegra->state)) {
> +		drm_kms_helper_poll_enable(drm);
> +		return PTR_ERR(tegra->state);
> +	}
>  
>  	return 0;
>  }
> @@ -1031,7 +1039,10 @@ static int host1x_drm_suspend(struct device *dev)
>  static int host1x_drm_resume(struct device *dev)
>  {
>  	struct drm_device *drm = dev_get_drvdata(dev);
> +	struct tegra_drm *tegra = drm->dev_private;
>  
> +	drm_atomic_helper_resume(drm, tegra->state);
> +	tegra_drm_fb_resume(drm);
>  	drm_kms_helper_poll_enable(drm);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 567831191f8b..112e07d81557 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -57,6 +57,8 @@ struct tegra_drm {
>  		struct work_struct work;
>  		struct mutex lock;
>  	} commit;
> +
> +	struct drm_atomic_state *state;
>  };
>  
>  struct tegra_drm_client;
> @@ -267,6 +269,8 @@ int tegra_drm_fb_prepare(struct drm_device *drm);
>  void tegra_drm_fb_free(struct drm_device *drm);
>  int tegra_drm_fb_init(struct drm_device *drm);
>  void tegra_drm_fb_exit(struct drm_device *drm);
> +void tegra_drm_fb_suspend(struct drm_device *drm);
> +void tegra_drm_fb_resume(struct drm_device *drm);
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
>  void tegra_fbdev_restore_mode(struct tegra_fbdev *fbdev);
>  void tegra_fb_output_poll_changed(struct drm_device *drm);
> diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
> index 9b8aa967fa71..6c60795a2622 100644
> --- a/drivers/gpu/drm/tegra/fb.c
> +++ b/drivers/gpu/drm/tegra/fb.c
> @@ -10,6 +10,8 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/console.h>
> +
>  #include "drm.h"
>  #include "gem.h"
>  
> @@ -414,3 +416,25 @@ void tegra_drm_fb_exit(struct drm_device *drm)
>  	tegra_fbdev_exit(tegra->fbdev);
>  #endif
>  }
> +
> +void tegra_drm_fb_suspend(struct drm_device *drm)
> +{
> +#ifdef CONFIG_DRM_FBDEV_EMULATION
> +	struct tegra_drm *tegra = drm->dev_private;
> +
> +	console_lock();
> +	drm_fb_helper_set_suspend(&tegra->fbdev->base, 1);
> +	console_unlock();
> +#endif

Should we move console_lock/unlock into the helper to avoid ugly ifdefery
like here? Just an aside really. We would need an _unlocked version then
though for i915, which tries to avoid the console_lock overhead with a
trylock and async worker. Or we could just move this into the fbdev helper
library too.
-Daniel

> +}
> +
> +void tegra_drm_fb_resume(struct drm_device *drm)
> +{
> +#ifdef CONFIG_DRM_FBDEV_EMULATION
> +	struct tegra_drm *tegra = drm->dev_private;
> +
> +	console_lock();
> +	drm_fb_helper_set_suspend(&tegra->fbdev->base, 0);
> +	console_unlock();
> +#endif
> +}
> -- 
> 2.4.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm/tegra: Implement subsystem-level suspend/resume

[Emil Velikov]

On 12 August 2015 at 15:37, Thierry Reding <thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Use the drm_atomic_helper_suspend() and drm_atomic_helper_resume()
> helpers to implement subsystem-level suspend/resume.
>
> v2: suspend framebuffer device to avoid concurrency issues
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/drm.c | 11 +++++++++++
>  drivers/gpu/drm/tegra/drm.h |  4 ++++
>  drivers/gpu/drm/tegra/fb.c  | 24 ++++++++++++++++++++++++
>  3 files changed, 39 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index b5f24c0f93dc..10d62e3077de 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1022,8 +1022,16 @@ static int host1x_drm_remove(struct host1x_device *dev)
>  static int host1x_drm_suspend(struct device *dev)
>  {
>         struct drm_device *drm = dev_get_drvdata(dev);
> +       struct tegra_drm *tegra = drm->dev_private;
>
>         drm_kms_helper_poll_disable(drm);
> +       tegra_drm_fb_suspend(drm);
> +
> +       tegra->state = drm_atomic_helper_suspend(drm);
> +       if (IS_ERR(tegra->state)) {
I'd assume that tegra_drm_fb_resume(drm) should be used here ?

> +               drm_kms_helper_poll_enable(drm);
> +               return PTR_ERR(tegra->state);
> +       }
>
>         return 0;
>  }

Cheers
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel