(no subject)

[roman.stratiienko]

This is series of various sun8i fixes.

Currently I am working on bringing-up Android on sunxi platform.
And during UI debugging process a lot of issues was observed.

This patch-set is far from perfect, but we could start from it and polish
during review process.

[RFC 1/4] drm/sun4i: Wait for previous mixing process finish before committing next

[roman.stratiienko]

From: Roman Stratiienko <roman.stratiienko@globallogic.com>

Screen composition that requires dynamic layout modification,
especially scaling is corrupted when layout changes.

For example if one of the layer scales down, misaligned lines can be
observed, and dynamic increasing of destination area makes mixer to hang
and draw nothing after drawing modified layer.

After deep investigation it turns that scaler double-buffered registers
are not latched by GLB_DBUFFER bit, instead thay are latched immidiately.

Only way to avoid artifacts is to change the registers after mixer finish
previous frame.

Similar was made in sunxi BSP - scaler register values was stored into RAM,
and moved into registers at sync together with GLB_DBUFFER.

Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.c | 15 +++++++++++++++
 drivers/gpu/drm/sun4i/sun8i_mixer.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index 8b803eb903b8..eea4813602b7 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -257,6 +257,20 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32 format)
 	return NULL;
 }
 
+static void sun8i_atomic_begin(struct sunxi_engine *engine,
+			       struct drm_crtc_state *old_state)
+{
+	int reg, ret;
+
+	ret = regmap_read_poll_timeout(engine->regs, SUN8I_MIXER_GLOBAL_STATUS,
+				       reg,
+				       !(reg & SUN8I_MIXER_GLOBAL_STATUS_BUSY),
+				       200, 100000);
+
+	if (ret)
+		pr_warn("%s: Wait for frame finish timeout\n", __func__);
+}
+
 static void sun8i_mixer_commit(struct sunxi_engine *engine)
 {
 	DRM_DEBUG_DRIVER("Committing changes\n");
@@ -310,6 +324,7 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
 static const struct sunxi_engine_ops sun8i_engine_ops = {
 	.commit		= sun8i_mixer_commit,
 	.layers_init	= sun8i_layers_init,
+	.atomic_begin	= sun8i_atomic_begin,
 };
 
 static struct regmap_config sun8i_mixer_regmap_config = {
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
index c6cc94057faf..915479cc3077 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
@@ -25,6 +25,8 @@
 
 #define SUN8I_MIXER_GLOBAL_DBUFF_ENABLE		BIT(0)
 
+#define SUN8I_MIXER_GLOBAL_STATUS_BUSY		BIT(4)
+
 #define DE2_MIXER_UNIT_SIZE			0x6000
 #define DE3_MIXER_UNIT_SIZE			0x3000
 
-- 
2.17.1

Re: [RFC 1/4] drm/sun4i: Wait for previous mixing process finish before committing next

[Jernej Škrabec]

Hi!

Dne sobota, 28. december 2019 ob 21:28:15 CET je 
roman.stratiienko@globallogic.com napisal(a):
> From: Roman Stratiienko <roman.stratiienko@globallogic.com>
> 
> Screen composition that requires dynamic layout modification,
> especially scaling is corrupted when layout changes.
> 
> For example if one of the layer scales down, misaligned lines can be
> observed, and dynamic increasing of destination area makes mixer to hang
> and draw nothing after drawing modified layer.
> 
> After deep investigation it turns that scaler double-buffered registers
> are not latched by GLB_DBUFFER bit, instead thay are latched immidiately.
> 
> Only way to avoid artifacts is to change the registers after mixer finish
> previous frame.
> 
> Similar was made in sunxi BSP - scaler register values was stored into RAM,
> and moved into registers at sync together with GLB_DBUFFER.
> 
> Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>

Nice catch! However, I'm a bit worried about blocking nature of this solution. 
What about shadowing scaler registers and applying them in "finish_irq" 
handler? You see, VI scaler can in some cases consume almost all time between 
two VSync events. That issue came up on A64 mixer1 with downscaling 4K videos 
in real time. I imagine that this solution might block for too long in this 
case.

Check VI coarse scaling code:
https://elixir.bootlin.com/linux/v5.5-rc3/source/drivers/gpu/drm/sun4i/
sun8i_vi_layer.c#L144

> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.c | 15 +++++++++++++++
>  drivers/gpu/drm/sun4i/sun8i_mixer.h |  2 ++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 8b803eb903b8..eea4813602b7
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -257,6 +257,20 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32
> format) return NULL;
>  }
> 
> +static void sun8i_atomic_begin(struct sunxi_engine *engine,
> +			       struct drm_crtc_state *old_state)
> +{
> +	int reg, ret;
> +
> +	ret = regmap_read_poll_timeout(engine->regs, 
SUN8I_MIXER_GLOBAL_STATUS,
> +				       reg,
> +				       !(reg & 
SUN8I_MIXER_GLOBAL_STATUS_BUSY),
> +				       200, 100000);
> +
> +	if (ret)
> +		pr_warn("%s: Wait for frame finish timeout\n", __func__);

Newly introduced drm_warn() should be used here.

Best regards,
Jernej

> +}
> +
>  static void sun8i_mixer_commit(struct sunxi_engine *engine)
>  {
>  	DRM_DEBUG_DRIVER("Committing changes\n");
> @@ -310,6 +324,7 @@ static struct drm_plane **sun8i_layers_init(struct
> drm_device *drm, static const struct sunxi_engine_ops sun8i_engine_ops = {
>  	.commit		= sun8i_mixer_commit,
>  	.layers_init	= sun8i_layers_init,
> +	.atomic_begin	= sun8i_atomic_begin,
>  };
> 
>  static struct regmap_config sun8i_mixer_regmap_config = {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> b/drivers/gpu/drm/sun4i/sun8i_mixer.h index c6cc94057faf..915479cc3077
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -25,6 +25,8 @@
> 
>  #define SUN8I_MIXER_GLOBAL_DBUFF_ENABLE		BIT(0)
> 
> +#define SUN8I_MIXER_GLOBAL_STATUS_BUSY		BIT(4)
> +
>  #define DE2_MIXER_UNIT_SIZE			0x6000
>  #define DE3_MIXER_UNIT_SIZE			0x3000

[RFC 2/4] drm/sun4i: Use CRTC size instead of PRIMARY plane size as mixer frame.

[roman.stratiienko]

From: Roman Stratiienko <roman.stratiienko@globallogic.com>

According to DRM documentation the only difference between PRIMARY
and OVERLAY plane is that each CRTC must have PRIMARY plane and
OVERLAY are optional.

Allow PRIMARY plane to have dimension different from full-screen.

Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
---
 drivers/gpu/drm/sun4i/sun4i_crtc.c     |  4 +++
 drivers/gpu/drm/sun4i/sun8i_mixer.c    | 35 ++++++++++++++++++++++++++
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 30 ----------------------
 drivers/gpu/drm/sun4i/sunxi_engine.h   |  8 ++++++
 4 files changed, 47 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c
index 3a153648b369..156ea8f19d7d 100644
--- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
@@ -139,8 +139,12 @@ static void sun4i_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
 	struct drm_encoder *encoder = sun4i_crtc_get_encoder(crtc);
 	struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc);
+	struct sunxi_engine *engine = scrtc->engine;
 
 	sun4i_tcon_mode_set(scrtc->tcon, encoder, mode);
+
+	if (engine->ops->mode_set)
+		engine->ops->mode_set(engine, mode);
 }
 
 static const struct drm_crtc_helper_funcs sun4i_crtc_helper_funcs = {
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index eea4813602b7..bb9a665fd053 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -257,6 +257,40 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32 format)
 	return NULL;
 }
 
+static void sun8i_mode_set(struct sunxi_engine *engine,
+			   struct drm_display_mode *mode)
+{
+	u32 dst_w = mode->crtc_hdisplay;
+	u32 dst_h = mode->crtc_vdisplay;
+	u32 outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
+	bool interlaced = false;
+	u32 val;
+	struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
+	u32 bld_base = sun8i_blender_base(mixer);
+
+	DRM_DEBUG_DRIVER("Mode change, updating global size W: %u H: %u\n",
+			 dst_w, dst_h);
+	regmap_write(mixer->engine.regs,
+		     SUN8I_MIXER_GLOBAL_SIZE,
+		     outsize);
+	regmap_write(mixer->engine.regs,
+		     SUN8I_MIXER_BLEND_OUTSIZE(bld_base), outsize);
+
+	interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
+
+	if (interlaced)
+		val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
+	else
+		val = 0;
+
+	regmap_update_bits(mixer->engine.regs,
+			   SUN8I_MIXER_BLEND_OUTCTL(bld_base),
+			   SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
+			   val);
+	DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
+			 interlaced ? "on" : "off");
+}
+
 static void sun8i_atomic_begin(struct sunxi_engine *engine,
 			       struct drm_crtc_state *old_state)
 {
@@ -325,6 +359,7 @@ static const struct sunxi_engine_ops sun8i_engine_ops = {
 	.commit		= sun8i_mixer_commit,
 	.layers_init	= sun8i_layers_init,
 	.atomic_begin	= sun8i_atomic_begin,
+	.mode_set	= sun8i_mode_set,
 };
 
 static struct regmap_config sun8i_mixer_regmap_config = {
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index c87fd842918e..893076716070 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -99,36 +99,6 @@ static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel,
 	insize = SUN8I_MIXER_SIZE(src_w, src_h);
 	outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
 
-	if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
-		bool interlaced = false;
-		u32 val;
-
-		DRM_DEBUG_DRIVER("Primary layer, updating global size W: %u H: %u\n",
-				 dst_w, dst_h);
-		regmap_write(mixer->engine.regs,
-			     SUN8I_MIXER_GLOBAL_SIZE,
-			     outsize);
-		regmap_write(mixer->engine.regs,
-			     SUN8I_MIXER_BLEND_OUTSIZE(bld_base), outsize);
-
-		if (state->crtc)
-			interlaced = state->crtc->state->adjusted_mode.flags
-				& DRM_MODE_FLAG_INTERLACE;
-
-		if (interlaced)
-			val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
-		else
-			val = 0;
-
-		regmap_update_bits(mixer->engine.regs,
-				   SUN8I_MIXER_BLEND_OUTCTL(bld_base),
-				   SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
-				   val);
-
-		DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
-				 interlaced ? "on" : "off");
-	}
-
 	/* Set height and width */
 	DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n",
 			 state->src.x1 >> 16, state->src.y1 >> 16);
