[PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Javier Martinez Canillas]

Provides a default plane state check handler for primary planes that are a
fullscreen scanout buffer and whose state scale and position can't change.

There are some drivers that duplicate this logic in their helpers, such as
simpledrm and ssd130x. Factor out this common code into a plane helper and
make drivers use it.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
 drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
 drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
 include/drm/drm_plane_helper.h     |  2 ++
 4 files changed, 33 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index c7785967f5bf..fb41eee74693 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
 	kfree(plane);
 }
 EXPORT_SYMBOL(drm_plane_helper_destroy);
+
+/**
+ * drm_plane_helper_atomic_check() - Helper to check primary planes states
+ * @plane: plane to check
+ * @new_state: plane state to check
+ *
+ * Provides a default plane state check handler for primary planes whose atomic
+ * state scale and position is not expected to change because the primary plane
+ * is always a fullscreen scanout buffer.
+ *
+ * RETURNS:
+ * Zero on success, or an errno code otherwise.
+ */
+int drm_plane_helper_atomic_check(struct drm_plane *plane,
+				  struct drm_atomic_state *new_state)
+{
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
+	struct drm_crtc *new_crtc = new_plane_state->crtc;
+	struct drm_crtc_state *new_crtc_state = NULL;
+
+	if (new_crtc)
+		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
+
+	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
+						   DRM_PLANE_NO_SCALING,
+						   DRM_PLANE_NO_SCALING,
+						   false, false);
+}
+EXPORT_SYMBOL(drm_plane_helper_atomic_check);
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 79e8e2017c68..28cf9c87f86d 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -565,22 +565,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 	return ret;
 }
 
-static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
-						     struct drm_atomic_state *new_state)
-{
-	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
-	struct drm_crtc *new_crtc = new_plane_state->crtc;
-	struct drm_crtc_state *new_crtc_state = NULL;
-
-	if (new_crtc)
-		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
-
-	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
-						   DRM_PLANE_NO_SCALING,
-						   DRM_PLANE_NO_SCALING,
-						   false, false);
-}
-
 static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
 						       struct drm_atomic_state *old_state)
 {
@@ -623,7 +607,7 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
 
 static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
-	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
+	.atomic_check = drm_plane_helper_atomic_check,
 	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
 	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
 };
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 777ccd250871..ea5b3239a659 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -469,29 +469,6 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
 	DRM_FORMAT_MOD_INVALID
 };
 
-static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
-						       struct drm_atomic_state *new_state)
-{
-	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
-	struct drm_crtc *new_crtc = new_plane_state->crtc;
-	struct drm_crtc_state *new_crtc_state = NULL;
-	int ret;
-
-	if (new_crtc)
-		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
-
-	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
-						  DRM_PLANE_NO_SCALING,
-						  DRM_PLANE_NO_SCALING,
-						  false, false);
-	if (ret)
-		return ret;
-	else if (!new_plane_state->visible)
-		return 0;
-
-	return 0;
-}
-
 static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
 							 struct drm_atomic_state *old_state)
 {
@@ -543,7 +520,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
 
 static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
-	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
+	.atomic_check = drm_plane_helper_atomic_check,
 	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
 	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
 };
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 1781fab24dd6..7ba414655d69 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -41,5 +41,7 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr
 int drm_plane_helper_disable_primary(struct drm_plane *plane,
 				     struct drm_modeset_acquire_ctx *ctx);
 void drm_plane_helper_destroy(struct drm_plane *plane);
+int drm_plane_helper_atomic_check(struct drm_plane *plane,
+				  struct drm_atomic_state *new_state);
 
 #endif
-- 
2.37.1

[PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Javier Martinez Canillas]

Provides a default plane state check handler for primary planes that are a
fullscreen scanout buffer and whose state scale and position can't change.

There are some drivers that duplicate this logic in their helpers, such as
simpledrm and ssd130x. Factor out this common code into a plane helper and
make drivers use it.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
 drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
 drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
 include/drm/drm_plane_helper.h     |  2 ++
 4 files changed, 33 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index c7785967f5bf..fb41eee74693 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
 	kfree(plane);
 }
 EXPORT_SYMBOL(drm_plane_helper_destroy);
+
+/**
+ * drm_plane_helper_atomic_check() - Helper to check primary planes states
+ * @plane: plane to check
+ * @new_state: plane state to check
+ *
+ * Provides a default plane state check handler for primary planes whose atomic
+ * state scale and position is not expected to change because the primary plane
+ * is always a fullscreen scanout buffer.
+ *
+ * RETURNS:
+ * Zero on success, or an errno code otherwise.
+ */
+int drm_plane_helper_atomic_check(struct drm_plane *plane,
+				  struct drm_atomic_state *new_state)
+{
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
+	struct drm_crtc *new_crtc = new_plane_state->crtc;
+	struct drm_crtc_state *new_crtc_state = NULL;
+
+	if (new_crtc)
+		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
+
+	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
+						   DRM_PLANE_NO_SCALING,
+						   DRM_PLANE_NO_SCALING,
+						   false, false);
+}
+EXPORT_SYMBOL(drm_plane_helper_atomic_check);
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 79e8e2017c68..28cf9c87f86d 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -565,22 +565,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 	return ret;
 }
 
-static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
-						     struct drm_atomic_state *new_state)
-{
-	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
-	struct drm_crtc *new_crtc = new_plane_state->crtc;
-	struct drm_crtc_state *new_crtc_state = NULL;
-
-	if (new_crtc)
-		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
-
-	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
-						   DRM_PLANE_NO_SCALING,
-						   DRM_PLANE_NO_SCALING,
-						   false, false);
-}
-
 static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
 						       struct drm_atomic_state *old_state)
 {
@@ -623,7 +607,7 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
 
 static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
-	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
+	.atomic_check = drm_plane_helper_atomic_check,
 	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
 	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
 };
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 777ccd250871..ea5b3239a659 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -469,29 +469,6 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
 	DRM_FORMAT_MOD_INVALID
 };
 
-static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
-						       struct drm_atomic_state *new_state)
-{
-	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
-	struct drm_crtc *new_crtc = new_plane_state->crtc;
-	struct drm_crtc_state *new_crtc_state = NULL;
-	int ret;
-
-	if (new_crtc)
-		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
-
-	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
-						  DRM_PLANE_NO_SCALING,
-						  DRM_PLANE_NO_SCALING,
-						  false, false);
-	if (ret)
-		return ret;
-	else if (!new_plane_state->visible)
-		return 0;
-
-	return 0;
-}
-
 static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
 							 struct drm_atomic_state *old_state)
 {
@@ -543,7 +520,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
 
 static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
-	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
+	.atomic_check = drm_plane_helper_atomic_check,
 	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
 	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
 };
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 1781fab24dd6..7ba414655d69 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -41,5 +41,7 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr
 int drm_plane_helper_disable_primary(struct drm_plane *plane,
 				     struct drm_modeset_acquire_ctx *ctx);
 void drm_plane_helper_destroy(struct drm_plane *plane);
+int drm_plane_helper_atomic_check(struct drm_plane *plane,
+				  struct drm_atomic_state *new_state);
 
 #endif
-- 
2.37.1

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Ville Syrjälä]

