All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] drm: Add PSR helpers
@ 2019-02-28 21:09 Sean Paul
  2019-02-28 21:09 ` [PATCH 1/5] drm: Add helpers to kick off PSR enable/disable Sean Paul
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Sean Paul @ 2019-02-28 21:09 UTC (permalink / raw)
  To: dri-devel; +Cc: Sean Paul

From: Sean Paul <seanpaul@chromium.org>

Hey all,
Here's a set to add some helpers for drivers looking to implement PSR.
I've converted rockchip as a proof of concept, and will likely do the
work for msm dsi sometime in the coming months.

PTAL,

Sean


Sean Paul (5):
  drm: Add helpers to kick off PSR enable/disable
  drm/rockchip: Check for fast link training before enabling psr
  drm/rockchip: Use the helpers for PSR
  drm/rockchip: Don't fully disable vop on PSR
  drm/rockchip: Use drm_atomic_helper_commit_tail_rpm

 Documentation/gpu/drm-kms-helpers.rst         |   9 +
 drivers/gpu/drm/Makefile                      |   2 +-
 .../drm/bridge/analogix/analogix_dp_core.c    | 206 +++++++----
 .../drm/bridge/analogix/analogix_dp_core.h    |   1 -
 drivers/gpu/drm/drm_atomic_helper.c           |  34 ++
 drivers/gpu/drm/drm_atomic_uapi.c             |   5 +
 drivers/gpu/drm/drm_fb_helper.c               |   9 +
 drivers/gpu/drm/drm_framebuffer.c             |  18 +
 drivers/gpu/drm/drm_psr_helper.c              | 343 ++++++++++++++++++
 drivers/gpu/drm/rockchip/Makefile             |   3 +-
 .../gpu/drm/rockchip/analogix_dp-rockchip.c   |  70 ++--
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |  12 +-
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c    |  37 +-
 drivers/gpu/drm/rockchip/rockchip_drm_psr.c   | 290 ---------------
 drivers/gpu/drm/rockchip/rockchip_drm_psr.h   |  30 --
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  59 ++-
 include/drm/bridge/analogix_dp.h              |   4 +-
 include/drm/drm_connector.h                   |  22 ++
 include/drm/drm_crtc.h                        |  11 +
 include/drm/drm_mode_config.h                 |   6 +
 include/drm/drm_psr_helper.h                  |  24 ++
 21 files changed, 699 insertions(+), 496 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_psr_helper.c
 delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.c
 delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.h
 create mode 100644 include/drm/drm_psr_helper.h

-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH 1/5] drm: Add helpers to kick off PSR enable/disable
  2019-02-28 21:09 [PATCH 0/5] drm: Add PSR helpers Sean Paul
@ 2019-02-28 21:09 ` Sean Paul
  2019-02-28 22:29   ` Souza, Jose
  2019-03-01  8:18   ` Daniel Vetter
  2019-02-28 21:09 ` [PATCH 2/5] drm/rockchip: Check for fast link training before enabling psr Sean Paul
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 10+ messages in thread
From: Sean Paul @ 2019-02-28 21:09 UTC (permalink / raw)
  To: dri-devel
  Cc: Zain Wang, David Airlie, Tomasz Figa, Maxime Ripard, Sean Paul,
	Sean Paul

From: Sean Paul <seanpaul@chromium.org>

This patch adds a new drm helper library to help drivers implement
PSR. Drivers choosing to use it will register connectors with
PSR-capable displays connected and will receive callbacks when it's time
to enter or exit PSR.

In its current form, it has a timer which will trigger after a
driver-specified amount of inactivity. When the timer triggers, the
helpers will save the current atomic state and issue a new state which
has the PSR-enabled pipes turned off. On the next update, the drm core
will poke the PSR helpers to restore the saved state to the driver before
servicing said update.

From the driver's perspective, this works like a regular disable/enable
cycle. The driver need only check the 'psr_transition' state in
connector_state and keep the panel turned on when in .disable(), while
everything else will cycle off as normal. If drivers want more control,
they can use the psr_transition state to enter a low-power state to
minimize PSR exit time.

While this carries the PSR moniker, it is not specific to the
DisplayPort technology. This can be used for power savings with other
types of self refresh, such as MIPI command mode.

Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 Documentation/gpu/drm-kms-helpers.rst |   9 +
 drivers/gpu/drm/Makefile              |   2 +-
 drivers/gpu/drm/drm_atomic_helper.c   |  34 +++
 drivers/gpu/drm/drm_atomic_uapi.c     |   5 +
 drivers/gpu/drm/drm_fb_helper.c       |   9 +
 drivers/gpu/drm/drm_framebuffer.c     |  18 ++
 drivers/gpu/drm/drm_psr_helper.c      | 343 ++++++++++++++++++++++++++
 include/drm/drm_connector.h           |  22 ++
 include/drm/drm_crtc.h                |  11 +
 include/drm/drm_mode_config.h         |   6 +
 include/drm/drm_psr_helper.h          |  24 ++
 11 files changed, 482 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_psr_helper.c
 create mode 100644 include/drm/drm_psr_helper.h

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 17ca7f8bf3d3..d218a113bd52 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -107,6 +107,15 @@ fbdev Helper Functions Reference
 .. kernel-doc:: drivers/gpu/drm/drm_fb_helper.c
    :export:
 
+Panel Self Refresh Helper Reference
+===================================
+
+.. kernel-doc:: drivers/gpu/drm/drm_psr_helper.c
+   :doc: overview
+
+.. kernel-doc:: drivers/gpu/drm/drm_psr_helper.c
+   :export:
+
 Framebuffer CMA Helper Functions Reference
 ==========================================
 
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1ac55c65eac0..bff80fb946c7 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -19,7 +19,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_plane.o drm_color_mgmt.o drm_print.o \
 		drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
 		drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
-		drm_atomic_uapi.o
+		drm_atomic_uapi.o drm_psr_helper.o
 
 drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
 drm-$(CONFIG_DRM_VM) += drm_vm.o
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index c53ecbd9abdd..f5284d55f170 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
 #include <drm/drm_atomic_uapi.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_psr_helper.h>
 #include <drm/drm_writeback.h>
 #include <drm/drm_damage_helper.h>
 #include <linux/dma-fence.h>
@@ -2746,6 +2747,10 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane,
 	struct drm_plane_state *plane_state;
 	int ret = 0;
 
+	ret = drm_psr_helper_flush(plane->dev, ctx);
+	if (ret)
+		return ret;
+
 	state = drm_atomic_state_alloc(plane->dev);
 	if (!state)
 		return -ENOMEM;
@@ -2797,6 +2802,10 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane,
 	struct drm_plane_state *plane_state;
 	int ret = 0;
 
+	ret = drm_psr_helper_flush(plane->dev, ctx);
+	if (ret)
+		return ret;
+
 	state = drm_atomic_state_alloc(plane->dev);
 	if (!state)
 		return -ENOMEM;
@@ -2934,11 +2943,16 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set,
 	struct drm_crtc *crtc = set->crtc;
 	int ret = 0;
 
+	ret = drm_psr_helper_flush(crtc->dev, ctx);
+	if (ret)
+		return ret;
+
 	state = drm_atomic_state_alloc(crtc->dev);
 	if (!state)
 		return -ENOMEM;
 
 	state->acquire_ctx = ctx;
+
 	ret = __drm_atomic_helper_set_config(set, state);
 	if (ret != 0)
 		goto fail;
@@ -3139,6 +3153,10 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
 
 	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
 
+	ret = drm_psr_helper_flush(dev, &ctx);
+	if (ret)
+		DRM_ERROR("PSR flush during shutdown failed with %i\n", ret);
+
 	ret = __drm_atomic_helper_disable_all(dev, &ctx, true);
 	if (ret)
 		DRM_ERROR("Disabling all crtc's during unload failed with %i\n", ret);
@@ -3271,6 +3289,10 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
 
 	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
 
+	err = drm_psr_helper_flush(dev, &ctx);
+	if (err)
+		goto unlock;
+
 	state = drm_atomic_helper_duplicate_state(dev, &ctx);
 	if (IS_ERR(state))
 		goto unlock;
@@ -3436,6 +3458,10 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 	struct drm_atomic_state *state;
 	int ret = 0;
 
+	ret = drm_psr_helper_flush(crtc->dev, ctx);
+	if (ret)
+		return ret;
+
 	state = drm_atomic_state_alloc(plane->dev);
 	if (!state)
 		return -ENOMEM;
@@ -3481,6 +3507,10 @@ int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc,
 	struct drm_crtc_state *crtc_state;
 	int ret = 0;
 
+	ret = drm_psr_helper_flush(plane->dev, ctx);
+	if (ret)
+		return ret;
+
 	state = drm_atomic_state_alloc(plane->dev);
 	if (!state)
 		return -ENOMEM;
@@ -3532,6 +3562,10 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
 	int i, ret = 0;
 	bool replaced;
 
+	ret = drm_psr_helper_flush(dev, ctx);
+	if (ret)
+		return ret;
+
 	state = drm_atomic_state_alloc(crtc->dev);
 	if (!state)
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 4eb81f10bc54..21d7771d6ec7 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -29,6 +29,7 @@
 #include <drm/drm_atomic_uapi.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_print.h>
+#include <drm/drm_psr_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_writeback.h>
 #include <drm/drm_vblank.h>
@@ -1314,6 +1315,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	fence_state = NULL;
 	num_fences = 0;
 
+	ret = drm_psr_helper_flush(dev, state->acquire_ctx);
+	if (ret)
+		goto out;
+
 	for (i = 0; i < arg->count_objs; i++) {
 		uint32_t obj_id, count_props;
 		struct drm_mode_object *obj;
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 04d23cb430bf..a56ae2acda90 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -41,6 +41,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_psr_helper.h>
 
 #include "drm_crtc_internal.h"
 #include "drm_crtc_helper_internal.h"
@@ -406,6 +407,10 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
 
 	state->acquire_ctx = &ctx;
 retry:
+	ret = drm_psr_helper_flush(dev, state->acquire_ctx);
+	if (ret)
+		goto out_state;
+
 	drm_for_each_plane(plane, dev) {
 		plane_state = drm_atomic_get_plane_state(state, plane);
 		if (IS_ERR(plane_state)) {
@@ -1442,6 +1447,10 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
 
 	state->acquire_ctx = &ctx;
 retry:
+	ret = drm_psr_helper_flush(dev, &ctx);
+	if (ret)
+		goto out_state;
+
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		crtc = fb_helper->crtc_info[i].mode_set.crtc;
 
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index d8d75e25f6fb..43b029a88578 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -27,6 +27,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_uapi.h>
 #include <drm/drm_print.h>
+#include <drm/drm_psr_helper.h>
 #include <drm/drm_util.h>
 
 #include "drm_internal.h"
@@ -572,6 +573,7 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
 	struct drm_clip_rect __user *clips_ptr;
 	struct drm_clip_rect *clips = NULL;
 	struct drm_mode_fb_dirty_cmd *r = data;
+	struct drm_modeset_acquire_ctx ctx;
 	struct drm_framebuffer *fb;
 	unsigned flags;
 	int num_clips;
@@ -580,10 +582,24 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
+	drm_modeset_acquire_init(&ctx, 0);
+
 	fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id);
 	if (!fb)
 		return -ENOENT;
 
+retry:
+	ret = drm_psr_helper_flush(fb->dev, &ctx);
+	if (ret) {
+		if (ret == -EDEADLK) {
+			ret = drm_modeset_backoff(&ctx);
+			if (!ret)
+				goto retry;
+		} else {
+			goto out_err1;
+		}
+	}
+
 	num_clips = r->num_clips;
 	clips_ptr = (struct drm_clip_rect __user *)(unsigned long)r->clips_ptr;
 
@@ -630,6 +646,8 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
 	kfree(clips);
 out_err1:
 	drm_framebuffer_put(fb);
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/drm_psr_helper.c b/drivers/gpu/drm/drm_psr_helper.c
new file mode 100644
index 000000000000..0b57a2a53075
--- /dev/null
+++ b/drivers/gpu/drm/drm_psr_helper.c
@@ -0,0 +1,343 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright (C) 2019 Google, Inc.
+ *
+ * Authors:
+ * Sean Paul <seanpaul@chromium.org>
+ */
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_device.h>
+#include <drm/drm_mode_config.h>
+#include <drm/drm_modeset_lock.h>
+#include <drm/drm_print.h>
+#include <drm/drm_psr_helper.h>
+#include <linux/bitops.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+/**
+ * DOC: overview
+ *
+ * This helper library provides an easy way for drivers to leverage the atomic
+ * framework to implement panel self refresh (PSR) support. Drivers are
+ * responsible for intializing and cleaning up the PSR helpers on load/unload.
+ * When drivers identify a display that supports self refreshing (eDP or MIPI
+ * command mode), it should register the affected connector with the PSR
+ * helpers.
+ *
+ * Once a connector is registered, the PSR helpers will monitor activity and
+ * call back into the driver to enable/disable PSR as appropriate. The best way
+ * to think about this is that it's a DPMS on/off request with a flag set in
+ * state that tells you to disable/enable PSR on the panel instead of power-
+ * cycling it.
+ *
+ * Drivers may choose to fully disable their crtc/encoder/bridge hardware, or
+ * they can use the "psr_transition" flag in crtc and connector state if they
+ * want to enter low power mode without full disable (in case full
+ * disable/enable is too slow).
+ *
+ * PSR will be deactivated if there are any atomic updates, even updates that do
+ * not affect the connectors which are self refreshing. Supporting this is
+ * possible but non-trivial due to sharing of hardware resources. Similarly, if
+ * a crtc is driving multiple connectors, PSR will not be initiated on any of
+ * those connectors.
+ */
+
+struct drm_psr_state {
+	struct drm_device *dev;
+	struct drm_modeset_lock mutex;
+	struct delayed_work entry_work;
+	struct drm_atomic_state *save_state;
+	unsigned int entry_delay_ms;
+};
+
+static void drm_psr_helper_entry_work(struct work_struct *work)
+{
+	struct drm_psr_state *psr_state = container_of(to_delayed_work(work),
+						       struct drm_psr_state,
+						       entry_work);
+	struct drm_atomic_state *save_state;
+	struct drm_device *dev = psr_state->dev;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_atomic_state *state;
+	struct drm_connector *conn;
+	struct drm_connector_list_iter conn_iter;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	bool commit = false;
+	int ret, i;
+
+	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
+
+	ret = drm_modeset_lock(&psr_state->mutex, &ctx);
+	if (ret)
+		goto out;
+
+	/*
+	 * The only way this can happen is if we schedule the worker while it's
+	 * already running and that timeout subsequently elapses. Since we hold
+	 * the psr_state->mutex when scheduling, we also know where the worker
+	 * is sitting in its execution (hint: look up). In this case, it's
+	 * possible for the entry worker to run twice for the same commit. Since
+	 * the hardware hasn't changed since the last save state, just kick out.
+	 */
+	if (psr_state->save_state)
+		goto out;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	save_state = drm_atomic_helper_duplicate_state(dev, &ctx);
+
+	/*
+	 * Now that we have the current the HW state saved, we have to flip the
+	 * psr_transition bit so we know what type of enable we're dealing with
+	 * when coming back on.
+	 *
+	 * NOTE: We don't check conn->capable here since that could change out
+	 * from under us. We'll trust the atomic core to only call enable if
+	 * necessary (ie: only for those connectors/crtcs that currently have
+	 * psr enabled).
+	 */
+	if (IS_ERR(save_state)) {
+		ret = PTR_ERR(save_state);
+		goto out;
+	}
+	for_each_new_connector_in_state(save_state, conn, conn_state, i) {
+		if (!conn_state->crtc)
+			continue;
+		conn_state->psr_transition = true;
+	}
+	for_each_new_crtc_in_state(save_state, crtc, crtc_state, i) {
+		if (!crtc_state->active)
+			continue;
+		crtc_state->psr_transition = true;
+	}
+
+	state->acquire_ctx = &ctx;
+	drm_connector_list_iter_begin(psr_state->dev, &conn_iter);
+	drm_for_each_connector_iter(conn, &conn_iter) {
+		if (!conn->psr_capable)
+			continue;
+
+		conn_state = drm_atomic_get_connector_state(state, conn);
+		if (IS_ERR(conn_state)) {
+			ret = PTR_ERR(conn_state);
+			drm_connector_list_iter_end(&conn_iter);
+			goto out_free_state;
+		}
+
+		if (!conn_state->crtc)
+			continue;
+
+		crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
+		if (IS_ERR(crtc_state)) {
+			ret = PTR_ERR(crtc_state);
+			drm_connector_list_iter_end(&conn_iter);
+			goto out_free_state;
+		}
+
+		/* Don't use PSR if the crtc is driving multiple connectors */
+		if (hweight_long(crtc_state->connector_mask) > 1)
+			continue;
+
+		commit = true;
+		crtc_state->active = false;
+		crtc_state->psr_transition = true;
+		conn_state->psr_transition = true;
+	}
+	drm_connector_list_iter_end(&conn_iter);
+
+	/* Nothing to commit, so just exit */
+	if (!commit)
+		goto out_free_state;
+
+	ret = drm_atomic_commit(state);
+	if (ret)
+		goto out_free_state;
+
+	psr_state->save_state = save_state;
+	goto out;
+
+out_free_state:
+	drm_atomic_state_put(save_state);
+	drm_atomic_state_put(state);
+out:
+	DRM_MODESET_LOCK_ALL_END(ctx, ret);
+}
+
+static int
+drm_psr_helper_acquire_modeset_locks(struct drm_atomic_state *state,
+				     struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_mode_config *config = &state->dev->mode_config;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *new_crtc_state;
+	struct drm_plane *plane;
+	struct drm_plane_state *new_plane_state;
+	int ret, i;
+
+	ret = drm_modeset_lock(&config->connection_mutex, ctx);
+	if (ret)
+		return ret;
+
+	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
+		ret = drm_modeset_lock(&plane->mutex, ctx);
+		if (ret)
+			return ret;
+	}
+
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+		ret = drm_modeset_lock(&crtc->mutex, ctx);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+/**
+ * drm_psr_helper_flush - Restore the hardware to pre-PSR state
+ * @dev: DRM device
+ * @ctx: An acquire context to use for restoring the state
+ *
+ * This function should be called before every drm_atomic_commit to ensure any
+ * connectors that are currently self-refreshing revert back to being bus
+ * driven. Drivers may call this function outside of the atomic hooks if
+ * they wish to disable PSR pre-emptively (such as upon an input event or when
+ * GPU becomes active).
+ *
+ * If everything is successful, this function will schedule the PSR entry worker
+ * to enable PSR after the driver-specified timeout.
+ *
+ * If the PSR helper is not being used, this is a no-op.
+ */
+int drm_psr_helper_flush(struct drm_device *dev,
+			 struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_psr_state *psr_state = config->psr_state;
+	int ret;
+
+	if (!psr_state)
+		return 0;
+
+	ret = drm_modeset_lock(&psr_state->mutex, ctx);
+	if (ret)
+		return ret;
+
+	if (!psr_state->save_state)
+		goto out;
+
+	ret = drm_psr_helper_acquire_modeset_locks(psr_state->save_state, ctx);
+	if (ret)
+		goto out;
+
+	ret = drm_atomic_helper_commit_duplicated_state(psr_state->save_state,
+							ctx);
+
+out:
+	psr_state->save_state = NULL;
+	if (!ret) {
+		mod_delayed_work(system_wq, &psr_state->entry_work,
+				 msecs_to_jiffies(psr_state->entry_delay_ms));
+	}
+	return ret;
+}
+EXPORT_SYMBOL(drm_psr_helper_flush);
+
+/**
+ * drm_psr_helper_register - Registers a connector with the PSR helpers
+ * @connector: the connector which has a PSR-supported display attached
+ *
+ * Note that this can be called once on initialization for fixed panels, or
+ * during enable/hotplug.
+ */
+int drm_psr_helper_register(struct drm_connector *connector)
+{
+	struct drm_mode_config *config = &connector->dev->mode_config;
+	struct drm_psr_state *psr_state = config->psr_state;
+
+	/* PSR helpers are uninitialized */
+	if (WARN_ON(!psr_state))
+		return -EINVAL;
+
+	/* Acquired via psr_helper_flush */
+	if (!drm_modeset_is_locked(&psr_state->mutex))
+		return -EINVAL;
+
+	connector->psr_capable = true;
+	return 0;
+}
+EXPORT_SYMBOL(drm_psr_helper_register);
+
+/**
+ * drm_psr_helper_unregister - Unregisters a connector with the PSR helpers
+ * @connector: the connector to unregister
+ */
+int drm_psr_helper_unregister(struct drm_connector *connector)
+{
+	struct drm_mode_config *config = &connector->dev->mode_config;
+	struct drm_psr_state *psr_state = config->psr_state;
+
+	/* PSR helpers are uninitialized */
+	if (WARN_ON(!psr_state))
+		return -EINVAL;
+
+	/* Acquired via psr_helper_flush */
+	if (!drm_modeset_is_locked(&psr_state->mutex))
+		return -EINVAL;
+
+	connector->psr_capable = false;
+	return 0;
+}
+EXPORT_SYMBOL(drm_psr_helper_unregister);
+
+/**
+ * drm_psr_helper_init - Initializes the PSR helpers
+ * @dev: DRM device
+ * @entry_delay_ms: The amount of time to wait after an atomic commit before
+ *		    activating PSR
+ *
+ * Drivers using the PSR helpers must call this some time after mode_config
+ * is initialized in order to make use of the PSR helpers. Typically
+ * entry_delay_ms is a function of how quickly the hardware can enter/exit PSR.
+ */
+int drm_psr_helper_init(struct drm_device *dev, unsigned int entry_delay_ms)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_psr_state *psr_state;
+
+	psr_state = kzalloc(sizeof(*psr_state), GFP_KERNEL);
+	if (!psr_state)
+		return -ENOMEM;
+
+	INIT_DELAYED_WORK(&psr_state->entry_work, drm_psr_helper_entry_work);
+	drm_modeset_lock_init(&psr_state->mutex);
+	psr_state->dev = dev;
+	psr_state->entry_delay_ms = entry_delay_ms;
+
+	config->psr_state = psr_state;
+	return 0;
+}
+EXPORT_SYMBOL(drm_psr_helper_init);
+
+/**
+ * drm_psr_helper_cleanup - De-initializes the PSR helpers
+ * @dev: DRM device
+ */
+void drm_psr_helper_cleanup(struct drm_device *dev)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+
+	cancel_delayed_work_sync(&config->psr_state->entry_work);
+	kfree(config->psr_state);
+	config->psr_state = NULL;
+}
+EXPORT_SYMBOL(drm_psr_helper_cleanup);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index c8061992d6cb..9612b3d56f30 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -501,6 +501,17 @@ struct drm_connector_state {
 	/** @tv: TV connector state */
 	struct drm_tv_connector_state tv;
 
+	/**
+	 * @psr_transition:
+	 *
+	 * Used by the PSR helpers to denote when a PSR transition is occuring.
+	 * If your connector is PSR-capable, register it with the helpers and
+	 * check this flag in .enable() and .disable(). If it is true, instead
+	 * of shutting off the panel, put it in or take it out of self
+	 * refreshing mode.
+	 */
+	bool psr_transition;
+
 	/**
 	 * @picture_aspect_ratio: Connector property to control the
 	 * HDMI infoframe aspect ratio setting.
@@ -993,6 +1004,17 @@ struct drm_connector {
 	 */
 	struct drm_display_info display_info;
 
+	/**
+	 * @psr_capable:
+	 *
+	 * Set by the driver via drm_psr_helper_register(). Signals that this
+	 * connector (and associated pipe) is PSR capable and should be put in
+	 * low-power mode when it is inactive.
+	 *
+	 * Protected by &drm_mode_config.psr_state.mutex
+	 */
+	bool psr_capable;
+
 	/** @funcs: connector control functions */
 	const struct drm_connector_funcs *funcs;
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f7c3022dbdf4..10acd4fc0991 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -299,6 +299,17 @@ struct drm_crtc_state {
 	 */
 	bool vrr_enabled;
 
+	/**
+	 * @psr_transition:
+	 *
+	 * Used by the PSR helpers to denote when a PSR transition is occuring.
+	 * This will be set on enable/disable callbacks when PSR is being
+	 * enabled or disabled. In some cases, it may not be desirable to fully
+	 * shut off the crtc during PSR. CRTC's can inspect this flag and
+	 * determine the best course of action.
+	 */
+	bool psr_transition;
+
 	/**
 	 * @event:
 	 *
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 7f60e8eb269a..371b80d090ab 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -37,6 +37,7 @@ struct drm_atomic_state;
 struct drm_mode_fb_cmd2;
 struct drm_format_info;
 struct drm_display_mode;
+struct drm_psr_state;
 
 /**
  * struct drm_mode_config_funcs - basic driver provided mode setting functions
@@ -900,6 +901,11 @@ struct drm_mode_config {
 	 */
 	struct drm_atomic_state *suspend_state;
 
+	/**
+	 * @psr_state: Holds the state for the psr helper
+	 */
+	struct drm_psr_state *psr_state;
+
 	const struct drm_mode_config_helper_funcs *helper_private;
 };
 
diff --git a/include/drm/drm_psr_helper.h b/include/drm/drm_psr_helper.h
new file mode 100644
index 000000000000..972c4ec98d05
--- /dev/null
+++ b/include/drm/drm_psr_helper.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright (C) 2019 Google, Inc.
+ *
+ * Authors:
+ * Sean Paul <seanpaul@chromium.org>
+ */
+#ifndef DRM_PSR_HELPER_H_
+#define DRM_PSR_HELPER_H_
+
+struct drm_connector;
+struct drm_device;
+struct drm_psr_state;
+struct drm_modeset_acquire_ctx;
+
+int drm_psr_helper_flush(struct drm_device *dev,
+			 struct drm_modeset_acquire_ctx *ctx);
+
+int drm_psr_helper_register(struct drm_connector *connector);
+int drm_psr_helper_unregister(struct drm_connector *connector);
+
+int drm_psr_helper_init(struct drm_device *dev, unsigned int entry_delay_ms);
+void drm_psr_helper_cleanup(struct drm_device *dev);
+#endif
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 2/5] drm/rockchip: Check for fast link training before enabling psr
  2019-02-28 21:09 [PATCH 0/5] drm: Add PSR helpers Sean Paul
  2019-02-28 21:09 ` [PATCH 1/5] drm: Add helpers to kick off PSR enable/disable Sean Paul