diff --git a/drivers/gpu/drm/sun4i/sunxi_engine.h b/drivers/gpu/drm/sun4i/sunxi_engine.h
index 548710a936d5..9783c112d512 100644
--- a/drivers/gpu/drm/sun4i/sunxi_engine.h
+++ b/drivers/gpu/drm/sun4i/sunxi_engine.h
@@ -108,6 +108,14 @@ struct sunxi_engine_ops {
 	 * This function is optional.
 	 */
 	void (*vblank_quirk)(struct sunxi_engine *engine);
+
+	/**
+	 * @mode_set:
+	 *
+	 * This function is optional.
+	 */
+	void (*mode_set)(struct sunxi_engine *engine,
+			 struct drm_display_mode *mode);
 };
 
 /**
-- 
2.17.1

Re: [RFC 2/4] drm/sun4i: Use CRTC size instead of PRIMARY plane size as mixer frame.

[Jernej Škrabec]

Hi!

Dne sobota, 28. december 2019 ob 21:28:16 CET je 
roman.stratiienko@globallogic.com napisal(a):
> From: Roman Stratiienko <roman.stratiienko@globallogic.com>
> 
> According to DRM documentation the only difference between PRIMARY
> and OVERLAY plane is that each CRTC must have PRIMARY plane and
> OVERLAY are optional.
> 
> Allow PRIMARY plane to have dimension different from full-screen.

I noticed this issue recently and I'm glad that you posted solution. Code is 
fine, just few nitpicks and I think it would be better to split it in two 
commits, one which adds callback and another which implements that callback in 
sun8i-mixer. DE1 also needs this fix, but it can be posted later.

> 
> Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_crtc.c     |  4 +++
>  drivers/gpu/drm/sun4i/sun8i_mixer.c    | 35 ++++++++++++++++++++++++++
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 30 ----------------------
>  drivers/gpu/drm/sun4i/sunxi_engine.h   |  8 ++++++
>  4 files changed, 47 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c
> b/drivers/gpu/drm/sun4i/sun4i_crtc.c index 3a153648b369..156ea8f19d7d
> 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
> @@ -139,8 +139,12 @@ static void sun4i_crtc_mode_set_nofb(struct drm_crtc
> *crtc) struct drm_display_mode *mode = &crtc->state->adjusted_mode;
>  	struct drm_encoder *encoder = sun4i_crtc_get_encoder(crtc);
>  	struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc);
> +	struct sunxi_engine *engine = scrtc->engine;
> 
>  	sun4i_tcon_mode_set(scrtc->tcon, encoder, mode);
> +
> +	if (engine->ops->mode_set)
> +		engine->ops->mode_set(engine, mode);
>  }
> 
>  static const struct drm_crtc_helper_funcs sun4i_crtc_helper_funcs = {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> b/drivers/gpu/drm/sun4i/sun8i_mixer.c index eea4813602b7..bb9a665fd053
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -257,6 +257,40 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32
> format) return NULL;
>  }
> 
> +static void sun8i_mode_set(struct sunxi_engine *engine,
> +			   struct drm_display_mode *mode)
> +{
> +	u32 dst_w = mode->crtc_hdisplay;
> +	u32 dst_h = mode->crtc_vdisplay;
> +	u32 outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
> +	bool interlaced = false;
> +	u32 val;
> +	struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> +	u32 bld_base = sun8i_blender_base(mixer);
> +
> +	DRM_DEBUG_DRIVER("Mode change, updating global size W: %u H: %u\n",
> +			 dst_w, dst_h);

We should start using newly introduced helpers for DRM debug output, in this 
case drm_dbg(), which replace those in in all caps.

> +	regmap_write(mixer->engine.regs,
> +		     SUN8I_MIXER_GLOBAL_SIZE,
> +		     outsize);
> +	regmap_write(mixer->engine.regs,
> +		     SUN8I_MIXER_BLEND_OUTSIZE(bld_base), outsize);
> +
> +	interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
> +
> +	if (interlaced)
> +		val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
> +	else
> +		val = 0;
> +
> +	regmap_update_bits(mixer->engine.regs,
> +			   SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> +			   SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
> +			   val);
> +	DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
> +			 interlaced ? "on" : "off");

Ditto.

> +}
> +
>  static void sun8i_atomic_begin(struct sunxi_engine *engine,
>  			       struct drm_crtc_state *old_state)
>  {
> @@ -325,6 +359,7 @@ static const struct sunxi_engine_ops sun8i_engine_ops =
> { .commit		= sun8i_mixer_commit,
>  	.layers_init	= sun8i_layers_init,
>  	.atomic_begin	= sun8i_atomic_begin,
> +	.mode_set	= sun8i_mode_set,
>  };
> 
>  static struct regmap_config sun8i_mixer_regmap_config = {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index c87fd842918e..893076716070
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> @@ -99,36 +99,6 @@ static int sun8i_ui_layer_update_coord(struct sun8i_mixer
> *mixer, int channel, insize = SUN8I_MIXER_SIZE(src_w, src_h);
>  	outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
> 
> -	if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> -		bool interlaced = false;
> -		u32 val;
> -
> -		DRM_DEBUG_DRIVER("Primary layer, updating global size 
W: %u H: %u\n",
> -				 dst_w, dst_h);
> -		regmap_write(mixer->engine.regs,
> -			     SUN8I_MIXER_GLOBAL_SIZE,
> -			     outsize);
> -		regmap_write(mixer->engine.regs,
> -			     SUN8I_MIXER_BLEND_OUTSIZE(bld_base), 
outsize);
> -
> -		if (state->crtc)
> -			interlaced = state->crtc->state-
>adjusted_mode.flags
> -				& DRM_MODE_FLAG_INTERLACE;
> -
> -		if (interlaced)
> -			val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
> -		else
> -			val = 0;
> -
> -		regmap_update_bits(mixer->engine.regs,
> -				   
SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> -				   
SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
> -				   val);
> -
> -		DRM_DEBUG_DRIVER("Switching display mixer interlaced 
mode %s\n",
> -				 interlaced ? "on" : "off");
> -	}
> -
>  	/* Set height and width */
>  	DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n",
>  			 state->src.x1 >> 16, state->src.y1 >> 16);
> diff --git a/drivers/gpu/drm/sun4i/sunxi_engine.h
> b/drivers/gpu/drm/sun4i/sunxi_engine.h index 548710a936d5..9783c112d512
> 100644
> --- a/drivers/gpu/drm/sun4i/sunxi_engine.h
> +++ b/drivers/gpu/drm/sun4i/sunxi_engine.h
> @@ -108,6 +108,14 @@ struct sunxi_engine_ops {
>  	 * This function is optional.
>  	 */
>  	void (*vblank_quirk)(struct sunxi_engine *engine);
> +
> +	/**
> +	 * @mode_set:
> +	 *

Please add description.

Best regards,
Jernej

> +	 * This function is optional.
> +	 */
> +	void (*mode_set)(struct sunxi_engine *engine,
> +			 struct drm_display_mode *mode);
>  };
> 
>  /**

[RFC 3/4] drm/sun4i: Reimplement plane z position setting logic

[roman.stratiienko]

From: Roman Stratiienko <roman.stratiienko@globallogic.com>

To set blending channel order register software needs to know state and
position of each channel, which impossible at plane commit stage.

Move this procedure to atomic_flush stage, where all necessary information
is available.

Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.c    | 47 +++++++++++++++++++++++++-
 drivers/gpu/drm/sun4i/sun8i_mixer.h    |  3 ++
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++-------------------
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------
 4 files changed, 60 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index bb9a665fd053..da84fccf7784 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine *engine,
 
 static void sun8i_mixer_commit(struct sunxi_engine *engine)
 {
-	DRM_DEBUG_DRIVER("Committing changes\n");
+	struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
+	u32 base = sun8i_blender_base(mixer);
+	int i, j;
+	int channel_by_zpos[4] = {-1, -1, -1, -1};
+	u32 route = 0, pipe_ctl = 0;
+
+	DRM_DEBUG_DRIVER("Update blender routing\n");
+	for (i = 0; i < 4; i++)	{
+		int zpos = mixer->channel_zpos[i];
+
+		if (zpos >= 0 && zpos < 4)
+			channel_by_zpos[zpos] = i;
+	}
+
+	j = 0;
+	for (i = 0; i < 4; i++)	{
+		int ch = channel_by_zpos[i];
+
+		if (ch >= 0) {
+			pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
+			route |= ch << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
+			j++;
+		}
+	}
+
+	for (i = 0; i < 4 && j < 4; i++) {
+		int zpos = mixer->channel_zpos[i];
 
+		if (zpos < 0) {
+			route |= i << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
+			j++;
+		}
+	}
+
+	regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
+			   SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, pipe_ctl);
+
+	regmap_write(mixer->engine.regs,
+		     SUN8I_MIXER_BLEND_ROUTE(base), route);
+
+	DRM_DEBUG_DRIVER("Committing changes\n");
 	regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
 		     SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
 }
@@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
 	mixer->engine.ops = &sun8i_engine_ops;
 	mixer->engine.node = dev->of_node;
 
+	mixer->channel_zpos[0] = -1;
+	mixer->channel_zpos[1] = -1;
+	mixer->channel_zpos[2] = -1;
+	mixer->channel_zpos[3] = -1;
+	mixer->channel_zpos[4] = -1;
+
 	/*
 	 * While this function can fail, we shouldn't do anything
 	 * if this happens. Some early DE2 DT entries don't provide
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
index 915479cc3077..9c2ff87923d8 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
@@ -178,6 +178,9 @@ struct sun8i_mixer {
 
 	struct clk			*bus_clk;
 	struct clk			*mod_clk;
+
+	/* -1 means that layer is disabled */
+	int channel_zpos[5];
 };
 
 static inline struct sun8i_mixer *
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index 893076716070..23c2f4b68c89 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -24,12 +24,10 @@
 #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)
+				  int overlay, bool enable, unsigned int zpos)
 {
-	u32 val, bld_base, ch_base;
+	u32 val, ch_base;
 
-	bld_base = sun8i_blender_base(mixer);
 	ch_base = sun8i_channel_base(mixer, channel);
 
 	DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
@@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
 			   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);
-	}
+	mixer->channel_zpos[channel] = enable ? zpos : -1;
 }
 
 static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel,
@@ -235,11 +208,9 @@ static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane,
 					  struct drm_plane_state *old_state)
 {
 	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_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);
+	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false, 0);
 }
 
 static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
@@ -247,12 +218,11 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
 {
 	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
 	unsigned int zpos = plane->state->normalized_zpos;
-	unsigned int old_zpos = old_state->normalized_zpos;
 	struct sun8i_mixer *mixer = layer->mixer;
 
 	if (!plane->state->visible) {
 		sun8i_ui_layer_enable(mixer, layer->channel,
-				      layer->overlay, false, 0, old_zpos);
+				      layer->overlay, false, 0);
 		return;
 	}
 
@@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *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);
+			      true, zpos);
 }
 
 static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index 42d445d23773..97cbc98bf781 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -17,8 +17,7 @@
 #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)
+				  int overlay, bool enable, unsigned int zpos)
 {
 	u32 val, bld_base, ch_base;
 
@@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
 			   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);
-	}
+	mixer->channel_zpos[channel] = enable ? zpos : -1;
 }
 
 static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
@@ -350,11 +324,9 @@ static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane,
 					  struct drm_plane_state *old_state)
 {
 	struct sun8i_vi_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);
+	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false, 0);
 }
 
 static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
@@ -362,12 +334,11 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
 {
 	struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
 	unsigned int zpos = plane->state->normalized_zpos;
-	unsigned int old_zpos = old_state->normalized_zpos;
 	struct sun8i_mixer *mixer = layer->mixer;
 
 	if (!plane->state->visible) {
 		sun8i_vi_layer_enable(mixer, layer->channel,
-				      layer->overlay, false, 0, old_zpos);
+				      layer->overlay, false, 0);
 		return;
 	}
 
@@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *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);
+			      true, zpos);
 }
 
 static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
-- 
2.17.1

Re: [RFC 3/4] drm/sun4i: Reimplement plane z position setting logic

[Jernej Škrabec]

Hi!

Dne sobota, 28. december 2019 ob 21:28:17 CET je 
roman.stratiienko@globallogic.com napisal(a):
> From: Roman Stratiienko <roman.stratiienko@globallogic.com>
> 
> To set blending channel order register software needs to know state and
> position of each channel, which impossible at plane commit stage.
> 
> Move this procedure to atomic_flush stage, where all necessary information
> is available.
> 
> Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.c    | 47 +++++++++++++++++++++++++-
>  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  3 ++
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++-------------------
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------
>  4 files changed, 60 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> b/drivers/gpu/drm/sun4i/sun8i_mixer.c index bb9a665fd053..da84fccf7784
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine
> *engine,
> 
>  static void sun8i_mixer_commit(struct sunxi_engine *engine)
>  {
> -	DRM_DEBUG_DRIVER("Committing changes\n");
> +	struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> +	u32 base = sun8i_blender_base(mixer);
> +	int i, j;
> +	int channel_by_zpos[4] = {-1, -1, -1, -1};
> +	u32 route = 0, pipe_ctl = 0;
> +
> +	DRM_DEBUG_DRIVER("Update blender routing\n");

Use drm_dbg().

> +	for (i = 0; i < 4; i++)	{
> +		int zpos = mixer->channel_zpos[i];

channel_zpos can hold 5 elements which is also theoretical maximum for current 
HW design. Why do you check only 4 elements?

It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so everyone 
would understand where this number comes from.

> +
> +		if (zpos >= 0 && zpos < 4)
> +			channel_by_zpos[zpos] = i;
> +	}
> +
> +	j = 0;
> +	for (i = 0; i < 4; i++)	{
> +		int ch = channel_by_zpos[i];
> +
> +		if (ch >= 0) {
> +			pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
> +			route |= ch << 
SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> +			j++;
> +		}
> +	}
> +
> +	for (i = 0; i < 4 && j < 4; i++) {
> +		int zpos = mixer->channel_zpos[i];
> 
> +		if (zpos < 0) {
> +			route |= i << 
SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> +			j++;
> +		}
> +	}
> +
> +	regmap_update_bits(mixer->engine.regs, 
SUN8I_MIXER_BLEND_PIPE_CTL(base),
> +			   SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 
pipe_ctl);
> +
> +	regmap_write(mixer->engine.regs,
> +		     SUN8I_MIXER_BLEND_ROUTE(base), route);
> +
> +	DRM_DEBUG_DRIVER("Committing changes\n");

Use drm_dbg().

>  	regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
>  		     SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
>  }
> @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev, struct
> device *master, mixer->engine.ops = &sun8i_engine_ops;
>  	mixer->engine.node = dev->of_node;
> 
> +	mixer->channel_zpos[0] = -1;
> +	mixer->channel_zpos[1] = -1;
> +	mixer->channel_zpos[2] = -1;
> +	mixer->channel_zpos[3] = -1;
> +	mixer->channel_zpos[4] = -1;
> +

