Re: [Intel-gfx] [PATCH v4 2/5] drm/atomic: Make drm_atomic_plane_disabling easier to understand.
Hi Maarten,
Thank you for the patch.
On Thursday 16 Feb 2017 15:47:07 Maarten Lankhorst wrote:
> This function becomes a lot simpler when having passed both the old and
> new state to it. Looking at all callers, it seems that old_plane_state
> is never NULL so the check can be dropped.
>
> Signed-off-by: Maarten Lankhorst
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 7 ---
> drivers/gpu/drm/drm_plane_helper.c | 2 +-
> include/drm/drm_atomic_helper.h | 26 --
> 3 files changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index 7d432d9a18cf..ea544bddc29b
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1757,7 +1757,8 @@ void drm_atomic_helper_commit_planes(struct drm_device
> *dev, if (!funcs)
> continue;
>
> - disabling = drm_atomic_plane_disabling(plane,
old_plane_state);
> + disabling = drm_atomic_plane_disabling(old_plane_state,
> +new_plane_state);
>
> if (active_only) {
> /*
> @@ -1852,11 +1853,11 @@ 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) &&
> + if (drm_atomic_plane_disabling(old_plane_state, 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))
> + drm_atomic_plane_disabling(old_plane_state, plane-
>state))
> plane_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 148688fb920a..f4d70dd5e3e4
> 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -470,7 +470,7 @@ int drm_plane_helper_commit(struct drm_plane *plane,
>* Drivers may optionally implement the ->atomic_disable callback, so
>* special-case that here.
>*/
> - if (drm_atomic_plane_disabling(plane, plane_state) &&
> + if (drm_atomic_plane_disabling(plane_state, plane->state) &&
> plane_funcs->atomic_disable)
> plane_funcs->atomic_disable(plane, plane_state);
> else
> diff --git a/include/drm/drm_atomic_helper.h
> b/include/drm/drm_atomic_helper.h index 9ceda379ce58..dc16274987c7 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -220,10 +220,10 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc
> *crtc, __drm_atomic_get_current_plane_state((crtc_state)->state, \ plane)))
>
> -/*
> +/**
> * drm_atomic_plane_disabling - check whether a plane is being disabled
> - * @plane: plane object
> - * @old_state: previous atomic state
> + * @old_plane_state: old atomic plane state
> + * @new_plane_state: new atomic plane 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 @@ -233,28 +233,18 @@ int
> drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, * 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)
> +drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state,
> +struct drm_plane_state *new_plane_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));
> + WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb != NULL)
||
> + (new_plane_state->crtc != NULL && new_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;
>
[Intel-gfx] [PATCH v4 2/5] drm/atomic: Make drm_atomic_plane_disabling easier to understand.
This function becomes a lot simpler when having passed both the old and
new state to it. Looking at all callers, it seems that old_plane_state
is never NULL so the check can be dropped.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/drm_atomic_helper.c | 7 ---
drivers/gpu/drm/drm_plane_helper.c | 2 +-
include/drm/drm_atomic_helper.h | 26 --
3 files changed, 13 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 7d432d9a18cf..ea544bddc29b 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1757,7 +1757,8 @@ void drm_atomic_helper_commit_planes(struct drm_device
*dev,
if (!funcs)
continue;
- disabling = drm_atomic_plane_disabling(plane, old_plane_state);
+ disabling = drm_atomic_plane_disabling(old_plane_state,
+ new_plane_state);
if (active_only) {
/*
@@ -1852,11 +1853,11 @@ 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) &&
+ if (drm_atomic_plane_disabling(old_plane_state, 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))
+drm_atomic_plane_disabling(old_plane_state,
plane->state))
plane_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 148688fb920a..f4d70dd5e3e4 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -470,7 +470,7 @@ int drm_plane_helper_commit(struct drm_plane *plane,
* Drivers may optionally implement the ->atomic_disable callback, so
* special-case that here.
*/
- if (drm_atomic_plane_disabling(plane, plane_state) &&
+ if (drm_atomic_plane_disabling(plane_state, plane->state) &&
plane_funcs->atomic_disable)
plane_funcs->atomic_disable(plane, plane_state);
else
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 9ceda379ce58..dc16274987c7 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -220,10 +220,10 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc
*crtc,
__drm_atomic_get_current_plane_state((crtc_state)->state, \
plane)))
-/*
+/**
* drm_atomic_plane_disabling - check whether a plane is being disabled
- * @plane: plane object
- * @old_state: previous atomic state
+ * @old_plane_state: old atomic plane state
+ * @new_plane_state: new atomic plane 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
@@ -233,28 +233,18 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc
*crtc,
* 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)
+drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state,
+ struct drm_plane_state *new_plane_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));
+ WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb != NULL)
||
+ (new_plane_state->crtc != NULL && new_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;
+ return old_plane_state->crtc && !new_plane_state->crtc;
}
#endif /* DRM_ATOMIC_HELPER_H_ */
--
2.7.4
__