@ 2019-02-28 21:09 ` Sean Paul
  2019-02-28 21:09 ` [PATCH 3/5] drm/rockchip: Use the helpers for PSR Sean Paul
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Sean Paul @ 2019-02-28 21:09 UTC (permalink / raw)
  To: dri-devel
  Cc: Zain Wang, David Airlie, Tomasz Figa, Sean Paul, Laurent Pinchart

From: Sean Paul <seanpaul@chromium.org>

Once we start shutting off the link during PSR, we're going to want fast
training to work. If the display doesn't support fast training, don't
enable psr.

Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 225f5e5dd69b..af34554a5a02 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1040,16 +1040,17 @@ static int analogix_dp_commit(struct analogix_dp_device *dp)
 	if (ret)
 		return ret;
 
+	/* Check whether panel supports fast training */
+	ret = analogix_dp_fast_link_train_detection(dp);
+	if (ret)
+		dp->psr_enable = false;
+
 	if (dp->psr_enable) {
 		ret = analogix_dp_enable_sink_psr(dp);
 		if (ret)
 			return ret;
 	}
 
-	/* Check whether panel supports fast training */
-	ret =  analogix_dp_fast_link_train_detection(dp);
-	if (ret)
-		dp->psr_enable = false;
 
 	return ret;
 }
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 3/5] drm/rockchip: Use the helpers for PSR
  2019-02-28 21:09 [PATCH 0/5] drm: Add PSR helpers Sean Paul
  2019-02-28 21:09 ` [PATCH 1/5] drm: Add helpers to kick off PSR enable/disable Sean Paul
  2019-02-28 21:09 ` [PATCH 2/5] drm/rockchip: Check for fast link training before enabling psr Sean Paul
