[PATCH 0/3] Move blender setup from individual planes to crtc commit in sun4i-drm

[PATCH 0/3] Move blender setup from individual planes to crtc commit in sun4i-drm

[Ondřej Jirman]

From: Ondrej Jirman <megi@xff.cz>

This series refactors blender setup from individual planes to a common
place where it can be performed at once and is easier to reason about.

In the process this fixes a few bugs that allowed blender pipes to be
disabled while corresponding DRM planes were requested to be enabled.

Please take a look. :)

Thank you very much,
	Ondřej Jirman

Ondrej Jirman (3):
  drm/sun4i: Unify sun8i_*_layer structs
  drm/sun4i: Add more parameters to sunxi_engine commit callback
  drm/sun4i: Fix layer zpos change/atomic modesetting

 drivers/gpu/drm/sun4i/sun4i_backend.c  |  4 +-
 drivers/gpu/drm/sun4i/sun4i_crtc.c     |  2 +-
 drivers/gpu/drm/sun4i/sun8i_mixer.c    | 82 +++++++++++++++++++++++-
 drivers/gpu/drm/sun4i/sun8i_mixer.h    | 20 ++++++
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 85 +++----------------------
 drivers/gpu/drm/sun4i/sun8i_ui_layer.h | 20 ++----
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 86 +++-----------------------
 drivers/gpu/drm/sun4i/sun8i_vi_layer.h | 20 ++----
 drivers/gpu/drm/sun4i/sunxi_engine.h   | 13 +++-
 9 files changed, 137 insertions(+), 195 deletions(-)

-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] drm/sun4i: Unify sun8i_*_layer structs

[Ondřej Jirman]

From: Ondrej Jirman <megi@xff.cz>

These structs are identical, use a single struct to represent private
data for the DRM plane. This is a preparation for configuring layer
routing from the CRTC (mixer) instead of current approach of setting
up routing from individual layer's atomic_update callback.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.c    |  4 ++--
 drivers/gpu/drm/sun4i/sun8i_mixer.h    | 14 ++++++++++++++
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 14 +++++++-------
 drivers/gpu/drm/sun4i/sun8i_ui_layer.h | 20 ++++----------------
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 14 +++++++-------
 drivers/gpu/drm/sun4i/sun8i_vi_layer.h | 20 ++++----------------
 6 files changed, 38 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index 01382860aaee..1e681314e379 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -271,7 +271,7 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
 		return ERR_PTR(-ENOMEM);
 
 	for (i = 0; i < mixer->cfg->vi_num; i++) {
-		struct sun8i_vi_layer *layer;
+		struct sun8i_layer *layer;
 
 		layer = sun8i_vi_layer_init_one(drm, mixer, i);
 		if (IS_ERR(layer)) {
@@ -284,7 +284,7 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
 	}
 
 	for (i = 0; i < mixer->cfg->ui_num; i++) {
-		struct sun8i_ui_layer *layer;
+		struct sun8i_layer *layer;
 
 		layer = sun8i_ui_layer_init_one(drm, mixer, i);
 		if (IS_ERR(layer)) {
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
index 85c94884fb9a..5a610ee30301 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
@@ -9,6 +9,7 @@
 #include <linux/clk.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
+#include <drm/drm_plane.h>
 
 #include "sunxi_engine.h"
 
@@ -185,6 +186,19 @@ struct sun8i_mixer {
 	struct clk			*mod_clk;
 };
 
+struct sun8i_layer {
+	struct drm_plane	plane;
+	struct sun8i_mixer	*mixer;
+	int			channel;
+	int			overlay;
+};
+
+static inline struct sun8i_layer *
+plane_to_sun8i_layer(struct drm_plane *plane)
+{
+	return container_of(plane, struct sun8i_layer, plane);
+}
+
 static inline struct sun8i_mixer *
 engine_to_sun8i_mixer(struct sunxi_engine *engine)
 {
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index ca75ca0835a6..248fbb606ede 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -232,7 +232,7 @@ static int sun8i_ui_layer_atomic_check(struct drm_plane *plane,
 {
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
-	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
+	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
 	struct drm_crtc *crtc = new_plane_state->crtc;
 	struct drm_crtc_state *crtc_state;
 	int min_scale, max_scale;
@@ -264,7 +264,7 @@ static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane,
 {
 	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
 									   plane);
-	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
+	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
 	unsigned int old_zpos = old_state->normalized_zpos;
 	struct sun8i_mixer *mixer = layer->mixer;
 
@@ -279,7 +279,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
 									   plane);
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
 									   plane);
-	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
+	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
 	unsigned int zpos = new_state->normalized_zpos;
 	unsigned int old_zpos = old_state->normalized_zpos;
 	struct sun8i_mixer *mixer = layer->mixer;
@@ -345,13 +345,13 @@ static const uint64_t sun8i_layer_modifiers[] = {
 	DRM_FORMAT_MOD_INVALID
 };
 
-struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
-					       struct sun8i_mixer *mixer,
-					       int index)
+struct sun8i_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
+					    struct sun8i_mixer *mixer,
+					    int index)
 {
 	enum drm_plane_type type = DRM_PLANE_TYPE_OVERLAY;
 	int channel = mixer->cfg->vi_num + index;
-	struct sun8i_ui_layer *layer;
+	struct sun8i_layer *layer;
 	unsigned int plane_cnt;
 	int ret;
 
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
index e3e32ee1178d..83892f6ff211 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
@@ -47,21 +47,9 @@
 #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED	((2) << 1)
 
 struct sun8i_mixer;
+struct sun8i_layer;
 
-struct sun8i_ui_layer {
-	struct drm_plane	plane;
-	struct sun8i_mixer	*mixer;
-	int			channel;
-	int			overlay;
-};
-
-static inline struct sun8i_ui_layer *
-plane_to_sun8i_ui_layer(struct drm_plane *plane)
-{
-	return container_of(plane, struct sun8i_ui_layer, plane);
-}
-
-struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
-					       struct sun8i_mixer *mixer,
-					       int index);
+struct sun8i_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
+					    struct sun8i_mixer *mixer,
+					    int index);
 #endif /* _SUN8I_UI_LAYER_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index f9c0a56d3a14..0c0f1ac80517 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -366,7 +366,7 @@ static int sun8i_vi_layer_atomic_check(struct drm_plane *plane,
 {
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
-	struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
+	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
 	struct drm_crtc *crtc = new_plane_state->crtc;
 	struct drm_crtc_state *crtc_state;
 	int min_scale, max_scale;
@@ -398,7 +398,7 @@ static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane,
 {
 	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
 									   plane);
-	struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
+	struct sun8i_layer *layer = plane_to_sun8i_vi_layer(plane);
 	unsigned int old_zpos = old_state->normalized_zpos;
 	struct sun8i_mixer *mixer = layer->mixer;
 
@@ -413,7 +413,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
 									   plane);
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
 									   plane);