for loop would be better, especially using proposed macro.

Best regards,
Jernej

>  	/*
>  	 * While this function can fail, we shouldn't do anything
>  	 * if this happens. Some early DE2 DT entries don't provide
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 915479cc3077..9c2ff87923d8
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -178,6 +178,9 @@ struct sun8i_mixer {
> 
>  	struct clk			*bus_clk;
>  	struct clk			*mod_clk;
> +
> +	/* -1 means that layer is disabled */
> +	int channel_zpos[5];
>  };
> 
>  static inline struct sun8i_mixer *
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index 893076716070..23c2f4b68c89
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> @@ -24,12 +24,10 @@
>  #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)
> +				  int overlay, bool enable, 
unsigned int zpos)
>  {
> -	u32 val, bld_base, ch_base;
> +	u32 val, ch_base;
> 
> -	bld_base = sun8i_blender_base(mixer);
>  	ch_base = sun8i_channel_base(mixer, channel);
> 
>  	DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
> @@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> *mixer, int channel, 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);
> -	}
> +	mixer->channel_zpos[channel] = enable ? zpos : -1;
>  }
> 
>  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> channel, @@ -235,11 +208,9 @@ static void
> sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct
> drm_plane_state *old_state)
>  {
>  	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_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);
> +	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false, 
0);
>  }
> 
>  static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> @@ -247,12 +218,11 @@ static void sun8i_ui_layer_atomic_update(struct
> drm_plane *plane, {
>  	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
>  	unsigned int zpos = plane->state->normalized_zpos;
> -	unsigned int old_zpos = old_state->normalized_zpos;
>  	struct sun8i_mixer *mixer = layer->mixer;
> 
>  	if (!plane->state->visible) {
>  		sun8i_ui_layer_enable(mixer, layer->channel,
> -				      layer->overlay, false, 0, 
old_zpos);
> +				      layer->overlay, false, 0);
>  		return;
>  	}
> 
> @@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct
> drm_plane *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);
> +			      true, zpos);
>  }
> 
>  static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 42d445d23773..97cbc98bf781
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> @@ -17,8 +17,7 @@
>  #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)
> +				  int overlay, bool enable, 
unsigned int zpos)
>  {
>  	u32 val, bld_base, ch_base;
> 
> @@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> *mixer, int channel, 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);
> -	}
> +	mixer->channel_zpos[channel] = enable ? zpos : -1;
>  }
> 
>  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> channel, @@ -350,11 +324,9 @@ static void
> sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct
> drm_plane_state *old_state)
>  {
>  	struct sun8i_vi_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);
> +	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false, 
0);
>  }
> 
>  static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> @@ -362,12 +334,11 @@ static void sun8i_vi_layer_atomic_update(struct
> drm_plane *plane, {
>  	struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
>  	unsigned int zpos = plane->state->normalized_zpos;
> -	unsigned int old_zpos = old_state->normalized_zpos;
>  	struct sun8i_mixer *mixer = layer->mixer;
> 
>  	if (!plane->state->visible) {
>  		sun8i_vi_layer_enable(mixer, layer->channel,
> -				      layer->overlay, false, 0, 
old_zpos);
> +				      layer->overlay, false, 0);
>  		return;
>  	}
> 
> @@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct
> drm_plane *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);
> +			      true, zpos);
>  }
> 
>  static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {

Re: [RFC 3/4] drm/sun4i: Reimplement plane z position setting logic

[Roman Stratiienko]

Hello Jernej,

Thank you for review.

On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec <jernej.skrabec@siol.net> wrote:
>
> Hi!
>
> Dne sobota, 28. december 2019 ob 21:28:17 CET je
> roman.stratiienko@globallogic.com napisal(a):
> > From: Roman Stratiienko <roman.stratiienko@globallogic.com>
> >
> > To set blending channel order register software needs to know state and
> > position of each channel, which impossible at plane commit stage.
> >
> > Move this procedure to atomic_flush stage, where all necessary information
> > is available.
> >
> > Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_mixer.c    | 47 +++++++++++++++++++++++++-
> >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  3 ++
> >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++-------------------
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------
> >  4 files changed, 60 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index bb9a665fd053..da84fccf7784
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine
> > *engine,
> >
> >  static void sun8i_mixer_commit(struct sunxi_engine *engine)
> >  {
> > -     DRM_DEBUG_DRIVER("Committing changes\n");
> > +     struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > +     u32 base = sun8i_blender_base(mixer);
> > +     int i, j;
> > +     int channel_by_zpos[4] = {-1, -1, -1, -1};
> > +     u32 route = 0, pipe_ctl = 0;
> > +
> > +     DRM_DEBUG_DRIVER("Update blender routing\n");
>
> Use drm_dbg().
>
> > +     for (i = 0; i < 4; i++) {
> > +             int zpos = mixer->channel_zpos[i];
>
> channel_zpos can hold 5 elements which is also theoretical maximum for current
> HW design. Why do you check only 4 elements?
>

I'll use plane_cnt as it done in mixer_bind

> It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so everyone
> would understand where this number comes from.

Will do.

>
> > +
> > +             if (zpos >= 0 && zpos < 4)
> > +                     channel_by_zpos[zpos] = i;
> > +     }
> > +
> > +     j = 0;
> > +     for (i = 0; i < 4; i++) {
> > +             int ch = channel_by_zpos[i];
> > +
> > +             if (ch >= 0) {
> > +                     pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
> > +                     route |= ch <<
> SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > +                     j++;
> > +             }
> > +     }
> > +
> > +     for (i = 0; i < 4 && j < 4; i++) {
> > +             int zpos = mixer->channel_zpos[i];
> >
> > +             if (zpos < 0) {
> > +                     route |= i <<
> SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > +                     j++;
> > +             }
> > +     }
> > +
> > +     regmap_update_bits(mixer->engine.regs,
> SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > +                        SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK,
> pipe_ctl);
> > +
> > +     regmap_write(mixer->engine.regs,
> > +                  SUN8I_MIXER_BLEND_ROUTE(base), route);
> > +
> > +     DRM_DEBUG_DRIVER("Committing changes\n");
>
> Use drm_dbg().

According to https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3beb5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289
,
DRM_DEBUG_DRIVER uses drm_dbg.
Also, using drm_dbg with category macro would require larger indent,
making harder to fit in 80 chars limit.
Are there any plans to deprecate DRM_DEBUG_DRIVER macro?

>
> >       regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
> >                    SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> >  }
> > @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev, struct
> > device *master, mixer->engine.ops = &sun8i_engine_ops;
> >       mixer->engine.node = dev->of_node;
> >
> > +     mixer->channel_zpos[0] = -1;
> > +     mixer->channel_zpos[1] = -1;
> > +     mixer->channel_zpos[2] = -1;
> > +     mixer->channel_zpos[3] = -1;
> > +     mixer->channel_zpos[4] = -1;
> > +
>
> for loop would be better, especially using proposed macro.

I'll put it into already existent for-loop below.

>
> Best regards,
> Jernej
>
> >       /*
> >        * While this function can fail, we shouldn't do anything
> >        * if this happens. Some early DE2 DT entries don't provide
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 915479cc3077..9c2ff87923d8
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > @@ -178,6 +178,9 @@ struct sun8i_mixer {
> >
> >       struct clk                      *bus_clk;
> >       struct clk                      *mod_clk;
> > +
> > +     /* -1 means that layer is disabled */
> > +     int channel_zpos[5];
> >  };
> >
> >  static inline struct sun8i_mixer *
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index 893076716070..23c2f4b68c89
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > @@ -24,12 +24,10 @@
> >  #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)
> > +                               int overlay, bool enable,
> unsigned int zpos)
> >  {
> > -     u32 val, bld_base, ch_base;
> > +     u32 val, ch_base;
> >
> > -     bld_base = sun8i_blender_base(mixer);
> >       ch_base = sun8i_channel_base(mixer, channel);
> >
> >       DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
> > @@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> > *mixer, int channel, 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);
> > -     }
> > +     mixer->channel_zpos[channel] = enable ? zpos : -1;
> >  }
> >
> >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> > channel, @@ -235,11 +208,9 @@ static void
> > sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct
> > drm_plane_state *old_state)
> >  {
> >       struct sun8i_ui_layer *layer = plane_to_sun8i_ui_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);
> > +     sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false,
> 0);
> >  }
> >
> >  static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > @@ -247,12 +218,11 @@ static void sun8i_ui_layer_atomic_update(struct
> > drm_plane *plane, {
> >       struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> >       unsigned int zpos = plane->state->normalized_zpos;
> > -     unsigned int old_zpos = old_state->normalized_zpos;
> >       struct sun8i_mixer *mixer = layer->mixer;
> >
> >       if (!plane->state->visible) {
> >               sun8i_ui_layer_enable(mixer, layer->channel,
> > -                                   layer->overlay, false, 0,
> old_zpos);
> > +                                   layer->overlay, false, 0);
> >               return;
> >       }
> >
> > @@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct
> > drm_plane *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);
> > +                           true, zpos);
> >  }
> >
> >  static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 42d445d23773..97cbc98bf781
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > @@ -17,8 +17,7 @@
> >  #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)
> > +                               int overlay, bool enable,
> unsigned int zpos)
> >  {
> >       u32 val, bld_base, ch_base;
> >
> > @@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> > *mixer, int channel, 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);
> > -     }
> > +     mixer->channel_zpos[channel] = enable ? zpos : -1;
> >  }
> >
> >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> > channel, @@ -350,11 +324,9 @@ static void
> > sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct
> > drm_plane_state *old_state)
> >  {
> >       struct sun8i_vi_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);
> > +     sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false,
> 0);
> >  }
> >
> >  static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > @@ -362,12 +334,11 @@ static void sun8i_vi_layer_atomic_update(struct
> > drm_plane *plane, {
> >       struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> >       unsigned int zpos = plane->state->normalized_zpos;
> > -     unsigned int old_zpos = old_state->normalized_zpos;
> >       struct sun8i_mixer *mixer = layer->mixer;
> >
> >       if (!plane->state->visible) {
> >               sun8i_vi_layer_enable(mixer, layer->channel,
> > -                                   layer->overlay, false, 0,
> old_zpos);
> > +                                   layer->overlay, false, 0);
> >               return;
> >       }
> >
> > @@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct
> > drm_plane *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);
> > +                           true, zpos);
> >  }
> >
> >  static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
>
>
>
>


-- 
Best regards,
Roman Stratiienko
Global Logic Inc.

Re: [RFC 3/4] drm/sun4i: Reimplement plane z position setting logic