@ 2019-02-28 21:09 ` Sean Paul
  2019-02-28 21:09 ` [PATCH 4/5] drm/rockchip: Don't fully disable vop on PSR Sean Paul
  2019-02-28 21:09 ` [PATCH 5/5] drm/rockchip: Use drm_atomic_helper_commit_tail_rpm Sean Paul
  4 siblings, 0 replies; 10+ messages in thread
From: Sean Paul @ 2019-02-28 21:09 UTC (permalink / raw)
  To: dri-devel
  Cc: Zain Wang, David Airlie, Tomasz Figa, Sean Paul, Laurent Pinchart

From: Sean Paul <seanpaul@chromium.org>

Instead of rolling our own implementation for tracking when PSR should
be [in]active, use the PSR helpers to do the heavy lifting.

Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 205 ++++++++-----
 .../drm/bridge/analogix/analogix_dp_core.h    |   1 -
 drivers/gpu/drm/rockchip/Makefile             |   3 +-
 .../gpu/drm/rockchip/analogix_dp-rockchip.c   |  70 ++---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |  12 +-
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c    |  16 -
 drivers/gpu/drm/rockchip/rockchip_drm_psr.c   | 290 ------------------
 drivers/gpu/drm/rockchip/rockchip_drm_psr.h   |  30 --
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  21 +-
 include/drm/bridge/analogix_dp.h              |   4 +-
 10 files changed, 177 insertions(+), 475 deletions(-)
 delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.c
 delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.h

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index af34554a5a02..de98cb3570be 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -28,6 +28,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_psr_helper.h>
 
 #include <drm/bridge/analogix_dp.h>
 
@@ -106,63 +107,19 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
 	return 0;
 }
 
-int analogix_dp_psr_enabled(struct analogix_dp_device *dp)
+bool analogix_dp_in_psr_transition(struct analogix_dp_device *dp)
 {
+	struct drm_mode_config *config = &dp->drm_dev->mode_config;
 
-	return dp->psr_enable;
-}
-EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
+	/* This should only be called from the encoder enable/disable */
+	if (WARN_ON(!drm_modeset_is_locked(&config->connection_mutex)))
+		return false;
 
-int analogix_dp_enable_psr(struct analogix_dp_device *dp)
-{
-	struct edp_vsc_psr psr_vsc;
-
-	if (!dp->psr_enable)
-		return 0;
-
-	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
-	memset(&psr_vsc, 0, sizeof(psr_vsc));
-	psr_vsc.sdp_header.HB0 = 0;
-	psr_vsc.sdp_header.HB1 = 0x7;
-	psr_vsc.sdp_header.HB2 = 0x2;
-	psr_vsc.sdp_header.HB3 = 0x8;
-
-	psr_vsc.DB0 = 0;
-	psr_vsc.DB1 = EDP_VSC_PSR_STATE_ACTIVE | EDP_VSC_PSR_CRC_VALUES_VALID;
-
-	return analogix_dp_send_psr_spd(dp, &psr_vsc, true);
+	return dp->connector.state->psr_transition;
 }
-EXPORT_SYMBOL_GPL(analogix_dp_enable_psr);
+EXPORT_SYMBOL_GPL(analogix_dp_in_psr_transition);
 
-int analogix_dp_disable_psr(struct analogix_dp_device *dp)
-{
-	struct edp_vsc_psr psr_vsc;
-	int ret;
-
-	if (!dp->psr_enable)
-		return 0;
-
-	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
-	memset(&psr_vsc, 0, sizeof(psr_vsc));
-	psr_vsc.sdp_header.HB0 = 0;
-	psr_vsc.sdp_header.HB1 = 0x7;
-	psr_vsc.sdp_header.HB2 = 0x2;
-	psr_vsc.sdp_header.HB3 = 0x8;
-
-	psr_vsc.DB0 = 0;
-	psr_vsc.DB1 = 0;
-
-	ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-	if (ret != 1) {
-		dev_err(dp->dev, "Failed to set DP Power0 %d\n", ret);
-		return ret;
-	}
-
-	return analogix_dp_send_psr_spd(dp, &psr_vsc, false);
-}
-EXPORT_SYMBOL_GPL(analogix_dp_disable_psr);
-
-static int analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
+static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
 {
 	unsigned char psr_version;
 	int ret;
@@ -170,14 +127,11 @@ static int analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
 	ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_SUPPORT, &psr_version);
 	if (ret != 1) {
 		dev_err(dp->dev, "failed to get PSR version, disable it\n");
-		return ret;
+		return false;
 	}
 
 	dev_dbg(dp->dev, "Panel PSR version : %x\n", psr_version);
-
-	dp->psr_enable = (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
-
-	return 0;
+	return psr_version & DP_PSR_IS_SUPPORTED;
 }
 
 static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
@@ -200,7 +154,7 @@ static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
 	}
 
 	/* Main-Link transmitter remains active during PSR active states */
-	psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;
+	psr_en = DP_PSR_CRC_VERIFICATION;
 	ret = drm_dp_dpcd_writeb(&dp->aux, DP_PSR_EN_CFG, psr_en);
 	if (ret != 1) {
 		dev_err(dp->dev, "failed to set panel psr\n");
@@ -208,8 +162,7 @@ static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
 	}
 
 	/* Enable psr function */
-	psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
-		 DP_PSR_CRC_VERIFICATION;
+	psr_en = DP_PSR_ENABLE | DP_PSR_CRC_VERIFICATION;
 	ret = drm_dp_dpcd_writeb(&dp->aux, DP_PSR_EN_CFG, psr_en);
 	if (ret != 1) {
 		dev_err(dp->dev, "failed to set panel psr\n");
@@ -221,7 +174,6 @@ static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
 	return 0;
 end:
 	dev_err(dp->dev, "enable psr fail, force to disable psr\n");
-	dp->psr_enable = false;
 
 	return ret;
 }
@@ -1036,25 +988,94 @@ static int analogix_dp_commit(struct analogix_dp_device *dp)
 		}
 	}
 
-	ret = analogix_dp_detect_sink_psr(dp);
-	if (ret)
-		return ret;
-
 	/* Check whether panel supports fast training */
 	ret = analogix_dp_fast_link_train_detection(dp);
 	if (ret)
-		dp->psr_enable = false;
+		return ret;
 
-	if (dp->psr_enable) {
+	if (analogix_dp_detect_sink_psr(dp)) {
 		ret = analogix_dp_enable_sink_psr(dp);
 		if (ret)
 			return ret;
+
+		ret = drm_psr_helper_register(&dp->connector);
+		if (ret)
+			return ret;
 	}
 
+	return ret;
+}
+
+static int analogix_dp_enable_psr(struct analogix_dp_device *dp)
+{
+	struct edp_vsc_psr psr_vsc;
+	int ret;
+	u8 sink;
+
+	ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &sink);
+	if (ret != 1)
+		DRM_DEV_ERROR(dp->dev, "Failed to read psr status %d\n", ret);
+	else if (sink == DP_PSR_SINK_ACTIVE_RFB)
+		return 0;
+
+	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
+	memset(&psr_vsc, 0, sizeof(psr_vsc));
+	psr_vsc.sdp_header.HB0 = 0;
+	psr_vsc.sdp_header.HB1 = 0x7;
+	psr_vsc.sdp_header.HB2 = 0x2;
+	psr_vsc.sdp_header.HB3 = 0x8;
+	psr_vsc.DB0 = 0;
+	psr_vsc.DB1 = EDP_VSC_PSR_STATE_ACTIVE | EDP_VSC_PSR_CRC_VALUES_VALID;
+
+	ret = analogix_dp_send_psr_spd(dp, &psr_vsc, true);
+	if (!ret)
+		analogix_dp_set_analog_power_down(dp, POWER_ALL, true);
 
 	return ret;
 }
 
+static int analogix_dp_disable_psr(struct analogix_dp_device *dp)
+{
+	struct edp_vsc_psr psr_vsc;
+	int ret;
+	u8 sink;
+
+	analogix_dp_set_analog_power_down(dp, POWER_ALL, false);
+
+	ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
+	if (ret != 1) {
+		DRM_DEV_ERROR(dp->dev, "Failed to set DP Power0 %d\n", ret);
+		return ret;
+	}
+
+	ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &sink);
+	if (ret != 1) {
+		DRM_DEV_ERROR(dp->dev, "Failed to read psr status %d\n", ret);
+		return ret;
+	} else if (sink == DP_PSR_SINK_INACTIVE) {
+		DRM_DEV_ERROR(dp->dev, "sink inactive, skip disable psr");
+		return 0;
+	}
+
+	ret = analogix_dp_train_link(dp);
+	if (ret) {
+		DRM_DEV_ERROR(dp->dev, "Failed to train the link %d\n", ret);
+		return ret;
+	}
+
+	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
+	memset(&psr_vsc, 0, sizeof(psr_vsc));
+	psr_vsc.sdp_header.HB0 = 0;
+	psr_vsc.sdp_header.HB1 = 0x7;
+	psr_vsc.sdp_header.HB2 = 0x2;
+	psr_vsc.sdp_header.HB3 = 0x8;
+
+	psr_vsc.DB0 = 0;
+	psr_vsc.DB1 = 0;
+
+	return analogix_dp_send_psr_spd(dp, &psr_vsc, true);
+}
+
 /*
  * This function is a bit of a catch-all for panel preparation, hopefully
  * simplifying the logic of functions that need to prepare/unprepare the panel
@@ -1244,6 +1265,10 @@ static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
 	struct analogix_dp_device *dp = bridge->driver_private;
 	int ret;
 
+	/* Don't touch the panel if we're coming back from PSR */
+	if (dp->connector.state->psr_transition)
+		return;
+
 	ret = analogix_dp_prepare_panel(dp, true, true);
 	if (ret)
 		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
@@ -1309,6 +1334,14 @@ static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
 	struct analogix_dp_device *dp = bridge->driver_private;
 	int timeout_loop = 0;
 
+	/* Not a full enable, just disable PSR and continue */
+	if (dp->connector.state->psr_transition) {
+		int ret = analogix_dp_disable_psr(dp);
+		if (ret)
+			DRM_ERROR("Failed to disable psr %d\n", ret);
+		return;
+	}
+
 	if (dp->dpms_mode == DRM_MODE_DPMS_ON)
 		return;
 
@@ -1356,11 +1389,38 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 	if (ret)
 		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
 
-	dp->psr_enable = false;
+	drm_psr_helper_unregister(&dp->connector);
+
 	dp->fast_train_enable = false;
 	dp->dpms_mode = DRM_MODE_DPMS_OFF;
 }
 
+static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge)
+{
+	struct analogix_dp_device *dp = bridge->driver_private;
+	struct drm_connector_state *conn_state = dp->connector.state;
+
+	/* Don't do a full disable on PSR transitions */
+	if (conn_state->psr_transition)
+		return;
+
+	analogix_dp_bridge_disable(bridge);
+}
+
+static void analogix_dp_bridge_post_disable(struct drm_bridge *bridge)
+{
+	struct analogix_dp_device *dp = bridge->driver_private;
+	struct drm_connector_state *conn_state = dp->connector.state;
+	int ret;
+
+	if (conn_state->psr_transition) {
+		ret = analogix_dp_enable_psr(dp);
+		if (ret)
+			DRM_ERROR("Failed to enable psr (%d)\n", ret);
+		return;
+	}
+}
+
 static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
 				const struct drm_display_mode *orig_mode,
 				const struct drm_display_mode *mode)
@@ -1440,16 +1500,11 @@ static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
 		video->interlaced = true;
 }
 
-static void analogix_dp_bridge_nop(struct drm_bridge *bridge)
-{
-	/* do nothing */
-}
-
 static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
 	.pre_enable = analogix_dp_bridge_pre_enable,
 	.enable = analogix_dp_bridge_enable,
-	.disable = analogix_dp_bridge_disable,
-	.post_disable = analogix_dp_bridge_nop,
+	.disable = analogix_dp_bridge_atomic_disable,
+	.post_disable = analogix_dp_bridge_post_disable,
 	.mode_set = analogix_dp_bridge_mode_set,
 	.attach = analogix_dp_bridge_attach,
 };
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index 769255dc6e99..e4cdd69107e3 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -173,7 +173,6 @@ struct analogix_dp_device {
 	int			dpms_mode;
 	int			hpd_gpio;
 	bool                    force_hpd;
-	bool			psr_enable;
 	bool			fast_train_enable;
 
 	struct mutex		panel_lock;
diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
index f6fc9d5dd0ad..a777726c23cb 100644
--- a/drivers/gpu/drm/rockchip/Makefile
+++ b/drivers/gpu/drm/rockchip/Makefile
@@ -4,8 +4,7 @@
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
 rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
-		rockchip_drm_gem.o rockchip_drm_psr.o \
-		rockchip_drm_vop.o rockchip_vop_reg.o
+		rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
 rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
 
 rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index bc4423624209..26090b4ba4ee 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -32,7 +32,6 @@
 #include <drm/bridge/analogix_dp.h>
 
 #include "rockchip_drm_drv.h"
-#include "rockchip_drm_psr.h"
 #include "rockchip_drm_vop.h"
 
 #define RK3288_GRF_SOC_CON6		0x25c
@@ -77,29 +76,6 @@ struct rockchip_dp_device {
 	struct analogix_dp_plat_data plat_data;
 };
 
-static int analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
-{
-	struct rockchip_dp_device *dp = to_dp(encoder);
-	int ret;
-
-	if (!analogix_dp_psr_enabled(dp->adp))
-		return 0;
-
-	DRM_DEV_DEBUG(dp->dev, "%s PSR...\n", enabled ? "Entry" : "Exit");
-
-	ret = rockchip_drm_wait_vact_end(dp->encoder.crtc,
-					 PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
-	if (ret) {
-		DRM_DEV_ERROR(dp->dev, "line flag interrupt did not arrive\n");
-		return -ETIMEDOUT;
-	}
-
-	if (enabled)
-		return analogix_dp_enable_psr(dp->adp);
-	else
-		return analogix_dp_disable_psr(dp->adp);
-}
-
 static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
 {
 	reset_control_assert(dp->rst);
@@ -130,21 +106,9 @@ static int rockchip_dp_poweron_start(struct analogix_dp_plat_data *plat_data)
 	return ret;
 }
 
-static int rockchip_dp_poweron_end(struct analogix_dp_plat_data *plat_data)
-{
-	struct rockchip_dp_device *dp = to_dp(plat_data);
-
-	return rockchip_drm_psr_inhibit_put(&dp->encoder);
-}
-
 static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data)
 {
 	struct rockchip_dp_device *dp = to_dp(plat_data);
-	int ret;
-
-	ret = rockchip_drm_psr_inhibit_get(&dp->encoder);
-	if (ret != 0)
-		return ret;
 
 	clk_disable_unprepare(dp->pclk);
 
@@ -190,6 +154,9 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
 	int ret;
 	u32 val;
 
+	if (analogix_dp_in_psr_transition(dp->adp))
+		return;
+
 	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
 	if (ret < 0)
 		return;
@@ -214,9 +181,24 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
 	clk_disable_unprepare(dp->grfclk);
 }
 
-static void rockchip_dp_drm_encoder_nop(struct drm_encoder *encoder)
+static void rockchip_dp_drm_encoder_disable(struct drm_encoder *encoder)
 {
-	/* do nothing */
+	struct rockchip_dp_device *dp = to_dp(encoder);
+	struct drm_crtc *crtc;
+	int ret;
+
+	drm_for_each_crtc(crtc, dp->drm_dev) {
+		if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder)))
+			continue;
+
+		if (!crtc->state->psr_transition)
+			continue;
+
+		ret = rockchip_drm_wait_vact_end(crtc,
+						 PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
+		if (ret)
+			DRM_DEV_ERROR(dp->dev, "line flag irq timed out\n");
+	}
 }
 
 static int
@@ -246,7 +228,7 @@ static struct drm_encoder_helper_funcs rockchip_dp_encoder_helper_funcs = {
 	.mode_fixup = rockchip_dp_drm_encoder_mode_fixup,
 	.mode_set = rockchip_dp_drm_encoder_mode_set,
 	.enable = rockchip_dp_drm_encoder_enable,
-	.disable = rockchip_dp_drm_encoder_nop,
+	.disable = rockchip_dp_drm_encoder_disable,
 	.atomic_check = rockchip_dp_drm_encoder_atomic_check,
 };
 
@@ -338,23 +320,16 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 
 	dp->plat_data.dev_type = dp->data->chip_type;
 	dp->plat_data.power_on_start = rockchip_dp_poweron_start;
-	dp->plat_data.power_on_end = rockchip_dp_poweron_end;
 	dp->plat_data.power_off = rockchip_dp_powerdown;
 	dp->plat_data.get_modes = rockchip_dp_get_modes;
 
-	ret = rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
-	if (ret < 0)
-		goto err_cleanup_encoder;
-
 	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
 	if (IS_ERR(dp->adp)) {
 		ret = PTR_ERR(dp->adp);
-		goto err_unreg_psr;
+		goto err_cleanup_encoder;
 	}
 
 	return 0;
-err_unreg_psr:
-	rockchip_drm_psr_unregister(&dp->encoder);
 err_cleanup_encoder:
 	dp->encoder.funcs->destroy(&dp->encoder);
 	return ret;
@@ -366,7 +341,6 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
 	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
 
 	analogix_dp_unbind(dp->adp);
-	rockchip_drm_psr_unregister(&dp->encoder);
 	dp->encoder.funcs->destroy(&dp->encoder);
 
 	dp->adp = ERR_PTR(-ENODEV);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index d7fa17f12769..222258497398 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -19,6 +19,7 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_of.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_psr_helper.h>
 #include <linux/dma-mapping.h>
 #include <linux/dma-iommu.h>
 #include <linux/pm_runtime.h>
@@ -40,6 +41,8 @@
 #define DRIVER_MAJOR	1
 #define DRIVER_MINOR	0
 
+#define ROCKCHIP_DRM_PSR_ENTRY_DELAY_MS 100
+
 static bool is_support_iommu = true;
 static struct drm_driver rockchip_drm_driver;
 
@@ -145,10 +148,14 @@ static int rockchip_drm_bind(struct device *dev)
 
 	rockchip_drm_mode_config_init(drm_dev);
 
+	ret = drm_psr_helper_init(drm_dev, ROCKCHIP_DRM_PSR_ENTRY_DELAY_MS);
+	if (ret)
+		goto err_mode_config_cleanup;
+
 	/* Try to bind all sub drivers. */
 	ret = component_bind_all(dev, drm_dev);
 	if (ret)
-		goto err_mode_config_cleanup;
+		goto err_psr_helper_cleanup;
 
 	ret = drm_vblank_init(drm_dev, drm_dev->mode_config.num_crtc);
 	if (ret)
@@ -179,6 +186,8 @@ static int rockchip_drm_bind(struct device *dev)
 	rockchip_drm_fbdev_fini(drm_dev);
 err_unbind_all:
 	component_unbind_all(dev, drm_dev);
+err_psr_helper_cleanup:
+	drm_psr_helper_cleanup(drm_dev);
 err_mode_config_cleanup:
 	drm_mode_config_cleanup(drm_dev);
 	rockchip_iommu_cleanup(drm_dev);
@@ -200,6 +209,7 @@ static void rockchip_drm_unbind(struct device *dev)
 
 	drm_atomic_helper_shutdown(drm_dev);
 	component_unbind_all(dev, drm_dev);
+	drm_psr_helper_cleanup(drm_dev);
 	drm_mode_config_cleanup(drm_dev);
 	rockchip_iommu_cleanup(drm_dev);
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 97438bbbe389..7e121875e3c9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -23,22 +23,10 @@
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_fb.h"
 #include "rockchip_drm_gem.h"
-#include "rockchip_drm_psr.h"
-
-static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
-				 struct drm_file *file,
-				 unsigned int flags, unsigned int color,
-				 struct drm_clip_rect *clips,
-				 unsigned int num_clips)
-{
-	rockchip_drm_psr_flush_all(fb->dev);
-	return 0;
-}
 
 static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
 	.destroy       = drm_gem_fb_destroy,
 	.create_handle = drm_gem_fb_create_handle,
-	.dirty	       = rockchip_drm_fb_dirty,
 };
 
 static struct drm_framebuffer *
@@ -132,8 +120,6 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
 
-	rockchip_drm_psr_inhibit_get_state(old_state);
-
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
@@ -141,8 +127,6 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
 	drm_atomic_helper_commit_planes(dev, old_state,
 					DRM_PLANE_COMMIT_ACTIVE_ONLY);
 
-	rockchip_drm_psr_inhibit_put_state(old_state);
-
 	drm_atomic_helper_commit_hw_done(old_state);
 
 	drm_atomic_helper_wait_for_vblanks(dev, old_state);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
deleted file mode 100644
index a0c8bd235b67..000000000000
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
+++ /dev/null
@@ -1,290 +0,0 @@
-/*
- * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
- * Author: Yakir Yang <ykk@rock-chips.com>
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#include <drm/drmP.h>
-#include <drm/drm_atomic.h>
-#include <drm/drm_probe_helper.h>
-
-#include "rockchip_drm_drv.h"
-#include "rockchip_drm_psr.h"
-
-#define PSR_FLUSH_TIMEOUT_MS	100
-
-struct psr_drv {
-	struct list_head	list;
-	struct drm_encoder	*encoder;
-
-	struct mutex		lock;
-	int			inhibit_count;
-	bool			enabled;
-
-	struct delayed_work	flush_work;
-
-	int (*set)(struct drm_encoder *encoder, bool enable);
-};
-
-static struct psr_drv *find_psr_by_encoder(struct drm_encoder *encoder)
-{
-	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
-	struct psr_drv *psr;
-
-	mutex_lock(&drm_drv->psr_list_lock);
-	list_for_each_entry(psr, &drm_drv->psr_list, list) {
-		if (psr->encoder == encoder)
-			goto out;
-	}
-	psr = ERR_PTR(-ENODEV);
-
-out:
-	mutex_unlock(&drm_drv->psr_list_lock);
-	return psr;
-}
-
-static int psr_set_state_locked(struct psr_drv *psr, bool enable)
-{
-	int ret;
-
-	if (psr->inhibit_count > 0)
-		return -EINVAL;
-
-	if (enable == psr->enabled)
-		return 0;
-
-	ret = psr->set(psr->encoder, enable);
-	if (ret)
-		return ret;
-
-	psr->enabled = enable;
-	return 0;
-}
-
-static void psr_flush_handler(struct work_struct *work)
-{
-	struct psr_drv *psr = container_of(to_delayed_work(work),
-					   struct psr_drv, flush_work);
-
-	mutex_lock(&psr->lock);
-	psr_set_state_locked(psr, true);
-	mutex_unlock(&psr->lock);
-}
-
-/**
- * rockchip_drm_psr_inhibit_put - release PSR inhibit on given encoder
- * @encoder: encoder to obtain the PSR encoder
- *
- * Decrements PSR inhibit count on given encoder. Should be called only
- * for a PSR inhibit count increment done before. If PSR inhibit counter
- * reaches zero, PSR flush work is scheduled to make the hardware enter
- * PSR mode in PSR_FLUSH_TIMEOUT_MS.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-int rockchip_drm_psr_inhibit_put(struct drm_encoder *encoder)
-{
-	struct psr_drv *psr = find_psr_by_encoder(encoder);
-
-	if (IS_ERR(psr))
-		return PTR_ERR(psr);
-
-	mutex_lock(&psr->lock);
-	--psr->inhibit_count;
-	WARN_ON(psr->inhibit_count < 0);
-	if (!psr->inhibit_count)
-		mod_delayed_work(system_wq, &psr->flush_work,
-				 PSR_FLUSH_TIMEOUT_MS);
-	mutex_unlock(&psr->lock);
-
-	return 0;
-}
-EXPORT_SYMBOL(rockchip_drm_psr_inhibit_put);
-
-void rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state)
-{
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	struct drm_encoder *encoder;
-	u32 encoder_mask = 0;
-	int i;
-
-	for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
-		encoder_mask |= crtc_state->encoder_mask;
-		encoder_mask |= crtc->state->encoder_mask;
-	}
-
-	drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
-		rockchip_drm_psr_inhibit_get(encoder);
-}
-EXPORT_SYMBOL(rockchip_drm_psr_inhibit_get_state);
-
-void rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state)
-{
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	struct drm_encoder *encoder;
-	u32 encoder_mask = 0;
-	int i;
-
-	for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
-		encoder_mask |= crtc_state->encoder_mask;
-		encoder_mask |= crtc->state->encoder_mask;
-	}
-
-	drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
-		rockchip_drm_psr_inhibit_put(encoder);
-}
-EXPORT_SYMBOL(rockchip_drm_psr_inhibit_put_state);
-
-/**
- * rockchip_drm_psr_inhibit_get - acquire PSR inhibit on given encoder
- * @encoder: encoder to obtain the PSR encoder
- *
- * Increments PSR inhibit count on given encoder. This function guarantees
- * that after it returns PSR is turned off on given encoder and no PSR-related
- * hardware state change occurs at least until a matching call to
- * rockchip_drm_psr_inhibit_put() is done.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-int rockchip_drm_psr_inhibit_get(struct drm_encoder *encoder)
-{
-	struct psr_drv *psr = find_psr_by_encoder(encoder);
-
-	if (IS_ERR(psr))
-		return PTR_ERR(psr);
-
-	mutex_lock(&psr->lock);
-	psr_set_state_locked(psr, false);
-	++psr->inhibit_count;
-	mutex_unlock(&psr->lock);
-	cancel_delayed_work_sync(&psr->flush_work);
-
-	return 0;
-}
-EXPORT_SYMBOL(rockchip_drm_psr_inhibit_get);
-
-static void rockchip_drm_do_flush(struct psr_drv *psr)
-{
-	cancel_delayed_work_sync(&psr->flush_work);
-
-	mutex_lock(&psr->lock);
-	if (!psr_set_state_locked(psr, false))
-		mod_delayed_work(system_wq, &psr->flush_work,
-				 PSR_FLUSH_TIMEOUT_MS);
-	mutex_unlock(&psr->lock);
-}
-
-/**
- * rockchip_drm_psr_flush_all - force to flush all registered PSR encoders
- * @dev: drm device
- *
- * Disable the PSR function for all registered encoders, and then enable the
- * PSR function back after PSR_FLUSH_TIMEOUT. If encoder PSR state have been
- * changed during flush time, then keep the state no change after flush
- * timeout.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-void rockchip_drm_psr_flush_all(struct drm_device *dev)
-{
-	struct rockchip_drm_private *drm_drv = dev->dev_private;
-	struct psr_drv *psr;
-
-	mutex_lock(&drm_drv->psr_list_lock);
-	list_for_each_entry(psr, &drm_drv->psr_list, list)
-		rockchip_drm_do_flush(psr);
-	mutex_unlock(&drm_drv->psr_list_lock);
-}
-EXPORT_SYMBOL(rockchip_drm_psr_flush_all);
-
-/**
- * rockchip_drm_psr_register - register encoder to psr driver
- * @encoder: encoder that obtain the PSR function
- * @psr_set: call back to set PSR state
- *
- * The function returns with PSR inhibit counter initialized with one
- * and the caller (typically encoder driver) needs to call
- * rockchip_drm_psr_inhibit_put() when it becomes ready to accept PSR
- * enable request.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-int rockchip_drm_psr_register(struct drm_encoder *encoder,
-			int (*psr_set)(struct drm_encoder *, bool enable))
-{
-	struct rockchip_drm_private *drm_drv;
-	struct psr_drv *psr;
-
-	if (!encoder || !psr_set)
-		return -EINVAL;
-
-	drm_drv = encoder->dev->dev_private;
-
-	psr = kzalloc(sizeof(struct psr_drv), GFP_KERNEL);
-	if (!psr)
-		return -ENOMEM;
-
-	INIT_DELAYED_WORK(&psr->flush_work, psr_flush_handler);
-	mutex_init(&psr->lock);
-
-	psr->inhibit_count = 1;
-	psr->enabled = false;
-	psr->encoder = encoder;
-	psr->set = psr_set;
-
-	mutex_lock(&drm_drv->psr_list_lock);
-	list_add_tail(&psr->list, &drm_drv->psr_list);
-	mutex_unlock(&drm_drv->psr_list_lock);
-
-	return 0;
-}
-EXPORT_SYMBOL(rockchip_drm_psr_register);
-
-/**
- * rockchip_drm_psr_unregister - unregister encoder to psr driver
- * @encoder: encoder that obtain the PSR function
- * @psr_set: call back to set PSR state
- *
- * It is expected that the PSR inhibit counter is 1 when this function is
- * called, which corresponds to a state when related encoder has been
- * disconnected from any CRTCs and its driver called
- * rockchip_drm_psr_inhibit_get() to stop the PSR logic.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-void rockchip_drm_psr_unregister(struct drm_encoder *encoder)
-{
-	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
-	struct psr_drv *psr, *n;
-
-	mutex_lock(&drm_drv->psr_list_lock);
-	list_for_each_entry_safe(psr, n, &drm_drv->psr_list, list) {
-		if (psr->encoder == encoder) {
-			/*
-			 * Any other value would mean that the encoder
-			 * is still in use.
-			 */
-			WARN_ON(psr->inhibit_count != 1);
-
-			list_del(&psr->list);
-			kfree(psr);
-		}
-	}
-	mutex_unlock(&drm_drv->psr_list_lock);
-}
-EXPORT_SYMBOL(rockchip_drm_psr_unregister);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
deleted file mode 100644
index 25350ba3237b..000000000000
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
+++ /dev/null
@@ -1,30 +0,0 @@
-/*
- * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
- * Author: Yakir Yang <ykk@rock-chips.com>
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#ifndef __ROCKCHIP_DRM_PSR___
-#define __ROCKCHIP_DRM_PSR___
-
-void rockchip_drm_psr_flush_all(struct drm_device *dev);
-
-int rockchip_drm_psr_inhibit_put(struct drm_encoder *encoder);
-int rockchip_drm_psr_inhibit_get(struct drm_encoder *encoder);
-
-void rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state);
-void rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state);
-
-int rockchip_drm_psr_register(struct drm_encoder *encoder,
-			int (*psr_set)(struct drm_encoder *, bool enable));
-void rockchip_drm_psr_unregister(struct drm_encoder *encoder);
-
-#endif /* __ROCKCHIP_DRM_PSR__ */
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index c7d4c6073ea5..66c99d57e62a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -42,7 +42,6 @@
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_gem.h"
 #include "rockchip_drm_fb.h"
-#include "rockchip_drm_psr.h"
 #include "rockchip_drm_vop.h"
 #include "rockchip_rgb.h"
 
@@ -541,7 +540,7 @@ static void vop_core_clks_disable(struct vop *vop)
 	clk_disable(vop->hclk);
 }
 
-static int vop_enable(struct drm_crtc *crtc)
+static int vop_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state)
 {
 	struct vop *vop = to_vop(crtc);
 	int ret, i;
@@ -581,12 +580,18 @@ static int vop_enable(struct drm_crtc *crtc)
 	 * We need to make sure that all windows are disabled before we
 	 * enable the crtc. Otherwise we might try to scan from a destroyed
 	 * buffer later.
+	 *
+	 * In the case of enable-after-PSR, we don't need to worry about this
+	 * case since the buffer is guaranteed to be valid and disabling the
+	 * window will result in screen glitches on PSR exit.
 	 */
-	for (i = 0; i < vop->data->win_size; i++) {
-		struct vop_win *vop_win = &vop->win[i];
-		const struct vop_win_data *win = vop_win->data;
+	if (!old_state->psr_transition) {
+		for (i = 0; i < vop->data->win_size; i++) {
+			struct vop_win *vop_win = &vop->win[i];
+			const struct vop_win_data *win = vop_win->data;
 
-		VOP_WIN_SET(vop, win, enable, 0);
+			VOP_WIN_SET(vop, win, enable, 0);
+		}
 	}
 	spin_unlock(&vop->reg_lock);
 
@@ -937,12 +942,10 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
 	}
 
 	if (vop->is_enabled) {
-		rockchip_drm_psr_inhibit_get_state(new_state->state);
 		vop_plane_atomic_update(plane, plane->state);
 		spin_lock(&vop->reg_lock);
 		vop_cfg_done(vop);
 		spin_unlock(&vop->reg_lock);
-		rockchip_drm_psr_inhibit_put_state(new_state->state);
 	}
 
 	plane->funcs->atomic_destroy_state(plane, plane_state);
@@ -1035,7 +1038,7 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	WARN_ON(vop->event);
 
-	ret = vop_enable(crtc);
+	ret = vop_enable(crtc, old_state);
 	if (ret) {
 		mutex_unlock(&vop->vop_lock);
 		DRM_DEV_ERROR(vop->dev, "Failed to enable vop (%d)\n", ret);
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index 475b706b49de..29ce16ef0926 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -42,9 +42,7 @@ struct analogix_dp_plat_data {
 			 struct drm_connector *);
 };
 
-int analogix_dp_psr_enabled(struct analogix_dp_device *dp);
-int analogix_dp_enable_psr(struct analogix_dp_device *dp);
-int analogix_dp_disable_psr(struct analogix_dp_device *dp);
+bool analogix_dp_in_psr_transition(struct analogix_dp_device *dp);
 
 int analogix_dp_resume(struct analogix_dp_device *dp);
 int analogix_dp_suspend(struct analogix_dp_device *dp);
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 4/5] drm/rockchip: Don't fully disable vop on PSR
  2019-02-28 21:09 [PATCH 0/5] drm: Add PSR helpers Sean Paul
                   ` (2 preceding siblings ...)
  2019-02-28 21:09 ` [PATCH 3/5] drm/rockchip: Use the helpers for PSR Sean Paul
@ 2019-02-28 21:09 ` Sean Paul
  2019-02-28 21:09 ` [PATCH 5/5] drm/rockchip: Use drm_atomic_helper_commit_tail_rpm Sean Paul
  4 siblings, 0 replies; 10+ messages in thread
From: Sean Paul @ 2019-02-28 21:09 UTC (permalink / raw)
  To: dri-devel
  Cc: Zain Wang, David Airlie, Tomasz Figa, Sean Paul, Kristian H . Kristensen

From: Sean Paul <seanpaul@chromium.org>

Instead of fully disabling and re-enabling the vop on PSR transitions,
only disable the active windows. This will speed up PSR exits
substantially and is still a power-savings win.

This patch integrates portions of Zain's patch from here:
https://patchwork.kernel.org/patch/9615063/

Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Cc: Kristian H. Kristensen <hoegsberg@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 38 +++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 66c99d57e62a..025cfb55ffe2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -131,6 +131,7 @@ struct vop {
 	bool is_enabled;
 
 	struct completion dsp_hold_completion;
+	unsigned int win_enabled;
 
 	/* protected by dev->event_lock */
 	struct drm_pending_vblank_event *event;
@@ -591,6 +592,7 @@ static int vop_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state)
 			const struct vop_win_data *win = vop_win->data;
 
 			VOP_WIN_SET(vop, win, enable, 0);
+			vop->win_enabled &= ~BIT(i);
 		}
 	}
 	spin_unlock(&vop->reg_lock);
@@ -621,6 +623,25 @@ static int vop_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state)
 	return ret;
 }
 
+static void rockchip_drm_set_win_enabled(struct drm_crtc *crtc, bool enabled)
+{
+        struct vop *vop = to_vop(crtc);
+        int i;
+
+        spin_lock(&vop->reg_lock);
+
+        for (i = 0; i < vop->data->win_size; i++) {
+                struct vop_win *vop_win = &vop->win[i];
+                const struct vop_win_data *win = vop_win->data;
+
+                VOP_WIN_SET(vop, win, enable,
+                            enabled && (vop->win_enabled & BIT(i)));
+        }
+        vop_cfg_done(vop);
+
+        spin_unlock(&vop->reg_lock);
+}
+
 static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
 				    struct drm_crtc_state *old_state)
 {
@@ -628,9 +649,15 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
 
 	WARN_ON(vop->event);
 
+	if (crtc->state->psr_transition)
+		rockchip_drm_set_win_enabled(crtc, false);
+
 	mutex_lock(&vop->vop_lock);
 	drm_crtc_vblank_off(crtc);
 
+	if (crtc->state->psr_transition)
+		goto out;
+
 	/*
 	 * Vop standby will take effect at end of current frame,
 	 * if dsp hold valid irq happen, it means standby complete.
@@ -661,6 +688,8 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
 	clk_disable(vop->dclk);
 	vop_core_clks_disable(vop);
 	pm_runtime_put(vop->dev);
+
+out:
 	mutex_unlock(&vop->vop_lock);
 
 	if (crtc->state->event && !crtc->state->active) {
@@ -741,6 +770,7 @@ static void vop_plane_atomic_disable(struct drm_plane *plane,
 	spin_lock(&vop->reg_lock);
 
 	VOP_WIN_SET(vop, win, enable, 0);
+	vop->win_enabled &= ~BIT(VOP_WIN_TO_INDEX(vop_win));
 
 	spin_unlock(&vop->reg_lock);
 }
@@ -879,6 +909,7 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 	}
 
 	VOP_WIN_SET(vop, win, enable, 1);
+	vop->win_enabled |= BIT(win_index);
 	spin_unlock(&vop->reg_lock);
 }
 
@@ -1034,6 +1065,12 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
 	uint32_t pin_pol, val;
 	int ret;
 
+	if (old_state->psr_transition) {
+		drm_crtc_vblank_on(crtc);
+		rockchip_drm_set_win_enabled(crtc, true);
+		return;
+	}
+
 	mutex_lock(&vop->vop_lock);
 
 	WARN_ON(vop->event);
@@ -1627,6 +1664,7 @@ static int vop_initial(struct vop *vop)
 		VOP_WIN_SET(vop, win, channel, (channel + 1) << 4 | channel);
 		VOP_WIN_SET(vop, win, enable, 0);
 		VOP_WIN_SET(vop, win, gate, 1);
+		vop->win_enabled &= ~BIT(i);
 	}
 
 	vop_cfg_done(vop);
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 5/5] drm/rockchip: Use drm_atomic_helper_commit_tail_rpm
  2019-02-28 21:09 [PATCH 0/5] drm: Add PSR helpers Sean Paul
                   ` (3 preceding siblings ...)
  2019-02-28 21:09 ` [PATCH 4/5] drm/rockchip: Don't fully disable vop on PSR Sean Paul
@ 2019-02-28 21:09 ` Sean Paul
  4 siblings, 0 replies; 10+ messages in thread