-	struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
+	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
 	unsigned int zpos = new_state->normalized_zpos;
 	unsigned int old_zpos = old_state->normalized_zpos;
 	struct sun8i_mixer *mixer = layer->mixer;
@@ -539,14 +539,14 @@ static const uint64_t sun8i_layer_modifiers[] = {
 	DRM_FORMAT_MOD_INVALID
 };
 
-struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
-					       struct sun8i_mixer *mixer,
-					       int index)
+struct sun8i_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
+					    struct sun8i_mixer *mixer,
+					    int index)
 {
 	enum drm_plane_type type = DRM_PLANE_TYPE_OVERLAY;
 	u32 supported_encodings, supported_ranges;
 	unsigned int plane_cnt, format_count;
-	struct sun8i_vi_layer *layer;
+	struct sun8i_layer *layer;
 	const u32 *formats;
 	int ret;
 
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
index 48c399e1c86d..655440cdc78f 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
@@ -52,21 +52,9 @@
 #define SUN8I_MIXER_CHAN_VI_DS_M(x)			((x) << 0)
 
 struct sun8i_mixer;
+struct sun8i_layer;
 
-struct sun8i_vi_layer {
-	struct drm_plane	plane;
-	struct sun8i_mixer	*mixer;
-	int			channel;
-	int			overlay;
-};
-
-static inline struct sun8i_vi_layer *
-plane_to_sun8i_vi_layer(struct drm_plane *plane)
-{
-	return container_of(plane, struct sun8i_vi_layer, plane);
-}
-
-struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
-					       struct sun8i_mixer *mixer,
-					       int index);
+struct sun8i_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
+					    struct sun8i_mixer *mixer,
+					    int index);
 #endif /* _SUN8I_VI_LAYER_H_ */
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] drm/sun4i: Unify sun8i_*_layer structs

[Maxime Ripard]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

On Fri, 16 Feb 2024 20:04:24 +0100, Ondřej Jirman wrote:
> From: Ondrej Jirman <megi@xff.cz>
> 
> These structs are identical, use a single struct to represent private
> data for the DRM plane. This is a preparation for configuring layer
> routing from the CRTC (mixer) instead of current approach of setting
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime


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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] drm/sun4i: Unify sun8i_*_layer structs

[Jernej Škrabec]

Dne petek, 16. februar 2024 ob 20:04:24 CET je Ondřej Jirman napisal(a):
> From: Ondrej Jirman <megi@xff.cz>
> 
> These structs are identical, use a single struct to represent private
> data for the DRM plane. This is a preparation for configuring layer
> routing from the CRTC (mixer) instead of current approach of setting
> up routing from individual layer's atomic_update callback.
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.c    |  4 ++--
>  drivers/gpu/drm/sun4i/sun8i_mixer.h    | 14 ++++++++++++++
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 14 +++++++-------
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.h | 20 ++++----------------
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 14 +++++++-------
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.h | 20 ++++----------------
>  6 files changed, 38 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index 01382860aaee..1e681314e379 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -271,7 +271,7 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
>  		return ERR_PTR(-ENOMEM);
>  
>  	for (i = 0; i < mixer->cfg->vi_num; i++) {
> -		struct sun8i_vi_layer *layer;
> +		struct sun8i_layer *layer;
>  
>  		layer = sun8i_vi_layer_init_one(drm, mixer, i);
>  		if (IS_ERR(layer)) {
> @@ -284,7 +284,7 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
>  	}
>  
>  	for (i = 0; i < mixer->cfg->ui_num; i++) {
> -		struct sun8i_ui_layer *layer;
> +		struct sun8i_layer *layer;
>  
>  		layer = sun8i_ui_layer_init_one(drm, mixer, i);
>  		if (IS_ERR(layer)) {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> index 85c94884fb9a..5a610ee30301 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -9,6 +9,7 @@
>  #include <linux/clk.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
> +#include <drm/drm_plane.h>
>  
>  #include "sunxi_engine.h"
>  
> @@ -185,6 +186,19 @@ struct sun8i_mixer {
>  	struct clk			*mod_clk;
>  };
>  
> +struct sun8i_layer {
> +	struct drm_plane	plane;
> +	struct sun8i_mixer	*mixer;
> +	int			channel;
> +	int			overlay;
> +};
> +
> +static inline struct sun8i_layer *
> +plane_to_sun8i_layer(struct drm_plane *plane)
> +{
> +	return container_of(plane, struct sun8i_layer, plane);
> +}
> +
>  static inline struct sun8i_mixer *
>  engine_to_sun8i_mixer(struct sunxi_engine *engine)
>  {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> index ca75ca0835a6..248fbb606ede 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> @@ -232,7 +232,7 @@ static int sun8i_ui_layer_atomic_check(struct drm_plane *plane,
>  {
>  	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
>  										 plane);
> -	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> +	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
>  	struct drm_crtc *crtc = new_plane_state->crtc;
>  	struct drm_crtc_state *crtc_state;
>  	int min_scale, max_scale;
> @@ -264,7 +264,7 @@ static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane,
>  {
>  	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
>  									   plane);
> -	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> +	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
>  	unsigned int old_zpos = old_state->normalized_zpos;
>  	struct sun8i_mixer *mixer = layer->mixer;
>  
> @@ -279,7 +279,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
>  									   plane);
>  	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
>  									   plane);
> -	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> +	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
>  	unsigned int zpos = new_state->normalized_zpos;
>  	unsigned int old_zpos = old_state->normalized_zpos;
>  	struct sun8i_mixer *mixer = layer->mixer;
> @@ -345,13 +345,13 @@ static const uint64_t sun8i_layer_modifiers[] = {
>  	DRM_FORMAT_MOD_INVALID
>  };
>  
> -struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
> -					       struct sun8i_mixer *mixer,
> -					       int index)
> +struct sun8i_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
> +					    struct sun8i_mixer *mixer,
> +					    int index)
>  {
>  	enum drm_plane_type type = DRM_PLANE_TYPE_OVERLAY;
>  	int channel = mixer->cfg->vi_num + index;
> -	struct sun8i_ui_layer *layer;
> +	struct sun8i_layer *layer;
>  	unsigned int plane_cnt;
>  	int ret;
>  
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> index e3e32ee1178d..83892f6ff211 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> @@ -47,21 +47,9 @@
>  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED	((2) << 1)
>  
>  struct sun8i_mixer;
> +struct sun8i_layer;
>  
> -struct sun8i_ui_layer {
> -	struct drm_plane	plane;
> -	struct sun8i_mixer	*mixer;
> -	int			channel;
> -	int			overlay;
> -};
> -
> -static inline struct sun8i_ui_layer *
> -plane_to_sun8i_ui_layer(struct drm_plane *plane)
> -{
> -	return container_of(plane, struct sun8i_ui_layer, plane);
> -}
> -
> -struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
> -					       struct sun8i_mixer *mixer,
> -					       int index);
> +struct sun8i_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
> +					    struct sun8i_mixer *mixer,
> +					    int index);
>  #endif /* _SUN8I_UI_LAYER_H_ */
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> index f9c0a56d3a14..0c0f1ac80517 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> @@ -366,7 +366,7 @@ static int sun8i_vi_layer_atomic_check(struct drm_plane *plane,
>  {
>  	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
>  										 plane);
> -	struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> +	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
>  	struct drm_crtc *crtc = new_plane_state->crtc;
>  	struct drm_crtc_state *crtc_state;
>  	int min_scale, max_scale;
> @@ -398,7 +398,7 @@ static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane,
>  {
>  	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
>  									   plane);
> -	struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> +	struct sun8i_layer *layer = plane_to_sun8i_vi_layer(plane);
>  	unsigned int old_zpos = old_state->normalized_zpos;
>  	struct sun8i_mixer *mixer = layer->mixer;
>  
> @@ -413,7 +413,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
>  									   plane);
>  	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
>  									   plane);
> -	struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> +	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
>  	unsigned int zpos = new_state->normalized_zpos;
>  	unsigned int old_zpos = old_state->normalized_zpos;
>  	struct sun8i_mixer *mixer = layer->mixer;
> @@ -539,14 +539,14 @@ static const uint64_t sun8i_layer_modifiers[] = {
>  	DRM_FORMAT_MOD_INVALID
>  };
>  
> -struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
> -					       struct sun8i_mixer *mixer,
> -					       int index)
> +struct sun8i_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
> +					    struct sun8i_mixer *mixer,
> +					    int index)
>  {
>  	enum drm_plane_type type = DRM_PLANE_TYPE_OVERLAY;
>  	u32 supported_encodings, supported_ranges;
>  	unsigned int plane_cnt, format_count;
> -	struct sun8i_vi_layer *layer;
> +	struct sun8i_layer *layer;
>  	const u32 *formats;
>  	int ret;
>  
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> index 48c399e1c86d..655440cdc78f 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> @@ -52,21 +52,9 @@
>  #define SUN8I_MIXER_CHAN_VI_DS_M(x)			((x) << 0)
>  
>  struct sun8i_mixer;
> +struct sun8i_layer;
>  
> -struct sun8i_vi_layer {
> -	struct drm_plane	plane;
> -	struct sun8i_mixer	*mixer;
> -	int			channel;
> -	int			overlay;
> -};
> -
> -static inline struct sun8i_vi_layer *
> -plane_to_sun8i_vi_layer(struct drm_plane *plane)
> -{
> -	return container_of(plane, struct sun8i_vi_layer, plane);
> -}
> -
> -struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
> -					       struct sun8i_mixer *mixer,
> -					       int index);
> +struct sun8i_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
> +					    struct sun8i_mixer *mixer,
> +					    int index);
>  #endif /* _SUN8I_VI_LAYER_H_ */
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] drm/sun4i: Add more parameters to sunxi_engine commit callback