On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote:
> Provides a default plane state check handler for primary planes that are a
> fullscreen scanout buffer and whose state scale and position can't change.
> 
> There are some drivers that duplicate this logic in their helpers, such as
> simpledrm and ssd130x. Factor out this common code into a plane helper and
> make drivers use it.
> 
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>  drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
>  drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
>  include/drm/drm_plane_helper.h     |  2 ++
>  4 files changed, 33 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index c7785967f5bf..fb41eee74693 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
>  	kfree(plane);
>  }
>  EXPORT_SYMBOL(drm_plane_helper_destroy);
> +
> +/**
> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
> + * @plane: plane to check
> + * @new_state: plane state to check

That is not a plane state. Also should s/new_// since it's just
the overall atomic state thing rather than some new or old state.

> + *
> + * Provides a default plane state check handler for primary planes whose atomic
> + * state scale and position is not expected to change because the primary plane
> + * is always a fullscreen scanout buffer.
> + *
> + * RETURNS:
> + * Zero on success, or an errno code otherwise.
> + */
> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
> +				  struct drm_atomic_state *new_state)
> +{
> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
> +	struct drm_crtc_state *new_crtc_state = NULL;
> +
> +	if (new_crtc)
> +		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> +
> +	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> +						   DRM_PLANE_NO_SCALING,
> +						   DRM_PLANE_NO_SCALING,
> +						   false, false);
> +}
> +EXPORT_SYMBOL(drm_plane_helper_atomic_check);
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 79e8e2017c68..28cf9c87f86d 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -565,22 +565,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>  	return ret;
>  }
>  
> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> -						     struct drm_atomic_state *new_state)
> -{
> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
> -	struct drm_crtc_state *new_crtc_state = NULL;
> -
> -	if (new_crtc)
> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> -
> -	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> -						   DRM_PLANE_NO_SCALING,
> -						   DRM_PLANE_NO_SCALING,
> -						   false, false);
> -}
> -
>  static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>  						       struct drm_atomic_state *old_state)
>  {
> @@ -623,7 +607,7 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
>  
>  static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
>  	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
> +	.atomic_check = drm_plane_helper_atomic_check,
>  	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
>  	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
>  };
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 777ccd250871..ea5b3239a659 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -469,29 +469,6 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
>  	DRM_FORMAT_MOD_INVALID
>  };
>  
> -static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> -						       struct drm_atomic_state *new_state)
> -{
> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
> -	struct drm_crtc_state *new_crtc_state = NULL;
> -	int ret;
> -
> -	if (new_crtc)
> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> -
> -	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> -						  DRM_PLANE_NO_SCALING,
> -						  DRM_PLANE_NO_SCALING,
> -						  false, false);
> -	if (ret)
> -		return ret;
> -	else if (!new_plane_state->visible)
> -		return 0;
> -
> -	return 0;
> -}
> -
>  static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>  							 struct drm_atomic_state *old_state)
>  {
> @@ -543,7 +520,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
>  
>  static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
>  	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> -	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
> +	.atomic_check = drm_plane_helper_atomic_check,
>  	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
>  	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
>  };
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 1781fab24dd6..7ba414655d69 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -41,5 +41,7 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr
>  int drm_plane_helper_disable_primary(struct drm_plane *plane,
>  				     struct drm_modeset_acquire_ctx *ctx);
>  void drm_plane_helper_destroy(struct drm_plane *plane);
> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
> +				  struct drm_atomic_state *new_state);
>  
>  #endif
> -- 
> 2.37.1

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Ville Syrjälä]

On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote:
> Provides a default plane state check handler for primary planes that are a
> fullscreen scanout buffer and whose state scale and position can't change.
> 
> There are some drivers that duplicate this logic in their helpers, such as
> simpledrm and ssd130x. Factor out this common code into a plane helper and
> make drivers use it.
> 
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>  drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
>  drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
>  include/drm/drm_plane_helper.h     |  2 ++
>  4 files changed, 33 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index c7785967f5bf..fb41eee74693 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
>  	kfree(plane);
>  }
>  EXPORT_SYMBOL(drm_plane_helper_destroy);
> +
> +/**
> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
> + * @plane: plane to check
> + * @new_state: plane state to check

That is not a plane state. Also should s/new_// since it's just
the overall atomic state thing rather than some new or old state.

> + *
> + * Provides a default plane state check handler for primary planes whose atomic
> + * state scale and position is not expected to change because the primary plane
> + * is always a fullscreen scanout buffer.
> + *
> + * RETURNS:
> + * Zero on success, or an errno code otherwise.
> + */
> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
> +				  struct drm_atomic_state *new_state)
> +{
> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
> +	struct drm_crtc_state *new_crtc_state = NULL;
> +
> +	if (new_crtc)
> +		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> +
> +	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> +						   DRM_PLANE_NO_SCALING,
> +						   DRM_PLANE_NO_SCALING,
> +						   false, false);
> +}
> +EXPORT_SYMBOL(drm_plane_helper_atomic_check);
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 79e8e2017c68..28cf9c87f86d 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -565,22 +565,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>  	return ret;
>  }
>  
> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> -						     struct drm_atomic_state *new_state)
> -{
> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
> -	struct drm_crtc_state *new_crtc_state = NULL;
> -
> -	if (new_crtc)
> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> -
> -	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> -						   DRM_PLANE_NO_SCALING,
> -						   DRM_PLANE_NO_SCALING,
> -						   false, false);
> -}
> -
>  static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>  						       struct drm_atomic_state *old_state)
>  {
> @@ -623,7 +607,7 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
>  
>  static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
>  	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
> +	.atomic_check = drm_plane_helper_atomic_check,
>  	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
>  	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
>  };
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 777ccd250871..ea5b3239a659 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -469,29 +469,6 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
>  	DRM_FORMAT_MOD_INVALID
>  };
>  
> -static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> -						       struct drm_atomic_state *new_state)
> -{
> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
> -	struct drm_crtc_state *new_crtc_state = NULL;
> -	int ret;
> -
> -	if (new_crtc)
> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> -
> -	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> -						  DRM_PLANE_NO_SCALING,
> -						  DRM_PLANE_NO_SCALING,
> -						  false, false);
> -	if (ret)
> -		return ret;
> -	else if (!new_plane_state->visible)
> -		return 0;
> -
> -	return 0;
> -}
> -
>  static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>  							 struct drm_atomic_state *old_state)
>  {
> @@ -543,7 +520,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
>  
>  static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
>  	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> -	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
> +	.atomic_check = drm_plane_helper_atomic_check,
>  	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
>  	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
>  };
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 1781fab24dd6..7ba414655d69 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -41,5 +41,7 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr
>  int drm_plane_helper_disable_primary(struct drm_plane *plane,
>  				     struct drm_modeset_acquire_ctx *ctx);
>  void drm_plane_helper_destroy(struct drm_plane *plane);
> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
> +				  struct drm_atomic_state *new_state);
>  
>  #endif
> -- 
> 2.37.1

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Thomas Zimmermann]

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

Hi

Am 12.09.22 um 12:40 schrieb Ville Syrjälä:
> On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote:
>> Provides a default plane state check handler for primary planes that are a
>> fullscreen scanout buffer and whose state scale and position can't change.
>>
>> There are some drivers that duplicate this logic in their helpers, such as
>> simpledrm and ssd130x. Factor out this common code into a plane helper and
>> make drivers use it.
>>
>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>
>>   drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
>>   drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
>>   include/drm/drm_plane_helper.h     |  2 ++
>>   4 files changed, 33 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
>> index c7785967f5bf..fb41eee74693 100644
>> --- a/drivers/gpu/drm/drm_plane_helper.c
>> +++ b/drivers/gpu/drm/drm_plane_helper.c
>> @@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
>>   	kfree(plane);
>>   }
>>   EXPORT_SYMBOL(drm_plane_helper_destroy);
>> +
>> +/**
>> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
>> + * @plane: plane to check
>> + * @new_state: plane state to check
> 
> That is not a plane state. Also should s/new_// since it's just
> the overall atomic state thing rather than some new or old state.

Using only 'state' is non-intuitive and has lead to bugs where sub-state 
was retrieved from the wrong state information. So we've been using 
'new_state' and 'old_state' explicitly in several places now.

Best regards
Thomas

> 
>> + *
>> + * Provides a default plane state check handler for primary planes whose atomic
>> + * state scale and position is not expected to change because the primary plane
>> + * is always a fullscreen scanout buffer.
>> + *
>> + * RETURNS:
>> + * Zero on success, or an errno code otherwise.
>> + */
>> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
>> +				  struct drm_atomic_state *new_state)
>> +{
>> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
>> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
>> +	struct drm_crtc_state *new_crtc_state = NULL;
>> +
>> +	if (new_crtc)
>> +		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
>> +
>> +	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> +						   DRM_PLANE_NO_SCALING,
>> +						   DRM_PLANE_NO_SCALING,
>> +						   false, false);
>> +}
>> +EXPORT_SYMBOL(drm_plane_helper_atomic_check);
>> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
>> index 79e8e2017c68..28cf9c87f86d 100644
>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> @@ -565,22 +565,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>>   	return ret;
>>   }
>>   
>> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
>> -						     struct drm_atomic_state *new_state)
>> -{
>> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
>> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
>> -	struct drm_crtc_state *new_crtc_state = NULL;
>> -
>> -	if (new_crtc)
>> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
>> -
>> -	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> -						   DRM_PLANE_NO_SCALING,
>> -						   DRM_PLANE_NO_SCALING,
>> -						   false, false);
>> -}
>> -
>>   static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>   						       struct drm_atomic_state *old_state)
>>   {
>> @@ -623,7 +607,7 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
>>   
>>   static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
>>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
>> +	.atomic_check = drm_plane_helper_atomic_check,
>>   	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
>>   	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
>>   };
>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
>> index 777ccd250871..ea5b3239a659 100644
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -469,29 +469,6 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
>>   	DRM_FORMAT_MOD_INVALID
>>   };
>>   
>> -static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>> -						       struct drm_atomic_state *new_state)
>> -{
>> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
>> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
>> -	struct drm_crtc_state *new_crtc_state = NULL;
>> -	int ret;
>> -
>> -	if (new_crtc)
>> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
>> -
>> -	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> -						  DRM_PLANE_NO_SCALING,
>> -						  DRM_PLANE_NO_SCALING,
>> -						  false, false);
>> -	if (ret)
>> -		return ret;
>> -	else if (!new_plane_state->visible)
>> -		return 0;
>> -
>> -	return 0;
>> -}
>> -
>>   static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>   							 struct drm_atomic_state *old_state)
>>   {
>> @@ -543,7 +520,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
>>   
>>   static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
>>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>> -	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
>> +	.atomic_check = drm_plane_helper_atomic_check,
>>   	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
>>   	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
>>   };
>> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
>> index 1781fab24dd6..7ba414655d69 100644
>> --- a/include/drm/drm_plane_helper.h
>> +++ b/include/drm/drm_plane_helper.h
>> @@ -41,5 +41,7 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr
>>   int drm_plane_helper_disable_primary(struct drm_plane *plane,
>>   				     struct drm_modeset_acquire_ctx *ctx);
>>   void drm_plane_helper_destroy(struct drm_plane *plane);
>> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
>> +				  struct drm_atomic_state *new_state);
>>   
>>   #endif
>> -- 
>> 2.37.1
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Thomas Zimmermann]

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