From: Sean Paul @ 2019-02-28 21:09 UTC (permalink / raw)
  To: dri-devel; +Cc: Zain Wang, David Airlie, Tomasz Figa, Sean Paul

From: Sean Paul <seanpaul@chromium.org>

Now that we use the drm psr helpers, we no longer need to hand-roll our
atomic_commit_tail implementation. So use the helper

Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 7e121875e3c9..75198df2b022 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -115,27 +115,8 @@ rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
 	return ERR_PTR(ret);
 }
 
-static void
-rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
-{
-	struct drm_device *dev = old_state->dev;
-
-	drm_atomic_helper_commit_modeset_disables(dev, old_state);
-
-	drm_atomic_helper_commit_modeset_enables(dev, old_state);
-
-	drm_atomic_helper_commit_planes(dev, old_state,
-					DRM_PLANE_COMMIT_ACTIVE_ONLY);
-
-	drm_atomic_helper_commit_hw_done(old_state);
-
-	drm_atomic_helper_wait_for_vblanks(dev, old_state);
-
-	drm_atomic_helper_cleanup_planes(dev, old_state);
-}
-
 static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
-	.atomic_commit_tail = rockchip_atomic_helper_commit_tail_rpm,
+	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
 };
 
 static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm: Add helpers to kick off PSR enable/disable
  2019-02-28 21:09 ` [PATCH 1/5] drm: Add helpers to kick off PSR enable/disable Sean Paul
@ 2019-02-28 22:29   ` Souza, Jose
  2019-03-01  8:18   ` Daniel Vetter
  1 sibling, 0 replies; 10+ messages in thread
From: Souza, Jose @ 2019-02-28 22:29 UTC (permalink / raw)
  To: dri-devel, sean; +Cc: maxime.ripard, airlied, wzz, seanpaul, tfiga


[-- Attachment #1.1: Type: text/plain, Size: 26550 bytes --]

On Thu, 2019-02-28 at 16:09 -0500, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> This patch adds a new drm helper library to help drivers implement
> PSR. Drivers choosing to use it will register connectors with
> PSR-capable displays connected and will receive callbacks when it's
> time
> to enter or exit PSR.

This helpers should target connector not the entire driver, a
modification in any sink would cause a PSR exit in all PSR panels.

Also do atomic commit in drm_psr_helper_flush() that is called from
another atomic commit sounds wrong.
During the atomic checks it should probably just set enable and active
states of drm_crtc_state and the CRTC would be active when committing
changes.

> 
> In its current form, it has a timer which will trigger after a
> driver-specified amount of inactivity. When the timer triggers, the
> helpers will save the current atomic state and issue a new state
> which
> has the PSR-enabled pipes turned off. On the next update, the drm
> core
> will poke the PSR helpers to restore the saved state to the driver
> before
> servicing said update.
> 
> From the driver's perspective, this works like a regular
> disable/enable
> cycle. The driver need only check the 'psr_transition' state in
> connector_state and keep the panel turned on when in .disable(),
> while
> everything else will cycle off as normal. If drivers want more
> control,
> they can use the psr_transition state to enter a low-power state to
> minimize PSR exit time.
> 
> While this carries the PSR moniker, it is not specific to the
> DisplayPort technology. This can be used for power savings with other
> types of self refresh, such as MIPI command mode.
> 
> Cc: Zain Wang <wzz@rock-chips.com>
> Cc: Tomasz Figa <tfiga@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  Documentation/gpu/drm-kms-helpers.rst |   9 +
>  drivers/gpu/drm/Makefile              |   2 +-
>  drivers/gpu/drm/drm_atomic_helper.c   |  34 +++
>  drivers/gpu/drm/drm_atomic_uapi.c     |   5 +
>  drivers/gpu/drm/drm_fb_helper.c       |   9 +
>  drivers/gpu/drm/drm_framebuffer.c     |  18 ++
>  drivers/gpu/drm/drm_psr_helper.c      | 343
> ++++++++++++++++++++++++++
>  include/drm/drm_connector.h           |  22 ++
>  include/drm/drm_crtc.h                |  11 +
>  include/drm/drm_mode_config.h         |   6 +
>  include/drm/drm_psr_helper.h          |  24 ++
>  11 files changed, 482 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_psr_helper.c
>  create mode 100644 include/drm/drm_psr_helper.h
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst
> b/Documentation/gpu/drm-kms-helpers.rst
> index 17ca7f8bf3d3..d218a113bd52 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -107,6 +107,15 @@ fbdev Helper Functions Reference
>  .. kernel-doc:: drivers/gpu/drm/drm_fb_helper.c
>     :export:
>  
> +Panel Self Refresh Helper Reference
> +===================================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_psr_helper.c
> +   :doc: overview
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_psr_helper.c
> +   :export:
> +
>  Framebuffer CMA Helper Functions Reference
>  ==========================================
>  
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 1ac55c65eac0..bff80fb946c7 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -19,7 +19,7 @@ drm-y       :=	drm_auth.o drm_bufs.o
> drm_cache.o \
>  		drm_plane.o drm_color_mgmt.o drm_print.o \
>  		drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
>  		drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o
> \
> -		drm_atomic_uapi.o
> +		drm_atomic_uapi.o drm_psr_helper.o
>  
>  drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
>  drm-$(CONFIG_DRM_VM) += drm_vm.o
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index c53ecbd9abdd..f5284d55f170 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_atomic_uapi.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_psr_helper.h>
>  #include <drm/drm_writeback.h>
>  #include <drm/drm_damage_helper.h>
>  #include <linux/dma-fence.h>
> @@ -2746,6 +2747,10 @@ int drm_atomic_helper_update_plane(struct
> drm_plane *plane,
>  	struct drm_plane_state *plane_state;
>  	int ret = 0;
>  
> +	ret = drm_psr_helper_flush(plane->dev, ctx);
> +	if (ret)
> +		return ret;
> +
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -2797,6 +2802,10 @@ int drm_atomic_helper_disable_plane(struct
> drm_plane *plane,
>  	struct drm_plane_state *plane_state;
>  	int ret = 0;
>  
> +	ret = drm_psr_helper_flush(plane->dev, ctx);
> +	if (ret)
> +		return ret;
> +
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -2934,11 +2943,16 @@ int drm_atomic_helper_set_config(struct
> drm_mode_set *set,
>  	struct drm_crtc *crtc = set->crtc;
>  	int ret = 0;
>  
> +	ret = drm_psr_helper_flush(crtc->dev, ctx);
> +	if (ret)
> +		return ret;
> +
>  	state = drm_atomic_state_alloc(crtc->dev);
>  	if (!state)
>  		return -ENOMEM;
>  
>  	state->acquire_ctx = ctx;
> +
>  	ret = __drm_atomic_helper_set_config(set, state);
>  	if (ret != 0)
>  		goto fail;
> @@ -3139,6 +3153,10 @@ void drm_atomic_helper_shutdown(struct
> drm_device *dev)
>  
>  	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>  
> +	ret = drm_psr_helper_flush(dev, &ctx);
> +	if (ret)
> +		DRM_ERROR("PSR flush during shutdown failed with %i\n",
> ret);
> +
>  	ret = __drm_atomic_helper_disable_all(dev, &ctx, true);
>  	if (ret)
>  		DRM_ERROR("Disabling all crtc's during unload failed
> with %i\n", ret);
> @@ -3271,6 +3289,10 @@ struct drm_atomic_state
> *drm_atomic_helper_suspend(struct drm_device *dev)
>  
>  	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
>  
> +	err = drm_psr_helper_flush(dev, &ctx);
> +	if (err)
> +		goto unlock;
> +
>  	state = drm_atomic_helper_duplicate_state(dev, &ctx);
>  	if (IS_ERR(state))
>  		goto unlock;
> @@ -3436,6 +3458,10 @@ int drm_atomic_helper_page_flip(struct
> drm_crtc *crtc,
>  	struct drm_atomic_state *state;
>  	int ret = 0;
>  
> +	ret = drm_psr_helper_flush(crtc->dev, ctx);
> +	if (ret)
> +		return ret;
> +
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -3481,6 +3507,10 @@ int drm_atomic_helper_page_flip_target(struct
> drm_crtc *crtc,
>  	struct drm_crtc_state *crtc_state;
>  	int ret = 0;
>  
> +	ret = drm_psr_helper_flush(plane->dev, ctx);
> +	if (ret)
> +		return ret;
> +
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -3532,6 +3562,10 @@ int drm_atomic_helper_legacy_gamma_set(struct
> drm_crtc *crtc,
>  	int i, ret = 0;
>  	bool replaced;
>  
> +	ret = drm_psr_helper_flush(dev, ctx);
> +	if (ret)
> +		return ret;
> +
>  	state = drm_atomic_state_alloc(crtc->dev);
>  	if (!state)
>  		return -ENOMEM;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index 4eb81f10bc54..21d7771d6ec7 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -29,6 +29,7 @@
>  #include <drm/drm_atomic_uapi.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_print.h>
> +#include <drm/drm_psr_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_writeback.h>
>  #include <drm/drm_vblank.h>
> @@ -1314,6 +1315,10 @@ int drm_mode_atomic_ioctl(struct drm_device
> *dev,
>  	fence_state = NULL;
>  	num_fences = 0;
>  
> +	ret = drm_psr_helper_flush(dev, state->acquire_ctx);
> +	if (ret)
> +		goto out;
> +
>  	for (i = 0; i < arg->count_objs; i++) {
>  		uint32_t obj_id, count_props;
>  		struct drm_mode_object *obj;
> diff --git a/drivers/gpu/drm/drm_fb_helper.c
> b/drivers/gpu/drm/drm_fb_helper.c
> index 04d23cb430bf..a56ae2acda90 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -41,6 +41,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_psr_helper.h>
>  
>  #include "drm_crtc_internal.h"
>  #include "drm_crtc_helper_internal.h"
> @@ -406,6 +407,10 @@ static int restore_fbdev_mode_atomic(struct
> drm_fb_helper *fb_helper, bool activ
>  
>  	state->acquire_ctx = &ctx;
>  retry:
> +	ret = drm_psr_helper_flush(dev, state->acquire_ctx);
> +	if (ret)
> +		goto out_state;
> +
>  	drm_for_each_plane(plane, dev) {
>  		plane_state = drm_atomic_get_plane_state(state, plane);
>  		if (IS_ERR(plane_state)) {
> @@ -1442,6 +1447,10 @@ static int setcmap_atomic(struct fb_cmap
> *cmap, struct fb_info *info)
>  
>  	state->acquire_ctx = &ctx;
>  retry:
> +	ret = drm_psr_helper_flush(dev, &ctx);
> +	if (ret)
> +		goto out_state;
> +
>  	for (i = 0; i < fb_helper->crtc_count; i++) {
>  		crtc = fb_helper->crtc_info[i].mode_set.crtc;
>  
> diff --git a/drivers/gpu/drm/drm_framebuffer.c
> b/drivers/gpu/drm/drm_framebuffer.c
> index d8d75e25f6fb..43b029a88578 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -27,6 +27,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_uapi.h>
>  #include <drm/drm_print.h>
> +#include <drm/drm_psr_helper.h>
>  #include <drm/drm_util.h>
>  
>  #include "drm_internal.h"
> @@ -572,6 +573,7 @@ int drm_mode_dirtyfb_ioctl(struct drm_device
> *dev,
>  	struct drm_clip_rect __user *clips_ptr;
>  	struct drm_clip_rect *clips = NULL;
>  	struct drm_mode_fb_dirty_cmd *r = data;
> +	struct drm_modeset_acquire_ctx ctx;
>  	struct drm_framebuffer *fb;
>  	unsigned flags;
>  	int num_clips;
> @@ -580,10 +582,24 @@ int drm_mode_dirtyfb_ioctl(struct drm_device
> *dev,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> +	drm_modeset_acquire_init(&ctx, 0);
> +
>  	fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id);
>  	if (!fb)
>  		return -ENOENT;
>  
> +retry:
> +	ret = drm_psr_helper_flush(fb->dev, &ctx);
> +	if (ret) {
> +		if (ret == -EDEADLK) {
> +			ret = drm_modeset_backoff(&ctx);
> +			if (!ret)
> +				goto retry;
> +		} else {
> +			goto out_err1;
> +		}
> +	}
> +
>  	num_clips = r->num_clips;
>  	clips_ptr = (struct drm_clip_rect __user *)(unsigned long)r-
> >clips_ptr;
>  
> @@ -630,6 +646,8 @@ int drm_mode_dirtyfb_ioctl(struct drm_device
> *dev,
>  	kfree(clips);
>  out_err1:
>  	drm_framebuffer_put(fb);
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/drm_psr_helper.c
> b/drivers/gpu/drm/drm_psr_helper.c
> new file mode 100644
> index 000000000000..0b57a2a53075
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_psr_helper.c
> @@ -0,0 +1,343 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2019 Google, Inc.
> + *
> + * Authors:
> + * Sean Paul <seanpaul@chromium.org>
> + */
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_mode_config.h>
> +#include <drm/drm_modeset_lock.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_psr_helper.h>
> +#include <linux/bitops.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +
> +/**
> + * DOC: overview
> + *
> + * This helper library provides an easy way for drivers to leverage
> the atomic
> + * framework to implement panel self refresh (PSR) support. Drivers
> are
> + * responsible for intializing and cleaning up the PSR helpers on
> load/unload.
> + * When drivers identify a display that supports self refreshing
> (eDP or MIPI
> + * command mode), it should register the affected connector with the
> PSR
> + * helpers.
> + *
> + * Once a connector is registered, the PSR helpers will monitor
> activity and
> + * call back into the driver to enable/disable PSR as appropriate.
> The best way
> + * to think about this is that it's a DPMS on/off request with a
> flag set in
> + * state that tells you to disable/enable PSR on the panel instead
> of power-
> + * cycling it.
> + *
> + * Drivers may choose to fully disable their crtc/encoder/bridge
> hardware, or
> + * they can use the "psr_transition" flag in crtc and connector
> state if they
> + * want to enter low power mode without full disable (in case full
> + * disable/enable is too slow).
> + *
> + * PSR will be deactivated if there are any atomic updates, even
> updates that do
> + * not affect the connectors which are self refreshing. Supporting
> this is
> + * possible but non-trivial due to sharing of hardware resources.
> Similarly, if
> + * a crtc is driving multiple connectors, PSR will not be initiated
> on any of
> + * those connectors.
> + */
> +
> +struct drm_psr_state {
> +	struct drm_device *dev;
> +	struct drm_modeset_lock mutex;
> +	struct delayed_work entry_work;
> +	struct drm_atomic_state *save_state;
> +	unsigned int entry_delay_ms;
> +};
> +
> +static void drm_psr_helper_entry_work(struct work_struct *work)
> +{
> +	struct drm_psr_state *psr_state =
> container_of(to_delayed_work(work),
> +						       struct
> drm_psr_state,
> +						       entry_work);
> +	struct drm_atomic_state *save_state;
> +	struct drm_device *dev = psr_state->dev;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_atomic_state *state;
> +	struct drm_connector *conn;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	bool commit = false;
> +	int ret, i;
> +
> +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> +
> +	ret = drm_modeset_lock(&psr_state->mutex, &ctx);
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * The only way this can happen is if we schedule the worker
> while it's
> +	 * already running and that timeout subsequently elapses. Since
> we hold
> +	 * the psr_state->mutex when scheduling, we also know where the
> worker
> +	 * is sitting in its execution (hint: look up). In this case,
> it's
> +	 * possible for the entry worker to run twice for the same
> commit. Since
> +	 * the hardware hasn't changed since the last save state, just
> kick out.
> +	 */
> +	if (psr_state->save_state)
> +		goto out;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	save_state = drm_atomic_helper_duplicate_state(dev, &ctx);
> +
> +	/*
> +	 * Now that we have the current the HW state saved, we have to
> flip the
> +	 * psr_transition bit so we know what type of enable we're
> dealing with
> +	 * when coming back on.
> +	 *
> +	 * NOTE: We don't check conn->capable here since that could
> change out
> +	 * from under us. We'll trust the atomic core to only call
> enable if
> +	 * necessary (ie: only for those connectors/crtcs that
> currently have
> +	 * psr enabled).
> +	 */
> +	if (IS_ERR(save_state)) {
> +		ret = PTR_ERR(save_state);
> +		goto out;
> +	}
> +	for_each_new_connector_in_state(save_state, conn, conn_state,
> i) {
> +		if (!conn_state->crtc)
> +			continue;
> +		conn_state->psr_transition = true;
> +	}
> +	for_each_new_crtc_in_state(save_state, crtc, crtc_state, i) {
> +		if (!crtc_state->active)
> +			continue;
> +		crtc_state->psr_transition = true;
> +	}
> +
> +	state->acquire_ctx = &ctx;
> +	drm_connector_list_iter_begin(psr_state->dev, &conn_iter);
> +	drm_for_each_connector_iter(conn, &conn_iter) {
> +		if (!conn->psr_capable)
> +			continue;
> +
> +		conn_state = drm_atomic_get_connector_state(state,
> conn);
> +		if (IS_ERR(conn_state)) {
> +			ret = PTR_ERR(conn_state);
> +			drm_connector_list_iter_end(&conn_iter);
> +			goto out_free_state;
> +		}
> +
> +		if (!conn_state->crtc)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state,
> conn_state->crtc);
> +		if (IS_ERR(crtc_state)) {
> +			ret = PTR_ERR(crtc_state);
> +			drm_connector_list_iter_end(&conn_iter);
> +			goto out_free_state;
> +		}
> +
> +		/* Don't use PSR if the crtc is driving multiple
> connectors */
> +		if (hweight_long(crtc_state->connector_mask) > 1)
> +			continue;
> +
> +		commit = true;
> +		crtc_state->active = false;
> +		crtc_state->psr_transition = true;
> +		conn_state->psr_transition = true;
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
> +	/* Nothing to commit, so just exit */
> +	if (!commit)
> +		goto out_free_state;
> +
> +	ret = drm_atomic_commit(state);
> +	if (ret)
> +		goto out_free_state;
> +
> +	psr_state->save_state = save_state;
> +	goto out;
> +
> +out_free_state:
> +	drm_atomic_state_put(save_state);
> +	drm_atomic_state_put(state);
> +out:
> +	DRM_MODESET_LOCK_ALL_END(ctx, ret);
> +}
> +
> +static int
> +drm_psr_helper_acquire_modeset_locks(struct drm_atomic_state *state,
> +				     struct drm_modeset_acquire_ctx
> *ctx)
> +{
> +	struct drm_mode_config *config = &state->dev->mode_config;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *new_crtc_state;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *new_plane_state;
> +	int ret, i;
> +
> +	ret = drm_modeset_lock(&config->connection_mutex, ctx);
> +	if (ret)
> +		return ret;
> +
> +	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> +		ret = drm_modeset_lock(&plane->mutex, ctx);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		ret = drm_modeset_lock(&crtc->mutex, ctx);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * drm_psr_helper_flush - Restore the hardware to pre-PSR state
> + * @dev: DRM device
> + * @ctx: An acquire context to use for restoring the state
> + *
> + * This function should be called before every drm_atomic_commit to
> ensure any
> + * connectors that are currently self-refreshing revert back to
> being bus
> + * driven. Drivers may call this function outside of the atomic
> hooks if
> + * they wish to disable PSR pre-emptively (such as upon an input
> event or when
> + * GPU becomes active).
> + *
> + * If everything is successful, this function will schedule the PSR
> entry worker
> + * to enable PSR after the driver-specified timeout.
> + *
> + * If the PSR helper is not being used, this is a no-op.
> + */
> +int drm_psr_helper_flush(struct drm_device *dev,
> +			 struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_psr_state *psr_state = config->psr_state;
> +	int ret;
> +
> +	if (!psr_state)
> +		return 0;
> +
> +	ret = drm_modeset_lock(&psr_state->mutex, ctx);
> +	if (ret)
> +		return ret;
> +
> +	if (!psr_state->save_state)
> +		goto out;
> +
> +	ret = drm_psr_helper_acquire_modeset_locks(psr_state-
> >save_state, ctx);
> +	if (ret)
> +		goto out;
> +
> +	ret = drm_atomic_helper_commit_duplicated_state(psr_state-
> >save_state,
> +							ctx);
> +
> +out:
> +	psr_state->save_state = NULL;
> +	if (!ret) {
> +		mod_delayed_work(system_wq, &psr_state->entry_work,
> +				 msecs_to_jiffies(psr_state-
> >entry_delay_ms));
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_psr_helper_flush);
> +
> +/**
> + * drm_psr_helper_register - Registers a connector with the PSR
> helpers
> + * @connector: the connector which has a PSR-supported display
> attached
> + *
> + * Note that this can be called once on initialization for fixed
> panels, or
> + * during enable/hotplug.
> + */
> +int drm_psr_helper_register(struct drm_connector *connector)
> +{
> +	struct drm_mode_config *config = &connector->dev->mode_config;
> +	struct drm_psr_state *psr_state = config->psr_state;
> +
> +	/* PSR helpers are uninitialized */
> +	if (WARN_ON(!psr_state))
> +		return -EINVAL;
> +
> +	/* Acquired via psr_helper_flush */
> +	if (!drm_modeset_is_locked(&psr_state->mutex))
> +		return -EINVAL;
> +
> +	connector->psr_capable = true;
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_psr_helper_register);
> +
> +/**
> + * drm_psr_helper_unregister - Unregisters a connector with the PSR
> helpers
> + * @connector: the connector to unregister
> + */
> +int drm_psr_helper_unregister(struct drm_connector *connector)
> +{
> +	struct drm_mode_config *config = &connector->dev->mode_config;
> +	struct drm_psr_state *psr_state = config->psr_state;
> +
> +	/* PSR helpers are uninitialized */
> +	if (WARN_ON(!psr_state))
> +		return -EINVAL;
> +
> +	/* Acquired via psr_helper_flush */
> +	if (!drm_modeset_is_locked(&psr_state->mutex))
> +		return -EINVAL;
> +
> +	connector->psr_capable = false;
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_psr_helper_unregister);
> +
> +/**
> + * drm_psr_helper_init - Initializes the PSR helpers
> + * @dev: DRM device
> + * @entry_delay_ms: The amount of time to wait after an atomic
> commit before
> + *		    activating PSR
> + *
> + * Drivers using the PSR helpers must call this some time after
> mode_config
> + * is initialized in order to make use of the PSR helpers. Typically
> + * entry_delay_ms is a function of how quickly the hardware can
> enter/exit PSR.
> + */
> +int drm_psr_helper_init(struct drm_device *dev, unsigned int
> entry_delay_ms)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_psr_state *psr_state;
> +
> +	psr_state = kzalloc(sizeof(*psr_state), GFP_KERNEL);
> +	if (!psr_state)
> +		return -ENOMEM;
> +
> +	INIT_DELAYED_WORK(&psr_state->entry_work,
> drm_psr_helper_entry_work);
> +	drm_modeset_lock_init(&psr_state->mutex);
> +	psr_state->dev = dev;
> +	psr_state->entry_delay_ms = entry_delay_ms;
> +
> +	config->psr_state = psr_state;
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_psr_helper_init);
> +
> +/**
> + * drm_psr_helper_cleanup - De-initializes the PSR helpers
> + * @dev: DRM device
> + */
> +void drm_psr_helper_cleanup(struct drm_device *dev)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	cancel_delayed_work_sync(&config->psr_state->entry_work);
> +	kfree(config->psr_state);
> +	config->psr_state = NULL;
> +}
> +EXPORT_SYMBOL(drm_psr_helper_cleanup);
> diff --git a/include/drm/drm_connector.h
> b/include/drm/drm_connector.h
> index c8061992d6cb..9612b3d56f30 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -501,6 +501,17 @@ struct drm_connector_state {
>  	/** @tv: TV connector state */
>  	struct drm_tv_connector_state tv;
>  
> +	/**
> +	 * @psr_transition:
> +	 *
> +	 * Used by the PSR helpers to denote when a PSR transition is
> occuring.
> +	 * If your connector is PSR-capable, register it with the
> helpers and
> +	 * check this flag in .enable() and .disable(). If it is true,
> instead
> +	 * of shutting off the panel, put it in or take it out of self
> +	 * refreshing mode.
> +	 */
> +	bool psr_transition;
> +
>  	/**
>  	 * @picture_aspect_ratio: Connector property to control the
>  	 * HDMI infoframe aspect ratio setting.
> @@ -993,6 +1004,17 @@ struct drm_connector {
>  	 */
>  	struct drm_display_info display_info;
>  
> +	/**
> +	 * @psr_capable:
> +	 *
> +	 * Set by the driver via drm_psr_helper_register(). Signals
> that this
> +	 * connector (and associated pipe) is PSR capable and should be
> put in
> +	 * low-power mode when it is inactive.
> +	 *
> +	 * Protected by &drm_mode_config.psr_state.mutex
> +	 */
> +	bool psr_capable;
> +
>  	/** @funcs: connector control functions */
>  	const struct drm_connector_funcs *funcs;
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index f7c3022dbdf4..10acd4fc0991 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -299,6 +299,17 @@ struct drm_crtc_state {
>  	 */
>  	bool vrr_enabled;
>  
> +	/**
> +	 * @psr_transition:
> +	 *
> +	 * Used by the PSR helpers to denote when a PSR transition is
> occuring.
> +	 * This will be set on enable/disable callbacks when PSR is
> being
> +	 * enabled or disabled. In some cases, it may not be desirable
> to fully
> +	 * shut off the crtc during PSR. CRTC's can inspect this flag
> and
> +	 * determine the best course of action.
> +	 */
> +	bool psr_transition;
> +
>  	/**
>  	 * @event:
>  	 *
> diff --git a/include/drm/drm_mode_config.h
> b/include/drm/drm_mode_config.h
> index 7f60e8eb269a..371b80d090ab 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -37,6 +37,7 @@ struct drm_atomic_state;
>  struct drm_mode_fb_cmd2;
>  struct drm_format_info;
>  struct drm_display_mode;
> +struct drm_psr_state;
>  
>  /**
>   * struct drm_mode_config_funcs - basic driver provided mode setting
> functions
> @@ -900,6 +901,11 @@ struct drm_mode_config {
>  	 */
>  	struct drm_atomic_state *suspend_state;
>  
> +	/**
> +	 * @psr_state: Holds the state for the psr helper
> +	 */
> +	struct drm_psr_state *psr_state;
> +
>  	const struct drm_mode_config_helper_funcs *helper_private;
>  };
>  
> diff --git a/include/drm/drm_psr_helper.h
> b/include/drm/drm_psr_helper.h
> new file mode 100644
> index 000000000000..972c4ec98d05
> --- /dev/null
> +++ b/include/drm/drm_psr_helper.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2019 Google, Inc.
> + *
> + * Authors:
> + * Sean Paul <seanpaul@chromium.org>
> + */
> +#ifndef DRM_PSR_HELPER_H_
> +#define DRM_PSR_HELPER_H_
> +
> +struct drm_connector;
> +struct drm_device;
> +struct drm_psr_state;
> +struct drm_modeset_acquire_ctx;
> +
> +int drm_psr_helper_flush(struct drm_device *dev,
> +			 struct drm_modeset_acquire_ctx *ctx);
> +
> +int drm_psr_helper_register(struct drm_connector *connector);
> +int drm_psr_helper_unregister(struct drm_connector *connector);
> +
> +int drm_psr_helper_init(struct drm_device *dev, unsigned int
> entry_delay_ms);
> +void drm_psr_helper_cleanup(struct drm_device *dev);
> +#endif

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm: Add helpers to kick off PSR enable/disable
  2019-02-28 21:09 ` [PATCH 1/5] drm: Add helpers to kick off PSR enable/disable Sean Paul
  2019-02-28 22:29   ` Souza, Jose
@ 2019-03-01  8:18   ` Daniel Vetter
  2019-03-01 16:00     ` Sean Paul
  1 sibling, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2019-03-01  8:18 UTC (permalink / raw)
  To: Sean Paul
  Cc: Zain Wang, David Airlie, dri-devel, Tomasz Figa, Maxime Ripard,
	Sean Paul

On Thu, Feb 28, 2019 at 04:09:30PM -0500, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> This patch adds a new drm helper library to help drivers implement
> PSR. Drivers choosing to use it will register connectors with
> PSR-capable displays connected and will receive callbacks when it's time
> to enter or exit PSR.
> 
> In its current form, it has a timer which will trigger after a
> driver-specified amount of inactivity. When the timer triggers, the
> helpers will save the current atomic state and issue a new state which
> has the PSR-enabled pipes turned off. On the next update, the drm core
> will poke the PSR helpers to restore the saved state to the driver before
> servicing said update.
> 
> From the driver's perspective, this works like a regular disable/enable
> cycle. The driver need only check the 'psr_transition' state in
> connector_state and keep the panel turned on when in .disable(), while
> everything else will cycle off as normal. If drivers want more control,
> they can use the psr_transition state to enter a low-power state to
> minimize PSR exit time.
> 
> While this carries the PSR moniker, it is not specific to the
> DisplayPort technology. This can be used for power savings with other
> types of self refresh, such as MIPI command mode.
> 
> Cc: Zain Wang <wzz@rock-chips.com>
> Cc: Tomasz Figa <tfiga@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

I very much like the idea, I think for sw driven self refresh (i.e. not
the stuff intel hw does) this is great. And neatly fits into the recent
work to add damage/dirty helpers I think.

> ---
>  Documentation/gpu/drm-kms-helpers.rst |   9 +
>  drivers/gpu/drm/Makefile              |   2 +-
>  drivers/gpu/drm/drm_atomic_helper.c   |  34 +++
>  drivers/gpu/drm/drm_atomic_uapi.c     |   5 +
>  drivers/gpu/drm/drm_fb_helper.c       |   9 +
>  drivers/gpu/drm/drm_framebuffer.c     |  18 ++
>  drivers/gpu/drm/drm_psr_helper.c      | 343 ++++++++++++++++++++++++++
>  include/drm/drm_connector.h           |  22 ++
>  include/drm/drm_crtc.h                |  11 +
>  include/drm/drm_mode_config.h         |   6 +
>  include/drm/drm_psr_helper.h          |  24 ++
>  11 files changed, 482 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_psr_helper.c
>  create mode 100644 include/drm/drm_psr_helper.h
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 17ca7f8bf3d3..d218a113bd52 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -107,6 +107,15 @@ fbdev Helper Functions Reference
>  .. kernel-doc:: drivers/gpu/drm/drm_fb_helper.c
>     :export:
>  
> +Panel Self Refresh Helper Reference
> +===================================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_psr_helper.c
> +   :doc: overview
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_psr_helper.c
> +   :export:
> +
>  Framebuffer CMA Helper Functions Reference
>  ==========================================
>  
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 1ac55c65eac0..bff80fb946c7 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -19,7 +19,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
>  		drm_plane.o drm_color_mgmt.o drm_print.o \
>  		drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
>  		drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> -		drm_atomic_uapi.o
> +		drm_atomic_uapi.o drm_psr_helper.o

Helpers should be in the helpers module so that modular builds catch you
when you do spaghetti layering between core and helpers. On the gem side
we've never really managed to pull that off, but on kms it's very much a
thing.

>  
>  drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
>  drm-$(CONFIG_DRM_VM) += drm_vm.o
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c53ecbd9abdd..f5284d55f170 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_atomic_uapi.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_psr_helper.h>
>  #include <drm/drm_writeback.h>
>  #include <drm/drm_damage_helper.h>
>  #include <linux/dma-fence.h>
> @@ -2746,6 +2747,10 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane,
>  	struct drm_plane_state *plane_state;
>  	int ret = 0;
>  
> +	ret = drm_psr_helper_flush(plane->dev, ctx);
> +	if (ret)
> +		return ret;
> +
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -2797,6 +2802,10 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane,
>  	struct drm_plane_state *plane_state;
>  	int ret = 0;
>  
> +	ret = drm_psr_helper_flush(plane->dev, ctx);
> +	if (ret)
> +		return ret;
> +
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -2934,11 +2943,16 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set,
>  	struct drm_crtc *crtc = set->crtc;
>  	int ret = 0;
>  
> +	ret = drm_psr_helper_flush(crtc->dev, ctx);
> +	if (ret)
> +		return ret;
> +
>  	state = drm_atomic_state_alloc(crtc->dev);
>  	if (!state)
>  		return -ENOMEM;
>  
>  	state->acquire_ctx = ctx;
> +
>  	ret = __drm_atomic_helper_set_config(set, state);
>  	if (ret != 0)
>  		goto fail;
> @@ -3139,6 +3153,10 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
>  
>  	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>  
> +	ret = drm_psr_helper_flush(dev, &ctx);
> +	if (ret)
> +		DRM_ERROR("PSR flush during shutdown failed with %i\n", ret);
> +
>  	ret = __drm_atomic_helper_disable_all(dev, &ctx, true);
>  	if (ret)
>  		DRM_ERROR("Disabling all crtc's during unload failed with %i\n", ret);
> @@ -3271,6 +3289,10 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
>  
>  	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
>  
> +	err = drm_psr_helper_flush(dev, &ctx);
> +	if (err)
> +		goto unlock;
> +
>  	state = drm_atomic_helper_duplicate_state(dev, &ctx);
>  	if (IS_ERR(state))
>  		goto unlock;
> @@ -3436,6 +3458,10 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  	struct drm_atomic_state *state;
>  	int ret = 0;
>  
> +	ret = drm_psr_helper_flush(crtc->dev, ctx);
> +	if (ret)
> +		return ret;
> +
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -3481,6 +3507,10 @@ int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc,
>  	struct drm_crtc_state *crtc_state;
>  	int ret = 0;
>  
> +	ret = drm_psr_helper_flush(plane->dev, ctx);
> +	if (ret)
> +		return ret;
> +
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -3532,6 +3562,10 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>  	int i, ret = 0;
>  	bool replaced;
>  
> +	ret = drm_psr_helper_flush(dev, ctx);
> +	if (ret)
> +		return ret;
> +
>  	state = drm_atomic_state_alloc(crtc->dev);
>  	if (!state)
>  		return -ENOMEM;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 4eb81f10bc54..21d7771d6ec7 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -29,6 +29,7 @@
>  #include <drm/drm_atomic_uapi.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_print.h>
> +#include <drm/drm_psr_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_writeback.h>
>  #include <drm/drm_vblank.h>
> @@ -1314,6 +1315,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	fence_state = NULL;
>  	num_fences = 0;
>  
> +	ret = drm_psr_helper_flush(dev, state->acquire_ctx);

Core code calling helpers. Tsk :-)
> +	if (ret)
> +		goto out;
> +
>  	for (i = 0; i < arg->count_objs; i++) {
>  		uint32_t obj_id, count_props;
>  		struct drm_mode_object *obj;
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 04d23cb430bf..a56ae2acda90 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -41,6 +41,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_psr_helper.h>
>  
>  #include "drm_crtc_internal.h"
>  #include "drm_crtc_helper_internal.h"
> @@ -406,6 +407,10 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>  
>  	state->acquire_ctx = &ctx;
>  retry:
> +	ret = drm_psr_helper_flush(dev, state->acquire_ctx);
> +	if (ret)
> +		goto out_state;
> +
>  	drm_for_each_plane(plane, dev) {
>  		plane_state = drm_atomic_get_plane_state(state, plane);
>  		if (IS_ERR(plane_state)) {
> @@ -1442,6 +1447,10 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>  
>  	state->acquire_ctx = &ctx;
>  retry:
> +	ret = drm_psr_helper_flush(dev, &ctx);
> +	if (ret)
> +		goto out_state;
> +
>  	for (i = 0; i < fb_helper->crtc_count; i++) {
>  		crtc = fb_helper->crtc_info[i].mode_set.crtc;
>  
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index d8d75e25f6fb..43b029a88578 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -27,6 +27,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_uapi.h>
>  #include <drm/drm_print.h>
> +#include <drm/drm_psr_helper.h>
>  #include <drm/drm_util.h>
>  
>  #include "drm_internal.h"
> @@ -572,6 +573,7 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
>  	struct drm_clip_rect __user *clips_ptr;
>  	struct drm_clip_rect *clips = NULL;
>  	struct drm_mode_fb_dirty_cmd *r = data;
> +	struct drm_modeset_acquire_ctx ctx;
>  	struct drm_framebuffer *fb;
>  	unsigned flags;
>  	int num_clips;
> @@ -580,10 +582,24 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> +	drm_modeset_acquire_init(&ctx, 0);
> +
>  	fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id);
>  	if (!fb)
>  		return -ENOENT;
>  
> +retry:
> +	ret = drm_psr_helper_flush(fb->dev, &ctx);
> +	if (ret) {
> +		if (ret == -EDEADLK) {
> +			ret = drm_modeset_backoff(&ctx);
> +			if (!ret)
> +				goto retry;
> +		} else {
> +			goto out_err1;
> +		}
> +	}
> +
>  	num_clips = r->num_clips;
>  	clips_ptr = (struct drm_clip_rect __user *)(unsigned long)r->clips_ptr;
>  
> @@ -630,6 +646,8 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
>  	kfree(clips);
>  out_err1:
>  	drm_framebuffer_put(fb);
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
>  
>  	return ret;
>  }

So for the high-level design, before looking at the implementation
details:
- I agree with Jose that a per-crtc state sounds more like what we want,
  instead of global. Just from a conceptual point.
- We can imo flat-out require that your driver is using the fancy new
  dirtyfb helpers to remap the dirtyfb call to an atomic update. It's the
  clean design for sw controlled manual update displays anyway.
- That means your single entry points (instead of trying to catch all of
  them like above, and missing some like you do) are ->atomic_check and
  ->atomic_commit. That also gives you the clean hook into helpers.
- The duplicated state only works when no one else is looking, it's very
  fragile. single-threaded (from a driver pov) suspend/resume is ok, I
  don't think it's a good idea to expand this.

Here's what I'd do:
- Stuff a new self_refresh_active state into drm_crtc_state (not sure we
  should call this psr or self_refresh - latter is imo better name since
  not associated with DP, but longer).

- Use crtc_state->self_refresh_active and crtc_state->active to compute
  the reported active state to userspace in the get_prop functions. This
  because doing a mass search&replace over all drivers is too much.
  Something like

	real_active == active || self_refresh

- In duplicate_state always reset the state
	active = real_active();
	self_refresh = false;
  to disable self refresh and set active to what userspace the "ACTIVE"
  property should be.

- self_refresh state in connector_state sounds like a good idea

- Your idle worker would do a normal atomic commit, no evil state
  duplicate, and then set:
  	self_fresh = active;
	active = false;
  Handling races left as an exercise :-)

- Somewhere in the commit helpers put a function to arm your idle work
  (per-crtc imo simplest).

This should give you a clean lasagne going from core ioctl -> core atomic
-> helpers <-> drivers. And not the spaghetti sprawl of finely sprinkling
the self refresh helpers all over.

Aside: Going snowboarding next week, so if this is a bonghits idea I'm
afraid will take 1 week until I can spin something new :-)

Cheers, Daniel

> diff --git a/drivers/gpu/drm/drm_psr_helper.c b/drivers/gpu/drm/drm_psr_helper.c
> new file mode 100644
> index 000000000000..0b57a2a53075
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_psr_helper.c
> @@ -0,0 +1,343 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2019 Google, Inc.
> + *
> + * Authors:
> + * Sean Paul <seanpaul@chromium.org>
> + */
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_mode_config.h>
> +#include <drm/drm_modeset_lock.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_psr_helper.h>
> +#include <linux/bitops.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +
> +/**
> + * DOC: overview
> + *
> + * This helper library provides an easy way for drivers to leverage the atomic
> + * framework to implement panel self refresh (PSR) support. Drivers are
> + * responsible for intializing and cleaning up the PSR helpers on load/unload.
> + * When drivers identify a display that supports self refreshing (eDP or MIPI
> + * command mode), it should register the affected connector with the PSR
> + * helpers.
> + *
> + * Once a connector is registered, the PSR helpers will monitor activity and
> + * call back into the driver to enable/disable PSR as appropriate. The best way
> + * to think about this is that it's a DPMS on/off request with a flag set in
> + * state that tells you to disable/enable PSR on the panel instead of power-
> + * cycling it.
> + *
> + * Drivers may choose to fully disable their crtc/encoder/bridge hardware, or
> + * they can use the "psr_transition" flag in crtc and connector state if they
> + * want to enter low power mode without full disable (in case full
> + * disable/enable is too slow).
> + *
> + * PSR will be deactivated if there are any atomic updates, even updates that do
> + * not affect the connectors which are self refreshing. Supporting this is
> + * possible but non-trivial due to sharing of hardware resources. Similarly, if
> + * a crtc is driving multiple connectors, PSR will not be initiated on any of
> + * those connectors.
> + */
> +
> +struct drm_psr_state {
> +	struct drm_device *dev;
> +	struct drm_modeset_lock mutex;
> +	struct delayed_work entry_work;
> +	struct drm_atomic_state *save_state;
> +	unsigned int entry_delay_ms;
> +};
> +
> +static void drm_psr_helper_entry_work(struct work_struct *work)
> +{
> +	struct drm_psr_state *psr_state = container_of(to_delayed_work(work),
> +						       struct drm_psr_state,
> +						       entry_work);
> +	struct drm_atomic_state *save_state;
> +	struct drm_device *dev = psr_state->dev;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_atomic_state *state;
> +	struct drm_connector *conn;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	bool commit = false;
> +	int ret, i;
> +
> +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> +
> +	ret = drm_modeset_lock(&psr_state->mutex, &ctx);
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * The only way this can happen is if we schedule the worker while it's
> +	 * already running and that timeout subsequently elapses. Since we hold
> +	 * the psr_state->mutex when scheduling, we also know where the worker
> +	 * is sitting in its execution (hint: look up). In this case, it's
> +	 * possible for the entry worker to run twice for the same commit. Since
> +	 * the hardware hasn't changed since the last save state, just kick out.
> +	 */
> +	if (psr_state->save_state)
> +		goto out;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	save_state = drm_atomic_helper_duplicate_state(dev, &ctx);
> +
> +	/*
> +	 * Now that we have the current the HW state saved, we have to flip the
> +	 * psr_transition bit so we know what type of enable we're dealing with
> +	 * when coming back on.
> +	 *
> +	 * NOTE: We don't check conn->capable here since that could change out
> +	 * from under us. We'll trust the atomic core to only call enable if
> +	 * necessary (ie: only for those connectors/crtcs that currently have
> +	 * psr enabled).
> +	 */
> +	if (IS_ERR(save_state)) {
> +		ret = PTR_ERR(save_state);
> +		goto out;
> +	}
> +	for_each_new_connector_in_state(save_state, conn, conn_state, i) {
> +		if (!conn_state->crtc)
> +			continue;
> +		conn_state->psr_transition = true;
> +	}
> +	for_each_new_crtc_in_state(save_state, crtc, crtc_state, i) {
> +		if (!crtc_state->active)
> +			continue;
> +		crtc_state->psr_transition = true;
> +	}
> +
> +	state->acquire_ctx = &ctx;
> +	drm_connector_list_iter_begin(psr_state->dev, &conn_iter);
> +	drm_for_each_connector_iter(conn, &conn_iter) {
> +		if (!conn->psr_capable)
> +			continue;
> +
> +		conn_state = drm_atomic_get_connector_state(state, conn);
> +		if (IS_ERR(conn_state)) {
> +			ret = PTR_ERR(conn_state);
> +			drm_connector_list_iter_end(&conn_iter);
> +			goto out_free_state;
> +		}
> +
> +		if (!conn_state->crtc)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
> +		if (IS_ERR(crtc_state)) {
> +			ret = PTR_ERR(crtc_state);
> +			drm_connector_list_iter_end(&conn_iter);
> +			goto out_free_state;
> +		}
> +
> +		/* Don't use PSR if the crtc is driving multiple connectors */
> +		if (hweight_long(crtc_state->connector_mask) > 1)
> +			continue;
> +
> +		commit = true;
> +		crtc_state->active = false;
> +		crtc_state->psr_transition = true;
> +		conn_state->psr_transition = true;
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
> +	/* Nothing to commit, so just exit */
> +	if (!commit)
> +		goto out_free_state;
> +
> +	ret = drm_atomic_commit(state);
> +	if (ret)
> +		goto out_free_state;
> +
> +	psr_state->save_state = save_state;
> +	goto out;
> +
> +out_free_state:
> +	drm_atomic_state_put(save_state);
> +	drm_atomic_state_put(state);
> +out:
> +	DRM_MODESET_LOCK_ALL_END(ctx, ret);
> +}
> +
> +static int
> +drm_psr_helper_acquire_modeset_locks(struct drm_atomic_state *state,
> +				     struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_mode_config *config = &state->dev->mode_config;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *new_crtc_state;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *new_plane_state;
> +	int ret, i;
> +
> +	ret = drm_modeset_lock(&config->connection_mutex, ctx);
> +	if (ret)
> +		return ret;
> +
> +	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> +		ret = drm_modeset_lock(&plane->mutex, ctx);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		ret = drm_modeset_lock(&crtc->mutex, ctx);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * drm_psr_helper_flush - Restore the hardware to pre-PSR state
> + * @dev: DRM device
> + * @ctx: An acquire context to use for restoring the state
> + *
> + * This function should be called before every drm_atomic_commit to ensure any
> + * connectors that are currently self-refreshing revert back to being bus
> + * driven. Drivers may call this function outside of the atomic hooks if
> + * they wish to disable PSR pre-emptively (such as upon an input event or when
> + * GPU becomes active).
> + *
> + * If everything is successful, this function will schedule the PSR entry worker
> + * to enable PSR after the driver-specified timeout.
> + *
> + * If the PSR helper is not being used, this is a no-op.
> + */
> +int drm_psr_helper_flush(struct drm_device *dev,
> +			 struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_psr_state *psr_state = config->psr_state;
> +	int ret;
> +
> +	if (!psr_state)
> +		return 0;
> +
> +	ret = drm_modeset_lock(&psr_state->mutex, ctx);
> +	if (ret)
> +		return ret;
> +
> +	if (!psr_state->save_state)
> +		goto out;
> +
> +	ret = drm_psr_helper_acquire_modeset_locks(psr_state->save_state, ctx);
> +	if (ret)
> +		goto out;
> +
> +	ret = drm_atomic_helper_commit_duplicated_state(psr_state->save_state,
> +							ctx);
> +
> +out:
> +	psr_state->save_state = NULL;
> +	if (!ret) {
> +		mod_delayed_work(system_wq, &psr_state->entry_work,
> +				 msecs_to_jiffies(psr_state->entry_delay_ms));
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_psr_helper_flush);
> +
> +/**
> + * drm_psr_helper_register - Registers a connector with the PSR helpers
> + * @connector: the connector which has a PSR-supported display attached
> + *
> + * Note that this can be called once on initialization for fixed panels, or
> + * during enable/hotplug.
> + */
> +int drm_psr_helper_register(struct drm_connector *connector)
> +{
> +	struct drm_mode_config *config = &connector->dev->mode_config;
> +	struct drm_psr_state *psr_state = config->psr_state;
> +
> +	/* PSR helpers are uninitialized */
> +	if (WARN_ON(!psr_state))
> +		return -EINVAL;
> +
> +	/* Acquired via psr_helper_flush */
> +	if (!drm_modeset_is_locked(&psr_state->mutex))
> +		return -EINVAL;
> +
> +	connector->psr_capable = true;
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_psr_helper_register);
> +
> +/**
> + * drm_psr_helper_unregister - Unregisters a connector with the PSR helpers
> + * @connector: the connector to unregister
> + */
> +int drm_psr_helper_unregister(struct drm_connector *connector)
> +{
> +	struct drm_mode_config *config = &connector->dev->mode_config;
> +	struct drm_psr_state *psr_state = config->psr_state;
> +
> +	/* PSR helpers are uninitialized */
> +	if (WARN_ON(!psr_state))
> +		return -EINVAL;
> +
> +	/* Acquired via psr_helper_flush */
> +	if (!drm_modeset_is_locked(&psr_state->mutex))
> +		return -EINVAL;
> +
> +	connector->psr_capable = false;
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_psr_helper_unregister);
> +
> +/**
> + * drm_psr_helper_init - Initializes the PSR helpers
> + * @dev: DRM device
> + * @entry_delay_ms: The amount of time to wait after an atomic commit before
> + *		    activating PSR
> + *
> + * Drivers using the PSR helpers must call this some time after mode_config
> + * is initialized in order to make use of the PSR helpers. Typically
> + * entry_delay_ms is a function of how quickly the hardware can enter/exit PSR.
> + */
> +int drm_psr_helper_init(struct drm_device *dev, unsigned int entry_delay_ms)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_psr_state *psr_state;
> +
> +	psr_state = kzalloc(sizeof(*psr_state), GFP_KERNEL);
> +	if (!psr_state)
> +		return -ENOMEM;
> +
> +	INIT_DELAYED_WORK(&psr_state->entry_work, drm_psr_helper_entry_work);
> +	drm_modeset_lock_init(&psr_state->mutex);
> +	psr_state->dev = dev;
> +	psr_state->entry_delay_ms = entry_delay_ms;
> +
> +	config->psr_state = psr_state;
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_psr_helper_init);
> +
> +/**
> + * drm_psr_helper_cleanup - De-initializes the PSR helpers
> + * @dev: DRM device
> + */
> +void drm_psr_helper_cleanup(struct drm_device *dev)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	cancel_delayed_work_sync(&config->psr_state->entry_work);
> +	kfree(config->psr_state);
> +	config->psr_state = NULL;
> +}
> +EXPORT_SYMBOL(drm_psr_helper_cleanup);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index c8061992d6cb..9612b3d56f30 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -501,6 +501,17 @@ struct drm_connector_state {
>  	/** @tv: TV connector state */
>  	struct drm_tv_connector_state tv;
>  
> +	/**
> +	 * @psr_transition:
> +	 *
> +	 * Used by the PSR helpers to denote when a PSR transition is occuring.
> +	 * If your connector is PSR-capable, register it with the helpers and
> +	 * check this flag in .enable() and .disable(). If it is true, instead
> +	 * of shutting off the panel, put it in or take it out of self
> +	 * refreshing mode.
> +	 */
> +	bool psr_transition;
> +
>  	/**
>  	 * @picture_aspect_ratio: Connector property to control the
>  	 * HDMI infoframe aspect ratio setting.
> @@ -993,6 +1004,17 @@ struct drm_connector {
>  	 */
>  	struct drm_display_info display_info;
>  
> +	/**
> +	 * @psr_capable:
> +	 *
> +	 * Set by the driver via drm_psr_helper_register(). Signals that this
> +	 * connector (and associated pipe) is PSR capable and should be put in
> +	 * low-power mode when it is inactive.
> +	 *
> +	 * Protected by &drm_mode_config.psr_state.mutex
> +	 */
> +	bool psr_capable;
> +
>  	/** @funcs: connector control functions */
>  	const struct drm_connector_funcs *funcs;
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index f7c3022dbdf4..10acd4fc0991 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -299,6 +299,17 @@ struct drm_crtc_state {
>  	 */
>  	bool vrr_enabled;
>  
> +	/**
> +	 * @psr_transition:
> +	 *
> +	 * Used by the PSR helpers to denote when a PSR transition is occuring.
> +	 * This will be set on enable/disable callbacks when PSR is being
> +	 * enabled or disabled. In some cases, it may not be desirable to fully
> +	 * shut off the crtc during PSR. CRTC's can inspect this flag and
> +	 * determine the best course of action.
> +	 */
> +	bool psr_transition;
> +
>  	/**
>  	 * @event:
>  	 *
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 7f60e8eb269a..371b80d090ab 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -37,6 +37,7 @@ struct drm_atomic_state;
>  struct drm_mode_fb_cmd2;
>  struct drm_format_info;
>  struct drm_display_mode;
> +struct drm_psr_state;
>  
>  /**
>   * struct drm_mode_config_funcs - basic driver provided mode setting functions
> @@ -900,6 +901,11 @@ struct drm_mode_config {
>  	 */
>  	struct drm_atomic_state *suspend_state;
>  
> +	/**
> +	 * @psr_state: Holds the state for the psr helper
> +	 */
> +	struct drm_psr_state *psr_state;
> +
>  	const struct drm_mode_config_helper_funcs *helper_private;
>  };
>  
> diff --git a/include/drm/drm_psr_helper.h b/include/drm/drm_psr_helper.h
> new file mode 100644
> index 000000000000..972c4ec98d05
> --- /dev/null
> +++ b/include/drm/drm_psr_helper.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2019 Google, Inc.
> + *
> + * Authors:
> + * Sean Paul <seanpaul@chromium.org>
> + */
> +#ifndef DRM_PSR_HELPER_H_
> +#define DRM_PSR_HELPER_H_
> +
> +struct drm_connector;
> +struct drm_device;
> +struct drm_psr_state;
> +struct drm_modeset_acquire_ctx;
> +
> +int drm_psr_helper_flush(struct drm_device *dev,
> +			 struct drm_modeset_acquire_ctx *ctx);
> +
> +int drm_psr_helper_register(struct drm_connector *connector);
> +int drm_psr_helper_unregister(struct drm_connector *connector);
> +
> +int drm_psr_helper_init(struct drm_device *dev, unsigned int entry_delay_ms);
> +void drm_psr_helper_cleanup(struct drm_device *dev);
> +#endif
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm: Add helpers to kick off PSR enable/disable
  2019-03-01  8:18   ` Daniel Vetter