[Ondřej Jirman]

From: Ondrej Jirman <megi@xff.cz>

These will be needed later on when we move layer configuration to
crtc update.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
---
 drivers/gpu/drm/sun4i/sun4i_backend.c |  4 +++-
 drivers/gpu/drm/sun4i/sun4i_crtc.c    |  2 +-
 drivers/gpu/drm/sun4i/sun8i_mixer.c   |  5 ++++-
 drivers/gpu/drm/sun4i/sunxi_engine.h  | 13 ++++++++++---
 4 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 335fd0edb904..e89eb96d3131 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -69,7 +69,9 @@ static void sun4i_backend_disable_color_correction(struct sunxi_engine *engine)
 			   SUN4I_BACKEND_OCCTL_ENABLE, 0);
 }
 
-static void sun4i_backend_commit(struct sunxi_engine *engine)
+static void sun4i_backend_commit(struct sunxi_engine *engine,
+				 struct drm_crtc *crtc,
+				 struct drm_atomic_state *state)
 {
 	DRM_DEBUG_DRIVER("Committing changes\n");
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c
index c06d7cd45388..18e74047b0f5 100644
--- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
@@ -91,7 +91,7 @@ static void sun4i_crtc_atomic_flush(struct drm_crtc *crtc,
 
 	DRM_DEBUG_DRIVER("Committing plane changes\n");
 
-	sunxi_engine_commit(scrtc->engine);
+	sunxi_engine_commit(scrtc->engine, crtc, state);
 
 	if (event) {
 		crtc->state->event = NULL;
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index 1e681314e379..bdeb9b80e038 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/reset.h>
 
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_framebuffer.h>
@@ -249,7 +250,9 @@ int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format)
 	return -EINVAL;
 }
 
-static void sun8i_mixer_commit(struct sunxi_engine *engine)
+static void sun8i_mixer_commit(struct sunxi_engine *engine,
+			       struct drm_crtc *crtc,
+			       struct drm_atomic_state *state)
 {
 	DRM_DEBUG_DRIVER("Committing changes\n");
 
diff --git a/drivers/gpu/drm/sun4i/sunxi_engine.h b/drivers/gpu/drm/sun4i/sunxi_engine.h
index ec8cf9b2bda4..ec0c4932f15c 100644
--- a/drivers/gpu/drm/sun4i/sunxi_engine.h
+++ b/drivers/gpu/drm/sun4i/sunxi_engine.h
@@ -7,6 +7,7 @@
 #define _SUNXI_ENGINE_H_
 
 struct drm_plane;
+struct drm_crtc;
 struct drm_device;
 struct drm_crtc_state;
 struct drm_display_mode;
@@ -59,7 +60,9 @@ struct sunxi_engine_ops {
 	 *
 	 * This function is optional.
 	 */
-	void (*commit)(struct sunxi_engine *engine);
+	void (*commit)(struct sunxi_engine *engine,
+		       struct drm_crtc *crtc,
+		       struct drm_atomic_state *state);
 
 	/**
 	 * @layers_init:
@@ -144,12 +147,16 @@ struct sunxi_engine {
 /**
  * sunxi_engine_commit() - commit all changes of the engine
  * @engine:	pointer to the engine
+ * @crtc:	pointer to crtc the engine is associated with
+ * @state:	atomic state
  */
 static inline void
-sunxi_engine_commit(struct sunxi_engine *engine)
+sunxi_engine_commit(struct sunxi_engine *engine,
+		    struct drm_crtc *crtc,
+		    struct drm_atomic_state *state)
 {
 	if (engine->ops && engine->ops->commit)
-		engine->ops->commit(engine);
+		engine->ops->commit(engine, crtc, state);
 }
 
 /**
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drm/sun4i: Add more parameters to sunxi_engine commit callback

[Maxime Ripard]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 276 bytes --]

On Fri, 16 Feb 2024 20:04:25 +0100, Ondřej Jirman wrote:
> From: Ondrej Jirman <megi@xff.cz>
> 
> These will be needed later on when we move layer configuration to
> crtc update.
> 
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime


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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drm/sun4i: Add more parameters to sunxi_engine commit callback

[Jernej Škrabec]

Dne petek, 16. februar 2024 ob 20:04:25 CET je Ondřej Jirman napisal(a):
> From: Ondrej Jirman <megi@xff.cz>
> 
> These will be needed later on when we move layer configuration to
> crtc update.
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
>  drivers/gpu/drm/sun4i/sun4i_backend.c |  4 +++-
>  drivers/gpu/drm/sun4i/sun4i_crtc.c    |  2 +-
>  drivers/gpu/drm/sun4i/sun8i_mixer.c   |  5 ++++-
>  drivers/gpu/drm/sun4i/sunxi_engine.h  | 13 ++++++++++---
>  4 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index 335fd0edb904..e89eb96d3131 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -69,7 +69,9 @@ static void sun4i_backend_disable_color_correction(struct sunxi_engine *engine)
>  			   SUN4I_BACKEND_OCCTL_ENABLE, 0);
>  }
>  
> -static void sun4i_backend_commit(struct sunxi_engine *engine)
> +static void sun4i_backend_commit(struct sunxi_engine *engine,
> +				 struct drm_crtc *crtc,
> +				 struct drm_atomic_state *state)
>  {
>  	DRM_DEBUG_DRIVER("Committing changes\n");
>  
> diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c
> index c06d7cd45388..18e74047b0f5 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
> @@ -91,7 +91,7 @@ static void sun4i_crtc_atomic_flush(struct drm_crtc *crtc,
>  
>  	DRM_DEBUG_DRIVER("Committing plane changes\n");
>  
> -	sunxi_engine_commit(scrtc->engine);
> +	sunxi_engine_commit(scrtc->engine, crtc, state);
>  
>  	if (event) {
>  		crtc->state->event = NULL;
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index 1e681314e379..bdeb9b80e038 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -16,6 +16,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/reset.h>
>  
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_framebuffer.h>
> @@ -249,7 +250,9 @@ int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format)
>  	return -EINVAL;
>  }
>  
> -static void sun8i_mixer_commit(struct sunxi_engine *engine)
> +static void sun8i_mixer_commit(struct sunxi_engine *engine,
> +			       struct drm_crtc *crtc,
> +			       struct drm_atomic_state *state)
>  {
>  	DRM_DEBUG_DRIVER("Committing changes\n");
>  
> diff --git a/drivers/gpu/drm/sun4i/sunxi_engine.h b/drivers/gpu/drm/sun4i/sunxi_engine.h
> index ec8cf9b2bda4..ec0c4932f15c 100644
> --- a/drivers/gpu/drm/sun4i/sunxi_engine.h
> +++ b/drivers/gpu/drm/sun4i/sunxi_engine.h
> @@ -7,6 +7,7 @@
>  #define _SUNXI_ENGINE_H_
>  
>  struct drm_plane;
> +struct drm_crtc;
>  struct drm_device;
>  struct drm_crtc_state;
>  struct drm_display_mode;
> @@ -59,7 +60,9 @@ struct sunxi_engine_ops {
>  	 *
>  	 * This function is optional.
>  	 */
> -	void (*commit)(struct sunxi_engine *engine);
> +	void (*commit)(struct sunxi_engine *engine,
> +		       struct drm_crtc *crtc,
> +		       struct drm_atomic_state *state);
>  
>  	/**
>  	 * @layers_init:
> @@ -144,12 +147,16 @@ struct sunxi_engine {
>  /**
>   * sunxi_engine_commit() - commit all changes of the engine
>   * @engine:	pointer to the engine
> + * @crtc:	pointer to crtc the engine is associated with
> + * @state:	atomic state
>   */
>  static inline void
> -sunxi_engine_commit(struct sunxi_engine *engine)
> +sunxi_engine_commit(struct sunxi_engine *engine,
> +		    struct drm_crtc *crtc,
> +		    struct drm_atomic_state *state)
>  {
>  	if (engine->ops && engine->ops->commit)
> -		engine->ops->commit(engine);
> +		engine->ops->commit(engine, crtc, state);
>  }
>  
>  /**
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] drm/sun4i: Fix layer zpos change/atomic modesetting

[Ondřej Jirman]

From: Ondrej Jirman <megi@xff.cz>

Identical configurations of planes can lead to different (and wrong)
layer -> pipe routing at HW level, depending on the order of atomic
plane changes.

For example:

- Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
  is enabled. This is a typical situation at boot.

- When a compositor takes over and layer 3 is enabled,
  sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
  will lead to incorrect disabling of pipe 0 and enabling of pipe 1.

What happens is that sun8i_ui_layer_enable() function may disable
blender pipes even if it is no longer assigned to its layer.

To correct this, move the routing setup out of individual plane's
atomic_update into crtc's atomic_update, where it can be calculated
and updated all at once.

Remove the atomic_disable callback because it is no longer needed.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.c    | 73 +++++++++++++++++++++++++
 drivers/gpu/drm/sun4i/sun8i_mixer.h    |  6 +++
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 73 +------------------------
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 74 +-------------------------
 4 files changed, 83 insertions(+), 143 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index bdeb9b80e038..21331d4ffe01 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -250,12 +250,85 @@ int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format)
 	return -EINVAL;
 }
 
+static void sun8i_layer_enable(struct sun8i_layer *layer, bool enable)
+{
+	u32 ch_base = sun8i_channel_base(layer->mixer, layer->channel);
+	u32 val, reg, mask;
+
+	if (layer->type == SUN8I_LAYER_TYPE_UI) {
+		val = enable ? SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN : 0;
+		mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN;
+		reg = SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, layer->overlay);
+	} else {
+		val = enable ? SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN : 0;
+		mask = SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN;
+		reg = SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, layer->overlay);
+	}
+
+	regmap_update_bits(layer->mixer->engine.regs, reg, mask, val);
+}
+
 static void sun8i_mixer_commit(struct sunxi_engine *engine,
 			       struct drm_crtc *crtc,
 			       struct drm_atomic_state *state)
 {
+	struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
+	u32 bld_base = sun8i_blender_base(mixer);
+	struct drm_plane_state *plane_state;
+	struct drm_plane *plane;
+	u32 route = 0, pipe_en = 0;
+
 	DRM_DEBUG_DRIVER("Committing changes\n");
 
+	drm_for_each_plane(plane, state->dev) {
+		struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
+		bool enable;
+		int zpos;
+
+		if (!(plane->possible_crtcs & drm_crtc_mask(crtc)) || layer->mixer != mixer)
+			continue;
+
+		plane_state = drm_atomic_get_new_plane_state(state, plane);
+		if (!plane_state)
+			plane_state = plane->state;
+
+		enable = plane_state->crtc && plane_state->visible;
+		zpos = plane_state->normalized_zpos;
+
+		DRM_DEBUG_DRIVER("  plane %d: chan=%d ovl=%d en=%d zpos=%d\n",
+				 plane->base.id, layer->channel, layer->overlay,
+				 enable, zpos);
+
+		/*
+		 * We always update the layer enable bit, because it can clear
+		 * spontaneously for unknown reasons.
+		 */
+		sun8i_layer_enable(layer, enable);
+
+		if (!enable)
+			continue;
+
+		/* Route layer to pipe based on zpos */
+		route |= layer->channel << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
+		pipe_en |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
+	}
+
+	regmap_update_bits(mixer->engine.regs,
+			   SUN8I_MIXER_BLEND_ROUTE(bld_base),
+			   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(0) |
+			   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(1) |
+			   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(2) |
+			   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(3),
+			   route);
+
+	regmap_update_bits(mixer->engine.regs,
+			   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
+			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(0) |
+			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(1) |
+			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(2) |
+			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(3),
+			   pipe_en);
+
 	regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
 		     SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
 }
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
index 5a610ee30301..d7898c9c9cc0 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
@@ -186,9 +186,15 @@ struct sun8i_mixer {
 	struct clk			*mod_clk;
 };
 
+enum {
+	SUN8I_LAYER_TYPE_UI,
+	SUN8I_LAYER_TYPE_VI,
+};
+
 struct sun8i_layer {
 	struct drm_plane	plane;
 	struct sun8i_mixer	*mixer;
+	int			type;
 	int			channel;
 	int			overlay;
 };
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index 248fbb606ede..b90e5edef4e8 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -24,55 +24,6 @@
 #include "sun8i_ui_layer.h"
 #include "sun8i_ui_scaler.h"
 
-static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
-				  int overlay, bool enable, unsigned int zpos,
-				  unsigned int old_zpos)
-{
-	u32 val, bld_base, ch_base;
-
-	bld_base = sun8i_blender_base(mixer);
-	ch_base = sun8i_channel_base(mixer, channel);
-
-	DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
-			 enable ? "En" : "Dis", channel, overlay);
-
-	if (enable)
-		val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN;
-	else
-		val = 0;
-
-	regmap_update_bits(mixer->engine.regs,
-			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
-			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
-
-	if (!enable || zpos != old_zpos) {
-		regmap_update_bits(mixer->engine.regs,
-				   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
-				   SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
-				   0);
-
-		regmap_update_bits(mixer->engine.regs,
-				   SUN8I_MIXER_BLEND_ROUTE(bld_base),
-				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
-				   0);
-	}
-
-	if (enable) {
-		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
-
-		regmap_update_bits(mixer->engine.regs,
-				   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
-				   val, val);
-
-		val = channel << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
-
-		regmap_update_bits(mixer->engine.regs,
-				   SUN8I_MIXER_BLEND_ROUTE(bld_base),
-				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
-				   val);
-	}
-}
-
 static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int channel,
 					int overlay, struct drm_plane *plane)
 {
@@ -259,36 +210,18 @@ static int sun8i_ui_layer_atomic_check(struct drm_plane *plane,
 						   true, true);
 }
 