Hi

Am 12.09.22 um 12:40 schrieb Ville Syrjälä:
> On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote:
>> Provides a default plane state check handler for primary planes that are a
>> fullscreen scanout buffer and whose state scale and position can't change.
>>
>> There are some drivers that duplicate this logic in their helpers, such as
>> simpledrm and ssd130x. Factor out this common code into a plane helper and
>> make drivers use it.
>>
>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>
>>   drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
>>   drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
>>   include/drm/drm_plane_helper.h     |  2 ++
>>   4 files changed, 33 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
>> index c7785967f5bf..fb41eee74693 100644
>> --- a/drivers/gpu/drm/drm_plane_helper.c
>> +++ b/drivers/gpu/drm/drm_plane_helper.c
>> @@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
>>   	kfree(plane);
>>   }
>>   EXPORT_SYMBOL(drm_plane_helper_destroy);
>> +
>> +/**
>> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
>> + * @plane: plane to check
>> + * @new_state: plane state to check
> 
> That is not a plane state. Also should s/new_// since it's just
> the overall atomic state thing rather than some new or old state.

Using only 'state' is non-intuitive and has lead to bugs where sub-state 
was retrieved from the wrong state information. So we've been using 
'new_state' and 'old_state' explicitly in several places now.

Best regards
Thomas

> 
>> + *
>> + * Provides a default plane state check handler for primary planes whose atomic
>> + * state scale and position is not expected to change because the primary plane
>> + * is always a fullscreen scanout buffer.
>> + *
>> + * RETURNS:
>> + * Zero on success, or an errno code otherwise.
>> + */
>> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
>> +				  struct drm_atomic_state *new_state)
>> +{
>> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
>> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
>> +	struct drm_crtc_state *new_crtc_state = NULL;
>> +
>> +	if (new_crtc)
>> +		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
>> +
>> +	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> +						   DRM_PLANE_NO_SCALING,
>> +						   DRM_PLANE_NO_SCALING,
>> +						   false, false);
>> +}
>> +EXPORT_SYMBOL(drm_plane_helper_atomic_check);
>> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
>> index 79e8e2017c68..28cf9c87f86d 100644
>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> @@ -565,22 +565,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>>   	return ret;
>>   }
>>   
>> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
>> -						     struct drm_atomic_state *new_state)
>> -{
>> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
>> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
>> -	struct drm_crtc_state *new_crtc_state = NULL;
>> -
>> -	if (new_crtc)
>> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
>> -
>> -	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> -						   DRM_PLANE_NO_SCALING,
>> -						   DRM_PLANE_NO_SCALING,
>> -						   false, false);
>> -}
>> -
>>   static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>   						       struct drm_atomic_state *old_state)
>>   {
>> @@ -623,7 +607,7 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
>>   
>>   static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
>>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
>> +	.atomic_check = drm_plane_helper_atomic_check,
>>   	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
>>   	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
>>   };
>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
>> index 777ccd250871..ea5b3239a659 100644
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -469,29 +469,6 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
>>   	DRM_FORMAT_MOD_INVALID
>>   };
>>   
>> -static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>> -						       struct drm_atomic_state *new_state)
>> -{
>> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
>> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
>> -	struct drm_crtc_state *new_crtc_state = NULL;
>> -	int ret;
>> -
>> -	if (new_crtc)
>> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
>> -
>> -	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> -						  DRM_PLANE_NO_SCALING,
>> -						  DRM_PLANE_NO_SCALING,
>> -						  false, false);
>> -	if (ret)
>> -		return ret;
>> -	else if (!new_plane_state->visible)
>> -		return 0;
>> -
>> -	return 0;
>> -}
>> -
>>   static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>   							 struct drm_atomic_state *old_state)
>>   {
>> @@ -543,7 +520,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
>>   
>>   static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
>>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>> -	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
>> +	.atomic_check = drm_plane_helper_atomic_check,
>>   	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
>>   	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
>>   };
>> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
>> index 1781fab24dd6..7ba414655d69 100644
>> --- a/include/drm/drm_plane_helper.h
>> +++ b/include/drm/drm_plane_helper.h
>> @@ -41,5 +41,7 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr
>>   int drm_plane_helper_disable_primary(struct drm_plane *plane,
>>   				     struct drm_modeset_acquire_ctx *ctx);
>>   void drm_plane_helper_destroy(struct drm_plane *plane);
>> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
>> +				  struct drm_atomic_state *new_state);
>>   
>>   #endif
>> -- 
>> 2.37.1
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Ville Syrjälä]

