[PATCH RFC v2 0/2] Disable planes on blanked CRTC and enable on unblank

[PATCH RFC v2 0/2] Disable planes on blanked CRTC and enable on unblank

[Jyri Sarha]

Since first RFC version:
- Added "drm/atomic: Track drm_plane's active state"-patch

We would need something like this to get rid off OMAPDSS somewhat
messy runtime_resume code. How does this look, could this generic
solution be refined to be acceptable for mainline, or should we start
looking a local solution to omapdrm?

Jyri Sarha (2):
  drm/atomic: Track drm_plane's active state
  drm/atomic: Disable planes on blanked CRTC and enable on unblank

 drivers/gpu/drm/drm_atomic_helper.c | 82 +++++++++++++++++++++++++++++--------
 drivers/gpu/drm/drm_plane_helper.c  | 10 +----
 include/drm/drm_atomic_helper.h     | 39 +-----------------
 include/drm/drm_crtc.h              |  2 +
 4 files changed, 70 insertions(+), 63 deletions(-)

-- 
1.9.1

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

[PATCH RFC v2 1/2] drm/atomic: Track drm_plane's active state

[Jyri Sarha]

Track drm_plane's active state with a new boolean in struct
drm_plane_state. The patch replaces drm_atomic_plane_disabling() with
drm_atomic_helper_update_plane_state(). In addition to all the same
things the drm_atomic_plane_disabling() used to do the new function
updates the active-flag and calls atomic_update() or atomic_disable()
from the plane's helper funcs as needed.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 63 ++++++++++++++++++++++++++++---------
 drivers/gpu/drm/drm_plane_helper.c  | 10 +-----
 include/drm/drm_atomic_helper.h     | 39 ++---------------------
 include/drm/drm_crtc.h              |  2 ++
 4 files changed, 53 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index aecb5d6..d03e2ac 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1197,15 +1197,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (!funcs)
 			continue;
 
-		/*
-		 * Special-case disabling the plane if drivers support it.
-		 */
-		if (drm_atomic_plane_disabling(plane, old_plane_state) &&
-		    funcs->atomic_disable)
-			funcs->atomic_disable(plane, old_plane_state);
-		else if (plane->state->crtc ||
-			 drm_atomic_plane_disabling(plane, old_plane_state))
-			funcs->atomic_update(plane, old_plane_state);
+		drm_atomic_helper_update_plane_state(plane, old_plane_state);
 	}
 
 	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
@@ -1266,12 +1258,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
 
 		WARN_ON(plane->state->crtc && plane->state->crtc != crtc);
 
-		if (drm_atomic_plane_disabling(plane, old_plane_state) &&
-		    plane_funcs->atomic_disable)
-			plane_funcs->atomic_disable(plane, old_plane_state);
-		else if (plane->state->crtc ||
-			 drm_atomic_plane_disabling(plane, old_plane_state))
-			plane_funcs->atomic_update(plane, old_plane_state);
+		drm_atomic_helper_update_plane_state(plane, old_plane_state);
 	}
 
 	if (crtc_funcs && crtc_funcs->atomic_flush)
@@ -1279,6 +1266,52 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_planes_on_crtc);
 