[Jernej Škrabec]

Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko 
napisal(a):
> Hello Jernej,
> 
> Thank you for review.
> 
> On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec <jernej.skrabec@siol.net> 
wrote:
> > Hi!
> > 
> > Dne sobota, 28. december 2019 ob 21:28:17 CET je
> > 
> > roman.stratiienko@globallogic.com napisal(a):
> > > From: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > > 
> > > To set blending channel order register software needs to know state and
> > > position of each channel, which impossible at plane commit stage.
> > > 
> > > Move this procedure to atomic_flush stage, where all necessary
> > > information
> > > is available.
> > > 
> > > Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > > ---
> > > 
> > >  drivers/gpu/drm/sun4i/sun8i_mixer.c    | 47 +++++++++++++++++++++++++-
> > >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  3 ++
> > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++-------------------
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------
> > >  4 files changed, 60 insertions(+), 71 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index bb9a665fd053..da84fccf7784
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine
> > > *engine,
> > > 
> > >  static void sun8i_mixer_commit(struct sunxi_engine *engine)
> > >  {
> > > 
> > > -     DRM_DEBUG_DRIVER("Committing changes\n");
> > > +     struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > > +     u32 base = sun8i_blender_base(mixer);
> > > +     int i, j;
> > > +     int channel_by_zpos[4] = {-1, -1, -1, -1};
> > > +     u32 route = 0, pipe_ctl = 0;
> > > +
> > > +     DRM_DEBUG_DRIVER("Update blender routing\n");
> > 
> > Use drm_dbg().
> > 
> > > +     for (i = 0; i < 4; i++) {
> > > +             int zpos = mixer->channel_zpos[i];
> > 
> > channel_zpos can hold 5 elements which is also theoretical maximum for
> > current HW design. Why do you check only 4 elements?
> 
> I'll use plane_cnt as it done in mixer_bind
> 
> > It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so
> > everyone would understand where this number comes from.
> 
> Will do.
> 
> > > +
> > > +             if (zpos >= 0 && zpos < 4)
> > > +                     channel_by_zpos[zpos] = i;
> > > +     }
> > > +
> > > +     j = 0;
> > > +     for (i = 0; i < 4; i++) {
> > > +             int ch = channel_by_zpos[i];
> > > +
> > > +             if (ch >= 0) {
> > > +                     pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
> > > +                     route |= ch <<
> > 
> > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > 
> > > +                     j++;
> > > +             }
> > > +     }
> > > +
> > > +     for (i = 0; i < 4 && j < 4; i++) {
> > > +             int zpos = mixer->channel_zpos[i];
> > > 
> > > +             if (zpos < 0) {
> > > +                     route |= i <<
> > 
> > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > 
> > > +                     j++;
> > > +             }
> > > +     }
> > > +
> > > +     regmap_update_bits(mixer->engine.regs,
> > 
> > SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > 
> > > +                        SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK,
> > 
> > pipe_ctl);
> > 
> > > +
> > > +     regmap_write(mixer->engine.regs,
> > > +                  SUN8I_MIXER_BLEND_ROUTE(base), route);
> > > +
> > > +     DRM_DEBUG_DRIVER("Committing changes\n");
> > 
> > Use drm_dbg().
> 
> According to
> https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3beb
> 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 ,
> DRM_DEBUG_DRIVER uses drm_dbg.
> Also, using drm_dbg with category macro would require larger indent,
> making harder to fit in 80 chars limit.

From what I can see, category is already defined by macro name. Check here: 
https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h#n465

So it should be actually shorter.

> Are there any plans to deprecate DRM_DEBUG_DRIVER macro?

Yes, at least I understand following commit message in such manner:
https://cgit.freedesktop.org/drm/drm-misc/commit/?
id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a

Best regards,
Jernej

> 
> > >       regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
> > >       
> > >                    SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> > >  
> > >  }
> > > 
> > > @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev,
> > > struct
> > > device *master, mixer->engine.ops = &sun8i_engine_ops;
> > > 
> > >       mixer->engine.node = dev->of_node;
> > > 
> > > +     mixer->channel_zpos[0] = -1;
> > > +     mixer->channel_zpos[1] = -1;
> > > +     mixer->channel_zpos[2] = -1;
> > > +     mixer->channel_zpos[3] = -1;
> > > +     mixer->channel_zpos[4] = -1;
> > > +
> > 
> > for loop would be better, especially using proposed macro.
> 
> I'll put it into already existent for-loop below.
> 
> > Best regards,
> > Jernej
> > 
> > >       /*
> > >       
> > >        * While this function can fail, we shouldn't do anything
> > >        * if this happens. Some early DE2 DT entries don't provide
> > > 
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 915479cc3077..9c2ff87923d8
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > @@ -178,6 +178,9 @@ struct sun8i_mixer {
> > > 
> > >       struct clk                      *bus_clk;
> > >       struct clk                      *mod_clk;
> > > 
> > > +
> > > +     /* -1 means that layer is disabled */
> > > +     int channel_zpos[5];
> > > 
> > >  };
> > >  
> > >  static inline struct sun8i_mixer *
> > > 
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index
> > > 893076716070..23c2f4b68c89
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > @@ -24,12 +24,10 @@
> > > 
> > >  #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)
> > > +                               int overlay, bool enable,
> > 
> > unsigned int zpos)
> > 
> > >  {
> > > 
> > > -     u32 val, bld_base, ch_base;
> > > +     u32 val, ch_base;
> > > 
> > > -     bld_base = sun8i_blender_base(mixer);
> > > 
> > >       ch_base = sun8i_channel_base(mixer, channel);
> > >       
> > >       DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
> > > 
> > > @@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> > > *mixer, int channel, 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);
> > > -     }
> > > +     mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > 
> > >  }
> > >  
> > >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> > > 
> > > channel, @@ -235,11 +208,9 @@ static void
> > > sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct
> > > drm_plane_state *old_state)
> > > 
> > >  {
> > >  
> > >       struct sun8i_ui_layer *layer = plane_to_sun8i_ui_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);
> > > +     sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > false,
> > 
> > 0);
> > 
> > >  }
> > >  
> > >  static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > > 
> > > @@ -247,12 +218,11 @@ static void sun8i_ui_layer_atomic_update(struct
> > > drm_plane *plane, {
> > > 
> > >       struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> > >       unsigned int zpos = plane->state->normalized_zpos;
> > > 
> > > -     unsigned int old_zpos = old_state->normalized_zpos;
> > > 
> > >       struct sun8i_mixer *mixer = layer->mixer;
> > >       
> > >       if (!plane->state->visible) {
> > >       
> > >               sun8i_ui_layer_enable(mixer, layer->channel,
> > > 
> > > -                                   layer->overlay, false, 0,
> > 
> > old_zpos);
> > 
> > > +                                   layer->overlay, false, 0);
> > > 
> > >               return;
> > >       
> > >       }
> > > 
> > > @@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct
> > > drm_plane *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);
> > > +                           true, zpos);
> > > 
> > >  }
> > >  
> > >  static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
> > > 
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index
> > > 42d445d23773..97cbc98bf781
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > @@ -17,8 +17,7 @@
> > > 
> > >  #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)
> > > +                               int overlay, bool enable,
> > 
> > unsigned int zpos)
> > 
> > >  {
> > >  
> > >       u32 val, bld_base, ch_base;
> > > 
> > > @@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> > > *mixer, int channel, 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);
> > > -     }
> > > +     mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > 
> > >  }
> > >  
> > >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> > > 
> > > channel, @@ -350,11 +324,9 @@ static void
> > > sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct
> > > drm_plane_state *old_state)
> > > 
> > >  {
> > >  
> > >       struct sun8i_vi_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);
> > > +     sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > false,
> > 
> > 0);
> > 
> > >  }
> > >  
> > >  static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > > 
> > > @@ -362,12 +334,11 @@ static void sun8i_vi_layer_atomic_update(struct
> > > drm_plane *plane, {
> > > 
> > >       struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> > >       unsigned int zpos = plane->state->normalized_zpos;
> > > 
> > > -     unsigned int old_zpos = old_state->normalized_zpos;
> > > 
> > >       struct sun8i_mixer *mixer = layer->mixer;
> > >       
> > >       if (!plane->state->visible) {
> > >       
> > >               sun8i_vi_layer_enable(mixer, layer->channel,
> > > 
> > > -                                   layer->overlay, false, 0,
> > 
> > old_zpos);
> > 
> > > +                                   layer->overlay, false, 0);
> > > 
> > >               return;
> > >       
> > >       }
> > > 
> > > @@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct
> > > drm_plane *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);
> > > +                           true, zpos);
> > > 
> > >  }
> > >  
> > >  static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {

Re: [RFC 3/4] drm/sun4i: Reimplement plane z position setting logic

[Roman Stratiienko]

On Sun, Dec 29, 2019 at 2:18 PM Jernej Škrabec <jernej.skrabec@siol.net> wrote:
>
> Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko
> napisal(a):
> > Hello Jernej,
> >
> > Thank you for review.
> >
> > On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec <jernej.skrabec@siol.net>
> wrote:
> > > Hi!
> > >
> > > Dne sobota, 28. december 2019 ob 21:28:17 CET je
> > >
> > > roman.stratiienko@globallogic.com napisal(a):
> > > > From: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > > >
> > > > To set blending channel order register software needs to know state and
> > > > position of each channel, which impossible at plane commit stage.
> > > >
> > > > Move this procedure to atomic_flush stage, where all necessary
> > > > information
> > > > is available.
> > > >
> > > > Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > > > ---
> > > >
> > > >  drivers/gpu/drm/sun4i/sun8i_mixer.c    | 47 +++++++++++++++++++++++++-
> > > >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  3 ++
> > > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++-------------------
> > > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------
> > > >  4 files changed, 60 insertions(+), 71 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index bb9a665fd053..da84fccf7784
> > > > 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine
> > > > *engine,
> > > >
> > > >  static void sun8i_mixer_commit(struct sunxi_engine *engine)
> > > >  {
> > > >
> > > > -     DRM_DEBUG_DRIVER("Committing changes\n");
> > > > +     struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > > > +     u32 base = sun8i_blender_base(mixer);
> > > > +     int i, j;
> > > > +     int channel_by_zpos[4] = {-1, -1, -1, -1};
> > > > +     u32 route = 0, pipe_ctl = 0;
> > > > +
> > > > +     DRM_DEBUG_DRIVER("Update blender routing\n");
> > >
> > > Use drm_dbg().
> > >
> > > > +     for (i = 0; i < 4; i++) {
> > > > +             int zpos = mixer->channel_zpos[i];
> > >
> > > channel_zpos can hold 5 elements which is also theoretical maximum for
> > > current HW design. Why do you check only 4 elements?
> >
> > I'll use plane_cnt as it done in mixer_bind
> >
> > > It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so
> > > everyone would understand where this number comes from.
> >
> > Will do.
> >
> > > > +
> > > > +             if (zpos >= 0 && zpos < 4)
> > > > +                     channel_by_zpos[zpos] = i;
> > > > +     }
> > > > +
> > > > +     j = 0;
> > > > +     for (i = 0; i < 4; i++) {
> > > > +             int ch = channel_by_zpos[i];
> > > > +
> > > > +             if (ch >= 0) {
> > > > +                     pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
> > > > +                     route |= ch <<
> > >
> > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > >
> > > > +                     j++;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     for (i = 0; i < 4 && j < 4; i++) {
> > > > +             int zpos = mixer->channel_zpos[i];
> > > >
> > > > +             if (zpos < 0) {
> > > > +                     route |= i <<
> > >
> > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > >
> > > > +                     j++;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     regmap_update_bits(mixer->engine.regs,
> > >
> > > SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > >
> > > > +                        SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK,
> > >
> > > pipe_ctl);
> > >
> > > > +
> > > > +     regmap_write(mixer->engine.regs,
> > > > +                  SUN8I_MIXER_BLEND_ROUTE(base), route);
> > > > +
> > > > +     DRM_DEBUG_DRIVER("Committing changes\n");
> > >
> > > Use drm_dbg().
> >
> > According to
> > https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3beb
> > 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 ,
> > DRM_DEBUG_DRIVER uses drm_dbg.
> > Also, using drm_dbg with category macro would require larger indent,
> > making harder to fit in 80 chars limit.
>
> From what I can see, category is already defined by macro name. Check here:
> https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h#n465
>
> So it should be actually shorter.
>

Ah, it something very recent.
drm_dbg also require drm_device struct
Do you know the best way to extract it from `struct engine`?

> > Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
>
> Yes, at least I understand following commit message in such manner:
> https://cgit.freedesktop.org/drm/drm-misc/commit/?
> id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a
>
> Best regards,
> Jernej
>
> >
> > > >       regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
> > > >
> > > >                    SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> > > >
> > > >  }
> > > >
> > > > @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev,
> > > > struct
> > > > device *master, mixer->engine.ops = &sun8i_engine_ops;
> > > >
> > > >       mixer->engine.node = dev->of_node;
> > > >
> > > > +     mixer->channel_zpos[0] = -1;
> > > > +     mixer->channel_zpos[1] = -1;
> > > > +     mixer->channel_zpos[2] = -1;
> > > > +     mixer->channel_zpos[3] = -1;
> > > > +     mixer->channel_zpos[4] = -1;
> > > > +
> > >
> > > for loop would be better, especially using proposed macro.
> >
> > I'll put it into already existent for-loop below.
> >
> > > Best regards,
> > > Jernej
> > >
> > > >       /*
> > > >
> > > >        * While this function can fail, we shouldn't do anything
> > > >        * if this happens. Some early DE2 DT entries don't provide
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 915479cc3077..9c2ff87923d8
> > > > 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > @@ -178,6 +178,9 @@ struct sun8i_mixer {
> > > >
> > > >       struct clk                      *bus_clk;
> > > >       struct clk                      *mod_clk;
> > > >
> > > > +
> > > > +     /* -1 means that layer is disabled */
> > > > +     int channel_zpos[5];
> > > >
> > > >  };
> > > >
> > > >  static inline struct sun8i_mixer *
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index
> > > > 893076716070..23c2f4b68c89
> > > > 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > @@ -24,12 +24,10 @@
> > > >
> > > >  #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)
> > > > +                               int overlay, bool enable,
> > >
> > > unsigned int zpos)
> > >
> > > >  {
> > > >
> > > > -     u32 val, bld_base, ch_base;
> > > > +     u32 val, ch_base;
> > > >
> > > > -     bld_base = sun8i_blender_base(mixer);
> > > >
> > > >       ch_base = sun8i_channel_base(mixer, channel);
> > > >
> > > >       DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
> > > >
> > > > @@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> > > > *mixer, int channel, 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);
> > > > -     }
> > > > +     mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > >
> > > >  }
> > > >
> > > >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> > > >
> > > > channel, @@ -235,11 +208,9 @@ static void
> > > > sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct
> > > > drm_plane_state *old_state)
> > > >
> > > >  {
> > > >
> > > >       struct sun8i_ui_layer *layer = plane_to_sun8i_ui_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);
> > > > +     sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > > false,
> > >
> > > 0);
> > >
> > > >  }
> > > >
> > > >  static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > > >
> > > > @@ -247,12 +218,11 @@ static void sun8i_ui_layer_atomic_update(struct
> > > > drm_plane *plane, {
> > > >
> > > >       struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> > > >       unsigned int zpos = plane->state->normalized_zpos;
> > > >
> > > > -     unsigned int old_zpos = old_state->normalized_zpos;
> > > >
> > > >       struct sun8i_mixer *mixer = layer->mixer;
> > > >
> > > >       if (!plane->state->visible) {
> > > >
> > > >               sun8i_ui_layer_enable(mixer, layer->channel,
> > > >
> > > > -                                   layer->overlay, false, 0,
> > >
> > > old_zpos);
> > >
> > > > +                                   layer->overlay, false, 0);
> > > >
> > > >               return;
> > > >
> > > >       }
> > > >
> > > > @@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct
> > > > drm_plane *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);
> > > > +                           true, zpos);
> > > >
> > > >  }
> > > >
> > > >  static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index
> > > > 42d445d23773..97cbc98bf781
> > > > 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > @@ -17,8 +17,7 @@
> > > >
> > > >  #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)
> > > > +                               int overlay, bool enable,
> > >
> > > unsigned int zpos)
> > >
> > > >  {
> > > >
> > > >       u32 val, bld_base, ch_base;
> > > >
> > > > @@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> > > > *mixer, int channel, 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);
> > > > -     }
> > > > +     mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > >
> > > >  }
> > > >
> > > >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> > > >
> > > > channel, @@ -350,11 +324,9 @@ static void
> > > > sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct
> > > > drm_plane_state *old_state)
> > > >
> > > >  {
> > > >
> > > >       struct sun8i_vi_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);
> > > > +     sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > > false,
> > >
> > > 0);
> > >
> > > >  }
> > > >
> > > >  static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > > >
> > > > @@ -362,12 +334,11 @@ static void sun8i_vi_layer_atomic_update(struct
> > > > drm_plane *plane, {
> > > >
> > > >       struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> > > >       unsigned int zpos = plane->state->normalized_zpos;
> > > >
> > > > -     unsigned int old_zpos = old_state->normalized_zpos;
> > > >
> > > >       struct sun8i_mixer *mixer = layer->mixer;
> > > >
> > > >       if (!plane->state->visible) {
> > > >
> > > >               sun8i_vi_layer_enable(mixer, layer->channel,
> > > >
> > > > -                                   layer->overlay, false, 0,
> > >
> > > old_zpos);
> > >
> > > > +                                   layer->overlay, false, 0);
> > > >
> > > >               return;
> > > >
> > > >       }
> > > >
> > > > @@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct
> > > > drm_plane *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);
> > > > +                           true, zpos);
> > > >
> > > >  }
> > > >
> > > >  static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
>
>
>
>