On Mon, Sep 12, 2022 at 01:05:45PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.09.22 um 12:40 schrieb Ville Syrjälä:
> > On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote:
> >> Provides a default plane state check handler for primary planes that are a
> >> fullscreen scanout buffer and whose state scale and position can't change.
> >>
> >> There are some drivers that duplicate this logic in their helpers, such as
> >> simpledrm and ssd130x. Factor out this common code into a plane helper and
> >> make drivers use it.
> >>
> >> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >> ---
> >>
> >>   drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
> >>   drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
> >>   drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
> >>   include/drm/drm_plane_helper.h     |  2 ++
> >>   4 files changed, 33 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> >> index c7785967f5bf..fb41eee74693 100644
> >> --- a/drivers/gpu/drm/drm_plane_helper.c
> >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> >> @@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
> >>   	kfree(plane);
> >>   }
> >>   EXPORT_SYMBOL(drm_plane_helper_destroy);
> >> +
> >> +/**
> >> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
> >> + * @plane: plane to check
> >> + * @new_state: plane state to check
> > 
> > That is not a plane state. Also should s/new_// since it's just
> > the overall atomic state thing rather than some new or old state.
> 
> Using only 'state' is non-intuitive and has lead to bugs where sub-state 
> was retrieved from the wrong state information. So we've been using 
> 'new_state' and 'old_state' explicitly in several places now.

There is no old or new drm_atomic_state. It contains both.

> 
> Best regards
> Thomas
> 
> > 
> >> + *
> >> + * Provides a default plane state check handler for primary planes whose atomic
> >> + * state scale and position is not expected to change because the primary plane
> >> + * is always a fullscreen scanout buffer.
> >> + *
> >> + * RETURNS:
> >> + * Zero on success, or an errno code otherwise.
> >> + */
> >> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
> >> +				  struct drm_atomic_state *new_state)
> >> +{
> >> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> >> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
> >> +	struct drm_crtc_state *new_crtc_state = NULL;
> >> +
> >> +	if (new_crtc)
> >> +		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> >> +
> >> +	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> >> +						   DRM_PLANE_NO_SCALING,
> >> +						   DRM_PLANE_NO_SCALING,
> >> +						   false, false);
> >> +}
> >> +EXPORT_SYMBOL(drm_plane_helper_atomic_check);
> >> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> >> index 79e8e2017c68..28cf9c87f86d 100644
> >> --- a/drivers/gpu/drm/solomon/ssd130x.c
> >> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> >> @@ -565,22 +565,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
> >>   	return ret;
> >>   }
> >>   
> >> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> >> -						     struct drm_atomic_state *new_state)
> >> -{
> >> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> >> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
> >> -	struct drm_crtc_state *new_crtc_state = NULL;
> >> -
> >> -	if (new_crtc)
> >> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> >> -
> >> -	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> >> -						   DRM_PLANE_NO_SCALING,
> >> -						   DRM_PLANE_NO_SCALING,
> >> -						   false, false);
> >> -}
> >> -
> >>   static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
> >>   						       struct drm_atomic_state *old_state)
> >>   {
> >> @@ -623,7 +607,7 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
> >>   
> >>   static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
> >>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> >> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
> >> +	.atomic_check = drm_plane_helper_atomic_check,
> >>   	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
> >>   	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
> >>   };
> >> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> >> index 777ccd250871..ea5b3239a659 100644
> >> --- a/drivers/gpu/drm/tiny/simpledrm.c
> >> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> >> @@ -469,29 +469,6 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
> >>   	DRM_FORMAT_MOD_INVALID
> >>   };
> >>   
> >> -static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> >> -						       struct drm_atomic_state *new_state)
> >> -{
> >> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> >> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
> >> -	struct drm_crtc_state *new_crtc_state = NULL;
> >> -	int ret;
> >> -
> >> -	if (new_crtc)
> >> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> >> -
> >> -	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> >> -						  DRM_PLANE_NO_SCALING,
> >> -						  DRM_PLANE_NO_SCALING,
> >> -						  false, false);
> >> -	if (ret)
> >> -		return ret;
> >> -	else if (!new_plane_state->visible)
> >> -		return 0;
> >> -
> >> -	return 0;
> >> -}
> >> -
> >>   static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
> >>   							 struct drm_atomic_state *old_state)
> >>   {
> >> @@ -543,7 +520,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
> >>   
> >>   static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
> >>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> >> -	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
> >> +	.atomic_check = drm_plane_helper_atomic_check,
> >>   	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
> >>   	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
> >>   };
> >> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> >> index 1781fab24dd6..7ba414655d69 100644
> >> --- a/include/drm/drm_plane_helper.h
> >> +++ b/include/drm/drm_plane_helper.h
> >> @@ -41,5 +41,7 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr
> >>   int drm_plane_helper_disable_primary(struct drm_plane *plane,
> >>   				     struct drm_modeset_acquire_ctx *ctx);
> >>   void drm_plane_helper_destroy(struct drm_plane *plane);
> >> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
> >> +				  struct drm_atomic_state *new_state);
> >>   
> >>   #endif
> >> -- 
> >> 2.37.1
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Ville Syrjälä]

On Mon, Sep 12, 2022 at 01:05:45PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.09.22 um 12:40 schrieb Ville Syrjälä:
> > On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote:
> >> Provides a default plane state check handler for primary planes that are a
> >> fullscreen scanout buffer and whose state scale and position can't change.
> >>
> >> There are some drivers that duplicate this logic in their helpers, such as
> >> simpledrm and ssd130x. Factor out this common code into a plane helper and
> >> make drivers use it.
> >>
> >> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >> ---
> >>
> >>   drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
> >>   drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
> >>   drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
> >>   include/drm/drm_plane_helper.h     |  2 ++
> >>   4 files changed, 33 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> >> index c7785967f5bf..fb41eee74693 100644
> >> --- a/drivers/gpu/drm/drm_plane_helper.c
> >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> >> @@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
> >>   	kfree(plane);
> >>   }
> >>   EXPORT_SYMBOL(drm_plane_helper_destroy);
> >> +
> >> +/**
> >> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
> >> + * @plane: plane to check
> >> + * @new_state: plane state to check
> > 
> > That is not a plane state. Also should s/new_// since it's just
> > the overall atomic state thing rather than some new or old state.
> 
> Using only 'state' is non-intuitive and has lead to bugs where sub-state 
> was retrieved from the wrong state information. So we've been using 
> 'new_state' and 'old_state' explicitly in several places now.

There is no old or new drm_atomic_state. It contains both.

> 
> Best regards
> Thomas
> 
> > 
> >> + *
> >> + * Provides a default plane state check handler for primary planes whose atomic
> >> + * state scale and position is not expected to change because the primary plane
> >> + * is always a fullscreen scanout buffer.
> >> + *
> >> + * RETURNS:
> >> + * Zero on success, or an errno code otherwise.
> >> + */
> >> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
> >> +				  struct drm_atomic_state *new_state)
> >> +{
> >> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> >> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
> >> +	struct drm_crtc_state *new_crtc_state = NULL;
> >> +
> >> +	if (new_crtc)
> >> +		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> >> +
> >> +	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> >> +						   DRM_PLANE_NO_SCALING,
> >> +						   DRM_PLANE_NO_SCALING,
> >> +						   false, false);
> >> +}
> >> +EXPORT_SYMBOL(drm_plane_helper_atomic_check);
> >> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> >> index 79e8e2017c68..28cf9c87f86d 100644
> >> --- a/drivers/gpu/drm/solomon/ssd130x.c
> >> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> >> @@ -565,22 +565,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
> >>   	return ret;
> >>   }
> >>   
> >> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> >> -						     struct drm_atomic_state *new_state)
> >> -{
> >> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> >> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
> >> -	struct drm_crtc_state *new_crtc_state = NULL;
> >> -
> >> -	if (new_crtc)
> >> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> >> -
> >> -	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> >> -						   DRM_PLANE_NO_SCALING,
> >> -						   DRM_PLANE_NO_SCALING,
> >> -						   false, false);
> >> -}
> >> -
> >>   static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
> >>   						       struct drm_atomic_state *old_state)
> >>   {
> >> @@ -623,7 +607,7 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
> >>   
> >>   static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
> >>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> >> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
> >> +	.atomic_check = drm_plane_helper_atomic_check,
> >>   	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
> >>   	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
> >>   };
> >> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> >> index 777ccd250871..ea5b3239a659 100644
> >> --- a/drivers/gpu/drm/tiny/simpledrm.c
> >> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> >> @@ -469,29 +469,6 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
> >>   	DRM_FORMAT_MOD_INVALID
> >>   };
> >>   
> >> -static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> >> -						       struct drm_atomic_state *new_state)
> >> -{
> >> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> >> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
> >> -	struct drm_crtc_state *new_crtc_state = NULL;
> >> -	int ret;
> >> -
> >> -	if (new_crtc)
> >> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
> >> -
> >> -	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> >> -						  DRM_PLANE_NO_SCALING,
> >> -						  DRM_PLANE_NO_SCALING,
> >> -						  false, false);
> >> -	if (ret)
> >> -		return ret;
> >> -	else if (!new_plane_state->visible)
> >> -		return 0;
> >> -
> >> -	return 0;
> >> -}
> >> -
> >>   static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
> >>   							 struct drm_atomic_state *old_state)
> >>   {
> >> @@ -543,7 +520,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
> >>   
> >>   static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
> >>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> >> -	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
> >> +	.atomic_check = drm_plane_helper_atomic_check,
> >>   	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
> >>   	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
> >>   };
> >> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> >> index 1781fab24dd6..7ba414655d69 100644
> >> --- a/include/drm/drm_plane_helper.h
> >> +++ b/include/drm/drm_plane_helper.h
> >> @@ -41,5 +41,7 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr
> >>   int drm_plane_helper_disable_primary(struct drm_plane *plane,
> >>   				     struct drm_modeset_acquire_ctx *ctx);
> >>   void drm_plane_helper_destroy(struct drm_plane *plane);
> >> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
> >> +				  struct drm_atomic_state *new_state);
> >>   
> >>   #endif
> >> -- 
> >> 2.37.1
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Thomas Zimmermann]

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