@ 2019-03-01 16:00     ` Sean Paul
  2019-03-11 12:34       ` Daniel Vetter
  0 siblings, 1 reply; 10+ messages in thread
From: Sean Paul @ 2019-03-01 16:00 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Zain Wang, David Airlie, dri-devel, Tomasz Figa, Maxime Ripard,
	Sean Paul, Sean Paul

On Fri, Mar 01, 2019 at 09:18:13AM +0100, Daniel Vetter wrote:
> On Thu, Feb 28, 2019 at 04:09:30PM -0500, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > This patch adds a new drm helper library to help drivers implement
> > PSR. Drivers choosing to use it will register connectors with
> > PSR-capable displays connected and will receive callbacks when it's time
> > to enter or exit PSR.
> > 
> > In its current form, it has a timer which will trigger after a
> > driver-specified amount of inactivity. When the timer triggers, the
> > helpers will save the current atomic state and issue a new state which
> > has the PSR-enabled pipes turned off. On the next update, the drm core
> > will poke the PSR helpers to restore the saved state to the driver before
> > servicing said update.
> > 
> > From the driver's perspective, this works like a regular disable/enable
> > cycle. The driver need only check the 'psr_transition' state in
> > connector_state and keep the panel turned on when in .disable(), while
> > everything else will cycle off as normal. If drivers want more control,
> > they can use the psr_transition state to enter a low-power state to
> > minimize PSR exit time.
> > 
> > While this carries the PSR moniker, it is not specific to the
> > DisplayPort technology. This can be used for power savings with other
> > types of self refresh, such as MIPI command mode.
> > 
> > Cc: Zain Wang <wzz@rock-chips.com>
> > Cc: Tomasz Figa <tfiga@chromium.org>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>