-- 
Best regards,
Roman Stratiienko
Global Logic Inc.

Re: [RFC 3/4] drm/sun4i: Reimplement plane z position setting logic

[Jernej Škrabec]

Dne nedelja, 29. december 2019 ob 13:47:38 CET je Roman Stratiienko 
napisal(a):
> On Sun, Dec 29, 2019 at 2:18 PM Jernej Škrabec <jernej.skrabec@siol.net> 
wrote:
> > Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko
> > 
> > napisal(a):
> > > Hello Jernej,
> > > 
> > > Thank you for review.
> > > 
> > > On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec
> > > <jernej.skrabec@siol.net>
> > 
> > wrote:
> > > > Hi!
> > > > 
> > > > Dne sobota, 28. december 2019 ob 21:28:17 CET je
> > > > 
> > > > roman.stratiienko@globallogic.com napisal(a):
> > > > > From: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > > > > 
> > > > > To set blending channel order register software needs to know state
> > > > > and
> > > > > position of each channel, which impossible at plane commit stage.
> > > > > 
> > > > > Move this procedure to atomic_flush stage, where all necessary
> > > > > information
> > > > > is available.
> > > > > 
> > > > > Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > > > > ---
> > > > > 
> > > > >  drivers/gpu/drm/sun4i/sun8i_mixer.c    | 47
> > > > >  +++++++++++++++++++++++++-
> > > > >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  3 ++
> > > > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++-------------------
> > > > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------
> > > > >  4 files changed, 60 insertions(+), 71 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index
> > > > > bb9a665fd053..da84fccf7784
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct
> > > > > sunxi_engine
> > > > > *engine,
> > > > > 
> > > > >  static void sun8i_mixer_commit(struct sunxi_engine *engine)
> > > > >  {
> > > > > 
> > > > > -     DRM_DEBUG_DRIVER("Committing changes\n");
> > > > > +     struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > > > > +     u32 base = sun8i_blender_base(mixer);
> > > > > +     int i, j;
> > > > > +     int channel_by_zpos[4] = {-1, -1, -1, -1};
> > > > > +     u32 route = 0, pipe_ctl = 0;
> > > > > +
> > > > > +     DRM_DEBUG_DRIVER("Update blender routing\n");
> > > > 
> > > > Use drm_dbg().
> > > > 
> > > > > +     for (i = 0; i < 4; i++) {
> > > > > +             int zpos = mixer->channel_zpos[i];
> > > > 
> > > > channel_zpos can hold 5 elements which is also theoretical maximum for
> > > > current HW design. Why do you check only 4 elements?
> > > 
> > > I'll use plane_cnt as it done in mixer_bind
> > > 
> > > > It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so
> > > > everyone would understand where this number comes from.
> > > 
> > > Will do.
> > > 
> > > > > +
> > > > > +             if (zpos >= 0 && zpos < 4)
> > > > > +                     channel_by_zpos[zpos] = i;
> > > > > +     }
> > > > > +
> > > > > +     j = 0;
> > > > > +     for (i = 0; i < 4; i++) {
> > > > > +             int ch = channel_by_zpos[i];
> > > > > +
> > > > > +             if (ch >= 0) {
> > > > > +                     pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
> > > > > +                     route |= ch <<
> > > > 
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > > > 
> > > > > +                     j++;
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > > +     for (i = 0; i < 4 && j < 4; i++) {
> > > > > +             int zpos = mixer->channel_zpos[i];
> > > > > 
> > > > > +             if (zpos < 0) {
> > > > > +                     route |= i <<
> > > > 
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > > > 
> > > > > +                     j++;
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > > +     regmap_update_bits(mixer->engine.regs,
> > > > 
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > > > 
> > > > > +                        SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK,
> > > > 
> > > > pipe_ctl);
> > > > 
> > > > > +
> > > > > +     regmap_write(mixer->engine.regs,
> > > > > +                  SUN8I_MIXER_BLEND_ROUTE(base), route);
> > > > > +
> > > > > +     DRM_DEBUG_DRIVER("Committing changes\n");
> > > > 
> > > > Use drm_dbg().
> > > 
> > > According to
> > > https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3
> > > beb
> > > 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 ,
> > > DRM_DEBUG_DRIVER uses drm_dbg.
> > > Also, using drm_dbg with category macro would require larger indent,
> > > making harder to fit in 80 chars limit.
> > 
> > From what I can see, category is already defined by macro name. Check
> > here:
> > https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h#n46
> > 5
> > 
> > So it should be actually shorter.
> 
> Ah, it something very recent.
> drm_dbg also require drm_device struct
> Do you know the best way to extract it from `struct engine`?

I don't think there is a way. I guess we can solve this later. Maxime, any 
thoughts?

Best regards,
Jernej

> 
> > > Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
> > 
> > Yes, at least I understand following commit message in such manner:
> > https://cgit.freedesktop.org/drm/drm-misc/commit/?
> > id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a
> > 
> > Best regards,
> > Jernej
> > 
> > > > >       regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
> > > > >       
> > > > >                    SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> > > > >  
> > > > >  }
> > > > > 
> > > > > @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev,
> > > > > struct
> > > > > device *master, mixer->engine.ops = &sun8i_engine_ops;
> > > > > 
> > > > >       mixer->engine.node = dev->of_node;
> > > > > 
> > > > > +     mixer->channel_zpos[0] = -1;
> > > > > +     mixer->channel_zpos[1] = -1;
> > > > > +     mixer->channel_zpos[2] = -1;
> > > > > +     mixer->channel_zpos[3] = -1;
> > > > > +     mixer->channel_zpos[4] = -1;
> > > > > +
> > > > 
> > > > for loop would be better, especially using proposed macro.
> > > 
> > > I'll put it into already existent for-loop below.
> > > 
> > > > Best regards,
> > > > Jernej
> > > > 
> > > > >       /*
> > > > >       
> > > > >        * While this function can fail, we shouldn't do anything
> > > > >        * if this happens. Some early DE2 DT entries don't provide
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index
> > > > > 915479cc3077..9c2ff87923d8
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > @@ -178,6 +178,9 @@ struct sun8i_mixer {
> > > > > 
> > > > >       struct clk                      *bus_clk;
> > > > >       struct clk                      *mod_clk;
> > > > > 
> > > > > +
> > > > > +     /* -1 means that layer is disabled */
> > > > > +     int channel_zpos[5];
> > > > > 
> > > > >  };
> > > > >  
> > > > >  static inline struct sun8i_mixer *
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index
> > > > > 893076716070..23c2f4b68c89
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > @@ -24,12 +24,10 @@
> > > > > 
> > > > >  #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)
> > > > > +                               int overlay, bool enable,
> > > > 
> > > > unsigned int zpos)
> > > > 
> > > > >  {
> > > > > 
> > > > > -     u32 val, bld_base, ch_base;
> > > > > +     u32 val, ch_base;
> > > > > 
> > > > > -     bld_base = sun8i_blender_base(mixer);
> > > > > 
> > > > >       ch_base = sun8i_channel_base(mixer, channel);
> > > > >       
> > > > >       DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
> > > > > 
> > > > > @@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct
> > > > > sun8i_mixer
> > > > > *mixer, int channel, 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);
> > > > > -     }
> > > > > +     mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > > > 
> > > > >  }
> > > > >  
> > > > >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer,
> > > > >  int
> > > > > 
> > > > > channel, @@ -235,11 +208,9 @@ static void
> > > > > sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct
> > > > > drm_plane_state *old_state)
> > > > > 
> > > > >  {
> > > > >  
> > > > >       struct sun8i_ui_layer *layer = plane_to_sun8i_ui_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);
> > > > > +     sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > > > false,
> > > > 
> > > > 0);
> > > > 
> > > > >  }
> > > > >  
> > > > >  static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > > > > 
> > > > > @@ -247,12 +218,11 @@ static void
> > > > > sun8i_ui_layer_atomic_update(struct
> > > > > drm_plane *plane, {
> > > > > 
> > > > >       struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> > > > >       unsigned int zpos = plane->state->normalized_zpos;
> > > > > 
> > > > > -     unsigned int old_zpos = old_state->normalized_zpos;
> > > > > 
> > > > >       struct sun8i_mixer *mixer = layer->mixer;
> > > > >       
> > > > >       if (!plane->state->visible) {
> > > > >       
> > > > >               sun8i_ui_layer_enable(mixer, layer->channel,
> > > > > 
> > > > > -                                   layer->overlay, false, 0,
> > > > 
> > > > old_zpos);
> > > > 
> > > > > +                                   layer->overlay, false, 0);
> > > > > 
> > > > >               return;
> > > > >       
> > > > >       }
> > > > > 
> > > > > @@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct
> > > > > drm_plane *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);
> > > > > +                           true, zpos);
> > > > > 
> > > > >  }
> > > > >  
> > > > >  static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs =
> > > > >  {
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index
> > > > > 42d445d23773..97cbc98bf781
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > @@ -17,8 +17,7 @@
> > > > > 
> > > > >  #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)
> > > > > +                               int overlay, bool enable,
> > > > 
> > > > unsigned int zpos)
> > > > 
> > > > >  {
> > > > >  
> > > > >       u32 val, bld_base, ch_base;
> > > > > 
> > > > > @@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct
> > > > > sun8i_mixer
> > > > > *mixer, int channel, 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);
> > > > > -     }
> > > > > +     mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > > > 
> > > > >  }
> > > > >  
> > > > >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer,
> > > > >  int
> > > > > 
> > > > > channel, @@ -350,11 +324,9 @@ static void
> > > > > sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct
> > > > > drm_plane_state *old_state)
> > > > > 
> > > > >  {
> > > > >  
> > > > >       struct sun8i_vi_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);
> > > > > +     sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > > > false,
> > > > 
> > > > 0);
> > > > 
> > > > >  }
> > > > >  
> > > > >  static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > > > > 
> > > > > @@ -362,12 +334,11 @@ static void
> > > > > sun8i_vi_layer_atomic_update(struct
> > > > > drm_plane *plane, {
> > > > > 
> > > > >       struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> > > > >       unsigned int zpos = plane->state->normalized_zpos;
> > > > > 
> > > > > -     unsigned int old_zpos = old_state->normalized_zpos;
> > > > > 
> > > > >       struct sun8i_mixer *mixer = layer->mixer;
> > > > >       
> > > > >       if (!plane->state->visible) {
> > > > >       
> > > > >               sun8i_vi_layer_enable(mixer, layer->channel,
> > > > > 
> > > > > -                                   layer->overlay, false, 0,
> > > > 
> > > > old_zpos);
> > > > 
> > > > > +                                   layer->overlay, false, 0);
> > > > > 
> > > > >               return;
> > > > >       
> > > > >       }
> > > > > 
> > > > > @@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct
> > > > > drm_plane *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);
> > > > > +                           true, zpos);
> > > > > 
> > > > >  }
> > > > >  
> > > > >  static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs =
> > > > >  {

Re: [RFC 3/4] drm/sun4i: Reimplement plane z position setting logic

[Roman Stratiienko]

My proposal is to go with DRM_DEBUG_DRIVER for now.
In case stable branches would be interested in these fixes, it will be
easier to backport.

Also I am using v5.4 for testing since Google AOSP patches does not
work correctly with mainline and missing for kernel-next.
Maintaining 2 variants will be much harder for me.

On Sun, Dec 29, 2019 at 3:02 PM Jernej Škrabec <jernej.skrabec@siol.net> wrote:
>
> Dne nedelja, 29. december 2019 ob 13:47:38 CET je Roman Stratiienko
> napisal(a):
> > On Sun, Dec 29, 2019 at 2:18 PM Jernej Škrabec <jernej.skrabec@siol.net>
> wrote:
> > > Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko
> > >
> > > napisal(a):
> > > > Hello Jernej,
> > > >
> > > > Thank you for review.
> > > >
> > > > On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec
> > > > <jernej.skrabec@siol.net>
> > >
> > > wrote:
> > > > > Hi!
> > > > >
> > > > > Dne sobota, 28. december 2019 ob 21:28:17 CET je
> > > > >
> > > > > roman.stratiienko@globallogic.com napisal(a):
> > > > > > From: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > > > > >
> > > > > > To set blending channel order register software needs to know state
> > > > > > and
> > > > > > position of each channel, which impossible at plane commit stage.
> > > > > >
> > > > > > Move this procedure to atomic_flush stage, where all necessary
> > > > > > information
> > > > > > is available.
> > > > > >
> > > > > > Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > > > > > ---
> > > > > >
> > > > > >  drivers/gpu/drm/sun4i/sun8i_mixer.c    | 47
> > > > > >  +++++++++++++++++++++++++-
> > > > > >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  3 ++
> > > > > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++-------------------
> > > > > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------
> > > > > >  4 files changed, 60 insertions(+), 71 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index
> > > > > > bb9a665fd053..da84fccf7784
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > > @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct
> > > > > > sunxi_engine
> > > > > > *engine,
> > > > > >
> > > > > >  static void sun8i_mixer_commit(struct sunxi_engine *engine)
> > > > > >  {
> > > > > >
> > > > > > -     DRM_DEBUG_DRIVER("Committing changes\n");
> > > > > > +     struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > > > > > +     u32 base = sun8i_blender_base(mixer);
> > > > > > +     int i, j;
> > > > > > +     int channel_by_zpos[4] = {-1, -1, -1, -1};
> > > > > > +     u32 route = 0, pipe_ctl = 0;
> > > > > > +
> > > > > > +     DRM_DEBUG_DRIVER("Update blender routing\n");
> > > > >
> > > > > Use drm_dbg().
> > > > >
> > > > > > +     for (i = 0; i < 4; i++) {
> > > > > > +             int zpos = mixer->channel_zpos[i];
> > > > >
> > > > > channel_zpos can hold 5 elements which is also theoretical maximum for
> > > > > current HW design. Why do you check only 4 elements?
> > > >
> > > > I'll use plane_cnt as it done in mixer_bind
> > > >
> > > > > It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so
> > > > > everyone would understand where this number comes from.
> > > >
> > > > Will do.
> > > >
> > > > > > +
> > > > > > +             if (zpos >= 0 && zpos < 4)
> > > > > > +                     channel_by_zpos[zpos] = i;
> > > > > > +     }
> > > > > > +
> > > > > > +     j = 0;
> > > > > > +     for (i = 0; i < 4; i++) {
> > > > > > +             int ch = channel_by_zpos[i];
> > > > > > +
> > > > > > +             if (ch >= 0) {
> > > > > > +                     pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
> > > > > > +                     route |= ch <<
> > > > >
> > > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > > > >
> > > > > > +                     j++;
> > > > > > +             }
> > > > > > +     }
> > > > > > +
> > > > > > +     for (i = 0; i < 4 && j < 4; i++) {
> > > > > > +             int zpos = mixer->channel_zpos[i];
> > > > > >
> > > > > > +             if (zpos < 0) {
> > > > > > +                     route |= i <<
> > > > >
> > > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > > > >
> > > > > > +                     j++;
> > > > > > +             }
> > > > > > +     }
> > > > > > +
> > > > > > +     regmap_update_bits(mixer->engine.regs,
> > > > >
> > > > > SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > > > >
> > > > > > +                        SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK,
> > > > >
> > > > > pipe_ctl);
> > > > >
> > > > > > +
> > > > > > +     regmap_write(mixer->engine.regs,
> > > > > > +                  SUN8I_MIXER_BLEND_ROUTE(base), route);
> > > > > > +
> > > > > > +     DRM_DEBUG_DRIVER("Committing changes\n");
> > > > >
> > > > > Use drm_dbg().
> > > >
> > > > According to
> > > > https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3
> > > > beb
> > > > 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 ,
> > > > DRM_DEBUG_DRIVER uses drm_dbg.
> > > > Also, using drm_dbg with category macro would require larger indent,
> > > > making harder to fit in 80 chars limit.
> > >
> > > From what I can see, category is already defined by macro name. Check
> > > here:
> > > https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h#n46
> > > 5
> > >
> > > So it should be actually shorter.
> >
> > Ah, it something very recent.
> > drm_dbg also require drm_device struct
> > Do you know the best way to extract it from `struct engine`?
>
> I don't think there is a way. I guess we can solve this later. Maxime, any
> thoughts?
>
> Best regards,
> Jernej
>
> >
> > > > Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
> > >
> > > Yes, at least I understand following commit message in such manner:
> > > https://cgit.freedesktop.org/drm/drm-misc/commit/?
> > > id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a
> > >
> > > Best regards,
> > > Jernej
> > >
> > > > > >       regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
> > > > > >
> > > > > >                    SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> > > > > >
> > > > > >  }
> > > > > >
> > > > > > @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev,
> > > > > > struct
> > > > > > device *master, mixer->engine.ops = &sun8i_engine_ops;
> > > > > >
> > > > > >       mixer->engine.node = dev->of_node;
> > > > > >
> > > > > > +     mixer->channel_zpos[0] = -1;
> > > > > > +     mixer->channel_zpos[1] = -1;
> > > > > > +     mixer->channel_zpos[2] = -1;
> > > > > > +     mixer->channel_zpos[3] = -1;
> > > > > > +     mixer->channel_zpos[4] = -1;
> > > > > > +
> > > > >
> > > > > for loop would be better, especially using proposed macro.
> > > >
> > > > I'll put it into already existent for-loop below.
> > > >
> > > > > Best regards,
> > > > > Jernej
> > > > >
> > > > > >       /*
> > > > > >
> > > > > >        * While this function can fail, we shouldn't do anything
> > > > > >        * if this happens. Some early DE2 DT entries don't provide
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index
> > > > > > 915479cc3077..9c2ff87923d8
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > > @@ -178,6 +178,9 @@ struct sun8i_mixer {
> > > > > >
> > > > > >       struct clk                      *bus_clk;
> > > > > >       struct clk                      *mod_clk;
> > > > > >
> > > > > > +
> > > > > > +     /* -1 means that layer is disabled */
> > > > > > +     int channel_zpos[5];
> > > > > >
> > > > > >  };
> > > > > >
> > > > > >  static inline struct sun8i_mixer *
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index
> > > > > > 893076716070..23c2f4b68c89
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > > @@ -24,12 +24,10 @@
> > > > > >
> > > > > >  #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)
> > > > > > +                               int overlay, bool enable,
> > > > >
> > > > > unsigned int zpos)
> > > > >
> > > > > >  {
> > > > > >
> > > > > > -     u32 val, bld_base, ch_base;
> > > > > > +     u32 val, ch_base;
> > > > > >
> > > > > > -     bld_base = sun8i_blender_base(mixer);
> > > > > >
> > > > > >       ch_base = sun8i_channel_base(mixer, channel);
> > > > > >
> > > > > >       DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
> > > > > >
> > > > > > @@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct
> > > > > > sun8i_mixer
> > > > > > *mixer, int channel, 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);
> > > > > > -     }
> > > > > > +     mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > > > >
> > > > > >  }
> > > > > >
> > > > > >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer,
> > > > > >  int
> > > > > >
> > > > > > channel, @@ -235,11 +208,9 @@ static void
> > > > > > sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct
> > > > > > drm_plane_state *old_state)
> > > > > >
> > > > > >  {
> > > > > >
> > > > > >       struct sun8i_ui_layer *layer = plane_to_sun8i_ui_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);
> > > > > > +     sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > > > > false,
> > > > >
> > > > > 0);
> > > > >
> > > > > >  }
> > > > > >
> > > > > >  static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > > > > >
> > > > > > @@ -247,12 +218,11 @@ static void
> > > > > > sun8i_ui_layer_atomic_update(struct
> > > > > > drm_plane *plane, {
> > > > > >
> > > > > >       struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> > > > > >       unsigned int zpos = plane->state->normalized_zpos;
> > > > > >
> > > > > > -     unsigned int old_zpos = old_state->normalized_zpos;
> > > > > >
> > > > > >       struct sun8i_mixer *mixer = layer->mixer;
> > > > > >
> > > > > >       if (!plane->state->visible) {
> > > > > >
> > > > > >               sun8i_ui_layer_enable(mixer, layer->channel,
> > > > > >
> > > > > > -                                   layer->overlay, false, 0,
> > > > >
> > > > > old_zpos);
> > > > >
> > > > > > +                                   layer->overlay, false, 0);
> > > > > >
> > > > > >               return;
> > > > > >
> > > > > >       }
> > > > > >
> > > > > > @@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct
> > > > > > drm_plane *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);
> > > > > > +                           true, zpos);
> > > > > >
> > > > > >  }
> > > > > >
> > > > > >  static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs =
> > > > > >  {
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index
> > > > > > 42d445d23773..97cbc98bf781
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > > @@ -17,8 +17,7 @@
> > > > > >
> > > > > >  #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)
> > > > > > +                               int overlay, bool enable,
> > > > >
> > > > > unsigned int zpos)
> > > > >
> > > > > >  {
> > > > > >
> > > > > >       u32 val, bld_base, ch_base;
> > > > > >
> > > > > > @@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct
> > > > > > sun8i_mixer
> > > > > > *mixer, int channel, 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);
> > > > > > -     }
> > > > > > +     mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > > > >
> > > > > >  }
> > > > > >
> > > > > >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer,
> > > > > >  int
> > > > > >
> > > > > > channel, @@ -350,11 +324,9 @@ static void
> > > > > > sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct
> > > > > > drm_plane_state *old_state)
> > > > > >
> > > > > >  {
> > > > > >
> > > > > >       struct sun8i_vi_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);
> > > > > > +     sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > > > > false,
> > > > >
> > > > > 0);
> > > > >
> > > > > >  }
> > > > > >
> > > > > >  static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > > > > >
> > > > > > @@ -362,12 +334,11 @@ static void
> > > > > > sun8i_vi_layer_atomic_update(struct
> > > > > > drm_plane *plane, {
> > > > > >
> > > > > >       struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> > > > > >       unsigned int zpos = plane->state->normalized_zpos;
> > > > > >
> > > > > > -     unsigned int old_zpos = old_state->normalized_zpos;
> > > > > >
> > > > > >       struct sun8i_mixer *mixer = layer->mixer;
> > > > > >
> > > > > >       if (!plane->state->visible) {
> > > > > >
> > > > > >               sun8i_vi_layer_enable(mixer, layer->channel,
> > > > > >
> > > > > > -                                   layer->overlay, false, 0,
> > > > >
> > > > > old_zpos);
> > > > >
> > > > > > +                                   layer->overlay, false, 0);
> > > > > >
> > > > > >               return;
> > > > > >
> > > > > >       }
> > > > > >
> > > > > > @@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct
> > > > > > drm_plane *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);
> > > > > > +                           true, zpos);
> > > > > >
> > > > > >  }
> > > > > >
> > > > > >  static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs =
> > > > > >  {
>
>
>
>


-- 
Best regards,
Roman Stratiienko
Global Logic Inc.

Re: [RFC 3/4] drm/sun4i: Reimplement plane z position setting logic

[Jernej Škrabec]

Dne nedelja, 29. december 2019 ob 14:13:06 CET je Roman Stratiienko 
napisal(a):
> My proposal is to go with DRM_DEBUG_DRIVER for now.
> In case stable branches would be interested in these fixes, it will be
> easier to backport.

Fair enough, but you need to add at least Fixes tag.

Best regards,
Jernej

> 
> Also I am using v5.4 for testing since Google AOSP patches does not
> work correctly with mainline and missing for kernel-next.
> Maintaining 2 variants will be much harder for me.
> 
> On Sun, Dec 29, 2019 at 3:02 PM Jernej Škrabec <jernej.skrabec@siol.net> 
wrote:
> > Dne nedelja, 29. december 2019 ob 13:47:38 CET je Roman Stratiienko
> > 
> > napisal(a):
> > > On Sun, Dec 29, 2019 at 2:18 PM Jernej Škrabec <jernej.skrabec@siol.net>
> > 
> > wrote:
> > > > Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko
> > > > 
> > > > napisal(a):
> > > > > Hello Jernej,
> > > > > 
> > > > > Thank you for review.
> > > > > 
> > > > > On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec
> > > > > <jernej.skrabec@siol.net>
> > > > 
> > > > wrote:
> > > > > > Hi!
> > > > > > 
> > > > > > Dne sobota, 28. december 2019 ob 21:28:17 CET je
> > > > > > 
> > > > > > roman.stratiienko@globallogic.com napisal(a):
> > > > > > > From: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > > > > > > 
> > > > > > > To set blending channel order register software needs to know
> > > > > > > state
> > > > > > > and
> > > > > > > position of each channel, which impossible at plane commit
> > > > > > > stage.
> > > > > > > 
> > > > > > > Move this procedure to atomic_flush stage, where all necessary
> > > > > > > information
> > > > > > > is available.
> > > > > > > 
> > > > > > > Signed-off-by: Roman Stratiienko
> > > > > > > <roman.stratiienko@globallogic.com>
> > > > > > > ---
> > > > > > > 
> > > > > > >  drivers/gpu/drm/sun4i/sun8i_mixer.c    | 47
> > > > > > >  +++++++++++++++++++++++++-
> > > > > > >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  3 ++
> > > > > > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42
> > > > > > >  ++++-------------------
> > > > > > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39
> > > > > > >  +++------------------
> > > > > > >  4 files changed, 60 insertions(+), 71 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index
> > > > > > > bb9a665fd053..da84fccf7784
> > > > > > > 100644
> > > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > > > @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct
> > > > > > > sunxi_engine
> > > > > > > *engine,
> > > > > > > 
> > > > > > >  static void sun8i_mixer_commit(struct sunxi_engine *engine)
> > > > > > >  {
> > > > > > > 
> > > > > > > -     DRM_DEBUG_DRIVER("Committing changes\n");
> > > > > > > +     struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > > > > > > +     u32 base = sun8i_blender_base(mixer);
> > > > > > > +     int i, j;
> > > > > > > +     int channel_by_zpos[4] = {-1, -1, -1, -1};
> > > > > > > +     u32 route = 0, pipe_ctl = 0;
> > > > > > > +
> > > > > > > +     DRM_DEBUG_DRIVER("Update blender routing\n");
> > > > > > 
> > > > > > Use drm_dbg().
> > > > > > 
> > > > > > > +     for (i = 0; i < 4; i++) {
> > > > > > > +             int zpos = mixer->channel_zpos[i];
> > > > > > 
> > > > > > channel_zpos can hold 5 elements which is also theoretical maximum
> > > > > > for
> > > > > > current HW design. Why do you check only 4 elements?
> > > > > 
> > > > > I'll use plane_cnt as it done in mixer_bind
> > > > > 
> > > > > > It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS
> > > > > > so
> > > > > > everyone would understand where this number comes from.
> > > > > 
> > > > > Will do.
> > > > > 
> > > > > > > +
> > > > > > > +             if (zpos >= 0 && zpos < 4)
> > > > > > > +                     channel_by_zpos[zpos] = i;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     j = 0;
> > > > > > > +     for (i = 0; i < 4; i++) {
> > > > > > > +             int ch = channel_by_zpos[i];
> > > > > > > +
> > > > > > > +             if (ch >= 0) {
> > > > > > > +                     pipe_ctl |=
> > > > > > > SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
> > > > > > > +                     route |= ch <<
> > > > > > 
> > > > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > > > > > 
> > > > > > > +                     j++;
> > > > > > > +             }
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     for (i = 0; i < 4 && j < 4; i++) {
> > > > > > > +             int zpos = mixer->channel_zpos[i];
> > > > > > > 
> > > > > > > +             if (zpos < 0) {
> > > > > > > +                     route |= i <<
> > > > > > 
> > > > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > > > > > 
> > > > > > > +                     j++;
> > > > > > > +             }
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     regmap_update_bits(mixer->engine.regs,
> > > > > > 
> > > > > > SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > > > > > 
> > > > > > > +                        SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK,
> > > > > > 
> > > > > > pipe_ctl);
> > > > > > 
> > > > > > > +
> > > > > > > +     regmap_write(mixer->engine.regs,
> > > > > > > +                  SUN8I_MIXER_BLEND_ROUTE(base), route);
> > > > > > > +
> > > > > > > +     DRM_DEBUG_DRIVER("Committing changes\n");
> > > > > > 
> > > > > > Use drm_dbg().
> > > > > 
> > > > > According to
> > > > > https://github.com/torvalds/linux/commit/99a954874e7b9f0c80584765755
> > > > > 93b3
> > > > > beb
> > > > > 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 ,
> > > > > DRM_DEBUG_DRIVER uses drm_dbg.
> > > > > Also, using drm_dbg with category macro would require larger indent,
> > > > > making harder to fit in 80 chars limit.
> > > > 
> > > > From what I can see, category is already defined by macro name. Check
> > > > here:
> > > > https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h
> > > > #n46
> > > > 5
> > > > 
> > > > So it should be actually shorter.
> > > 
> > > Ah, it something very recent.
> > > drm_dbg also require drm_device struct
> > > Do you know the best way to extract it from `struct engine`?
> > 
> > I don't think there is a way. I guess we can solve this later. Maxime, any
> > thoughts?
> > 
> > Best regards,
> > Jernej
> > 
> > > > > Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
> > > > 
> > > > Yes, at least I understand following commit message in such manner:
> > > > https://cgit.freedesktop.org/drm/drm-misc/commit/?
> > > > id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a
> > > > 
> > > > Best regards,
> > > > Jernej
> > > > 
> > > > > > >       regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
> > > > > > >       
> > > > > > >                    SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> > > > > > >  
> > > > > > >  }
> > > > > > > 
> > > > > > > @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device
> > > > > > > *dev,
> > > > > > > struct
> > > > > > > device *master, mixer->engine.ops = &sun8i_engine_ops;
> > > > > > > 
> > > > > > >       mixer->engine.node = dev->of_node;
> > > > > > > 
> > > > > > > +     mixer->channel_zpos[0] = -1;
> > > > > > > +     mixer->channel_zpos[1] = -1;
> > > > > > > +     mixer->channel_zpos[2] = -1;
> > > > > > > +     mixer->channel_zpos[3] = -1;
> > > > > > > +     mixer->channel_zpos[4] = -1;
> > > > > > > +
> > > > > > 
> > > > > > for loop would be better, especially using proposed macro.
> > > > > 
> > > > > I'll put it into already existent for-loop below.
> > > > > 
> > > > > > Best regards,
> > > > > > Jernej
> > > > > > 
> > > > > > >       /*
> > > > > > >       
> > > > > > >        * While this function can fail, we shouldn't do anything
> > > > > > >        * if this happens. Some early DE2 DT entries don't
> > > > > > >        provide
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index
> > > > > > > 915479cc3077..9c2ff87923d8
> > > > > > > 100644
> > > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > > > @@ -178,6 +178,9 @@ struct sun8i_mixer {
> > > > > > > 
> > > > > > >       struct clk                      *bus_clk;
> > > > > > >       struct clk                      *mod_clk;
> > > > > > > 
> > > > > > > +
> > > > > > > +     /* -1 means that layer is disabled */
> > > > > > > +     int channel_zpos[5];
> > > > > > > 
> > > > > > >  };
> > > > > > >  
> > > > > > >  static inline struct sun8i_mixer *
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index
> > > > > > > 893076716070..23c2f4b68c89
> > > > > > > 100644
> > > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > > > @@ -24,12 +24,10 @@
> > > > > > > 
> > > > > > >  #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)
> > > > > > > +                               int overlay, bool enable,
> > > > > > 
> > > > > > unsigned int zpos)
> > > > > > 
> > > > > > >  {
> > > > > > > 
> > > > > > > -     u32 val, bld_base, ch_base;
> > > > > > > +     u32 val, ch_base;
> > > > > > > 
> > > > > > > -     bld_base = sun8i_blender_base(mixer);
> > > > > > > 
> > > > > > >       ch_base = sun8i_channel_base(mixer, channel);
> > > > > > >       
> > > > > > >       DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
> > > > > > > 
> > > > > > > @@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct
> > > > > > > sun8i_mixer
> > > > > > > *mixer, int channel, 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);
> > > > > > > -     }
> > > > > > > +     mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > > > > > 
> > > > > > >  }
> > > > > > >  
> > > > > > >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer
> > > > > > >  *mixer,
> > > > > > >  int
> > > > > > > 
> > > > > > > channel, @@ -235,11 +208,9 @@ static void
> > > > > > > sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct
> > > > > > > drm_plane_state *old_state)
> > > > > > > 
> > > > > > >  {
> > > > > > >  
> > > > > > >       struct sun8i_ui_layer *layer =
> > > > > > >       plane_to_sun8i_ui_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);
> > > > > > > +     sun8i_ui_layer_enable(mixer, layer->channel,
> > > > > > > layer->overlay,
> > > > > > > false,
> > > > > > 
> > > > > > 0);
> > > > > > 
> > > > > > >  }
> > > > > > >  
> > > > > > >  static void sun8i_ui_layer_atomic_update(struct drm_plane
> > > > > > >  *plane,
> > > > > > > 
> > > > > > > @@ -247,12 +218,11 @@ static void
> > > > > > > sun8i_ui_layer_atomic_update(struct
> > > > > > > drm_plane *plane, {
> > > > > > > 
> > > > > > >       struct sun8i_ui_layer *layer =
> > > > > > >       plane_to_sun8i_ui_layer(plane);
> > > > > > >       unsigned int zpos = plane->state->normalized_zpos;
> > > > > > > 
> > > > > > > -     unsigned int old_zpos = old_state->normalized_zpos;
> > > > > > > 
> > > > > > >       struct sun8i_mixer *mixer = layer->mixer;
> > > > > > >       
> > > > > > >       if (!plane->state->visible) {
> > > > > > >       
> > > > > > >               sun8i_ui_layer_enable(mixer, layer->channel,
> > > > > > > 
> > > > > > > -                                   layer->overlay, false, 0,
> > > > > > 
> > > > > > old_zpos);
> > > > > > 
> > > > > > > +                                   layer->overlay, false, 0);
> > > > > > > 
> > > > > > >               return;
> > > > > > >       
> > > > > > >       }
> > > > > > > 
> > > > > > > @@ -263,7 +233,7 @@ static void
> > > > > > > sun8i_ui_layer_atomic_update(struct
> > > > > > > drm_plane *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);
> > > > > > > +                           true, zpos);
> > > > > > > 
> > > > > > >  }
> > > > > > >  
> > > > > > >  static struct drm_plane_helper_funcs
> > > > > > >  sun8i_ui_layer_helper_funcs =
> > > > > > >  {
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index
> > > > > > > 42d445d23773..97cbc98bf781
> > > > > > > 100644
> > > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > > > @@ -17,8 +17,7 @@
> > > > > > > 
> > > > > > >  #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)
> > > > > > > +                               int overlay, bool enable,
> > > > > > 
> > > > > > unsigned int zpos)
> > > > > > 
> > > > > > >  {
> > > > > > >  
> > > > > > >       u32 val, bld_base, ch_base;
> > > > > > > 
> > > > > > > @@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct
> > > > > > > sun8i_mixer
> > > > > > > *mixer, int channel, 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);
> > > > > > > -     }
> > > > > > > +     mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > > > > > 
> > > > > > >  }
> > > > > > >  
> > > > > > >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer
> > > > > > >  *mixer,
> > > > > > >  int
> > > > > > > 
> > > > > > > channel, @@ -350,11 +324,9 @@ static void
> > > > > > > sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct
> > > > > > > drm_plane_state *old_state)
> > > > > > > 
> > > > > > >  {
> > > > > > >  
> > > > > > >       struct sun8i_vi_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);
> > > > > > > +     sun8i_vi_layer_enable(mixer, layer->channel,
> > > > > > > layer->overlay,
> > > > > > > false,
> > > > > > 
> > > > > > 0);
> > > > > > 
> > > > > > >  }
> > > > > > >  
> > > > > > >  static void sun8i_vi_layer_atomic_update(struct drm_plane
> > > > > > >  *plane,
> > > > > > > 
> > > > > > > @@ -362,12 +334,11 @@ static void
> > > > > > > sun8i_vi_layer_atomic_update(struct
> > > > > > > drm_plane *plane, {
> > > > > > > 
> > > > > > >       struct sun8i_vi_layer *layer =
> > > > > > >       plane_to_sun8i_vi_layer(plane);
> > > > > > >       unsigned int zpos = plane->state->normalized_zpos;
> > > > > > > 
> > > > > > > -     unsigned int old_zpos = old_state->normalized_zpos;
> > > > > > > 
> > > > > > >       struct sun8i_mixer *mixer = layer->mixer;
> > > > > > >       
> > > > > > >       if (!plane->state->visible) {
> > > > > > >       
> > > > > > >               sun8i_vi_layer_enable(mixer, layer->channel,
> > > > > > > 
> > > > > > > -                                   layer->overlay, false, 0,
> > > > > > 
> > > > > > old_zpos);
> > > > > > 
> > > > > > > +                                   layer->overlay, false, 0);
> > > > > > > 
> > > > > > >               return;
> > > > > > >       
> > > > > > >       }
> > > > > > > 
> > > > > > > @@ -378,7 +349,7 @@ static void
> > > > > > > sun8i_vi_layer_atomic_update(struct
> > > > > > > drm_plane *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);
> > > > > > > +                           true, zpos);
> > > > > > > 
> > > > > > >  }
> > > > > > >  
> > > > > > >  static struct drm_plane_helper_funcs
> > > > > > >  sun8i_vi_layer_helper_funcs =
> > > > > > >  {

Re: [RFC 3/4] drm/sun4i: Reimplement plane z position setting logic

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5594 bytes --]