Hi

Am 12.09.22 um 13:18 schrieb Ville Syrjälä:
> On Mon, Sep 12, 2022 at 01:05:45PM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 12.09.22 um 12:40 schrieb Ville Syrjälä:
>>> On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote:
>>>> Provides a default plane state check handler for primary planes that are a
>>>> fullscreen scanout buffer and whose state scale and position can't change.
>>>>
>>>> There are some drivers that duplicate this logic in their helpers, such as
>>>> simpledrm and ssd130x. Factor out this common code into a plane helper and
>>>> make drivers use it.
>>>>
>>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>>> ---
>>>>
>>>>    drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
>>>>    drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
>>>>    drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
>>>>    include/drm/drm_plane_helper.h     |  2 ++
>>>>    4 files changed, 33 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
>>>> index c7785967f5bf..fb41eee74693 100644
>>>> --- a/drivers/gpu/drm/drm_plane_helper.c
>>>> +++ b/drivers/gpu/drm/drm_plane_helper.c
>>>> @@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
>>>>    	kfree(plane);
>>>>    }
>>>>    EXPORT_SYMBOL(drm_plane_helper_destroy);
>>>> +
>>>> +/**
>>>> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
>>>> + * @plane: plane to check
>>>> + * @new_state: plane state to check
>>>
>>> That is not a plane state. Also should s/new_// since it's just
>>> the overall atomic state thing rather than some new or old state.
>>
>> Using only 'state' is non-intuitive and has lead to bugs where sub-state
>> was retrieved from the wrong state information. So we've been using
>> 'new_state' and 'old_state' explicitly in several places now.
> 
> There is no old or new drm_atomic_state. It contains both.

I (vaguely) remember a bug where a driver tried 
drm_atomic_get_new_plane_state() with the (old) state that's passed to 
atomic_update. It didn't return the expected results and modesetting 
gave slightly wrong results. So we began to be more precise about new 
and old. And whatever is stored in 'plane->state' is then just 'the state'.

I understand that the semantics of atomic_check are different from 
atomic_update, but it still doesn't hurt to talk of new_state IMHO.

Best regards
Thomas

> 
>>
>> Best regards
>> Thomas
>>
>>>
>>>> + *
>>>> + * Provides a default plane state check handler for primary planes whose atomic
>>>> + * state scale and position is not expected to change because the primary plane
>>>> + * is always a fullscreen scanout buffer.
>>>> + *
>>>> + * RETURNS:
>>>> + * Zero on success, or an errno code otherwise.
>>>> + */
>>>> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
>>>> +				  struct drm_atomic_state *new_state)
>>>> +{
>>>> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
>>>> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
>>>> +	struct drm_crtc_state *new_crtc_state = NULL;
>>>> +
>>>> +	if (new_crtc)
>>>> +		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
>>>> +
>>>> +	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>>>> +						   DRM_PLANE_NO_SCALING,
>>>> +						   DRM_PLANE_NO_SCALING,
>>>> +						   false, false);
>>>> +}
>>>> +EXPORT_SYMBOL(drm_plane_helper_atomic_check);
>>>> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
>>>> index 79e8e2017c68..28cf9c87f86d 100644
>>>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>>>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>>>> @@ -565,22 +565,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>>>>    	return ret;
>>>>    }
>>>>    
>>>> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>>> -						     struct drm_atomic_state *new_state)
>>>> -{
>>>> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
>>>> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
>>>> -	struct drm_crtc_state *new_crtc_state = NULL;
>>>> -
>>>> -	if (new_crtc)
>>>> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
>>>> -
>>>> -	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>>>> -						   DRM_PLANE_NO_SCALING,
>>>> -						   DRM_PLANE_NO_SCALING,
>>>> -						   false, false);
>>>> -}
>>>> -
>>>>    static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>>>    						       struct drm_atomic_state *old_state)
>>>>    {
>>>> @@ -623,7 +607,7 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
>>>>    
>>>>    static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
>>>>    	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>>>> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
>>>> +	.atomic_check = drm_plane_helper_atomic_check,
>>>>    	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
>>>>    	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
>>>>    };
>>>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
>>>> index 777ccd250871..ea5b3239a659 100644
>>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>>> @@ -469,29 +469,6 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
>>>>    	DRM_FORMAT_MOD_INVALID
>>>>    };
>>>>    
>>>> -static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>>> -						       struct drm_atomic_state *new_state)
>>>> -{
>>>> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
>>>> -	struct drm_crtc *new_crtc = new_plane_state->crtc;
>>>> -	struct drm_crtc_state *new_crtc_state = NULL;
>>>> -	int ret;
>>>> -
>>>> -	if (new_crtc)
>>>> -		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
>>>> -
>>>> -	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>>>> -						  DRM_PLANE_NO_SCALING,
>>>> -						  DRM_PLANE_NO_SCALING,
>>>> -						  false, false);
>>>> -	if (ret)
>>>> -		return ret;
>>>> -	else if (!new_plane_state->visible)
>>>> -		return 0;
>>>> -
>>>> -	return 0;
>>>> -}
>>>> -
>>>>    static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>>>    							 struct drm_atomic_state *old_state)
>>>>    {
>>>> @@ -543,7 +520,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
>>>>    
>>>>    static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
>>>>    	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>>>> -	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
>>>> +	.atomic_check = drm_plane_helper_atomic_check,
>>>>    	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
>>>>    	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
>>>>    };
>>>> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
>>>> index 1781fab24dd6..7ba414655d69 100644
>>>> --- a/include/drm/drm_plane_helper.h
>>>> +++ b/include/drm/drm_plane_helper.h
>>>> @@ -41,5 +41,7 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr
>>>>    int drm_plane_helper_disable_primary(struct drm_plane *plane,
>>>>    				     struct drm_modeset_acquire_ctx *ctx);
>>>>    void drm_plane_helper_destroy(struct drm_plane *plane);
>>>> +int drm_plane_helper_atomic_check(struct drm_plane *plane,
>>>> +				  struct drm_atomic_state *new_state);
>>>>    
>>>>    #endif
>>>> -- 
>>>> 2.37.1
>>>
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev
> 
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Ville Syrjälä]