/snip to the good part

> 
> So for the high-level design, before looking at the implementation
> details:
> - I agree with Jose that a per-crtc state sounds more like what we want,
>   instead of global. Just from a conceptual point.

Agreed this would be better. I was mostly worried about resource sharing
between crtcs. ie: If 2 crtcs share a clock, 1 goes to PSR and the other
changes the rate to something incompatible. However I suppose the driver would
be responsible for sussing this all out, so sure.

It'll be nice to avoid the global lock for psr.

> - We can imo flat-out require that your driver is using the fancy new
>   dirtyfb helpers to remap the dirtyfb call to an atomic update. It's the
>   clean design for sw controlled manual update displays anyway.
> - That means your single entry points (instead of trying to catch all of
>   them like above, and missing some like you do) are ->atomic_check and
>   ->atomic_commit. That also gives you the clean hook into helpers.

The reason I did this was because new state was being calculated off the
disabled state. So the psr disable commit needed to precede the atomic
check. However, with your suggestions below I think we can avoid that :)

> - The duplicated state only works when no one else is looking, it's very
>   fragile. single-threaded (from a driver pov) suspend/resume is ok, I
>   don't think it's a good idea to expand this.

Yeah, fair. I was trying to stay out of the way of the existing code, but I'll
integrate more tightly.

> 
> Here's what I'd do:
> - Stuff a new self_refresh_active state into drm_crtc_state (not sure we
>   should call this psr or self_refresh - latter is imo better name since
>   not associated with DP, but longer).

I struggled with this too, naming is hard.

> 
> - Use crtc_state->self_refresh_active and crtc_state->active to compute
>   the reported active state to userspace in the get_prop functions. This
>   because doing a mass search&replace over all drivers is too much.
>   Something like
> 
> 	real_active == active || self_refresh
> 
> - In duplicate_state always reset the state
> 	active = real_active();
> 	self_refresh = false;
>   to disable self refresh and set active to what userspace the "ACTIVE"
>   property should be.

We'll also have to alter the flags when they don't include
DRM_MODE_ATOMIC_ALLOW_MODESET, and async commits would need to go through the
synchronous path. Conversely, non-blocking commits will get back to userspace
more quickly with this design.

This is the primary reason I kept the psr disable in a separate commit, so the
original request could go through as intended (albeit a bit delayed, but there's
no getting around that).

> 
> - self_refresh state in connector_state sounds like a good idea
> 
> - Your idle worker would do a normal atomic commit, no evil state
>   duplicate, and then set:
>   	self_fresh = active;
> 	active = false;
>   Handling races left as an exercise :-)
> 
> - Somewhere in the commit helpers put a function to arm your idle work
>   (per-crtc imo simplest).
> 
> This should give you a clean lasagne going from core ioctl -> core atomic
> -> helpers <-> drivers. And not the spaghetti sprawl of finely sprinkling
> the self refresh helpers all over.

Alright, I'll mock this up and see if it floats. Thanks for the detailed
feedback!

Sean