+/*
+ * drm_atomic_helper_update_plane_state - update plane's atomic state
+ * @plane: plane object
+ * @old_state: previous atomic state
+ *
+ * Update plane's atomic state, disables it if that is required, and
+ * updates drm_palnes_state's active-flag. This also WARNs if it
+ * detects an invalid state (both CRTC and FB need to either both be
+ * NULL or both be non-NULL).
+ */
+void drm_atomic_helper_update_plane_state(struct drm_plane *plane,
+					  struct drm_plane_state *old_state)
+{
+	const struct drm_plane_helper_funcs *funcs = plane->helper_private;
+
+	/*
+	 * When disabling a plane, CRTC and FB should always be NULL together.
+	 * Anything else should be considered a bug in the atomic core, so we
+	 * gently warn about it.
+	 */
+	WARN_ON((plane->state->crtc == NULL && plane->state->fb != NULL) ||
+		(plane->state->crtc != NULL && plane->state->fb == NULL));
+
+	if (!funcs)
+		return;
+
+	/*
+	 * The plane needs to be active only if it has an associated CRTC
+	 * and the CRTC is active. Use atomic_disable() if available.
+	 */
+	if (plane->state->active) {
+		if (!plane->state->crtc || !plane->state->crtc->state->active) {
+			plane->state->active = false;
+			if (funcs->atomic_disable) {
+				funcs->atomic_disable(plane, old_state);
+				return;
+			}
+		}
+		funcs->atomic_update(plane, old_state);
+	} else if (plane->state->crtc && plane->state->crtc->state->active) {
+		plane->state->active = true;
+		funcs->atomic_update(plane, old_state);
+	}
+}
+EXPORT_SYMBOL(drm_atomic_helper_update_plane_state);
+
 /**
  * drm_atomic_helper_cleanup_planes - cleanup plane resources after commit
  * @dev: DRM device
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 5e5a07a..93052de 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -440,15 +440,7 @@ int drm_plane_helper_commit(struct drm_plane *plane,
 			crtc_funcs[i]->atomic_begin(crtc[i], crtc[i]->state);
 	}
 
-	/*
-	 * Drivers may optionally implement the ->atomic_disable callback, so
-	 * special-case that here.
-	 */
-	if (drm_atomic_plane_disabling(plane, plane_state) &&
-	    plane_funcs->atomic_disable)
-		plane_funcs->atomic_disable(plane, plane_state);
-	else
-		plane_funcs->atomic_update(plane, plane_state);
+	drm_atomic_helper_update_plane_state(plane, plane_state);
 
 	for (i = 0; i < 2; i++) {
 		if (crtc_funcs[i] && crtc_funcs[i]->atomic_flush)
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 11266d14..2fbdf5e 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -59,6 +59,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 				      struct drm_atomic_state *old_state);
 void drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state);
+void drm_atomic_helper_update_plane_state(struct drm_plane *plane,
+					  struct drm_plane_state *old_state);
 
 void drm_atomic_helper_swap_state(struct drm_device *dev,
 				  struct drm_atomic_state *state);
@@ -148,41 +150,4 @@ void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
 #define drm_atomic_crtc_state_for_each_plane(plane, crtc_state) \
 	drm_for_each_plane_mask(plane, (crtc_state)->state->dev, (crtc_state)->plane_mask)
 
-/*
- * drm_atomic_plane_disabling - check whether a plane is being disabled
- * @plane: plane object
- * @old_state: previous atomic state
- *
- * Checks the atomic state of a plane to determine whether it's being disabled
- * or not. This also WARNs if it detects an invalid state (both CRTC and FB
- * need to either both be NULL or both be non-NULL).
- *
- * RETURNS:
- * True if the plane is being disabled, false otherwise.
- */
-static inline bool
-drm_atomic_plane_disabling(struct drm_plane *plane,
-			   struct drm_plane_state *old_state)
-{
-	/*
-	 * When disabling a plane, CRTC and FB should always be NULL together.
-	 * Anything else should be considered a bug in the atomic core, so we
-	 * gently warn about it.
-	 */
-	WARN_ON((plane->state->crtc == NULL && plane->state->fb != NULL) ||
-		(plane->state->crtc != NULL && plane->state->fb == NULL));
-
-	/*
-	 * When using the transitional helpers, old_state may be NULL. If so,
-	 * we know nothing about the current state and have to assume that it
-	 * might be enabled.
-	 *
-	 * When using the atomic helpers, old_state won't be NULL. Therefore
-	 * this check assumes that either the driver will have reconstructed
-	 * the correct state in ->reset() or that the driver will have taken
-	 * appropriate measures to disable all planes.
-	 */
-	return (!old_state || old_state->crtc) && !plane->state->crtc;
-}
-
 #endif /* DRM_ATOMIC_HELPER_H_ */
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index faaeff7..c5ee6b6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -768,6 +768,8 @@ struct drm_connector {
 struct drm_plane_state {
 	struct drm_plane *plane;
 
+	bool active;
+
 	struct drm_crtc *crtc;   /* do not write directly, use drm_atomic_set_crtc_for_plane() */
 	struct drm_framebuffer *fb;  /* do not write directly, use drm_atomic_set_fb_for_plane() */
 	struct fence *fence;
-- 
1.9.1

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

[PATCH RFC v2 2/2] drm/atomic: Disable planes on blanked CRTC and enable on unblank

[Jyri Sarha]

Disable planes if they are on to be blanked CRTC and enable them when
the CRTC is turned back on by DMPS.

This is desirable on HW that loses its context on blanking. When
planes are enabled and disabled with the associated CRTCs, there is no
need to restore the plane context in runtime_resume callback.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d03e2ac..5cd8016 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2027,8 +2027,8 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip);
  *
  * This is the main helper function provided by the atomic helper framework for
  * implementing the legacy DPMS connector interface. It computes the new desired
- * ->active state for the corresponding CRTC (if the connector is enabled) and
- *  updates it.
+ * ->active state for the corresponding CRTC and planes on it (if the connector
+ * is enabled) and updates it.
  *
  * Returns:
  * Returns 0 on success, negative errno numbers on failure.
@@ -2041,6 +2041,7 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
 	struct drm_connector *tmp_connector;
+	struct drm_plane *tmp_plane;
 	int ret;
 	bool active = false;
 	int old_mode = connector->dpms;
@@ -2079,6 +2080,20 @@ retry:
 	}
 	crtc_state->active = active;
 
+	/* Collect associated plane states to global state object. */
+	list_for_each_entry(tmp_plane, &config->plane_list, head) {
+		struct drm_plane_state *plane_state;
+
+		if (tmp_plane->state->crtc != crtc)
+			continue;
+
+		plane_state = drm_atomic_get_plane_state(state, tmp_plane);
+		if (IS_ERR(plane_state)) {
+			ret = PTR_ERR(plane_state);
+			goto fail;
+		}
+	}
+
 	ret = drm_atomic_commit(state);
 	if (ret != 0)
 		goto fail;
-- 
1.9.1

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

Re: [PATCH RFC v2 0/2] Disable planes on blanked CRTC and enable on unblank

[Daniel Vetter]

On Fri, Nov 13, 2015 at 05:53:13PM +0200, Jyri Sarha wrote:
> Since first RFC version:
> - Added "drm/atomic: Track drm_plane's active state"-patch
> 
> We would need something like this to get rid off OMAPDSS somewhat
> messy runtime_resume code. How does this look, could this generic
> solution be refined to be acceptable for mainline, or should we start
> looking a local solution to omapdrm?

One more comment besides what I've written in the older thread: You're not
on latest drm-next, which has gained an active_only paramter to the plane
commit helper.

It might be best to discuss this topic on #dri-devel on freenode irc a
bit.

Cheers, Daniel

> 
> Jyri Sarha (2):
>   drm/atomic: Track drm_plane's active state
>   drm/atomic: Disable planes on blanked CRTC and enable on unblank
> 
>  drivers/gpu/drm/drm_atomic_helper.c | 82 +++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/drm_plane_helper.c  | 10 +----
>  include/drm/drm_atomic_helper.h     | 39 +-----------------
>  include/drm/drm_crtc.h              |  2 +
>  4 files changed, 70 insertions(+), 63 deletions(-)
> 
> -- 
> 1.9.1
> 

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

Re: [PATCH RFC v2 0/2] Disable planes on blanked CRTC and enable on unblank

[Jyri Sarha]

On 11/17/15 12:28, Daniel Vetter wrote:
> On Fri, Nov 13, 2015 at 05:53:13PM +0200, Jyri Sarha wrote:
>> Since first RFC version:
>> - Added "drm/atomic: Track drm_plane's active state"-patch
>>
>> We would need something like this to get rid off OMAPDSS somewhat
>> messy runtime_resume code. How does this look, could this generic
>> solution be refined to be acceptable for mainline, or should we start
>> looking a local solution to omapdrm?
>
> One more comment besides what I've written in the older thread: You're not
> on latest drm-next, which has gained an active_only paramter to the plane
> commit helper.
>
> It might be best to discuss this topic on #dri-devel on freenode irc a
> bit.
>

Thanks a lot for you comments!

I just rebased my stuff on top the the latest drm-next. I think I need 
digest your earlier comments about drm_atomic_add_affected_planes etc. a 
bit before I can take advantage of any interactive communication. I'll 
try to follow those instruction first and see where it leads me.

Thanks,
Jyri

> Cheers, Daniel
>
>>
>> Jyri Sarha (2):
>>    drm/atomic: Track drm_plane's active state
>>    drm/atomic: Disable planes on blanked CRTC and enable on unblank
>>
>>   drivers/gpu/drm/drm_atomic_helper.c | 82 +++++++++++++++++++++++++++++--------
>>   drivers/gpu/drm/drm_plane_helper.c  | 10 +----
>>   include/drm/drm_atomic_helper.h     | 39 +-----------------
>>   include/drm/drm_crtc.h              |  2 +
>>   4 files changed, 70 insertions(+), 63 deletions(-)
>>
>> --
>> 1.9.1
>>
>

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