-static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane,
-					  struct drm_atomic_state *state)
-{
-	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
-									   plane);
-	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
-	unsigned int old_zpos = old_state->normalized_zpos;
-	struct sun8i_mixer *mixer = layer->mixer;
-
-	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false, 0,
-			      old_zpos);
-}
 
 static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
 					 struct drm_atomic_state *state)
 {
-	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
-									   plane);
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
 									   plane);
 	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
 	unsigned int zpos = new_state->normalized_zpos;
-	unsigned int old_zpos = old_state->normalized_zpos;
 	struct sun8i_mixer *mixer = layer->mixer;
 
-	if (!new_state->visible) {
-		sun8i_ui_layer_enable(mixer, layer->channel,
-				      layer->overlay, false, 0, old_zpos);
+	if (!new_state->crtc || !new_state->visible)
 		return;
-	}
 
 	sun8i_ui_layer_update_coord(mixer, layer->channel,
 				    layer->overlay, plane, zpos);
@@ -298,13 +231,10 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
 				      layer->overlay, plane);
 	sun8i_ui_layer_update_buffer(mixer, layer->channel,
 				     layer->overlay, plane);
-	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
-			      true, zpos, old_zpos);
 }
 
 static const struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
 	.atomic_check	= sun8i_ui_layer_atomic_check,