On Sun, Dec 29, 2019 at 02:02:33PM +0100, Jernej Škrabec wrote:
> Dne nedelja, 29. december 2019 ob 13:47:38 CET je Roman Stratiienko
> napisal(a):
> > On Sun, Dec 29, 2019 at 2:18 PM Jernej Škrabec <jernej.skrabec@siol.net>
> wrote:
> > > Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko
> > >
> > > napisal(a):
> > > > Hello Jernej,
> > > >
> > > > Thank you for review.
> > > >
> > > > On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec
> > > > <jernej.skrabec@siol.net>
> > >
> > > wrote:
> > > > > Hi!
> > > > >
> > > > > Dne sobota, 28. december 2019 ob 21:28:17 CET je
> > > > >
> > > > > roman.stratiienko@globallogic.com napisal(a):
> > > > > > From: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > > > > >
> > > > > > To set blending channel order register software needs to know state
> > > > > > and
> > > > > > position of each channel, which impossible at plane commit stage.
> > > > > >
> > > > > > Move this procedure to atomic_flush stage, where all necessary
> > > > > > information
> > > > > > is available.
> > > > > >
> > > > > > Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > > > > > ---
> > > > > >
> > > > > >  drivers/gpu/drm/sun4i/sun8i_mixer.c    | 47
> > > > > >  +++++++++++++++++++++++++-
> > > > > >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  3 ++
> > > > > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++-------------------
> > > > > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------
> > > > > >  4 files changed, 60 insertions(+), 71 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index
> > > > > > bb9a665fd053..da84fccf7784
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > > @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct
> > > > > > sunxi_engine
> > > > > > *engine,
> > > > > >
> > > > > >  static void sun8i_mixer_commit(struct sunxi_engine *engine)
> > > > > >  {
> > > > > >
> > > > > > -     DRM_DEBUG_DRIVER("Committing changes\n");
> > > > > > +     struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > > > > > +     u32 base = sun8i_blender_base(mixer);
> > > > > > +     int i, j;
> > > > > > +     int channel_by_zpos[4] = {-1, -1, -1, -1};
> > > > > > +     u32 route = 0, pipe_ctl = 0;
> > > > > > +
> > > > > > +     DRM_DEBUG_DRIVER("Update blender routing\n");
> > > > >
> > > > > Use drm_dbg().
> > > > >
> > > > > > +     for (i = 0; i < 4; i++) {
> > > > > > +             int zpos = mixer->channel_zpos[i];
> > > > >
> > > > > channel_zpos can hold 5 elements which is also theoretical maximum for
> > > > > current HW design. Why do you check only 4 elements?
> > > >
> > > > I'll use plane_cnt as it done in mixer_bind
> > > >
> > > > > It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so
> > > > > everyone would understand where this number comes from.
> > > >
> > > > Will do.
> > > >
> > > > > > +
> > > > > > +             if (zpos >= 0 && zpos < 4)
> > > > > > +                     channel_by_zpos[zpos] = i;
> > > > > > +     }
> > > > > > +
> > > > > > +     j = 0;
> > > > > > +     for (i = 0; i < 4; i++) {
> > > > > > +             int ch = channel_by_zpos[i];
> > > > > > +
> > > > > > +             if (ch >= 0) {
> > > > > > +                     pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
> > > > > > +                     route |= ch <<
> > > > >
> > > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > > > >
> > > > > > +                     j++;
> > > > > > +             }
> > > > > > +     }
> > > > > > +
> > > > > > +     for (i = 0; i < 4 && j < 4; i++) {
> > > > > > +             int zpos = mixer->channel_zpos[i];
> > > > > >
> > > > > > +             if (zpos < 0) {
> > > > > > +                     route |= i <<
> > > > >
> > > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > > > >
> > > > > > +                     j++;
> > > > > > +             }
> > > > > > +     }
> > > > > > +
> > > > > > +     regmap_update_bits(mixer->engine.regs,
> > > > >
> > > > > SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > > > >
> > > > > > +                        SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK,
> > > > >
> > > > > pipe_ctl);
> > > > >
> > > > > > +
> > > > > > +     regmap_write(mixer->engine.regs,
> > > > > > +                  SUN8I_MIXER_BLEND_ROUTE(base), route);
> > > > > > +
> > > > > > +     DRM_DEBUG_DRIVER("Committing changes\n");
> > > > >
> > > > > Use drm_dbg().
> > > >
> > > > According to
> > > > https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3
> > > > beb
> > > > 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 ,
> > > > DRM_DEBUG_DRIVER uses drm_dbg.
> > > > Also, using drm_dbg with category macro would require larger indent,
> > > > making harder to fit in 80 chars limit.
> > >
> > > From what I can see, category is already defined by macro name. Check
> > > here:
> > > https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h#n46
> > > 5
> > >
> > > So it should be actually shorter.
> >
> > Ah, it something very recent.
> > drm_dbg also require drm_device struct
> > Do you know the best way to extract it from `struct engine`?
>
> I don't think there is a way. I guess we can solve this later. Maxime, any
> thoughts?

There's no way at the moment, but it would make sense to add a pointer
to it.

Maximey

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

[RFC 4/4] drm/sun4i: Update mixer's internal registers after initialization

[roman.stratiienko]

From: Roman Stratiienko <roman.stratiienko@globallogic.com>

At system start blink of u-boot ghost framebuffer can be observed.
Fix it.

Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index da84fccf7784..b906b8cc464e 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -588,6 +588,9 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
 	regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
 			   SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
 
+	regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_DBUFF,
+		     SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
+
 	return 0;
 
 err_disable_bus_clk:
-- 
2.17.1

Re: [RFC 4/4] drm/sun4i: Update mixer's internal registers after initialization

[Jernej Škrabec]

Hi!

Dne sobota, 28. december 2019 ob 21:28:18 CET je 
roman.stratiienko@globallogic.com napisal(a):
> From: Roman Stratiienko <roman.stratiienko@globallogic.com>
> 
> At system start blink of u-boot ghost framebuffer can be observed.
> Fix it.

Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>

Please note that U-Boot to Linux handover may not be without issues. I noticed 
that in some cases, HDMI EDID readout is broken. I guess already initialized 
HDMI controller doesn't like to be reinitialized. Or it may have something to 
do with the fact that U-Boot HDMI driver sets I2C timing and Linux kernel uses 
default one.

Best regards,
Jernej

> 
> Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> b/drivers/gpu/drm/sun4i/sun8i_mixer.c index da84fccf7784..b906b8cc464e
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -588,6 +588,9 @@ static int sun8i_mixer_bind(struct device *dev, struct
> device *master, regmap_update_bits(mixer->engine.regs,
> SUN8I_MIXER_BLEND_PIPE_CTL(base), SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
> 
> +	regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_DBUFF,
> +		     SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> +
>  	return 0;
> 
>  err_disable_bus_clk: