[PATCH v2 1/6] drm/plane: Pass old state to ->atomic_update()

[PATCH v2 1/6] drm/plane: Pass old state to ->atomic_update()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

In most situations it will be useful to have the old state passed to the
->atomic_update() callback. For example if a plane is being disabled the
new state's .crtc field will be NULL, but some drivers may rely on this
field to program the CRTCs registers.

v2: rename variable to old_plane_state and remove redundant comment as
suggested by Daniel Vetter, remove an Exynos hunk that doesn't apply to
drm-next and add a hunk for pending MSM mdp5 changes

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_atomic_helper.c       | 5 ++++-
 drivers/gpu/drm/drm_plane_helper.c        | 2 +-
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 3 ++-
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 3 ++-
 include/drm/drm_plane_helper.h            | 3 ++-
 5 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 25d8263f9869..7f020403ffe0 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1014,6 +1014,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 	for (i = 0; i < nplanes; i++) {
 		struct drm_plane_helper_funcs *funcs;
 		struct drm_plane *plane = old_state->planes[i];
+		struct drm_plane_state *old_plane_state;
 
 		if (!plane)
 			continue;
@@ -1023,7 +1024,9 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (!funcs || !funcs->atomic_update)
 			continue;
 
-		funcs->atomic_update(plane);
+		old_plane_state = old_state->plane_states[i];
+
+		funcs->atomic_update(plane, old_plane_state);
 	}
 
 	for (i = 0; i < ncrtcs; i++) {
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 2dd30518f9a2..aa399db5d36d 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -443,7 +443,7 @@ int drm_plane_helper_commit(struct drm_plane *plane,
 			crtc_funcs[i]->atomic_begin(crtc[i]);
 	}
 
-	plane_funcs->atomic_update(plane);
+	plane_funcs->atomic_update(plane, plane_state);
 
 	for (i = 0; i < 2; i++) {
 		if (crtc_funcs[i] && crtc_funcs[i]->atomic_flush)
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
index 76d0a40c7138..1e5ebe83647d 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
@@ -107,7 +107,8 @@ static int mdp4_plane_atomic_check(struct drm_plane *plane,
 	return 0;
 }
 
-static void mdp4_plane_atomic_update(struct drm_plane *plane)
+static void mdp4_plane_atomic_update(struct drm_plane *plane,
+				     struct drm_plane_state *old_state)
 {
 	struct drm_plane_state *state = plane->state;
 	int ret;
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
index 533df7caa310..26e5fdea6594 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
@@ -213,7 +213,8 @@ static int mdp5_plane_atomic_check(struct drm_plane *plane,
 	return 0;
 }
 
-static void mdp5_plane_atomic_update(struct drm_plane *plane)
+static void mdp5_plane_atomic_update(struct drm_plane *plane,
+				     struct drm_plane_state *old_state)
 {
 	struct mdp5_plane *mdp5_plane = to_mdp5_plane(plane);
 	struct drm_plane_state *state = plane->state;
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 00ad3b3c5324..d3122cd0609b 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -59,7 +59,8 @@ struct drm_plane_helper_funcs {
 
 	int (*atomic_check)(struct drm_plane *plane,
 			    struct drm_plane_state *state);
-	void (*atomic_update)(struct drm_plane *plane);
+	void (*atomic_update)(struct drm_plane *plane,
+			      struct drm_plane_state *old_state);
 };
 
 static inline void drm_plane_helper_add(struct drm_plane *plane,
-- 
2.1.3

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

[PATCH v2 2/6] drm/plane: Add missing kerneldoc

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The plane helpers aren't pulled into the DocBook yet, so these weren't
noticed.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/drm/drm_plane_helper.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index d3122cd0609b..0f2230311aa8 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -48,6 +48,10 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 
 /**
  * drm_plane_helper_funcs - helper operations for CRTCs
+ * @prepare_fb: prepare a framebuffer for use by the plane
+ * @cleanup_fb: cleanup a framebuffer when it's no longer used by the plane
+ * @atomic_check: check that a given atomic state is valid and can be applied
+ * @atomic_update: apply an atomic state to the plane
  *
  * The helper operations are called by the mid-layer CRTC helper.
  */
-- 
2.1.3

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

[PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

In order to prevent drivers from having to perform the same checks over
and over again, add an optional ->atomic_disable callback which the core
calls under the right circumstances.

v2: pass old state and detect edges to avoid calling ->atomic_disable on
already disabled planes, remove redundant comment (Daniel Vetter)

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++++--
 drivers/gpu/drm/drm_plane_helper.c  | 10 +++++++++-
 include/drm/drm_crtc.h              | 26 ++++++++++++++++++++++++++
 include/drm/drm_plane_helper.h      |  3 +++
 4 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 7f020403ffe0..a1c7d1b73f86 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1020,12 +1020,23 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 			continue;
 
 		funcs = plane->helper_private;
-
-		if (!funcs || !funcs->atomic_update)
+		if (!funcs)
 			continue;
 
 		old_plane_state = old_state->plane_states[i];
 
+		/*
+		 * Special-case disabling the plane if drivers support it.
+		 */
+		if (drm_plane_disabled(plane, old_plane_state) &&
+		    funcs->atomic_disable) {
+			funcs->atomic_disable(plane, old_plane_state);
+			continue;
+		}
+
+		if (!funcs->atomic_update)
+			continue;
+
 		funcs->atomic_update(plane, old_plane_state);
 	}
 
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index aa399db5d36d..8c81d3a9e625 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -443,7 +443,15 @@ int drm_plane_helper_commit(struct drm_plane *plane,
 			crtc_funcs[i]->atomic_begin(crtc[i]);
 	}
 
-	plane_funcs->atomic_update(plane, plane_state);
+	/*
+	 * Drivers may optionally implement the ->atomic_disable callback, so
+	 * special-case that here.
+	 */
+	if (drm_plane_disabled(plane, plane_state) &&
+	    plane_funcs->atomic_disable)
+		plane_funcs->atomic_disable(plane, plane_state);
+	else
+		plane_funcs->atomic_update(plane, plane_state);
 
 	for (i = 0; i < 2; i++) {
 		if (crtc_funcs[i] && crtc_funcs[i]->atomic_flush)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 6da67dfcb6fc..80d0f1c2b265 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -795,6 +795,32 @@ struct drm_plane {
 	struct drm_plane_state *state;
 };
 
+/*
+ * drm_plane_disabled - 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_plane_disabled(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));
+
+	return (!old_state || old_state->crtc) && !plane->state->crtc;
+}
+
 /**
  * struct drm_bridge_funcs - drm_bridge control functions
  * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 0f2230311aa8..680be61ef20a 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -52,6 +52,7 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
  * @cleanup_fb: cleanup a framebuffer when it's no longer used by the plane
  * @atomic_check: check that a given atomic state is valid and can be applied
  * @atomic_update: apply an atomic state to the plane
+ * @atomic_disable: disable the plane
  *
  * The helper operations are called by the mid-layer CRTC helper.
  */
@@ -65,6 +66,8 @@ struct drm_plane_helper_funcs {
 			    struct drm_plane_state *state);
 	void (*atomic_update)(struct drm_plane *plane,
 			      struct drm_plane_state *old_state);
+	void (*atomic_disable)(struct drm_plane *plane,
+			       struct drm_plane_state *old_state);
 };
 
 static inline void drm_plane_helper_add(struct drm_plane *plane,
-- 
2.1.3

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

[PATCH v2 4/6] drm: Make drm_atomic_helper.h standalone includible

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This header uses a bunch of declarations from the drm/drm_crtc.h header,
so make sure to include that as well so that drm_atomic_helper.h can be
included standalone.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/drm/drm_atomic_helper.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 64b4e91b93bc..70a83197ef66 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -28,6 +28,8 @@
 #ifndef DRM_ATOMIC_HELPER_H_
 #define DRM_ATOMIC_HELPER_H_
 
+#include <drm/drm_crtc.h>
+
 int drm_atomic_helper_check(struct drm_device *dev,
 			    struct drm_atomic_state *state);
 int drm_atomic_helper_commit(struct drm_device *dev,
-- 
2.1.3

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

[PATCH v2 5/6] drm: Make drm_atomic.h standalone includible

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This header file makes use of a bunch of structures declared in the
drm_crtc.h header file. Include that to make sure the drm_atomic.h
header can be included standalone.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/drm/drm_atomic.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 9d919168bc11..e224ccfa11ca 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -28,6 +28,8 @@
 #ifndef DRM_ATOMIC_H_
 #define DRM_ATOMIC_H_
 
+#include <drm/drm_crtc.h>
+
 struct drm_atomic_state * __must_check
 drm_atomic_state_alloc(struct drm_device *dev);
 void drm_atomic_state_clear(struct drm_atomic_state *state);
-- 
2.1.3

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

[PATCH v2 6/6] drm: Free atomic state during cleanup

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The current state of CRTCs, planes and connectors currently leaks during
DRM driver ->unload() unless drivers explicitly clean it up. Since there
is nothing driver-specific about it, that cleanup can be done within the
DRM core.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_crtc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index de09c1ff0714..9f736417a95d 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -721,6 +721,10 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
 	drm_mode_object_put(dev, &crtc->base);
 	list_del(&crtc->head);
 	dev->mode_config.num_crtc--;
+
+	WARN_ON(crtc->state && !crtc->funcs->atomic_destroy_state);
+	if (crtc->state && crtc->funcs->atomic_destroy_state)
+		crtc->funcs->atomic_destroy_state(crtc, crtc->state);
 }
 EXPORT_SYMBOL(drm_crtc_cleanup);
 
@@ -918,6 +922,11 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	connector->name = NULL;
 	list_del(&connector->head);
 	dev->mode_config.num_connector--;
+
+	WARN_ON(connector->state && !connector->funcs->atomic_destroy_state);
+	if (connector->state && connector->funcs->atomic_destroy_state)
+		connector->funcs->atomic_destroy_state(connector,
+						       connector->state);
 }
 EXPORT_SYMBOL(drm_connector_cleanup);
 
@@ -1244,6 +1253,10 @@ void drm_plane_cleanup(struct drm_plane *plane)
 	if (plane->type == DRM_PLANE_TYPE_OVERLAY)
 		dev->mode_config.num_overlay_plane--;
 	drm_modeset_unlock_all(dev);
+
+	WARN_ON(plane->state && !plane->funcs->atomic_destroy_state);
+	if (plane->state && plane->funcs->atomic_destroy_state)
+		plane->funcs->atomic_destroy_state(plane, plane->state);
 }
 EXPORT_SYMBOL(drm_plane_cleanup);
 
-- 
2.1.3

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

Re: [PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback

[Daniel Vetter]

On Tue, Nov 25, 2014 at 12:09:46PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> In order to prevent drivers from having to perform the same checks over
> and over again, add an optional ->atomic_disable callback which the core
> calls under the right circumstances.
> 
> v2: pass old state and detect edges to avoid calling ->atomic_disable on
> already disabled planes, remove redundant comment (Daniel Vetter)
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Some minor bikesheds about consistency and clarity below.
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++++--
>  drivers/gpu/drm/drm_plane_helper.c  | 10 +++++++++-
>  include/drm/drm_crtc.h              | 26 ++++++++++++++++++++++++++
>  include/drm/drm_plane_helper.h      |  3 +++
>  4 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 7f020403ffe0..a1c7d1b73f86 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1020,12 +1020,23 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  			continue;
>  
>  		funcs = plane->helper_private;
> -
> -		if (!funcs || !funcs->atomic_update)
> +		if (!funcs)
>  			continue;
>  
>  		old_plane_state = old_state->plane_states[i];
>  
> +		/*
> +		 * Special-case disabling the plane if drivers support it.
> +		 */
> +		if (drm_plane_disabled(plane, old_plane_state) &&
> +		    funcs->atomic_disable) {
> +			funcs->atomic_disable(plane, old_plane_state);
> +			continue;
> +		}
> +
> +		if (!funcs->atomic_update)
> +			continue;

This looks dangerous - we really should either have the atomic_disable or
at least atomic_update. And plane transitional helpers exactly require
that. On the bikeshed front I also like the plane helper structure more
with the if () atomic_disalbel else atomic_update instead of the continue.

> +
>  		funcs->atomic_update(plane, old_plane_state);
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index aa399db5d36d..8c81d3a9e625 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -443,7 +443,15 @@ int drm_plane_helper_commit(struct drm_plane *plane,
>  			crtc_funcs[i]->atomic_begin(crtc[i]);
>  	}
>  
> -	plane_funcs->atomic_update(plane, plane_state);
> +	/*
> +	 * Drivers may optionally implement the ->atomic_disable callback, so
> +	 * special-case that here.
> +	 */
> +	if (drm_plane_disabled(plane, plane_state) &&
> +	    plane_funcs->atomic_disable)
> +		plane_funcs->atomic_disable(plane, plane_state);
> +	else
> +		plane_funcs->atomic_update(plane, plane_state);
>  
>  	for (i = 0; i < 2; i++) {
>  		if (crtc_funcs[i] && crtc_funcs[i]->atomic_flush)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 6da67dfcb6fc..80d0f1c2b265 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -795,6 +795,32 @@ struct drm_plane {
>  	struct drm_plane_state *state;
>  };
>  
> +/*
> + * drm_plane_disabled - 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_plane_disabled(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));
> +
> +	return (!old_state || old_state->crtc) && !plane->state->crtc;

The !old_state check here confused me a bit, until I've realized that you
use this for transitional helpers too. What about adding

	/* Cope with NULL old_state for transitional helpers. */

right above?

> +}

Hm, given that this is used by helpers maybe place it into
drm_atomic_helper.h? I'm also not too happy about the name, disabled
doesn't clearly only mean the enable->disable transition. What about
drm_atomic_plane_disabling instead, to make it clear we only care about
the transition? After all your kerneldoc also uses continuous ("being
disabled").

> +
>  /**
>   * struct drm_bridge_funcs - drm_bridge control functions
>   * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 0f2230311aa8..680be61ef20a 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -52,6 +52,7 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>   * @cleanup_fb: cleanup a framebuffer when it's no longer used by the plane
>   * @atomic_check: check that a given atomic state is valid and can be applied
>   * @atomic_update: apply an atomic state to the plane
> + * @atomic_disable: disable the plane
>   *
>   * The helper operations are called by the mid-layer CRTC helper.
>   */
> @@ -65,6 +66,8 @@ struct drm_plane_helper_funcs {
>  			    struct drm_plane_state *state);
>  	void (*atomic_update)(struct drm_plane *plane,
>  			      struct drm_plane_state *old_state);
> +	void (*atomic_disable)(struct drm_plane *plane,
> +			       struct drm_plane_state *old_state);
>  };
>  
>  static inline void drm_plane_helper_add(struct drm_plane *plane,
> -- 
> 2.1.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback

[Thierry Reding]

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

On Tue, Nov 25, 2014 at 12:22:34PM +0100, Daniel Vetter wrote:
> On Tue, Nov 25, 2014 at 12:09:46PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > In order to prevent drivers from having to perform the same checks over
> > and over again, add an optional ->atomic_disable callback which the core
> > calls under the right circumstances.
> > 
> > v2: pass old state and detect edges to avoid calling ->atomic_disable on
> > already disabled planes, remove redundant comment (Daniel Vetter)
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> Some minor bikesheds about consistency and clarity below.
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++++--
> >  drivers/gpu/drm/drm_plane_helper.c  | 10 +++++++++-
> >  include/drm/drm_crtc.h              | 26 ++++++++++++++++++++++++++
> >  include/drm/drm_plane_helper.h      |  3 +++
> >  4 files changed, 51 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 7f020403ffe0..a1c7d1b73f86 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1020,12 +1020,23 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
> >  			continue;
> >  
> >  		funcs = plane->helper_private;
> > -
> > -		if (!funcs || !funcs->atomic_update)
> > +		if (!funcs)
> >  			continue;
> >  
> >  		old_plane_state = old_state->plane_states[i];
> >  
> > +		/*
> > +		 * Special-case disabling the plane if drivers support it.
> > +		 */
> > +		if (drm_plane_disabled(plane, old_plane_state) &&
> > +		    funcs->atomic_disable) {
> > +			funcs->atomic_disable(plane, old_plane_state);
> > +			continue;
> > +		}
> > +
> > +		if (!funcs->atomic_update)
> > +			continue;
> 
> This looks dangerous - we really should either have the atomic_disable or
> at least atomic_update. And plane transitional helpers exactly require
> that.

Note that this isn't anything that this patch introduces. This function
has been optional since the drm_atomic_helper_commit_planes() was first
introduced. That said, I agree that ->atomic_update() should not be
optional, but I'd propose making that change in a precursory patch. That
is, remove the check for !funcs->atomic_update and let the kernel crash
if the driver doesn't provide it (drm_plane_helper_commit() will already
oops in that case anyway).

>       On the bikeshed front I also like the plane helper structure more
> with the if () atomic_disalbel else atomic_update instead of the continue.

The existing code was using this structure, but I think with the above
change to make ->atomic_update() mandatory it would make more sense to
turn this into a more idiomatic if/else.

> > +
> >  		funcs->atomic_update(plane, old_plane_state);
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> > index aa399db5d36d..8c81d3a9e625 100644
> > --- a/drivers/gpu/drm/drm_plane_helper.c
> > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > @@ -443,7 +443,15 @@ int drm_plane_helper_commit(struct drm_plane *plane,
> >  			crtc_funcs[i]->atomic_begin(crtc[i]);
> >  	}
> >  
> > -	plane_funcs->atomic_update(plane, plane_state);
> > +	/*
> > +	 * Drivers may optionally implement the ->atomic_disable callback, so
> > +	 * special-case that here.
> > +	 */
> > +	if (drm_plane_disabled(plane, plane_state) &&
> > +	    plane_funcs->atomic_disable)
> > +		plane_funcs->atomic_disable(plane, plane_state);
> > +	else
> > +		plane_funcs->atomic_update(plane, plane_state);
> >  
> >  	for (i = 0; i < 2; i++) {
> >  		if (crtc_funcs[i] && crtc_funcs[i]->atomic_flush)
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 6da67dfcb6fc..80d0f1c2b265 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -795,6 +795,32 @@ struct drm_plane {
> >  	struct drm_plane_state *state;
> >  };
> >  
> > +/*
> > + * drm_plane_disabled - 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_plane_disabled(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));
> > +
> > +	return (!old_state || old_state->crtc) && !plane->state->crtc;
> 
> The !old_state check here confused me a bit, until I've realized that you
> use this for transitional helpers too. What about adding
> 
> 	/* Cope with NULL old_state for transitional helpers. */
> 
> right above?

Sounds good.

> 
> > +}
> 
> Hm, given that this is used by helpers maybe place it into
> drm_atomic_helper.h?

It technically operates only on the drm_plane and drm_plane_state so
could be useful outside of helpers, but I have no objections to moving
it to the helpers.

> I'm also not too happy about the name, disabled
> doesn't clearly only mean the enable->disable transition. What about
> drm_atomic_plane_disabling instead, to make it clear we only care about
> the transition? After all your kerneldoc also uses continuous ("being
> disabled").

Okay, I can't think of anything better, so drm_atomic_plane_disabling()
it is.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

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

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

Re: [PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback

[Thierry Reding]

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

On Tue, Nov 25, 2014 at 12:45:46PM +0100, Thierry Reding wrote:
> On Tue, Nov 25, 2014 at 12:22:34PM +0100, Daniel Vetter wrote:
> > On Tue, Nov 25, 2014 at 12:09:46PM +0100, Thierry Reding wrote:
[...]
> > > +/*
> > > + * drm_plane_disabled - 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_plane_disabled(struct drm_plane *plane,
> > > +				      struct drm_plane_state *old_state)
> > > +{
[...]
> > > +	return (!old_state || old_state->crtc) && !plane->state->crtc;
> > 
> > The !old_state check here confused me a bit, until I've realized that you
> > use this for transitional helpers too. What about adding
> > 
> > 	/* Cope with NULL old_state for transitional helpers. */
> > 
> > right above?
> 
> Sounds good.

When I now thought about the reason again it took me a while to
reconstruct, so I figured I'd be extra verbose and used this:

	/*
	 * 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.
	 */

Does that sound accurate and sufficient to you?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

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

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

Re: [PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback

[Daniel Vetter]

On Tue, Nov 25, 2014 at 12:57:04PM +0100, Thierry Reding wrote:
> On Tue, Nov 25, 2014 at 12:45:46PM +0100, Thierry Reding wrote:
> > On Tue, Nov 25, 2014 at 12:22:34PM +0100, Daniel Vetter wrote:
> > > On Tue, Nov 25, 2014 at 12:09:46PM +0100, Thierry Reding wrote:
> [...]
> > > > +/*
> > > > + * drm_plane_disabled - 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_plane_disabled(struct drm_plane *plane,
> > > > +				      struct drm_plane_state *old_state)
> > > > +{
> [...]
> > > > +	return (!old_state || old_state->crtc) && !plane->state->crtc;
> > > 
> > > The !old_state check here confused me a bit, until I've realized that you
> > > use this for transitional helpers too. What about adding
> > > 
> > > 	/* Cope with NULL old_state for transitional helpers. */
> > > 
> > > right above?
> > 
> > Sounds good.
> 
> When I now thought about the reason again it took me a while to
> reconstruct, so I figured I'd be extra verbose and used this:
> 
> 	/*
> 	 * 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.
> 	 */
> 
> Does that sound accurate and sufficient to you?

Hm, thinking about this some more this will result in a slight difference
in behaviour, at least when drivers just use the helper ->reset functions
but don't disable everything:
- With transitional helpers we assume we know nothing and call
  ->atomic_disable.
- With atomic old_state->crtc == NULL in the same situation right after
  boot-up, but we asssume the plane is really off and _dont_ call
  ->atomic_disable.

Should we instead check for (old_state && old_state->crtc) and state that
drivers need to make sure they don't have stuff hanging around?

Or maybe just a note that there's a difference in behaviour here?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback

[Daniel Vetter]

On Tue, Nov 25, 2014 at 12:45:46PM +0100, Thierry Reding wrote:
> On Tue, Nov 25, 2014 at 12:22:34PM +0100, Daniel Vetter wrote:
> > On Tue, Nov 25, 2014 at 12:09:46PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > In order to prevent drivers from having to perform the same checks over
> > > and over again, add an optional ->atomic_disable callback which the core
> > > calls under the right circumstances.
> > > 
> > > v2: pass old state and detect edges to avoid calling ->atomic_disable on
> > > already disabled planes, remove redundant comment (Daniel Vetter)
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > 
> > Some minor bikesheds about consistency and clarity below.
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++++--
> > >  drivers/gpu/drm/drm_plane_helper.c  | 10 +++++++++-
> > >  include/drm/drm_crtc.h              | 26 ++++++++++++++++++++++++++
> > >  include/drm/drm_plane_helper.h      |  3 +++
> > >  4 files changed, 51 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 7f020403ffe0..a1c7d1b73f86 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1020,12 +1020,23 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
> > >  			continue;
> > >  
> > >  		funcs = plane->helper_private;
> > > -
> > > -		if (!funcs || !funcs->atomic_update)
> > > +		if (!funcs)
> > >  			continue;
> > >  
> > >  		old_plane_state = old_state->plane_states[i];
> > >  
> > > +		/*
> > > +		 * Special-case disabling the plane if drivers support it.
> > > +		 */
> > > +		if (drm_plane_disabled(plane, old_plane_state) &&
> > > +		    funcs->atomic_disable) {
> > > +			funcs->atomic_disable(plane, old_plane_state);
> > > +			continue;
> > > +		}
> > > +
> > > +		if (!funcs->atomic_update)
> > > +			continue;
> > 
> > This looks dangerous - we really should either have the atomic_disable or
> > at least atomic_update. And plane transitional helpers exactly require
> > that.
> 
> Note that this isn't anything that this patch introduces. This function
> has been optional since the drm_atomic_helper_commit_planes() was first
> introduced. That said, I agree that ->atomic_update() should not be
> optional, but I'd propose making that change in a precursory patch. That
> is, remove the check for !funcs->atomic_update and let the kernel crash
> if the driver doesn't provide it (drm_plane_helper_commit() will already
> oops in that case anyway).
> 
> >       On the bikeshed front I also like the plane helper structure more
> > with the if () atomic_disalbel else atomic_update instead of the continue.
> 
> The existing code was using this structure, but I think with the above
> change to make ->atomic_update() mandatory it would make more sense to
> turn this into a more idiomatic if/else.

Oh right I've missed that there's you just moved the check around in your
patch. Still transitional helpers requires this, and I can't think of a
case where we don't need this really. So if you feel like a quick
follow-up patch that would be great.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback

[Thierry Reding]

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

On Tue, Nov 25, 2014 at 01:26:34PM +0100, Daniel Vetter wrote:
> On Tue, Nov 25, 2014 at 12:57:04PM +0100, Thierry Reding wrote:
> > On Tue, Nov 25, 2014 at 12:45:46PM +0100, Thierry Reding wrote:
> > > On Tue, Nov 25, 2014 at 12:22:34PM +0100, Daniel Vetter wrote:
> > > > On Tue, Nov 25, 2014 at 12:09:46PM +0100, Thierry Reding wrote:
> > [...]
> > > > > +/*
> > > > > + * drm_plane_disabled - 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_plane_disabled(struct drm_plane *plane,
> > > > > +				      struct drm_plane_state *old_state)
> > > > > +{
> > [...]
> > > > > +	return (!old_state || old_state->crtc) && !plane->state->crtc;
> > > > 
> > > > The !old_state check here confused me a bit, until I've realized that you
> > > > use this for transitional helpers too. What about adding
> > > > 
> > > > 	/* Cope with NULL old_state for transitional helpers. */
> > > > 
> > > > right above?
> > > 
> > > Sounds good.
> > 
> > When I now thought about the reason again it took me a while to
> > reconstruct, so I figured I'd be extra verbose and used this:
> > 
> > 	/*
> > 	 * 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.
> > 	 */
> > 
> > Does that sound accurate and sufficient to you?
> 
> Hm, thinking about this some more this will result in a slight difference
> in behaviour, at least when drivers just use the helper ->reset functions
> but don't disable everything:
> - With transitional helpers we assume we know nothing and call
>   ->atomic_disable.
> - With atomic old_state->crtc == NULL in the same situation right after
>   boot-up, but we asssume the plane is really off and _dont_ call
>   ->atomic_disable.
> 
> Should we instead check for (old_state && old_state->crtc) and state that
> drivers need to make sure they don't have stuff hanging around?

I don't think we can check for old_state because otherwise this will
always return false, whereas we really want it to force-disable planes
that could be on (lacking any more accurate information). For
transitional helpers anyway.

For the atomic helpers, old_state will never be NULL, but I'd assume
that the driver would reconstruct the current state in ->reset().

> Or maybe just a note that there's a difference in behaviour here?

One other option would be to split this up into two functions:

	- drm_plane_helper_disabling() which is called from
	  drm_plane_helper_commit() and checks for the validity of
	  old_state

	- drm_atomic_plane_disabling() which is called from
	  drm_atomic_helper_commit_planes() and doesn't have to check
	  for the validity of old_state because it's always valid.

On second thought, that doesn't really help because the very first call
would still not know whether old_state->crtc is NULL because it's just
the default or because the plane is really disabled.

So I think the only sane solution to this would be to either have the
drivers reconstruct the correct value for state->crtc in ->reset() or
make sure to comply with the semantics that planes are initially
considered to be disabled.

Maybe complementing the above comment like this would help:

	/*
	 * 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. This usually happens only on the very first call
	 * of the drm_plane_helper_commit() helper.
	 *
	 * When using the atomic helpers, old_state won't be NULL. Therefore
	 * this check assumes that either the driver will have reconstructed
	 * the current state in ->reset() or that the driver will have taken
	 * measures to disable all planes.
	 */

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

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

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

Re: [PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback

[Thierry Reding]

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

On Fri, Jan 16, 2015 at 03:35:10PM +0100, Thierry Reding wrote:
> On Tue, Nov 25, 2014 at 01:26:34PM +0100, Daniel Vetter wrote:
> > On Tue, Nov 25, 2014 at 12:57:04PM +0100, Thierry Reding wrote:
> > > On Tue, Nov 25, 2014 at 12:45:46PM +0100, Thierry Reding wrote:
> > > > On Tue, Nov 25, 2014 at 12:22:34PM +0100, Daniel Vetter wrote:
> > > > > On Tue, Nov 25, 2014 at 12:09:46PM +0100, Thierry Reding wrote:
> > > [...]
> > > > > > +/*
> > > > > > + * drm_plane_disabled - 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_plane_disabled(struct drm_plane *plane,
> > > > > > +				      struct drm_plane_state *old_state)
> > > > > > +{
> > > [...]
> > > > > > +	return (!old_state || old_state->crtc) && !plane->state->crtc;
> > > > > 
> > > > > The !old_state check here confused me a bit, until I've realized that you
> > > > > use this for transitional helpers too. What about adding
> > > > > 
> > > > > 	/* Cope with NULL old_state for transitional helpers. */
> > > > > 
> > > > > right above?
> > > > 
> > > > Sounds good.
> > > 
> > > When I now thought about the reason again it took me a while to
> > > reconstruct, so I figured I'd be extra verbose and used this:
> > > 
> > > 	/*
> > > 	 * 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.
> > > 	 */
> > > 
> > > Does that sound accurate and sufficient to you?
> > 
> > Hm, thinking about this some more this will result in a slight difference
> > in behaviour, at least when drivers just use the helper ->reset functions
> > but don't disable everything:
> > - With transitional helpers we assume we know nothing and call
> >   ->atomic_disable.
> > - With atomic old_state->crtc == NULL in the same situation right after
> >   boot-up, but we asssume the plane is really off and _dont_ call
> >   ->atomic_disable.
> > 
> > Should we instead check for (old_state && old_state->crtc) and state that
> > drivers need to make sure they don't have stuff hanging around?
> 
> I don't think we can check for old_state because otherwise this will
> always return false, whereas we really want it to force-disable planes
> that could be on (lacking any more accurate information). For
> transitional helpers anyway.
> 
> For the atomic helpers, old_state will never be NULL, but I'd assume
> that the driver would reconstruct the current state in ->reset().

By the way, the reason for why old_state can be NULL with transitional
helpers is the ordering of the steps in the atomic transition. Currently
the Tegra patches do this (based on your blog post and the Exynos proto-
type):

	1) atomic conversion, phase 1:
	   - implement ->atomic_{check,update,disable}()
	   - use drm_plane_helper_{update,disable}()

	2) atomic conversion, phase 2:
	   - call drm_mode_config_reset() from ->load()
	   - implement ->reset()

That's only a partial list of what's done in these steps, but that's the
only relevant pieces for why old_state is NULL.

What happens is that without ->reset() implemented there won't be any
initial state, hence plane->state (the old_state here) will be NULL the
first time atomic state is applied.

We could of course reorder the sequence such that drivers are required
to hook up ->reset() before they can (or at the same as they) hook up
the transitional helpers. We could add an appropriate WARN_ON to this
helper to make that more obvious.

However, that will not solve the problem because it only gets rid of the
special case. We still don't know whether old_state->crtc == NULL is the
current state or just the initial default.

So no matter which way we do this, I don't see a way to get away without
requiring specific semantics from drivers. They would be that:

	- drivers recreate the correct state in ->reset() so that
	  old_state->crtc != NULL if the plane is really enabled

or

	- drivers have to ensure that the real state in fact mirrors the
	  initial default as encoded in the state (plane disabled)

So I think the comment below that I proposed earlier is the best we can
do.

	/*
	 * 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. This usually happens only on the very first call
	 * of the drm_plane_helper_commit() helper.
	 *
	 * 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.
	 */

Or perhaps I'm missing something?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

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

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

Re: [PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback

[Daniel Vetter]

On Fri, Jan 16, 2015 at 03:53:04PM +0100, Thierry Reding wrote:
> On Fri, Jan 16, 2015 at 03:35:10PM +0100, Thierry Reding wrote:
> > On Tue, Nov 25, 2014 at 01:26:34PM +0100, Daniel Vetter wrote:
> > > On Tue, Nov 25, 2014 at 12:57:04PM +0100, Thierry Reding wrote:
> > > > On Tue, Nov 25, 2014 at 12:45:46PM +0100, Thierry Reding wrote:
> > > > > On Tue, Nov 25, 2014 at 12:22:34PM +0100, Daniel Vetter wrote:
> > > > > > On Tue, Nov 25, 2014 at 12:09:46PM +0100, Thierry Reding wrote:
> > > > [...]
> > > > > > > +/*
> > > > > > > + * drm_plane_disabled - 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_plane_disabled(struct drm_plane *plane,
> > > > > > > +      struct drm_plane_state *old_state)
> > > > > > > +{
> > > > [...]
> > > > > > > + return (!old_state || old_state->crtc) && !plane->state->crtc;
> > > > > >
> > > > > > The !old_state check here confused me a bit, until I've realized that you
> > > > > > use this for transitional helpers too. What about adding
> > > > > >
> > > > > > /* Cope with NULL old_state for transitional helpers. */
> > > > > >
> > > > > > right above?
> > > > >
> > > > > Sounds good.
> > > >
> > > > When I now thought about the reason again it took me a while to
> > > > reconstruct, so I figured I'd be extra verbose and used this:
> > > >
> > > > /*
> > > > * 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.
> > > > */
> > > >
> > > > Does that sound accurate and sufficient to you?
> > >
> > > Hm, thinking about this some more this will result in a slight difference
> > > in behaviour, at least when drivers just use the helper ->reset functions
> > > but don't disable everything:
> > > - With transitional helpers we assume we know nothing and call
> > >   ->atomic_disable.
> > > - With atomic old_state->crtc == NULL in the same situation right after
> > >   boot-up, but we asssume the plane is really off and _dont_ call
> > >   ->atomic_disable.
> > >
> > > Should we instead check for (old_state && old_state->crtc) and state that
> > > drivers need to make sure they don't have stuff hanging around?
> >
> > I don't think we can check for old_state because otherwise this will
> > always return false, whereas we really want it to force-disable planes
> > that could be on (lacking any more accurate information). For
> > transitional helpers anyway.
> >
> > For the atomic helpers, old_state will never be NULL, but I'd assume
> > that the driver would reconstruct the current state in ->reset().
>
> By the way, the reason for why old_state can be NULL with transitional
> helpers is the ordering of the steps in the atomic transition. Currently
> the Tegra patches do this (based on your blog post and the Exynos proto-
> type):
>
> 1) atomic conversion, phase 1:
>   - implement ->atomic_{check,update,disable}()
>   - use drm_plane_helper_{update,disable}()
>
> 2) atomic conversion, phase 2:
>   - call drm_mode_config_reset() from ->load()
>   - implement ->reset()
>
> That's only a partial list of what's done in these steps, but that's the
> only relevant pieces for why old_state is NULL.
>
> What happens is that without ->reset() implemented there won't be any
> initial state, hence plane->state (the old_state here) will be NULL the
> first time atomic state is applied.
>
> We could of course reorder the sequence such that drivers are required
> to hook up ->reset() before they can (or at the same as they) hook up
> the transitional helpers. We could add an appropriate WARN_ON to this
> helper to make that more obvious.
>
> However, that will not solve the problem because it only gets rid of the
> special case. We still don't know whether old_state->crtc == NULL is the
> current state or just the initial default.
>
> So no matter which way we do this, I don't see a way to get away without
> requiring specific semantics from drivers. They would be that:
>
> - drivers recreate the correct state in ->reset() so that
>  old_state->crtc != NULL if the plane is really enabled
>
> or
>
> - drivers have to ensure that the real state in fact mirrors the
>  initial default as encoded in the state (plane disabled)
>
> So I think the comment below that I proposed earlier is the best we can
> do.
>
> /*
> * 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. This usually happens only on the very first call
> * of the drm_plane_helper_commit() helper.
> *
> * 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.
> */
>
> Or perhaps I'm missing something?

Yeah, comments sounds very good. Any imo you should quote your two mails
here holesale in the commit message too, so that when someone git blames
the code comment all the details are there.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback

[Thierry Reding]

On January 17, 2015 4:48:53 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Jan 16, 2015 at 03:53:04PM +0100, Thierry Reding wrote:
> > On Fri, Jan 16, 2015 at 03:35:10PM +0100, Thierry Reding wrote:
> > > On Tue, Nov 25, 2014 at 01:26:34PM +0100, Daniel Vetter wrote:
> > > > On Tue, Nov 25, 2014 at 12:57:04PM +0100, Thierry Reding wrote:
> > > > > On Tue, Nov 25, 2014 at 12:45:46PM +0100, Thierry Reding wrote:
> > > > > > On Tue, Nov 25, 2014 at 12:22:34PM +0100, Daniel Vetter wrote:
> > > > > > > On Tue, Nov 25, 2014 at 12:09:46PM +0100, Thierry Reding wrote:
> > > > > [...]
> > > > > > > > +/*
> > > > > > > > + * drm_plane_disabled - 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_plane_disabled(struct drm_plane *plane,
> > > > > > > > +      struct drm_plane_state *old_state)
> > > > > > > > +{
> > > > > [...]
> > > > > > > > + return (!old_state || old_state->crtc) && !plane->state->crtc;
> > > > > > >
> > > > > > > The !old_state check here confused me a bit, until I've 
> realized that you
> > > > > > > use this for transitional helpers too. What about adding
> > > > > > >
> > > > > > > /* Cope with NULL old_state for transitional helpers. */
> > > > > > >
> > > > > > > right above?
> > > > > >
> > > > > > Sounds good.
> > > > >
> > > > > When I now thought about the reason again it took me a while to
> > > > > reconstruct, so I figured I'd be extra verbose and used this:
> > > > >
> > > > > /*
> > > > > * 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.
> > > > > */
> > > > >
> > > > > Does that sound accurate and sufficient to you?
> > > >
> > > > Hm, thinking about this some more this will result in a slight difference
> > > > in behaviour, at least when drivers just use the helper ->reset functions
> > > > but don't disable everything:
> > > > - With transitional helpers we assume we know nothing and call
> > > >   ->atomic_disable.
> > > > - With atomic old_state->crtc == NULL in the same situation right after
> > > >   boot-up, but we asssume the plane is really off and _dont_ call
> > > >   ->atomic_disable.
> > > >
> > > > Should we instead check for (old_state && old_state->crtc) and state that
> > > > drivers need to make sure they don't have stuff hanging around?
> > >
> > > I don't think we can check for old_state because otherwise this will
> > > always return false, whereas we really want it to force-disable planes
> > > that could be on (lacking any more accurate information). For
> > > transitional helpers anyway.
> > >
> > > For the atomic helpers, old_state will never be NULL, but I'd assume
> > > that the driver would reconstruct the current state in ->reset().
> >
> > By the way, the reason for why old_state can be NULL with transitional
> > helpers is the ordering of the steps in the atomic transition. Currently
> > the Tegra patches do this (based on your blog post and the Exynos proto-
> > type):
> >
> > 1) atomic conversion, phase 1:
> >   - implement ->atomic_{check,update,disable}()
> >   - use drm_plane_helper_{update,disable}()
> >
> > 2) atomic conversion, phase 2:
> >   - call drm_mode_config_reset() from ->load()
> >   - implement ->reset()
> >
> > That's only a partial list of what's done in these steps, but that's the
> > only relevant pieces for why old_state is NULL.
> >
> > What happens is that without ->reset() implemented there won't be any
> > initial state, hence plane->state (the old_state here) will be NULL the
> > first time atomic state is applied.
> >
> > We could of course reorder the sequence such that drivers are required
> > to hook up ->reset() before they can (or at the same as they) hook up
> > the transitional helpers. We could add an appropriate WARN_ON to this
> > helper to make that more obvious.
> >
> > However, that will not solve the problem because it only gets rid of the
> > special case. We still don't know whether old_state->crtc == NULL is the
> > current state or just the initial default.
> >
> > So no matter which way we do this, I don't see a way to get away without
> > requiring specific semantics from drivers. They would be that:
> >
> > - drivers recreate the correct state in ->reset() so that
> >  old_state->crtc != NULL if the plane is really enabled
> >
> > or
> >
> > - drivers have to ensure that the real state in fact mirrors the
> >  initial default as encoded in the state (plane disabled)
> >
> > So I think the comment below that I proposed earlier is the best we can
> > do.
> >
> > /*
> > * 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. This usually happens only on the very first call
> > * of the drm_plane_helper_commit() helper.
> > *
> > * 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.
> > */
> >
> > Or perhaps I'm missing something?
>
> Yeah, comments sounds very good. Any imo you should quote your two mails
> here holesale in the commit message too, so that when someone git blames
> the code comment all the details are there.

Will do. Does this count as Reviewed-by? Also the above discussion should
probably be distilled into the Docbook at some point.

Thierry


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