-	.atomic_disable	= sun8i_ui_layer_atomic_disable,
 	.atomic_update	= sun8i_ui_layer_atomic_update,
 };
 
@@ -390,6 +320,7 @@ struct sun8i_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
 
 	drm_plane_helper_add(&layer->plane, &sun8i_ui_layer_helper_funcs);
 	layer->mixer = mixer;
+	layer->type = SUN8I_LAYER_TYPE_UI;
 	layer->channel = channel;
 	layer->overlay = 0;
 
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index 0c0f1ac80517..9c09d9c08496 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -18,55 +18,6 @@
 #include "sun8i_vi_layer.h"
 #include "sun8i_vi_scaler.h"
 
-static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
-				  int overlay, bool enable, unsigned int zpos,
-				  unsigned int old_zpos)
-{
-	u32 val, bld_base, ch_base;
-
-	bld_base = sun8i_blender_base(mixer);
-	ch_base = sun8i_channel_base(mixer, channel);
-
-	DRM_DEBUG_DRIVER("%sabling VI channel %d overlay %d\n",
-			 enable ? "En" : "Dis", channel, overlay);
-
-	if (enable)
-		val = SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN;
-	else
-		val = 0;
-
-	regmap_update_bits(mixer->engine.regs,
-			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay),
-			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
-
-	if (!enable || zpos != old_zpos) {
-		regmap_update_bits(mixer->engine.regs,
-				   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
-				   SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
-				   0);
-
-		regmap_update_bits(mixer->engine.regs,
-				   SUN8I_MIXER_BLEND_ROUTE(bld_base),
-				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
-				   0);
-	}
-
-	if (enable) {
-		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
-
-		regmap_update_bits(mixer->engine.regs,
-				   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
-				   val, val);
-
-		val = channel << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
-
-		regmap_update_bits(mixer->engine.regs,
-				   SUN8I_MIXER_BLEND_ROUTE(bld_base),
-				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
-				   val);
-	}
-}
-
 static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int channel,
 					int overlay, struct drm_plane *plane)
 {
@@ -393,36 +344,17 @@ static int sun8i_vi_layer_atomic_check(struct drm_plane *plane,
 						   true, true);
 }
 