On Mon, Sep 12, 2022 at 02:05:36PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.09.22 um 13:18 schrieb Ville Syrjälä:
> > On Mon, Sep 12, 2022 at 01:05:45PM +0200, Thomas Zimmermann wrote:
> >> Hi
> >>
> >> Am 12.09.22 um 12:40 schrieb Ville Syrjälä:
> >>> On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote:
> >>>> Provides a default plane state check handler for primary planes that are a
> >>>> fullscreen scanout buffer and whose state scale and position can't change.
> >>>>
> >>>> There are some drivers that duplicate this logic in their helpers, such as
> >>>> simpledrm and ssd130x. Factor out this common code into a plane helper and
> >>>> make drivers use it.
> >>>>
> >>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >>>> ---
> >>>>
> >>>>    drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
> >>>>    drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
> >>>>    drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
> >>>>    include/drm/drm_plane_helper.h     |  2 ++
> >>>>    4 files changed, 33 insertions(+), 41 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> >>>> index c7785967f5bf..fb41eee74693 100644
> >>>> --- a/drivers/gpu/drm/drm_plane_helper.c
> >>>> +++ b/drivers/gpu/drm/drm_plane_helper.c
> >>>> @@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
> >>>>    	kfree(plane);
> >>>>    }
> >>>>    EXPORT_SYMBOL(drm_plane_helper_destroy);
> >>>> +
> >>>> +/**
> >>>> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
> >>>> + * @plane: plane to check
> >>>> + * @new_state: plane state to check
> >>>
> >>> That is not a plane state. Also should s/new_// since it's just
> >>> the overall atomic state thing rather than some new or old state.
> >>
> >> Using only 'state' is non-intuitive and has lead to bugs where sub-state
> >> was retrieved from the wrong state information. So we've been using
> >> 'new_state' and 'old_state' explicitly in several places now.
> > 
> > There is no old or new drm_atomic_state. It contains both.
> 
> I (vaguely) remember a bug where a driver tried 
> drm_atomic_get_new_plane_state() with the (old) state that's passed to 
> atomic_update. It didn't return the expected results and modesetting 
> gave slightly wrong results. 

As there is no wrong drm_atomic_state to pass I don't think it could
have been the case. 

> So we began to be more precise about new 
> and old. And whatever is stored in 'plane->state' is then just 'the state'.

There were certainly a lot of confusion before the explicit new/old
state stuff was added whether foo->state/etc. was the old or the
new state. And labeling things as explicitly old vs. new when passing
in individual object states certainly makes sense. But that doesn't
really have anything to do with mislabeling the overall drm_atomic_state.

> 
> I understand that the semantics of atomic_check are different from 
> atomic_update, but it still doesn't hurt to talk of new_state IMHO.

IMO it's just confusing. Makes the reader think there is somehow
different drm_atomic_states for old vs. new states when there isn't.
I also wouldn't call it new_state for .atomic_update() either.

In both cases you have the old and new states in there and how
exactly they get used in the hooks is more of an implementation
detail. The only rules you would have to follow is that at the
end of .atomic_update() the hardware state matches the new state,
and .atomic_check() makes sure the transition from the old to the
new state is possible.

I've proposed renaming drm_atomic_state to eg. drm_atomic_transaction
a few times before but no one took the bait so far...

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Thomas Zimmermann]

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

Hi

Am 12.09.22 um 14:34 schrieb Ville Syrjälä:
> On Mon, Sep 12, 2022 at 02:05:36PM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 12.09.22 um 13:18 schrieb Ville Syrjälä:
>>> On Mon, Sep 12, 2022 at 01:05:45PM +0200, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 12.09.22 um 12:40 schrieb Ville Syrjälä:
>>>>> On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote:
>>>>>> Provides a default plane state check handler for primary planes that are a
>>>>>> fullscreen scanout buffer and whose state scale and position can't change.
>>>>>>
>>>>>> There are some drivers that duplicate this logic in their helpers, such as
>>>>>> simpledrm and ssd130x. Factor out this common code into a plane helper and
>>>>>> make drivers use it.
>>>>>>
>>>>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>>>>> ---
>>>>>>
>>>>>>     drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
>>>>>>     drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
>>>>>>     drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
>>>>>>     include/drm/drm_plane_helper.h     |  2 ++
>>>>>>     4 files changed, 33 insertions(+), 41 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
>>>>>> index c7785967f5bf..fb41eee74693 100644
>>>>>> --- a/drivers/gpu/drm/drm_plane_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_plane_helper.c
>>>>>> @@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
>>>>>>     	kfree(plane);
>>>>>>     }
>>>>>>     EXPORT_SYMBOL(drm_plane_helper_destroy);
>>>>>> +
>>>>>> +/**
>>>>>> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
>>>>>> + * @plane: plane to check
>>>>>> + * @new_state: plane state to check
>>>>>
>>>>> That is not a plane state. Also should s/new_// since it's just
>>>>> the overall atomic state thing rather than some new or old state.
>>>>
>>>> Using only 'state' is non-intuitive and has lead to bugs where sub-state
>>>> was retrieved from the wrong state information. So we've been using
>>>> 'new_state' and 'old_state' explicitly in several places now.
>>>
>>> There is no old or new drm_atomic_state. It contains both.
>>
>> I (vaguely) remember a bug where a driver tried
>> drm_atomic_get_new_plane_state() with the (old) state that's passed to
>> atomic_update. It didn't return the expected results and modesetting
>> gave slightly wrong results.
> 
> As there is no wrong drm_atomic_state to pass I don't think it could
> have been the case.
> 
>> So we began to be more precise about new
>> and old. And whatever is stored in 'plane->state' is then just 'the state'.
> 
> There were certainly a lot of confusion before the explicit new/old
> state stuff was added whether foo->state/etc. was the old or the
> new state. And labeling things as explicitly old vs. new when passing
> in individual object states certainly makes sense. But that doesn't
> really have anything to do with mislabeling the overall drm_atomic_state.
> 
>>
>> I understand that the semantics of atomic_check are different from
>> atomic_update, but it still doesn't hurt to talk of new_state IMHO.
> 
> IMO it's just confusing. Makes the reader think there is somehow
> different drm_atomic_states for old vs. new states when there isn't.
> I also wouldn't call it new_state for .atomic_update() either.
> 
> In both cases you have the old and new states in there and how
> exactly they get used in the hooks is more of an implementation
> detail. The only rules you would have to follow is that at the
> end of .atomic_update() the hardware state matches the new state,
> and .atomic_check() makes sure the transition from the old to the
> new state is possible.

 From what I understand:

In atomic_check(), plane->state is the current state and the state 
argument is the state to be validated. Calling 
drm_atomic_get_new_plane_state() will return the plane's new state.

If you call drm_atomic_get_old_plane_state() from atomic_check(), what 
will it return?

In atomic_update() plane->state is the state to be committed and the 
state argument is the old state before the start of the atomic commit. 
And calling drm_atomic_get_new_plane_state() will *not* the return the 
plane's new state (i.e., the one in plane->state) IIRC. (As I mentioned, 
there was a related bug in one of the drivers.) So we began to call this 
'old_state'.

My point is: the state passed to the check and commit functions are 
different things, even though they appear to be the same.

> 
> I've proposed renaming drm_atomic_state to eg. drm_atomic_transaction
> a few times before but no one took the bait so far...
> 

If you really don't like new_state, then let's call it state_tx.

Best regards
Thomas


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Javier Martinez Canillas]

Hello Ville and Thomas,

On 9/12/22 16:22, Thomas Zimmermann wrote:

[...]

>>>>>>> +
>>>>>>> +/**
>>>>>>> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
>>>>>>> + * @plane: plane to check
>>>>>>> + * @new_state: plane state to check
>>>>>>
>>>>>> That is not a plane state. Also should s/new_// since it's just

Right. What's the proper name? hardware state, modeset state, atomic state ?

>>>>>> the overall atomic state thing rather than some new or old state.
>>>>>
>>>>> Using only 'state' is non-intuitive and has lead to bugs where sub-state
>>>>> was retrieved from the wrong state information. So we've been using
>>>>> 'new_state' and 'old_state' explicitly in several places now.
>>>>
>>>> There is no old or new drm_atomic_state. It contains both.
>>>
>>> I (vaguely) remember a bug where a driver tried
>>> drm_atomic_get_new_plane_state() with the (old) state that's passed to
>>> atomic_update. It didn't return the expected results and modesetting
>>> gave slightly wrong results.
>>
>> As there is no wrong drm_atomic_state to pass I don't think it could
>> have been the case.
>>
>>> So we began to be more precise about new
>>> and old. And whatever is stored in 'plane->state' is then just 'the state'.
>>
>> There were certainly a lot of confusion before the explicit new/old
>> state stuff was added whether foo->state/etc. was the old or the
>> new state. And labeling things as explicitly old vs. new when passing
>> in individual object states certainly makes sense. But that doesn't
>> really have anything to do with mislabeling the overall drm_atomic_state.
>>
>>>
>>> I understand that the semantics of atomic_check are different from
>>> atomic_update, but it still doesn't hurt to talk of new_state IMHO.
>>
>> IMO it's just confusing. Makes the reader think there is somehow
>> different drm_atomic_states for old vs. new states when there isn't.
>> I also wouldn't call it new_state for .atomic_update() either.
>>
>> In both cases you have the old and new states in there and how
>> exactly they get used in the hooks is more of an implementation
>> detail. The only rules you would have to follow is that at the
>> end of .atomic_update() the hardware state matches the new state,
>> and .atomic_check() makes sure the transition from the old to the
>> new state is possible.
> 
>  From what I understand:
> 
> In atomic_check(), plane->state is the current state and the state 
> argument is the state to be validated. Calling 
> drm_atomic_get_new_plane_state() will return the plane's new state.
>

That's my understanding as well.
 
> If you call drm_atomic_get_old_plane_state() from atomic_check(), what 
> will it return?
> 
> In atomic_update() plane->state is the state to be committed and the 
> state argument is the old state before the start of the atomic commit. 
> And calling drm_atomic_get_new_plane_state() will *not* the return the 
> plane's new state (i.e., the one in plane->state) IIRC. (As I mentioned, 
> there was a related bug in one of the drivers.) So we began to call this 
> 'old_state'.
> 
> My point is: the state passed to the check and commit functions are 
> different things, even though they appear to be the same.
>

Agree.

Maybe instead of new and old `current_state` and `commit_state` would
be more clear ?
 
>>
>> I've proposed renaming drm_atomic_state to eg. drm_atomic_transaction
>> a few times before but no one took the bait so far...
>>
> 
> If you really don't like new_state, then let's call it state_tx.
>

I would prefer if for this patch we call it either `new_state` or just
`state` to be consistent with the other helpers. And then we can as a
follow-up change the naming used by all the helpers.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Javier Martinez Canillas]

Hello Ville and Thomas,

On 9/12/22 16:22, Thomas Zimmermann wrote:

[...]

>>>>>>> +
>>>>>>> +/**
>>>>>>> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
>>>>>>> + * @plane: plane to check
>>>>>>> + * @new_state: plane state to check
>>>>>>
>>>>>> That is not a plane state. Also should s/new_// since it's just

Right. What's the proper name? hardware state, modeset state, atomic state ?

>>>>>> the overall atomic state thing rather than some new or old state.
>>>>>
>>>>> Using only 'state' is non-intuitive and has lead to bugs where sub-state
>>>>> was retrieved from the wrong state information. So we've been using
>>>>> 'new_state' and 'old_state' explicitly in several places now.
>>>>
>>>> There is no old or new drm_atomic_state. It contains both.
>>>
>>> I (vaguely) remember a bug where a driver tried
>>> drm_atomic_get_new_plane_state() with the (old) state that's passed to
>>> atomic_update. It didn't return the expected results and modesetting
>>> gave slightly wrong results.
>>
>> As there is no wrong drm_atomic_state to pass I don't think it could
>> have been the case.
>>
>>> So we began to be more precise about new
>>> and old. And whatever is stored in 'plane->state' is then just 'the state'.
>>
>> There were certainly a lot of confusion before the explicit new/old
>> state stuff was added whether foo->state/etc. was the old or the
>> new state. And labeling things as explicitly old vs. new when passing
>> in individual object states certainly makes sense. But that doesn't
>> really have anything to do with mislabeling the overall drm_atomic_state.
>>
>>>
>>> I understand that the semantics of atomic_check are different from
>>> atomic_update, but it still doesn't hurt to talk of new_state IMHO.
>>
>> IMO it's just confusing. Makes the reader think there is somehow
>> different drm_atomic_states for old vs. new states when there isn't.
>> I also wouldn't call it new_state for .atomic_update() either.
>>
>> In both cases you have the old and new states in there and how
>> exactly they get used in the hooks is more of an implementation
>> detail. The only rules you would have to follow is that at the
>> end of .atomic_update() the hardware state matches the new state,
>> and .atomic_check() makes sure the transition from the old to the
>> new state is possible.
> 
>  From what I understand:
> 
> In atomic_check(), plane->state is the current state and the state 
> argument is the state to be validated. Calling 
> drm_atomic_get_new_plane_state() will return the plane's new state.
>

That's my understanding as well.
 
> If you call drm_atomic_get_old_plane_state() from atomic_check(), what 
> will it return?
> 
> In atomic_update() plane->state is the state to be committed and the 
> state argument is the old state before the start of the atomic commit. 
> And calling drm_atomic_get_new_plane_state() will *not* the return the 
> plane's new state (i.e., the one in plane->state) IIRC. (As I mentioned, 
> there was a related bug in one of the drivers.) So we began to call this 
> 'old_state'.
> 
> My point is: the state passed to the check and commit functions are 
> different things, even though they appear to be the same.
>

Agree.

Maybe instead of new and old `current_state` and `commit_state` would
be more clear ?
 
>>
>> I've proposed renaming drm_atomic_state to eg. drm_atomic_transaction
>> a few times before but no one took the bait so far...
>>
> 
> If you really don't like new_state, then let's call it state_tx.
>

I would prefer if for this patch we call it either `new_state` or just
`state` to be consistent with the other helpers. And then we can as a
follow-up change the naming used by all the helpers.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Ville Syrjälä]

On Mon, Sep 12, 2022 at 04:22:49PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.09.22 um 14:34 schrieb Ville Syrjälä:
> > On Mon, Sep 12, 2022 at 02:05:36PM +0200, Thomas Zimmermann wrote:
> >> Hi
> >>
> >> Am 12.09.22 um 13:18 schrieb Ville Syrjälä:
> >>> On Mon, Sep 12, 2022 at 01:05:45PM +0200, Thomas Zimmermann wrote:
> >>>> Hi
> >>>>
> >>>> Am 12.09.22 um 12:40 schrieb Ville Syrjälä:
> >>>>> On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote:
> >>>>>> Provides a default plane state check handler for primary planes that are a
> >>>>>> fullscreen scanout buffer and whose state scale and position can't change.
> >>>>>>
> >>>>>> There are some drivers that duplicate this logic in their helpers, such as
> >>>>>> simpledrm and ssd130x. Factor out this common code into a plane helper and
> >>>>>> make drivers use it.
> >>>>>>
> >>>>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >>>>>> ---
> >>>>>>
> >>>>>>     drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
> >>>>>>     drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
> >>>>>>     drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
> >>>>>>     include/drm/drm_plane_helper.h     |  2 ++
> >>>>>>     4 files changed, 33 insertions(+), 41 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> >>>>>> index c7785967f5bf..fb41eee74693 100644
> >>>>>> --- a/drivers/gpu/drm/drm_plane_helper.c
> >>>>>> +++ b/drivers/gpu/drm/drm_plane_helper.c
> >>>>>> @@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
> >>>>>>     	kfree(plane);
> >>>>>>     }
> >>>>>>     EXPORT_SYMBOL(drm_plane_helper_destroy);
> >>>>>> +
> >>>>>> +/**
> >>>>>> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
> >>>>>> + * @plane: plane to check
> >>>>>> + * @new_state: plane state to check
> >>>>>
> >>>>> That is not a plane state. Also should s/new_// since it's just
> >>>>> the overall atomic state thing rather than some new or old state.
> >>>>
> >>>> Using only 'state' is non-intuitive and has lead to bugs where sub-state
> >>>> was retrieved from the wrong state information. So we've been using
> >>>> 'new_state' and 'old_state' explicitly in several places now.
> >>>
> >>> There is no old or new drm_atomic_state. It contains both.
> >>
> >> I (vaguely) remember a bug where a driver tried
> >> drm_atomic_get_new_plane_state() with the (old) state that's passed to
> >> atomic_update. It didn't return the expected results and modesetting
> >> gave slightly wrong results.
> > 
> > As there is no wrong drm_atomic_state to pass I don't think it could
> > have been the case.
> > 
> >> So we began to be more precise about new
> >> and old. And whatever is stored in 'plane->state' is then just 'the state'.
> > 
> > There were certainly a lot of confusion before the explicit new/old
> > state stuff was added whether foo->state/etc. was the old or the
> > new state. And labeling things as explicitly old vs. new when passing
> > in individual object states certainly makes sense. But that doesn't
> > really have anything to do with mislabeling the overall drm_atomic_state.
> > 
> >>
> >> I understand that the semantics of atomic_check are different from
> >> atomic_update, but it still doesn't hurt to talk of new_state IMHO.
> > 
> > IMO it's just confusing. Makes the reader think there is somehow
> > different drm_atomic_states for old vs. new states when there isn't.
> > I also wouldn't call it new_state for .atomic_update() either.
> > 
> > In both cases you have the old and new states in there and how
> > exactly they get used in the hooks is more of an implementation
> > detail. The only rules you would have to follow is that at the
> > end of .atomic_update() the hardware state matches the new state,
> > and .atomic_check() makes sure the transition from the old to the
> > new state is possible.
> 
>  From what I understand:
> 
> In atomic_check(), plane->state is the current state and the state 
> argument is the state to be validated. Calling 
> drm_atomic_get_new_plane_state() will return the plane's new state.

You should pretty much never use plane->state anywhere. Just use
drm_atomic_get_{,old,new}_plane_state() & co. Outside of exceptional
cases plane->state should only be accessed by duplicate_state()
and swap_state().

> 
> If you call drm_atomic_get_old_plane_state() from atomic_check(), what 
> will it return?

Before swap state:
- drm_atomic_get_old_plane_state() points to the same thing
  as plane->state, or NULL if the plane is not part of the
  drm_atomic_state
- drm_atomic_get_new_plane_state() points to the newly
  duplicated state only tracked within drm_atomic_state,
  or NULL if the plane is not part of the drm_atomic_state

After swap state:
- drm_atomic_get_old_plane_state() still points to the same
  thing as before, even though plane->state no longer points there.
  This old state is no longer visible outside the drm_atomic_state
  and will get destoyed when the drm_atomic_state gets nuked
  once the commit has been done
- drm_atomic_get_new_plane_state() still points to the same
  thing as before, and now plane->state also points to it

But all you really need to know is you have a transaction
(drm_atomic_state) and each object taking part in it
will have an old state (= the object's state before the
transaction has been commited), and new state (= the object's
state after the transaction has been commited).

> 
> In atomic_update() plane->state is the state to be committed and the 
> state argument is the old state before the start of the atomic commit. 
> And calling drm_atomic_get_new_plane_state() will *not* the return the 
> plane's new state (i.e., the one in plane->state) IIRC. (As I mentioned, 
> there was a related bug in one of the drivers.) So we began to call this 
> 'old_state'.
> 
> My point is: the state passed to the check and commit functions are 
> different things, even though they appear to be the same.
> 
> > 
> > I've proposed renaming drm_atomic_state to eg. drm_atomic_transaction
> > a few times before but no one took the bait so far...
> > 
> 
> If you really don't like new_state, then let's call it state_tx.
> 
> Best regards
> Thomas
> 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

[Thomas Zimmermann]

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

Hi

Am 12.09.22 um 19:16 schrieb Ville Syrjälä:
> On Mon, Sep 12, 2022 at 04:22:49PM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 12.09.22 um 14:34 schrieb Ville Syrjälä:
>>> On Mon, Sep 12, 2022 at 02:05:36PM +0200, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 12.09.22 um 13:18 schrieb Ville Syrjälä:
>>>>> On Mon, Sep 12, 2022 at 01:05:45PM +0200, Thomas Zimmermann wrote:
>>>>>> Hi
>>>>>>
>>>>>> Am 12.09.22 um 12:40 schrieb Ville Syrjälä:
>>>>>>> On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote:
>>>>>>>> Provides a default plane state check handler for primary planes that are a
>>>>>>>> fullscreen scanout buffer and whose state scale and position can't change.
>>>>>>>>
>>>>>>>> There are some drivers that duplicate this logic in their helpers, such as
>>>>>>>> simpledrm and ssd130x. Factor out this common code into a plane helper and
>>>>>>>> make drivers use it.
>>>>>>>>
>>>>>>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>      drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
>>>>>>>>      drivers/gpu/drm/solomon/ssd130x.c  | 18 +-----------------
>>>>>>>>      drivers/gpu/drm/tiny/simpledrm.c   | 25 +------------------------
>>>>>>>>      include/drm/drm_plane_helper.h     |  2 ++
>>>>>>>>      4 files changed, 33 insertions(+), 41 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
>>>>>>>> index c7785967f5bf..fb41eee74693 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_plane_helper.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_plane_helper.c
>>>>>>>> @@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
>>>>>>>>      	kfree(plane);
>>>>>>>>      }
>>>>>>>>      EXPORT_SYMBOL(drm_plane_helper_destroy);
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * drm_plane_helper_atomic_check() - Helper to check primary planes states
>>>>>>>> + * @plane: plane to check
>>>>>>>> + * @new_state: plane state to check
>>>>>>>
>>>>>>> That is not a plane state. Also should s/new_// since it's just
>>>>>>> the overall atomic state thing rather than some new or old state.
>>>>>>
>>>>>> Using only 'state' is non-intuitive and has lead to bugs where sub-state
>>>>>> was retrieved from the wrong state information. So we've been using
>>>>>> 'new_state' and 'old_state' explicitly in several places now.
>>>>>
>>>>> There is no old or new drm_atomic_state. It contains both.
>>>>
>>>> I (vaguely) remember a bug where a driver tried
>>>> drm_atomic_get_new_plane_state() with the (old) state that's passed to
>>>> atomic_update. It didn't return the expected results and modesetting
>>>> gave slightly wrong results.
>>>
>>> As there is no wrong drm_atomic_state to pass I don't think it could
>>> have been the case.
>>>
>>>> So we began to be more precise about new
>>>> and old. And whatever is stored in 'plane->state' is then just 'the state'.
>>>
>>> There were certainly a lot of confusion before the explicit new/old
>>> state stuff was added whether foo->state/etc. was the old or the
>>> new state. And labeling things as explicitly old vs. new when passing
>>> in individual object states certainly makes sense. But that doesn't
>>> really have anything to do with mislabeling the overall drm_atomic_state.
>>>
>>>>
>>>> I understand that the semantics of atomic_check are different from
>>>> atomic_update, but it still doesn't hurt to talk of new_state IMHO.
>>>
>>> IMO it's just confusing. Makes the reader think there is somehow
>>> different drm_atomic_states for old vs. new states when there isn't.
>>> I also wouldn't call it new_state for .atomic_update() either.
>>>
>>> In both cases you have the old and new states in there and how
>>> exactly they get used in the hooks is more of an implementation
>>> detail. The only rules you would have to follow is that at the
>>> end of .atomic_update() the hardware state matches the new state,
>>> and .atomic_check() makes sure the transition from the old to the
>>> new state is possible.
>>
>>   From what I understand:
>>
>> In atomic_check(), plane->state is the current state and the state
>> argument is the state to be validated. Calling
>> drm_atomic_get_new_plane_state() will return the plane's new state.
> 
> You should pretty much never use plane->state anywhere. Just use
> drm_atomic_get_{,old,new}_plane_state() & co. Outside of exceptional
> cases plane->state should only be accessed by duplicate_state()
> and swap_state().
> 
>>
>> If you call drm_atomic_get_old_plane_state() from atomic_check(), what
>> will it return?
> 
> Before swap state:
> - drm_atomic_get_old_plane_state() points to the same thing
>    as plane->state, or NULL if the plane is not part of the
>    drm_atomic_state
> - drm_atomic_get_new_plane_state() points to the newly
>    duplicated state only tracked within drm_atomic_state,
>    or NULL if the plane is not part of the drm_atomic_state
> 
> After swap state:
> - drm_atomic_get_old_plane_state() still points to the same
>    thing as before, even though plane->state no longer points there.
>    This old state is no longer visible outside the drm_atomic_state
>    and will get destoyed when the drm_atomic_state gets nuked
>    once the commit has been done
> - drm_atomic_get_new_plane_state() still points to the same
>    thing as before, and now plane->state also points to it

This is exactly what I always assumed, but I remember finding a 
situation where this didn't work as expected. (If only I could find it 
again.) Anyway, as it's supposed to be the correct let's do exactly this.

Best regards
Thomas

> 
> But all you really need to know is you have a transaction
> (drm_atomic_state) and each object taking part in it
> will have an old state (= the object's state before the
> transaction has been commited), and new state (= the object's
> state after the transaction has been commited).
> 
>>
>> In atomic_update() plane->state is the state to be committed and the
>> state argument is the old state before the start of the atomic commit.
>> And calling drm_atomic_get_new_plane_state() will *not* the return the
>> plane's new state (i.e., the one in plane->state) IIRC. (As I mentioned,
>> there was a related bug in one of the drivers.) So we began to call this
>> 'old_state'.
>>
>> My point is: the state passed to the check and commit functions are
>> different things, even though they appear to be the same.
>>
>>>
>>> I've proposed renaming drm_atomic_state to eg. drm_atomic_transaction
>>> a few times before but no one took the bait so far...
>>>
>>
>> If you really don't like new_state, then let's call it state_tx.
>>
>> Best regards
>> Thomas
>>
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev
> 
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]