> 
> Aside: Going snowboarding next week, so if this is a bonghits idea I'm
> afraid will take 1 week until I can spin something new :-)
> 
> Cheers, Daniel
> 
> > diff --git a/drivers/gpu/drm/drm_psr_helper.c b/drivers/gpu/drm/drm_psr_helper.c
> > new file mode 100644
> > index 000000000000..0b57a2a53075
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_psr_helper.c
> > @@ -0,0 +1,343 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright (C) 2019 Google, Inc.
> > + *
> > + * Authors:
> > + * Sean Paul <seanpaul@chromium.org>
> > + */
> > +#include <drm/drm_atomic.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_connector.h>
> > +#include <drm/drm_device.h>
> > +#include <drm/drm_mode_config.h>
> > +#include <drm/drm_modeset_lock.h>
> > +#include <drm/drm_print.h>
> > +#include <drm/drm_psr_helper.h>
> > +#include <linux/bitops.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +#include <linux/workqueue.h>
> > +
> > +/**
> > + * DOC: overview
> > + *
> > + * This helper library provides an easy way for drivers to leverage the atomic
> > + * framework to implement panel self refresh (PSR) support. Drivers are
> > + * responsible for intializing and cleaning up the PSR helpers on load/unload.
> > + * When drivers identify a display that supports self refreshing (eDP or MIPI
> > + * command mode), it should register the affected connector with the PSR
> > + * helpers.
> > + *
> > + * Once a connector is registered, the PSR helpers will monitor activity and
> > + * call back into the driver to enable/disable PSR as appropriate. The best way
> > + * to think about this is that it's a DPMS on/off request with a flag set in
> > + * state that tells you to disable/enable PSR on the panel instead of power-
> > + * cycling it.
> > + *
> > + * Drivers may choose to fully disable their crtc/encoder/bridge hardware, or
> > + * they can use the "psr_transition" flag in crtc and connector state if they
> > + * want to enter low power mode without full disable (in case full
> > + * disable/enable is too slow).
> > + *
> > + * PSR will be deactivated if there are any atomic updates, even updates that do
> > + * not affect the connectors which are self refreshing. Supporting this is
> > + * possible but non-trivial due to sharing of hardware resources. Similarly, if
> > + * a crtc is driving multiple connectors, PSR will not be initiated on any of
> > + * those connectors.
> > + */
> > +
> > +struct drm_psr_state {
> > +	struct drm_device *dev;
> > +	struct drm_modeset_lock mutex;
> > +	struct delayed_work entry_work;
> > +	struct drm_atomic_state *save_state;
> > +	unsigned int entry_delay_ms;
> > +};
> > +
> > +static void drm_psr_helper_entry_work(struct work_struct *work)
> > +{
> > +	struct drm_psr_state *psr_state = container_of(to_delayed_work(work),
> > +						       struct drm_psr_state,
> > +						       entry_work);
> > +	struct drm_atomic_state *save_state;
> > +	struct drm_device *dev = psr_state->dev;
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_atomic_state *state;
> > +	struct drm_connector *conn;
> > +	struct drm_connector_list_iter conn_iter;
> > +	struct drm_connector_state *conn_state;
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *crtc_state;
> > +	bool commit = false;
> > +	int ret, i;
> > +
> > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> > +
> > +	ret = drm_modeset_lock(&psr_state->mutex, &ctx);
> > +	if (ret)
> > +		goto out;
> > +
> > +	/*
> > +	 * The only way this can happen is if we schedule the worker while it's
> > +	 * already running and that timeout subsequently elapses. Since we hold
> > +	 * the psr_state->mutex when scheduling, we also know where the worker
> > +	 * is sitting in its execution (hint: look up). In this case, it's
> > +	 * possible for the entry worker to run twice for the same commit. Since
> > +	 * the hardware hasn't changed since the last save state, just kick out.
> > +	 */
> > +	if (psr_state->save_state)
> > +		goto out;
> > +
> > +	state = drm_atomic_state_alloc(dev);
> > +	if (!state) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	save_state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > +
> > +	/*
> > +	 * Now that we have the current the HW state saved, we have to flip the
> > +	 * psr_transition bit so we know what type of enable we're dealing with
> > +	 * when coming back on.
> > +	 *
> > +	 * NOTE: We don't check conn->capable here since that could change out
> > +	 * from under us. We'll trust the atomic core to only call enable if
> > +	 * necessary (ie: only for those connectors/crtcs that currently have
> > +	 * psr enabled).
> > +	 */
> > +	if (IS_ERR(save_state)) {
> > +		ret = PTR_ERR(save_state);
> > +		goto out;
> > +	}
> > +	for_each_new_connector_in_state(save_state, conn, conn_state, i) {
> > +		if (!conn_state->crtc)
> > +			continue;
> > +		conn_state->psr_transition = true;
> > +	}
> > +	for_each_new_crtc_in_state(save_state, crtc, crtc_state, i) {
> > +		if (!crtc_state->active)
> > +			continue;
> > +		crtc_state->psr_transition = true;
> > +	}
> > +
> > +	state->acquire_ctx = &ctx;
> > +	drm_connector_list_iter_begin(psr_state->dev, &conn_iter);
> > +	drm_for_each_connector_iter(conn, &conn_iter) {
> > +		if (!conn->psr_capable)
> > +			continue;
> > +
> > +		conn_state = drm_atomic_get_connector_state(state, conn);
> > +		if (IS_ERR(conn_state)) {
> > +			ret = PTR_ERR(conn_state);
> > +			drm_connector_list_iter_end(&conn_iter);
> > +			goto out_free_state;
> > +		}
> > +
> > +		if (!conn_state->crtc)
> > +			continue;
> > +
> > +		crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
> > +		if (IS_ERR(crtc_state)) {
> > +			ret = PTR_ERR(crtc_state);
> > +			drm_connector_list_iter_end(&conn_iter);
> > +			goto out_free_state;
> > +		}
> > +
> > +		/* Don't use PSR if the crtc is driving multiple connectors */
> > +		if (hweight_long(crtc_state->connector_mask) > 1)
> > +			continue;
> > +
> > +		commit = true;
> > +		crtc_state->active = false;
> > +		crtc_state->psr_transition = true;
> > +		conn_state->psr_transition = true;
> > +	}
> > +	drm_connector_list_iter_end(&conn_iter);
> > +
> > +	/* Nothing to commit, so just exit */
> > +	if (!commit)
> > +		goto out_free_state;
> > +
> > +	ret = drm_atomic_commit(state);
> > +	if (ret)
> > +		goto out_free_state;
> > +
> > +	psr_state->save_state = save_state;
> > +	goto out;
> > +
> > +out_free_state:
> > +	drm_atomic_state_put(save_state);
> > +	drm_atomic_state_put(state);
> > +out:
> > +	DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > +}
> > +
> > +static int
> > +drm_psr_helper_acquire_modeset_locks(struct drm_atomic_state *state,
> > +				     struct drm_modeset_acquire_ctx *ctx)
> > +{
> > +	struct drm_mode_config *config = &state->dev->mode_config;
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *new_crtc_state;
> > +	struct drm_plane *plane;
> > +	struct drm_plane_state *new_plane_state;
> > +	int ret, i;
> > +
> > +	ret = drm_modeset_lock(&config->connection_mutex, ctx);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> > +		ret = drm_modeset_lock(&plane->mutex, ctx);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > +		ret = drm_modeset_lock(&crtc->mutex, ctx);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> > +/**
> > + * drm_psr_helper_flush - Restore the hardware to pre-PSR state
> > + * @dev: DRM device
> > + * @ctx: An acquire context to use for restoring the state
> > + *
> > + * This function should be called before every drm_atomic_commit to ensure any
> > + * connectors that are currently self-refreshing revert back to being bus
> > + * driven. Drivers may call this function outside of the atomic hooks if
> > + * they wish to disable PSR pre-emptively (such as upon an input event or when
> > + * GPU becomes active).
> > + *
> > + * If everything is successful, this function will schedule the PSR entry worker
> > + * to enable PSR after the driver-specified timeout.
> > + *
> > + * If the PSR helper is not being used, this is a no-op.
> > + */
> > +int drm_psr_helper_flush(struct drm_device *dev,
> > +			 struct drm_modeset_acquire_ctx *ctx)
> > +{
> > +	struct drm_mode_config *config = &dev->mode_config;
> > +	struct drm_psr_state *psr_state = config->psr_state;
> > +	int ret;
> > +
> > +	if (!psr_state)
> > +		return 0;
> > +
> > +	ret = drm_modeset_lock(&psr_state->mutex, ctx);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!psr_state->save_state)
> > +		goto out;
> > +
> > +	ret = drm_psr_helper_acquire_modeset_locks(psr_state->save_state, ctx);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = drm_atomic_helper_commit_duplicated_state(psr_state->save_state,
> > +							ctx);
> > +
> > +out:
> > +	psr_state->save_state = NULL;
> > +	if (!ret) {
> > +		mod_delayed_work(system_wq, &psr_state->entry_work,
> > +				 msecs_to_jiffies(psr_state->entry_delay_ms));
> > +	}
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(drm_psr_helper_flush);
> > +
> > +/**
> > + * drm_psr_helper_register - Registers a connector with the PSR helpers
> > + * @connector: the connector which has a PSR-supported display attached
> > + *
> > + * Note that this can be called once on initialization for fixed panels, or
> > + * during enable/hotplug.
> > + */
> > +int drm_psr_helper_register(struct drm_connector *connector)
> > +{
> > +	struct drm_mode_config *config = &connector->dev->mode_config;
> > +	struct drm_psr_state *psr_state = config->psr_state;
> > +
> > +	/* PSR helpers are uninitialized */
> > +	if (WARN_ON(!psr_state))
> > +		return -EINVAL;
> > +
> > +	/* Acquired via psr_helper_flush */
> > +	if (!drm_modeset_is_locked(&psr_state->mutex))
> > +		return -EINVAL;
> > +
> > +	connector->psr_capable = true;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_psr_helper_register);
> > +
> > +/**
> > + * drm_psr_helper_unregister - Unregisters a connector with the PSR helpers
> > + * @connector: the connector to unregister
> > + */
> > +int drm_psr_helper_unregister(struct drm_connector *connector)
> > +{
> > +	struct drm_mode_config *config = &connector->dev->mode_config;
> > +	struct drm_psr_state *psr_state = config->psr_state;
> > +
> > +	/* PSR helpers are uninitialized */
> > +	if (WARN_ON(!psr_state))
> > +		return -EINVAL;
> > +
> > +	/* Acquired via psr_helper_flush */
> > +	if (!drm_modeset_is_locked(&psr_state->mutex))
> > +		return -EINVAL;
> > +
> > +	connector->psr_capable = false;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_psr_helper_unregister);
> > +
> > +/**
> > + * drm_psr_helper_init - Initializes the PSR helpers
> > + * @dev: DRM device
> > + * @entry_delay_ms: The amount of time to wait after an atomic commit before
> > + *		    activating PSR
> > + *
> > + * Drivers using the PSR helpers must call this some time after mode_config
> > + * is initialized in order to make use of the PSR helpers. Typically
> > + * entry_delay_ms is a function of how quickly the hardware can enter/exit PSR.
> > + */
> > +int drm_psr_helper_init(struct drm_device *dev, unsigned int entry_delay_ms)
> > +{
> > +	struct drm_mode_config *config = &dev->mode_config;
> > +	struct drm_psr_state *psr_state;
> > +
> > +	psr_state = kzalloc(sizeof(*psr_state), GFP_KERNEL);
> > +	if (!psr_state)
> > +		return -ENOMEM;
> > +
> > +	INIT_DELAYED_WORK(&psr_state->entry_work, drm_psr_helper_entry_work);
> > +	drm_modeset_lock_init(&psr_state->mutex);
> > +	psr_state->dev = dev;
> > +	psr_state->entry_delay_ms = entry_delay_ms;
> > +
> > +	config->psr_state = psr_state;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_psr_helper_init);
> > +
> > +/**
> > + * drm_psr_helper_cleanup - De-initializes the PSR helpers
> > + * @dev: DRM device
> > + */
> > +void drm_psr_helper_cleanup(struct drm_device *dev)
> > +{
> > +	struct drm_mode_config *config = &dev->mode_config;
> > +
> > +	cancel_delayed_work_sync(&config->psr_state->entry_work);
> > +	kfree(config->psr_state);
> > +	config->psr_state = NULL;
> > +}
> > +EXPORT_SYMBOL(drm_psr_helper_cleanup);
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index c8061992d6cb..9612b3d56f30 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -501,6 +501,17 @@ struct drm_connector_state {
> >  	/** @tv: TV connector state */
> >  	struct drm_tv_connector_state tv;
> >  
> > +	/**
> > +	 * @psr_transition:
> > +	 *
> > +	 * Used by the PSR helpers to denote when a PSR transition is occuring.
> > +	 * If your connector is PSR-capable, register it with the helpers and
> > +	 * check this flag in .enable() and .disable(). If it is true, instead
> > +	 * of shutting off the panel, put it in or take it out of self
> > +	 * refreshing mode.
> > +	 */
> > +	bool psr_transition;
> > +
> >  	/**
> >  	 * @picture_aspect_ratio: Connector property to control the
> >  	 * HDMI infoframe aspect ratio setting.
> > @@ -993,6 +1004,17 @@ struct drm_connector {
> >  	 */
> >  	struct drm_display_info display_info;
> >  
> > +	/**
> > +	 * @psr_capable:
> > +	 *
> > +	 * Set by the driver via drm_psr_helper_register(). Signals that this
> > +	 * connector (and associated pipe) is PSR capable and should be put in
> > +	 * low-power mode when it is inactive.
> > +	 *
> > +	 * Protected by &drm_mode_config.psr_state.mutex
> > +	 */
> > +	bool psr_capable;
> > +
> >  	/** @funcs: connector control functions */
> >  	const struct drm_connector_funcs *funcs;
> >  
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index f7c3022dbdf4..10acd4fc0991 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -299,6 +299,17 @@ struct drm_crtc_state {
> >  	 */
> >  	bool vrr_enabled;
> >  
> > +	/**
> > +	 * @psr_transition:
> > +	 *
> > +	 * Used by the PSR helpers to denote when a PSR transition is occuring.
> > +	 * This will be set on enable/disable callbacks when PSR is being
> > +	 * enabled or disabled. In some cases, it may not be desirable to fully
> > +	 * shut off the crtc during PSR. CRTC's can inspect this flag and
> > +	 * determine the best course of action.
> > +	 */
> > +	bool psr_transition;
> > +
> >  	/**
> >  	 * @event:
> >  	 *
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index 7f60e8eb269a..371b80d090ab 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -37,6 +37,7 @@ struct drm_atomic_state;
> >  struct drm_mode_fb_cmd2;
> >  struct drm_format_info;
> >  struct drm_display_mode;
> > +struct drm_psr_state;
> >  
> >  /**
> >   * struct drm_mode_config_funcs - basic driver provided mode setting functions
> > @@ -900,6 +901,11 @@ struct drm_mode_config {
> >  	 */
> >  	struct drm_atomic_state *suspend_state;
> >  
> > +	/**
> > +	 * @psr_state: Holds the state for the psr helper
> > +	 */
> > +	struct drm_psr_state *psr_state;
> > +
> >  	const struct drm_mode_config_helper_funcs *helper_private;
> >  };
> >  
> > diff --git a/include/drm/drm_psr_helper.h b/include/drm/drm_psr_helper.h
> > new file mode 100644
> > index 000000000000..972c4ec98d05
> > --- /dev/null
> > +++ b/include/drm/drm_psr_helper.h
> > @@ -0,0 +1,24 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright (C) 2019 Google, Inc.
> > + *
> > + * Authors:
> > + * Sean Paul <seanpaul@chromium.org>
> > + */
> > +#ifndef DRM_PSR_HELPER_H_
> > +#define DRM_PSR_HELPER_H_
> > +
> > +struct drm_connector;
> > +struct drm_device;
> > +struct drm_psr_state;
> > +struct drm_modeset_acquire_ctx;
> > +
> > +int drm_psr_helper_flush(struct drm_device *dev,
> > +			 struct drm_modeset_acquire_ctx *ctx);
> > +
> > +int drm_psr_helper_register(struct drm_connector *connector);
> > +int drm_psr_helper_unregister(struct drm_connector *connector);
> > +
> > +int drm_psr_helper_init(struct drm_device *dev, unsigned int entry_delay_ms);
> > +void drm_psr_helper_cleanup(struct drm_device *dev);
> > +#endif
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm: Add helpers to kick off PSR enable/disable
  2019-03-01 16:00     ` Sean Paul
@ 2019-03-11 12:34       ` Daniel Vetter
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2019-03-11 12:34 UTC (permalink / raw)
  To: Sean Paul
  Cc: Zain Wang, David Airlie, dri-devel, Tomasz Figa, Maxime Ripard,
	Sean Paul

On Fri, Mar 01, 2019 at 11:00:02AM -0500, Sean Paul wrote:
> On Fri, Mar 01, 2019 at 09:18:13AM +0100, Daniel Vetter wrote:
> > On Thu, Feb 28, 2019 at 04:09:30PM -0500, Sean Paul wrote:
> > > From: Sean Paul <seanpaul@chromium.org>
> > > 
> > > This patch adds a new drm helper library to help drivers implement
> > > PSR. Drivers choosing to use it will register connectors with
> > > PSR-capable displays connected and will receive callbacks when it's time
> > > to enter or exit PSR.
> > > 
> > > In its current form, it has a timer which will trigger after a
> > > driver-specified amount of inactivity. When the timer triggers, the
> > > helpers will save the current atomic state and issue a new state which
> > > has the PSR-enabled pipes turned off. On the next update, the drm core
> > > will poke the PSR helpers to restore the saved state to the driver before
> > > servicing said update.
> > > 
> > > From the driver's perspective, this works like a regular disable/enable
> > > cycle. The driver need only check the 'psr_transition' state in
> > > connector_state and keep the panel turned on when in .disable(), while
> > > everything else will cycle off as normal. If drivers want more control,
> > > they can use the psr_transition state to enter a low-power state to
> > > minimize PSR exit time.
> > > 
> > > While this carries the PSR moniker, it is not specific to the
> > > DisplayPort technology. This can be used for power savings with other
> > > types of self refresh, such as MIPI command mode.
> > > 
> > > Cc: Zain Wang <wzz@rock-chips.com>
> > > Cc: Tomasz Figa <tfiga@chromium.org>
> > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> 
> /snip to the good part
> 
> > 
> > So for the high-level design, before looking at the implementation
> > details:
> > - I agree with Jose that a per-crtc state sounds more like what we want,
> >   instead of global. Just from a conceptual point.
> 
> Agreed this would be better. I was mostly worried about resource sharing
> between crtcs. ie: If 2 crtcs share a clock, 1 goes to PSR and the other
> changes the rate to something incompatible. However I suppose the driver would
> be responsible for sussing this all out, so sure.
> 
> It'll be nice to avoid the global lock for psr.

Yeah atomic check must fail in this case. But that's not new, if
ACTVE=false then the clock will also be disabled (or should, for a good
driver), and atomic_check still must fail.

I don't think there's going to be a semantic difference between psr
self-refresh and ACTIVE=false. It might turn up more pre-existing driver
bugs, but then we just need to fix those (good igt coverage would be good,
maybe we can build on top of kms_frontbuffer_rendering a bit here, without
the intel-ism).

> > - We can imo flat-out require that your driver is using the fancy new
> >   dirtyfb helpers to remap the dirtyfb call to an atomic update. It's the
> >   clean design for sw controlled manual update displays anyway.
> > - That means your single entry points (instead of trying to catch all of
> >   them like above, and missing some like you do) are ->atomic_check and
> >   ->atomic_commit. That also gives you the clean hook into helpers.
> 
> The reason I did this was because new state was being calculated off the
> disabled state. So the psr disable commit needed to precede the atomic
> check. However, with your suggestions below I think we can avoid that :)
> 
> > - The duplicated state only works when no one else is looking, it's very
> >   fragile. single-threaded (from a driver pov) suspend/resume is ok, I
> >   don't think it's a good idea to expand this.
> 
> Yeah, fair. I was trying to stay out of the way of the existing code, but I'll
> integrate more tightly.
> 
> > 
> > Here's what I'd do:
> > - Stuff a new self_refresh_active state into drm_crtc_state (not sure we
> >   should call this psr or self_refresh - latter is imo better name since
> >   not associated with DP, but longer).
> 
> I struggled with this too, naming is hard.
> 
> > 
> > - Use crtc_state->self_refresh_active and crtc_state->active to compute
> >   the reported active state to userspace in the get_prop functions. This
> >   because doing a mass search&replace over all drivers is too much.
> >   Something like
> > 
> > 	real_active == active || self_refresh
> > 
> > - In duplicate_state always reset the state
> > 	active = real_active();
> > 	self_refresh = false;
> >   to disable self refresh and set active to what userspace the "ACTIVE"
> >   property should be.
> 
> We'll also have to alter the flags when they don't include
> DRM_MODE_ATOMIC_ALLOW_MODESET, and async commits would need to go through the
> synchronous path. Conversely, non-blocking commits will get back to userspace
> more quickly with this design.
> 
> This is the primary reason I kept the psr disable in a separate commit, so the
> original request could go through as intended (albeit a bit delayed, but there's
> no getting around that).

Hm. In a way we're lying to userspace here. But the lie is intentional I'd
say, so what about we make it official in the atomic ioctl? Something like
the below:

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index aa0c9af78b71..322bee5ab28c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -368,6 +368,18 @@ static int drm_atomic_crtc_check(const struct drm_crtc_state *old_crtc_state,
 		return -EINVAL;
 	}
 
+	/* 
+	 * Transparent self-refresh is a bit a lie to userspace, keep up the
+	 * illusion. We must allow a modeset if one of the crtc is currently in
+	 * self-refresh mode and should be enabled.
+	 */
+	if (old_crtc_state->self_refresh && new_crtc_state->active) {
+		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] self-refresh, transparently allowing modesets\n",
+				 crtc->base.id, crtc->name);
+
+		state->allow_modeset = true;
+	}
+
 	return 0;
 }
 
Note how this is in the core atomic ioctl code, not helpers, so mandatory
lying for all drivers :-)

igt coverage for this would be really nice, or maybe some in-kernel self-tests ....

> > - self_refresh state in connector_state sounds like a good idea
> > 
> > - Your idle worker would do a normal atomic commit, no evil state
> >   duplicate, and then set:
> >   	self_fresh = active;
> > 	active = false;
> >   Handling races left as an exercise :-)
> > 
> > - Somewhere in the commit helpers put a function to arm your idle work
> >   (per-crtc imo simplest).
> > 
> > This should give you a clean lasagne going from core ioctl -> core atomic
> > -> helpers <-> drivers. And not the spaghetti sprawl of finely sprinkling
> > the self refresh helpers all over.
> 
> Alright, I'll mock this up and see if it floats. Thanks for the detailed
> feedback!

Still catching up on mails, hopefully not too much confusion ...

Cheers, Daniel

> 
> Sean
> 
> > 
> > Aside: Going snowboarding next week, so if this is a bonghits idea I'm
> > afraid will take 1 week until I can spin something new :-)
> > 
> > Cheers, Daniel
> > 
> > > diff --git a/drivers/gpu/drm/drm_psr_helper.c b/drivers/gpu/drm/drm_psr_helper.c
> > > new file mode 100644
> > > index 000000000000..0b57a2a53075
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_psr_helper.c
> > > @@ -0,0 +1,343 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright (C) 2019 Google, Inc.
> > > + *
> > > + * Authors:
> > > + * Sean Paul <seanpaul@chromium.org>
> > > + */
> > > +#include <drm/drm_atomic.h>
> > > +#include <drm/drm_atomic_helper.h>
> > > +#include <drm/drm_connector.h>
> > > +#include <drm/drm_device.h>
> > > +#include <drm/drm_mode_config.h>
> > > +#include <drm/drm_modeset_lock.h>
> > > +#include <drm/drm_print.h>
> > > +#include <drm/drm_psr_helper.h>
> > > +#include <linux/bitops.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/workqueue.h>
> > > +
> > > +/**
> > > + * DOC: overview
> > > + *
> > > + * This helper library provides an easy way for drivers to leverage the atomic
> > > + * framework to implement panel self refresh (PSR) support. Drivers are
> > > + * responsible for intializing and cleaning up the PSR helpers on load/unload.
> > > + * When drivers identify a display that supports self refreshing (eDP or MIPI
> > > + * command mode), it should register the affected connector with the PSR
> > > + * helpers.
> > > + *
> > > + * Once a connector is registered, the PSR helpers will monitor activity and
> > > + * call back into the driver to enable/disable PSR as appropriate. The best way
> > > + * to think about this is that it's a DPMS on/off request with a flag set in
> > > + * state that tells you to disable/enable PSR on the panel instead of power-
> > > + * cycling it.
> > > + *
> > > + * Drivers may choose to fully disable their crtc/encoder/bridge hardware, or
> > > + * they can use the "psr_transition" flag in crtc and connector state if they
> > > + * want to enter low power mode without full disable (in case full
> > > + * disable/enable is too slow).
> > > + *
> > > + * PSR will be deactivated if there are any atomic updates, even updates that do
> > > + * not affect the connectors which are self refreshing. Supporting this is
> > > + * possible but non-trivial due to sharing of hardware resources. Similarly, if
> > > + * a crtc is driving multiple connectors, PSR will not be initiated on any of
> > > + * those connectors.
> > > + */
> > > +
> > > +struct drm_psr_state {
> > > +	struct drm_device *dev;
> > > +	struct drm_modeset_lock mutex;
> > > +	struct delayed_work entry_work;
> > > +	struct drm_atomic_state *save_state;
> > > +	unsigned int entry_delay_ms;
> > > +};
> > > +
> > > +static void drm_psr_helper_entry_work(struct work_struct *work)
> > > +{
> > > +	struct drm_psr_state *psr_state = container_of(to_delayed_work(work),
> > > +						       struct drm_psr_state,
> > > +						       entry_work);
> > > +	struct drm_atomic_state *save_state;
> > > +	struct drm_device *dev = psr_state->dev;
> > > +	struct drm_modeset_acquire_ctx ctx;
> > > +	struct drm_atomic_state *state;
> > > +	struct drm_connector *conn;
> > > +	struct drm_connector_list_iter conn_iter;
> > > +	struct drm_connector_state *conn_state;
> > > +	struct drm_crtc *crtc;
> > > +	struct drm_crtc_state *crtc_state;
> > > +	bool commit = false;
> > > +	int ret, i;
> > > +
> > > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> > > +
> > > +	ret = drm_modeset_lock(&psr_state->mutex, &ctx);
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	/*
> > > +	 * The only way this can happen is if we schedule the worker while it's
> > > +	 * already running and that timeout subsequently elapses. Since we hold
> > > +	 * the psr_state->mutex when scheduling, we also know where the worker
> > > +	 * is sitting in its execution (hint: look up). In this case, it's
> > > +	 * possible for the entry worker to run twice for the same commit. Since
> > > +	 * the hardware hasn't changed since the last save state, just kick out.
> > > +	 */
> > > +	if (psr_state->save_state)
> > > +		goto out;
> > > +
> > > +	state = drm_atomic_state_alloc(dev);
> > > +	if (!state) {
> > > +		ret = -ENOMEM;
> > > +		goto out;
> > > +	}
> > > +
> > > +	save_state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > +
> > > +	/*
> > > +	 * Now that we have the current the HW state saved, we have to flip the
> > > +	 * psr_transition bit so we know what type of enable we're dealing with
> > > +	 * when coming back on.
> > > +	 *
> > > +	 * NOTE: We don't check conn->capable here since that could change out
> > > +	 * from under us. We'll trust the atomic core to only call enable if
> > > +	 * necessary (ie: only for those connectors/crtcs that currently have
> > > +	 * psr enabled).
> > > +	 */
> > > +	if (IS_ERR(save_state)) {
> > > +		ret = PTR_ERR(save_state);
> > > +		goto out;
> > > +	}
> > > +	for_each_new_connector_in_state(save_state, conn, conn_state, i) {
> > > +		if (!conn_state->crtc)
> > > +			continue;
> > > +		conn_state->psr_transition = true;
> > > +	}
> > > +	for_each_new_crtc_in_state(save_state, crtc, crtc_state, i) {
> > > +		if (!crtc_state->active)
> > > +			continue;
> > > +		crtc_state->psr_transition = true;
> > > +	}
> > > +
> > > +	state->acquire_ctx = &ctx;
> > > +	drm_connector_list_iter_begin(psr_state->dev, &conn_iter);
> > > +	drm_for_each_connector_iter(conn, &conn_iter) {
> > > +		if (!conn->psr_capable)
> > > +			continue;
> > > +
> > > +		conn_state = drm_atomic_get_connector_state(state, conn);
> > > +		if (IS_ERR(conn_state)) {
> > > +			ret = PTR_ERR(conn_state);
> > > +			drm_connector_list_iter_end(&conn_iter);
> > > +			goto out_free_state;
> > > +		}
> > > +
> > > +		if (!conn_state->crtc)
> > > +			continue;
> > > +
> > > +		crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
> > > +		if (IS_ERR(crtc_state)) {
> > > +			ret = PTR_ERR(crtc_state);
> > > +			drm_connector_list_iter_end(&conn_iter);
> > > +			goto out_free_state;
> > > +		}
> > > +
> > > +		/* Don't use PSR if the crtc is driving multiple connectors */
> > > +		if (hweight_long(crtc_state->connector_mask) > 1)
> > > +			continue;
> > > +
> > > +		commit = true;
> > > +		crtc_state->active = false;
> > > +		crtc_state->psr_transition = true;
> > > +		conn_state->psr_transition = true;
> > > +	}
> > > +	drm_connector_list_iter_end(&conn_iter);
> > > +
> > > +	/* Nothing to commit, so just exit */
> > > +	if (!commit)
> > > +		goto out_free_state;
> > > +
> > > +	ret = drm_atomic_commit(state);
> > > +	if (ret)
> > > +		goto out_free_state;
> > > +
> > > +	psr_state->save_state = save_state;
> > > +	goto out;
> > > +
> > > +out_free_state:
> > > +	drm_atomic_state_put(save_state);
> > > +	drm_atomic_state_put(state);
> > > +out:
> > > +	DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > > +}
> > > +
> > > +static int
> > > +drm_psr_helper_acquire_modeset_locks(struct drm_atomic_state *state,
> > > +				     struct drm_modeset_acquire_ctx *ctx)
> > > +{
> > > +	struct drm_mode_config *config = &state->dev->mode_config;
> > > +	struct drm_crtc *crtc;
> > > +	struct drm_crtc_state *new_crtc_state;
> > > +	struct drm_plane *plane;
> > > +	struct drm_plane_state *new_plane_state;
> > > +	int ret, i;
> > > +
> > > +	ret = drm_modeset_lock(&config->connection_mutex, ctx);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> > > +		ret = drm_modeset_lock(&plane->mutex, ctx);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > +		ret = drm_modeset_lock(&crtc->mutex, ctx);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * drm_psr_helper_flush - Restore the hardware to pre-PSR state
> > > + * @dev: DRM device
> > > + * @ctx: An acquire context to use for restoring the state
> > > + *
> > > + * This function should be called before every drm_atomic_commit to ensure any
> > > + * connectors that are currently self-refreshing revert back to being bus
> > > + * driven. Drivers may call this function outside of the atomic hooks if
> > > + * they wish to disable PSR pre-emptively (such as upon an input event or when
> > > + * GPU becomes active).
> > > + *
> > > + * If everything is successful, this function will schedule the PSR entry worker
> > > + * to enable PSR after the driver-specified timeout.
> > > + *
> > > + * If the PSR helper is not being used, this is a no-op.
> > > + */
> > > +int drm_psr_helper_flush(struct drm_device *dev,
> > > +			 struct drm_modeset_acquire_ctx *ctx)
> > > +{
> > > +	struct drm_mode_config *config = &dev->mode_config;
> > > +	struct drm_psr_state *psr_state = config->psr_state;
> > > +	int ret;
> > > +
> > > +	if (!psr_state)
> > > +		return 0;
> > > +
> > > +	ret = drm_modeset_lock(&psr_state->mutex, ctx);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (!psr_state->save_state)
> > > +		goto out;
> > > +
> > > +	ret = drm_psr_helper_acquire_modeset_locks(psr_state->save_state, ctx);
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	ret = drm_atomic_helper_commit_duplicated_state(psr_state->save_state,
> > > +							ctx);
> > > +
> > > +out:
> > > +	psr_state->save_state = NULL;
> > > +	if (!ret) {
> > > +		mod_delayed_work(system_wq, &psr_state->entry_work,
> > > +				 msecs_to_jiffies(psr_state->entry_delay_ms));
> > > +	}
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL(drm_psr_helper_flush);
> > > +
> > > +/**
> > > + * drm_psr_helper_register - Registers a connector with the PSR helpers
> > > + * @connector: the connector which has a PSR-supported display attached
> > > + *
> > > + * Note that this can be called once on initialization for fixed panels, or
> > > + * during enable/hotplug.
> > > + */
> > > +int drm_psr_helper_register(struct drm_connector *connector)
> > > +{
> > > +	struct drm_mode_config *config = &connector->dev->mode_config;
> > > +	struct drm_psr_state *psr_state = config->psr_state;
> > > +
> > > +	/* PSR helpers are uninitialized */
> > > +	if (WARN_ON(!psr_state))
> > > +		return -EINVAL;
> > > +
> > > +	/* Acquired via psr_helper_flush */
> > > +	if (!drm_modeset_is_locked(&psr_state->mutex))
> > > +		return -EINVAL;
> > > +
> > > +	connector->psr_capable = true;
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_psr_helper_register);
> > > +
> > > +/**
> > > + * drm_psr_helper_unregister - Unregisters a connector with the PSR helpers
> > > + * @connector: the connector to unregister
> > > + */
> > > +int drm_psr_helper_unregister(struct drm_connector *connector)
> > > +{
> > > +	struct drm_mode_config *config = &connector->dev->mode_config;
> > > +	struct drm_psr_state *psr_state = config->psr_state;
> > > +
> > > +	/* PSR helpers are uninitialized */
> > > +	if (WARN_ON(!psr_state))
> > > +		return -EINVAL;
> > > +
> > > +	/* Acquired via psr_helper_flush */
> > > +	if (!drm_modeset_is_locked(&psr_state->mutex))
> > > +		return -EINVAL;
> > > +
> > > +	connector->psr_capable = false;
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_psr_helper_unregister);
> > > +
> > > +/**
> > > + * drm_psr_helper_init - Initializes the PSR helpers
> > > + * @dev: DRM device
> > > + * @entry_delay_ms: The amount of time to wait after an atomic commit before
> > > + *		    activating PSR
> > > + *
> > > + * Drivers using the PSR helpers must call this some time after mode_config
> > > + * is initialized in order to make use of the PSR helpers. Typically
> > > + * entry_delay_ms is a function of how quickly the hardware can enter/exit PSR.
> > > + */
> > > +int drm_psr_helper_init(struct drm_device *dev, unsigned int entry_delay_ms)
> > > +{
> > > +	struct drm_mode_config *config = &dev->mode_config;
> > > +	struct drm_psr_state *psr_state;
> > > +
> > > +	psr_state = kzalloc(sizeof(*psr_state), GFP_KERNEL);
> > > +	if (!psr_state)
> > > +		return -ENOMEM;
> > > +
> > > +	INIT_DELAYED_WORK(&psr_state->entry_work, drm_psr_helper_entry_work);
> > > +	drm_modeset_lock_init(&psr_state->mutex);
> > > +	psr_state->dev = dev;
> > > +	psr_state->entry_delay_ms = entry_delay_ms;
> > > +
> > > +	config->psr_state = psr_state;
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_psr_helper_init);
> > > +
> > > +/**
> > > + * drm_psr_helper_cleanup - De-initializes the PSR helpers
> > > + * @dev: DRM device
> > > + */
> > > +void drm_psr_helper_cleanup(struct drm_device *dev)
> > > +{
> > > +	struct drm_mode_config *config = &dev->mode_config;
> > > +
> > > +	cancel_delayed_work_sync(&config->psr_state->entry_work);
> > > +	kfree(config->psr_state);
> > > +	config->psr_state = NULL;
> > > +}
> > > +EXPORT_SYMBOL(drm_psr_helper_cleanup);
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index c8061992d6cb..9612b3d56f30 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -501,6 +501,17 @@ struct drm_connector_state {
> > >  	/** @tv: TV connector state */
> > >  	struct drm_tv_connector_state tv;
> > >  
> > > +	/**
> > > +	 * @psr_transition:
> > > +	 *
> > > +	 * Used by the PSR helpers to denote when a PSR transition is occuring.
> > > +	 * If your connector is PSR-capable, register it with the helpers and
> > > +	 * check this flag in .enable() and .disable(). If it is true, instead
> > > +	 * of shutting off the panel, put it in or take it out of self
> > > +	 * refreshing mode.
> > > +	 */
> > > +	bool psr_transition;
> > > +
> > >  	/**
> > >  	 * @picture_aspect_ratio: Connector property to control the
> > >  	 * HDMI infoframe aspect ratio setting.
> > > @@ -993,6 +1004,17 @@ struct drm_connector {
> > >  	 */
> > >  	struct drm_display_info display_info;
> > >  
> > > +	/**
> > > +	 * @psr_capable:
> > > +	 *
> > > +	 * Set by the driver via drm_psr_helper_register(). Signals that this
> > > +	 * connector (and associated pipe) is PSR capable and should be put in
> > > +	 * low-power mode when it is inactive.
> > > +	 *
> > > +	 * Protected by &drm_mode_config.psr_state.mutex
> > > +	 */
> > > +	bool psr_capable;
> > > +
> > >  	/** @funcs: connector control functions */
> > >  	const struct drm_connector_funcs *funcs;
> > >  
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index f7c3022dbdf4..10acd4fc0991 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -299,6 +299,17 @@ struct drm_crtc_state {
> > >  	 */
> > >  	bool vrr_enabled;
> > >  
> > > +	/**
> > > +	 * @psr_transition:
> > > +	 *
> > > +	 * Used by the PSR helpers to denote when a PSR transition is occuring.
> > > +	 * This will be set on enable/disable callbacks when PSR is being
> > > +	 * enabled or disabled. In some cases, it may not be desirable to fully
> > > +	 * shut off the crtc during PSR. CRTC's can inspect this flag and
> > > +	 * determine the best course of action.
> > > +	 */
> > > +	bool psr_transition;
> > > +
> > >  	/**
> > >  	 * @event:
> > >  	 *
> > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > > index 7f60e8eb269a..371b80d090ab 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -37,6 +37,7 @@ struct drm_atomic_state;
> > >  struct drm_mode_fb_cmd2;
> > >  struct drm_format_info;
> > >  struct drm_display_mode;
> > > +struct drm_psr_state;
> > >  
> > >  /**
> > >   * struct drm_mode_config_funcs - basic driver provided mode setting functions
> > > @@ -900,6 +901,11 @@ struct drm_mode_config {
> > >  	 */
> > >  	struct drm_atomic_state *suspend_state;
> > >  
> > > +	/**
> > > +	 * @psr_state: Holds the state for the psr helper
> > > +	 */
> > > +	struct drm_psr_state *psr_state;
> > > +
> > >  	const struct drm_mode_config_helper_funcs *helper_private;
> > >  };
> > >  
> > > diff --git a/include/drm/drm_psr_helper.h b/include/drm/drm_psr_helper.h
> > > new file mode 100644
> > > index 000000000000..972c4ec98d05
> > > --- /dev/null
> > > +++ b/include/drm/drm_psr_helper.h
> > > @@ -0,0 +1,24 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright (C) 2019 Google, Inc.
> > > + *
> > > + * Authors:
> > > + * Sean Paul <seanpaul@chromium.org>
> > > + */
> > > +#ifndef DRM_PSR_HELPER_H_
> > > +#define DRM_PSR_HELPER_H_
> > > +
> > > +struct drm_connector;
> > > +struct drm_device;
> > > +struct drm_psr_state;
> > > +struct drm_modeset_acquire_ctx;
> > > +
> > > +int drm_psr_helper_flush(struct drm_device *dev,
> > > +			 struct drm_modeset_acquire_ctx *ctx);
> > > +
> > > +int drm_psr_helper_register(struct drm_connector *connector);
> > > +int drm_psr_helper_unregister(struct drm_connector *connector);
> > > +
> > > +int drm_psr_helper_init(struct drm_device *dev, unsigned int entry_delay_ms);
> > > +void drm_psr_helper_cleanup(struct drm_device *dev);
> > > +#endif
> > > -- 
> > > Sean Paul, Software Engineer, Google / Chromium OS
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-03-11 12:34 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-28 21:09 [PATCH 0/5] drm: Add PSR helpers Sean Paul
2019-02-28 21:09 ` [PATCH 1/5] drm: Add helpers to kick off PSR enable/disable Sean Paul
2019-02-28 22:29   ` Souza, Jose
2019-03-01  8:18   ` Daniel Vetter
2019-03-01 16:00     ` Sean Paul
2019-03-11 12:34       ` Daniel Vetter
2019-02-28 21:09 ` [PATCH 2/5] drm/rockchip: Check for fast link training before enabling psr Sean Paul
2019-02-28 21:09 ` [PATCH 3/5] drm/rockchip: Use the helpers for PSR Sean Paul
2019-02-28 21:09 ` [PATCH 4/5] drm/rockchip: Don't fully disable vop on PSR Sean Paul
2019-02-28 21:09 ` [PATCH 5/5] drm/rockchip: Use drm_atomic_helper_commit_tail_rpm Sean Paul

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.