-static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane,
-					  struct drm_atomic_state *state)
-{
-	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
-									   plane);
-	struct sun8i_layer *layer = plane_to_sun8i_vi_layer(plane);
-	unsigned int old_zpos = old_state->normalized_zpos;
-	struct sun8i_mixer *mixer = layer->mixer;
-
-	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false, 0,
-			      old_zpos);
-}
-
 static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
 					 struct drm_atomic_state *state)
 {
-	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
-									   plane);
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
 									   plane);
 	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
 	unsigned int zpos = new_state->normalized_zpos;
-	unsigned int old_zpos = old_state->normalized_zpos;
 	struct sun8i_mixer *mixer = layer->mixer;
 
-	if (!new_state->visible) {
-		sun8i_vi_layer_enable(mixer, layer->channel,
-				      layer->overlay, false, 0, old_zpos);
+	if (!new_state->crtc || !new_state->visible)
 		return;
-	}
 
 	sun8i_vi_layer_update_coord(mixer, layer->channel,
 				    layer->overlay, plane, zpos);
@@ -432,13 +364,10 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
 				      layer->overlay, plane);
 	sun8i_vi_layer_update_buffer(mixer, layer->channel,
 				     layer->overlay, plane);
-	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
-			      true, zpos, old_zpos);
 }
 
 static const struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
 	.atomic_check	= sun8i_vi_layer_atomic_check,
-	.atomic_disable	= sun8i_vi_layer_atomic_disable,
 	.atomic_update	= sun8i_vi_layer_atomic_update,
 };
 
@@ -613,6 +542,7 @@ struct sun8i_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
 
 	drm_plane_helper_add(&layer->plane, &sun8i_vi_layer_helper_funcs);
 	layer->mixer = mixer;
+	layer->type = SUN8I_LAYER_TYPE_VI;
 	layer->channel = index;
 	layer->overlay = 0;
 
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] drm/sun4i: Fix layer zpos change/atomic modesetting

[Maxime Ripard]

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

Hi,

On Fri, Feb 16, 2024 at 08:04:26PM +0100, Ondřej Jirman wrote:
> From: Ondrej Jirman <megi@xff.cz>
> 
> Identical configurations of planes can lead to different (and wrong)
> layer -> pipe routing at HW level, depending on the order of atomic
> plane changes.
> 
> For example:
> 
> - Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
>   is enabled. This is a typical situation at boot.
> 
> - When a compositor takes over and layer 3 is enabled,
>   sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
>   will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
> 
> What happens is that sun8i_ui_layer_enable() function may disable
> blender pipes even if it is no longer assigned to its layer.
> 
> To correct this, move the routing setup out of individual plane's
> atomic_update into crtc's atomic_update, where it can be calculated
> and updated all at once.
> 
> Remove the atomic_disable callback because it is no longer needed.
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>

I don't have enough knowledge about the mixers code to comment on your
patch, so I'll let Jernej review it. However, this feels to me like the
pipe assignment is typically the sort of things that should be dealt
with device-wide, and in atomic_check.

If I'm talking non-sense, it would be great to mention at least why that
can't be an option in the commit log.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] drm/sun4i: Fix layer zpos change/atomic modesetting

[Ondřej Jirman]

Hi Maxime,

On Wed, Feb 21, 2024 at 02:45:20PM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Feb 16, 2024 at 08:04:26PM +0100, Ondřej Jirman wrote:
> > From: Ondrej Jirman <megi@xff.cz>
> > 
> > Identical configurations of planes can lead to different (and wrong)
> > layer -> pipe routing at HW level, depending on the order of atomic
> > plane changes.
> > 
> > For example:
> > 
> > - Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
> >   is enabled. This is a typical situation at boot.
> > 
> > - When a compositor takes over and layer 3 is enabled,
> >   sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
> >   will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
> > 
> > What happens is that sun8i_ui_layer_enable() function may disable
> > blender pipes even if it is no longer assigned to its layer.
> > 
> > To correct this, move the routing setup out of individual plane's
> > atomic_update into crtc's atomic_update, where it can be calculated
> > and updated all at once.
> > 
> > Remove the atomic_disable callback because it is no longer needed.
> > 
> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> 
> I don't have enough knowledge about the mixers code to comment on your
> patch, so I'll let Jernej review it. However, this feels to me like the
> pipe assignment is typically the sort of things that should be dealt
> with device-wide, and in atomic_check.

Yes, this is what this patch does. It moves blender setup from individual
planes' attomic commits to a single place in crtc atomic commit.

kind regards,
	o.

> If I'm talking non-sense, it would be great to mention at least why that
> can't be an option in the commit log.
> 
> Maxime



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] drm/sun4i: Fix layer zpos change/atomic modesetting

[Jernej Škrabec]

Dne sreda, 21. februar 2024 ob 14:45:20 CET je Maxime Ripard napisal(a):
> Hi,
> 
> On Fri, Feb 16, 2024 at 08:04:26PM +0100, Ondřej Jirman wrote:
> > From: Ondrej Jirman <megi@xff.cz>
> > 
> > Identical configurations of planes can lead to different (and wrong)
> > layer -> pipe routing at HW level, depending on the order of atomic
> > plane changes.
> > 
> > For example:
> > 
> > - Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
> >   is enabled. This is a typical situation at boot.
> > 
> > - When a compositor takes over and layer 3 is enabled,
> >   sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
> >   will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
> > 
> > What happens is that sun8i_ui_layer_enable() function may disable
> > blender pipes even if it is no longer assigned to its layer.
> > 
> > To correct this, move the routing setup out of individual plane's
> > atomic_update into crtc's atomic_update, where it can be calculated
> > and updated all at once.
> > 
> > Remove the atomic_disable callback because it is no longer needed.
> > 
> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> 
> I don't have enough knowledge about the mixers code to comment on your
> patch, so I'll let Jernej review it. However, this feels to me like the
> pipe assignment is typically the sort of things that should be dealt
> with device-wide, and in atomic_check.

In DE2 and DE3.0, you cannot move planes between mixers (crtcs), because each
one is hardwired to specific mixer. Movable planes are the feature of DE3.3
and one of the pain points for upstreaming the code. Anyway, this commit only
addresses current issue of enabling and disabling planes and handling zpos.

In atomic check you can only precalculate final register values, but I don't
see any benefit doing that. I think that this code elegantly solves current
issue of enabling or disabling wrong plane in certain situations, so:

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Note, if there is new revision, please rewrite blender regmap_update_bits()
to regmap_write(). Since there is HW issue with reads, I would like to
get rid of regmap_update_bits() calls eventually.

Best regards,
Jernej

> 
> If I'm talking non-sense, it would be great to mention at least why that
> can't be an option in the commit log.
> 
> Maxime
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] drm/sun4i: Fix layer zpos change/atomic modesetting

[Ondřej Jirman]

On Thu, Feb 22, 2024 at 09:02:53PM +0100, Jernej Škrabec wrote:
> Dne sreda, 21. februar 2024 ob 14:45:20 CET je Maxime Ripard napisal(a):
> > Hi,
> > 
> > On Fri, Feb 16, 2024 at 08:04:26PM +0100, Ondřej Jirman wrote:
> > > From: Ondrej Jirman <megi@xff.cz>
> > > 
> > > Identical configurations of planes can lead to different (and wrong)
> > > layer -> pipe routing at HW level, depending on the order of atomic
> > > plane changes.
> > > 
> > > For example:
> > > 
> > > - Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
> > >   is enabled. This is a typical situation at boot.
> > > 
> > > - When a compositor takes over and layer 3 is enabled,
> > >   sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
> > >   will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
> > > 
> > > What happens is that sun8i_ui_layer_enable() function may disable
> > > blender pipes even if it is no longer assigned to its layer.
> > > 
> > > To correct this, move the routing setup out of individual plane's
> > > atomic_update into crtc's atomic_update, where it can be calculated
> > > and updated all at once.
> > > 
> > > Remove the atomic_disable callback because it is no longer needed.
> > > 
> > > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > 
> > I don't have enough knowledge about the mixers code to comment on your
> > patch, so I'll let Jernej review it. However, this feels to me like the
> > pipe assignment is typically the sort of things that should be dealt
> > with device-wide, and in atomic_check.
> 
> In DE2 and DE3.0, you cannot move planes between mixers (crtcs), because each
> one is hardwired to specific mixer. Movable planes are the feature of DE3.3
> and one of the pain points for upstreaming the code. Anyway, this commit only
> addresses current issue of enabling and disabling planes and handling zpos.
> 
> In atomic check you can only precalculate final register values, but I don't
> see any benefit doing that. I think that this code elegantly solves current
> issue of enabling or disabling wrong plane in certain situations, so:
> 
> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Thanks for the review.

> Note, if there is new revision, please rewrite blender regmap_update_bits()
> to regmap_write(). Since there is HW issue with reads, I would like to
> get rid of regmap_update_bits() calls eventually.

BTW as a side note, one observation I made about a year back after some testing
with a setup like this:

- PIN photodiode in a black box directed at a display (Pinephone)
  attached to the oscilloscope
- GPIO signal routed to another input of the scope
- sun4i-drm driver modification that toggles the GPIO any number of times so
  that I can correlate brightness changes as seen by photodiode on the
  scope with the timing of what's happening in the driver

is that if you do atomic commit right after receiving the flip event there's
a high chance that what you're commiting will get displayed right away,
basically skipping the update from the previous atomic commit.

It's very easy to reproduce with a test application that flips between two
completely white and completely black framebuffers back and forth right
after flip event. What you see on the display is seemingly random blinking
pattern: https://megous.com/dl/tmp/c9a286427ce66d80.png when it should be
a peridic flipping between black and white pixels like this
https://megous.com/dl/tmp/c085a50ccd387257.png

This goes away for me when introducing a >150us delay between flip event
and next atomic commit.

I did a more complex test with a test app that switches between 4 framebuffers
of decreasing shades of gray, so that the resulting pattern of brightness should
be a stariwise stepping signal pattern on the scope, along with gpio indicating which
framebuffer just got commited (by the number of pulses 1-4). What I saw was
that normally there's a 1 frame delay between commit and the corresponding
brightness change, but often enough the brightness change is immediate.

I think the hardware indicates a flip (vsync irq) some time before it performs
the double buffered register update to shadow register for the next frame. So
sometimes the driver manages to overwrite the values stored during the previous
commit before they get flipped to shadow registers. Again, intorducing a 150us
delay between flip event and atomic commit reliably fixes this, and I see
a perfect stairwise pattern on the scope instead of occasional skipped ahead
steps.

It seems to me that those 150us are related to display timing.

Vertical blanking cycle on Pinephone lasts about 400us with 45us long vsync
pulse roughly in the middle. In my observation it seems likely that HW signals
VSYNC interrupt at the start or end of the VSYNC pulse and flips the double
buffered HW registers at the end of the blanking cycle, leaving some 150us of
time in between for a quick atomic commit to still mess up the register settings
for the upcoming scan out, before they get commited to shadow regs. I don't have
a way to observe vsync signal on a scope, because it's MIPI-DSI display to
verify this exactly.

The certain thing is that after vsync interrupt, the double buffered registers
are not switched, yet. There's still some time left before that happens.

Anyway, just something I'd thought I share that may help us understand this
HW better. :)

kind regards,
	o.

> Best regards,
> Jernej
> 
> > 
> > If I'm talking non-sense, it would be great to mention at least why that
> > can't be an option in the commit log.
> > 
> > Maxime
> > 
> 
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] drm/sun4i: Fix layer zpos change/atomic modesetting

[Ondřej Jirman]

On Thu, Feb 22, 2024 at 09:02:53PM +0100, Jernej Škrabec wrote:
> Dne sreda, 21. februar 2024 ob 14:45:20 CET je Maxime Ripard napisal(a):
> > Hi,
> > 
> > On Fri, Feb 16, 2024 at 08:04:26PM +0100, Ondřej Jirman wrote:
> > > From: Ondrej Jirman <megi@xff.cz>
> > > 
> > > Identical configurations of planes can lead to different (and wrong)
> > > layer -> pipe routing at HW level, depending on the order of atomic
> > > plane changes.
> > > 
> > > For example:
> > > 
> > > - Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
> > >   is enabled. This is a typical situation at boot.
> > > 
> > > - When a compositor takes over and layer 3 is enabled,
> > >   sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
> > >   will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
> > > 
> > > What happens is that sun8i_ui_layer_enable() function may disable
> > > blender pipes even if it is no longer assigned to its layer.
> > > 
> > > To correct this, move the routing setup out of individual plane's
> > > atomic_update into crtc's atomic_update, where it can be calculated
> > > and updated all at once.
> > > 
> > > Remove the atomic_disable callback because it is no longer needed.
> > > 
> > > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > 
> > I don't have enough knowledge about the mixers code to comment on your
> > patch, so I'll let Jernej review it. However, this feels to me like the
> > pipe assignment is typically the sort of things that should be dealt
> > with device-wide, and in atomic_check.
> 
> In DE2 and DE3.0, you cannot move planes between mixers (crtcs), because each
> one is hardwired to specific mixer. Movable planes are the feature of DE3.3
> and one of the pain points for upstreaming the code. Anyway, this commit only
> addresses current issue of enabling and disabling planes and handling zpos.
> 
> In atomic check you can only precalculate final register values, but I don't
> see any benefit doing that. I think that this code elegantly solves current
> issue of enabling or disabling wrong plane in certain situations, so:
> 
> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> 
> Note, if there is new revision, please rewrite blender regmap_update_bits()
> to regmap_write(). Since there is HW issue with reads, I would like to
> get rid of regmap_update_bits() calls eventually.

I've looked into it and I can probably rewrite these quite readily:

+	regmap_update_bits(mixer->engine.regs,
+			   SUN8I_MIXER_BLEND_ROUTE(bld_base),
+			   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(0) |
+			   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(1) |
+			   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(2) |
+			   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(3),
+			   route);

The mask here covers all implemented bits in the register.

+	regmap_update_bits(mixer->engine.regs,
+			   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
+			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(0) |
+			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(1) |
+			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(2) |
+			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(3),
+			   pipe_en);
+

The mask here doesn't cover BLD_FILL_COLOR_CTL.Px_FCEN bits that implement solid
color filling. But those can be 0 anyway except for pipe0 which is hardcoded by
the driver to 1, I think:

631         /*
632          * Set fill color of bottom plane to black. Generally not needed
633          * except when VI plane is at bottom (zpos = 0) and enabled.
634          */
635         regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
636                      SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));

I will not be able to get rid of regmap_update_bits in sun8i_layer_enable
because that register there has other important things in it like framebuffer
pixel format, etc.

kind regards,
	o.

> Best regards,
> Jernej
> 
> > 
> > If I'm talking non-sense, it would be great to mention at least why that
> > can't be an option in the commit log.
> > 
> > Maxime
> > 
> 
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] drm/sun4i: Fix layer zpos change/atomic modesetting

[Jernej Škrabec]

On Saturday, February 24, 2024 3:20:43 AM CET Ondřej Jirman wrote:
> On Thu, Feb 22, 2024 at 09:02:53PM +0100, Jernej Škrabec wrote:
> > Dne sreda, 21. februar 2024 ob 14:45:20 CET je Maxime Ripard napisal(a):
> > > Hi,
> > > 
> > > On Fri, Feb 16, 2024 at 08:04:26PM +0100, Ondřej Jirman wrote:
> > > > From: Ondrej Jirman <megi@xff.cz>
> > > > 
> > > > Identical configurations of planes can lead to different (and wrong)
> > > > layer -> pipe routing at HW level, depending on the order of atomic
> > > > plane changes.
> > > > 
> > > > For example:
> > > > 
> > > > - Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
> > > >   is enabled. This is a typical situation at boot.
> > > > 
> > > > - When a compositor takes over and layer 3 is enabled,
> > > >   sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
> > > >   will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
> > > > 
> > > > What happens is that sun8i_ui_layer_enable() function may disable
> > > > blender pipes even if it is no longer assigned to its layer.
> > > > 
> > > > To correct this, move the routing setup out of individual plane's
> > > > atomic_update into crtc's atomic_update, where it can be calculated
> > > > and updated all at once.
> > > > 
> > > > Remove the atomic_disable callback because it is no longer needed.
> > > > 
> > > > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > > 
> > > I don't have enough knowledge about the mixers code to comment on your
> > > patch, so I'll let Jernej review it. However, this feels to me like the
> > > pipe assignment is typically the sort of things that should be dealt
> > > with device-wide, and in atomic_check.
> > 
> > In DE2 and DE3.0, you cannot move planes between mixers (crtcs), because each
> > one is hardwired to specific mixer. Movable planes are the feature of DE3.3
> > and one of the pain points for upstreaming the code. Anyway, this commit only
> > addresses current issue of enabling and disabling planes and handling zpos.
> > 
> > In atomic check you can only precalculate final register values, but I don't
> > see any benefit doing that. I think that this code elegantly solves current
> > issue of enabling or disabling wrong plane in certain situations, so:
> > 
> > Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > 
> > Note, if there is new revision, please rewrite blender regmap_update_bits()
> > to regmap_write(). Since there is HW issue with reads, I would like to
> > get rid of regmap_update_bits() calls eventually.
> 
> I've looked into it and I can probably rewrite these quite readily:
> 
> +	regmap_update_bits(mixer->engine.regs,
> +			   SUN8I_MIXER_BLEND_ROUTE(bld_base),
> +			   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(0) |
> +			   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(1) |
> +			   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(2) |
> +			   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(3),
> +			   route);
> 
> The mask here covers all implemented bits in the register.
> 
> +	regmap_update_bits(mixer->engine.regs,
> +			   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> +			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(0) |
> +			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(1) |
> +			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(2) |
> +			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(3),
> +			   pipe_en);
> +
> 
> The mask here doesn't cover BLD_FILL_COLOR_CTL.Px_FCEN bits that implement solid
> color filling. But those can be 0 anyway except for pipe0 which is hardcoded by
> the driver to 1, I think:
> 
> 631         /*
> 632          * Set fill color of bottom plane to black. Generally not needed
> 633          * except when VI plane is at bottom (zpos = 0) and enabled.
> 634          */
> 635         regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> 636                      SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));

Correct on all counts. That's what I've meant.

> 
> I will not be able to get rid of regmap_update_bits in sun8i_layer_enable
> because that register there has other important things in it like framebuffer
> pixel format, etc.

Yeah, this rework would certainly be more involved, so it's out of the scope of
this series.

Best regards,
Jernej

> 
> kind regards,
> 	o.
> 
> > Best regards,
> > Jernej
> > 
> > > 
> > > If I'm talking non-sense, it would be great to mention at least why that
> > > can't be an option in the commit log.
> > > 
> > > Maxime
> > > 
> > 
> > 
> > 
> > 
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel