[PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

[PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

[megous]

From: Ondrej Jirman <megous@megous.com>

There are various issues that this re-work of sun8i_[uv]i_layer_enable
function fixes:

- Make sure that we re-initialize zpos on reset
- Minimize register updates by doing them only when state changes
- Fix issue where DE pipe might get disabled even if it is no longer
  used by the layer that's currently calling sun8i_ui_layer_enable
- .atomic_disable callback is not really needed because .atomic_update
  can do the disable too, so drop the duplicate code

Signed-off-by: Ondrej Jirman <megous@megous.com>
---
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 112 ++++++++++++++++---------
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 112 ++++++++++++++++---------
 2 files changed, 142 insertions(+), 82 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index dd2a1c851939..b88e8ac5ad1c 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -24,10 +24,11 @@
 #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 was_enabled, bool enable,
+				  unsigned int zpos, unsigned int old_zpos)
 {
 	u32 val, bld_base, ch_base;
+	unsigned int old_pipe_ch;
 
 	bld_base = sun8i_blender_base(mixer);
 	ch_base = sun8i_channel_base(mixer, channel);
@@ -35,28 +36,57 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
 	DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
 			 enable ? "En" : "Dis", channel, overlay);
 
-	if (enable)
-		val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN;
-	else
-		val = 0;
+	if (!was_enabled != !enable) {
+		val = enable ? SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN : 0;
 
-	regmap_update_bits(mixer->engine.regs,
-			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
-			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
-
-	if (!enable || zpos != old_zpos) {
 		regmap_update_bits(mixer->engine.regs,
-				   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
-				   SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
-				   0);
+				   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
+				   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
+	}
 
-		regmap_update_bits(mixer->engine.regs,
+	/*
+	 * If this layer was enabled and is being disabled or if it is
+	 * enabled and just changing zpos, clear the old route, if it is
+	 * still configured to this layer in HW.
+	 */
+	if ((was_enabled && !enable) || (enable && zpos != old_zpos)) {
+		/* get channel the pipe for old_zpos is routed to from the HW */
+		regmap_read(mixer->engine.regs,
 				   SUN8I_MIXER_BLEND_ROUTE(bld_base),
-				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
-				   0);
+				   &old_pipe_ch);
+		old_pipe_ch &= SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos);
+		old_pipe_ch >>= SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(old_zpos);
+
+		/*
+		 * Check that pipe for old_zpos is still routed to our layer,
+		 * and clear/disable it if it is.
+		 */
+
+		if (old_pipe_ch == channel) {
+			DRM_DEBUG_DRIVER("chan=%d en=%d->%d zpos=%d->%d\n",
+			       channel, was_enabled, enable, old_zpos, zpos);
+
+			DRM_DEBUG_DRIVER("  disable pipe %d\n", old_zpos);
+
+			regmap_update_bits(mixer->engine.regs,
+					   SUN8I_MIXER_BLEND_ROUTE(bld_base),
+					   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
+					   0);
+
+			regmap_update_bits(mixer->engine.regs,
+					   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
+					   SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
+					   0);
+		}
 	}
 
-	if (enable) {
+	/*
+	 * If enabling this layer or changin zpos, set route to this layer.
+	 */
+	if ((enable && !was_enabled) || (enable && zpos != old_zpos)) {
+		DRM_DEBUG_DRIVER("chan=%d en=%d->%d zpos=%d->%d\n",
+		       channel, was_enabled, enable, old_zpos, zpos);
+
 		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
 
 		regmap_update_bits(mixer->engine.regs,
@@ -69,6 +99,8 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
 				   SUN8I_MIXER_BLEND_ROUTE(bld_base),
 				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
 				   val);
+
+		DRM_DEBUG_DRIVER("  enable pipe %d <- ch %d\n", zpos, channel);
 	}
 }
 
@@ -261,45 +293,43 @@ static int sun8i_ui_layer_atomic_check(struct drm_plane *plane,
 						   true, true);
 }
 
-static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane,
-					  struct drm_plane_state *old_state)
+static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
+					 struct drm_plane_state *old_state)
 {
 	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;
+	bool was_enabled = old_state->crtc && old_state->visible;
+	bool enable = plane->state->crtc && plane->state->visible;
 
-	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false, 0,
-			      old_zpos);
+	if (enable) {
+		sun8i_ui_layer_update_coord(mixer, layer->channel,
+					    layer->overlay, plane, zpos);
+		sun8i_ui_layer_update_formats(mixer, layer->channel,
+					      layer->overlay, plane);
+		sun8i_ui_layer_update_buffer(mixer, layer->channel,
+					     layer->overlay, plane);
+	}
+
+	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
+			      was_enabled, enable, zpos, old_zpos);
 }
 
-static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
-					 struct drm_plane_state *old_state)
+void sun8i_ui_layer_plane_reset(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);
+	drm_atomic_helper_plane_reset(plane);
+	if (!plane->state)
 		return;
-	}
 
-	sun8i_ui_layer_update_coord(mixer, layer->channel,
-				    layer->overlay, plane, zpos);
-	sun8i_ui_layer_update_formats(mixer, layer->channel,
-				      layer->overlay, plane);
-	sun8i_ui_layer_update_buffer(mixer, layer->channel,
-				     layer->overlay, plane);
-	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
-			      true, zpos, old_zpos);
+	plane->state->zpos = layer->channel;
 }
 
 static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
 	.prepare_fb	= drm_gem_fb_prepare_fb,
 	.atomic_check	= sun8i_ui_layer_atomic_check,
-	.atomic_disable	= sun8i_ui_layer_atomic_disable,
 	.atomic_update	= sun8i_ui_layer_atomic_update,
 };
 
@@ -308,7 +338,7 @@ static const struct drm_plane_funcs sun8i_ui_layer_funcs = {
 	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
 	.destroy		= drm_plane_cleanup,
 	.disable_plane		= drm_atomic_helper_disable_plane,
-	.reset			= drm_atomic_helper_plane_reset,
+	.reset			= sun8i_ui_layer_plane_reset,
 	.update_plane		= drm_atomic_helper_update_plane,
 };
 
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index bd0e6a52d1d8..675ebcdac00b 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -18,10 +18,11 @@
 #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 was_enabled, bool enable,
+				  unsigned int zpos, unsigned int old_zpos)
 {
 	u32 val, bld_base, ch_base;
+	unsigned int old_pipe_ch;
 
 	bld_base = sun8i_blender_base(mixer);
 	ch_base = sun8i_channel_base(mixer, channel);
@@ -29,28 +30,57 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
 	DRM_DEBUG_DRIVER("%sabling VI channel %d overlay %d\n",
 			 enable ? "En" : "Dis", channel, overlay);
 
-	if (enable)
-		val = SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN;
-	else
-		val = 0;
-
-	regmap_update_bits(mixer->engine.regs,
-			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay),
-			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
+	if (!was_enabled != !enable) {
+		val = enable ? SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN : 0;
 
-	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);
+				   SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay),
+				   SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
+	}
 
-		regmap_update_bits(mixer->engine.regs,
+	/*
+	 * If this layer was enabled and is being disabled or if it is
+	 * enabled and just changing zpos, clear the old route, if it is
+	 * still configured to this layer in HW.
+	 */
+	if ((was_enabled && !enable) || (enable && zpos != old_zpos)) {
+		/* get channel the pipe for old_zpos is routed to from the HW */
+		regmap_read(mixer->engine.regs,
 				   SUN8I_MIXER_BLEND_ROUTE(bld_base),
-				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
-				   0);
+				   &old_pipe_ch);
+		old_pipe_ch &= SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos);
+		old_pipe_ch >>= SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(old_zpos);
+
+		/*
+		 * Check that pipe for old_zpos is still routed to our layer,
+		 * and clear/disable it if it is.
+		 */
+
+		if (old_pipe_ch == channel) {
+			DRM_DEBUG_DRIVER("chan=%d en=%d->%d zpos=%d->%d\n",
+			       channel, was_enabled, enable, old_zpos, zpos);
+
+			DRM_DEBUG_DRIVER("  disable pipe %d\n", old_zpos);
+
+			regmap_update_bits(mixer->engine.regs,
+					   SUN8I_MIXER_BLEND_ROUTE(bld_base),
+					   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
+					   0);
+
+			regmap_update_bits(mixer->engine.regs,
+					   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
+					   SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
+					   0);
+		}
 	}
 
-	if (enable) {
+	/*
+	 * If enabling this layer or changin zpos, set route to this layer.
+	 */
+	if ((enable && !was_enabled) || (enable && zpos != old_zpos)) {
+		DRM_DEBUG_DRIVER("chan=%d en=%d->%d zpos=%d->%d\n",
+		       channel, was_enabled, enable, old_zpos, zpos);
+
 		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
 
 		regmap_update_bits(mixer->engine.regs,
@@ -63,6 +93,8 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
 				   SUN8I_MIXER_BLEND_ROUTE(bld_base),
 				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
 				   val);
+
+		DRM_DEBUG_DRIVER("  enable pipe %d <- ch %d\n", zpos, channel);
 	}
 }
 
@@ -345,45 +377,43 @@ static int sun8i_vi_layer_atomic_check(struct drm_plane *plane,
 						   true, true);
 }
 
-static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane,
-					  struct drm_plane_state *old_state)
+static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
+					 struct drm_plane_state *old_state)
 {
 	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;
+	bool was_enabled = old_state->crtc && old_state->visible;
+	bool enable = plane->state->crtc && plane->state->visible;
 
-	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false, 0,
-			      old_zpos);
+	if (enable) {
+		sun8i_vi_layer_update_coord(mixer, layer->channel,
+					    layer->overlay, plane, zpos);
+		sun8i_vi_layer_update_formats(mixer, layer->channel,
+					      layer->overlay, plane);
+		sun8i_vi_layer_update_buffer(mixer, layer->channel,
+					     layer->overlay, plane);
+	}
+
+	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
+			      was_enabled, enable, zpos, old_zpos);
 }
 
-static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
-					 struct drm_plane_state *old_state)
+void sun8i_vi_layer_plane_reset(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);
+	drm_atomic_helper_plane_reset(plane);
+	if (!plane->state)
 		return;
-	}
 
-	sun8i_vi_layer_update_coord(mixer, layer->channel,
-				    layer->overlay, plane, zpos);
-	sun8i_vi_layer_update_formats(mixer, layer->channel,
-				      layer->overlay, plane);
-	sun8i_vi_layer_update_buffer(mixer, layer->channel,
-				     layer->overlay, plane);
-	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
-			      true, zpos, old_zpos);
+	plane->state->zpos = layer->channel;
 }
 
 static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
 	.prepare_fb	= drm_gem_fb_prepare_fb,
 	.atomic_check	= sun8i_vi_layer_atomic_check,
-	.atomic_disable	= sun8i_vi_layer_atomic_disable,
 	.atomic_update	= sun8i_vi_layer_atomic_update,
 };
 
@@ -392,7 +422,7 @@ static const struct drm_plane_funcs sun8i_vi_layer_funcs = {
 	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
 	.destroy		= drm_plane_cleanup,
 	.disable_plane		= drm_atomic_helper_disable_plane,
-	.reset			= drm_atomic_helper_plane_reset,
+	.reset			= sun8i_vi_layer_plane_reset,
 	.update_plane		= drm_atomic_helper_update_plane,
 };
 
-- 
2.23.0

Re: [PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

[Ondřej Jirman]

On Sun, Sep 15, 2019 at 12:03:37AM +0200, megous hlavni wrote:
> From: Ondrej Jirman <megous@megous.com>
> 
> There are various issues that this re-work of sun8i_[uv]i_layer_enable
> function fixes:
> 
> - Make sure that we re-initialize zpos on reset
> - Minimize register updates by doing them only when state changes
> - Fix issue where DE pipe might get disabled even if it is no longer
>   used by the layer that's currently calling sun8i_ui_layer_enable
> - .atomic_disable callback is not really needed because .atomic_update
>   can do the disable too, so drop the duplicate code

See more discussion here:

  https://groups.google.com/d/msg/linux-sunxi/9A7ukdtvNpM/2Z2bAhA9AwAJ
 
	o.

> Signed-off-by: Ondrej Jirman <megous@megous.com>
> ---
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 112 ++++++++++++++++---------
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 112 ++++++++++++++++---------
>  2 files changed, 142 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> index dd2a1c851939..b88e8ac5ad1c 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> @@ -24,10 +24,11 @@
>  #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 was_enabled, bool enable,
> +				  unsigned int zpos, unsigned int old_zpos)
>  {
>  	u32 val, bld_base, ch_base;
> +	unsigned int old_pipe_ch;
>  
>  	bld_base = sun8i_blender_base(mixer);
>  	ch_base = sun8i_channel_base(mixer, channel);
> @@ -35,28 +36,57 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
>  	DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
>  			 enable ? "En" : "Dis", channel, overlay);
>  
> -	if (enable)
> -		val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN;
> -	else
> -		val = 0;
> +	if (!was_enabled != !enable) {
> +		val = enable ? SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN : 0;
>  
> -	regmap_update_bits(mixer->engine.regs,
> -			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
> -			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
> -
> -	if (!enable || zpos != old_zpos) {
>  		regmap_update_bits(mixer->engine.regs,
> -				   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> -				   SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> -				   0);
> +				   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
> +				   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
> +	}
>  
> -		regmap_update_bits(mixer->engine.regs,
> +	/*
> +	 * If this layer was enabled and is being disabled or if it is
> +	 * enabled and just changing zpos, clear the old route, if it is
> +	 * still configured to this layer in HW.
> +	 */
> +	if ((was_enabled && !enable) || (enable && zpos != old_zpos)) {
> +		/* get channel the pipe for old_zpos is routed to from the HW */
> +		regmap_read(mixer->engine.regs,
>  				   SUN8I_MIXER_BLEND_ROUTE(bld_base),
> -				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> -				   0);
> +				   &old_pipe_ch);
> +		old_pipe_ch &= SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos);
> +		old_pipe_ch >>= SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(old_zpos);
> +
> +		/*
> +		 * Check that pipe for old_zpos is still routed to our layer,
> +		 * and clear/disable it if it is.
> +		 */
> +
> +		if (old_pipe_ch == channel) {
> +			DRM_DEBUG_DRIVER("chan=%d en=%d->%d zpos=%d->%d\n",
> +			       channel, was_enabled, enable, old_zpos, zpos);
> +
> +			DRM_DEBUG_DRIVER("  disable pipe %d\n", old_zpos);
> +
> +			regmap_update_bits(mixer->engine.regs,
> +					   SUN8I_MIXER_BLEND_ROUTE(bld_base),
> +					   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> +					   0);
> +
> +			regmap_update_bits(mixer->engine.regs,
> +					   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> +					   SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> +					   0);
> +		}
>  	}
>  
> -	if (enable) {
> +	/*
> +	 * If enabling this layer or changin zpos, set route to this layer.
> +	 */
> +	if ((enable && !was_enabled) || (enable && zpos != old_zpos)) {
> +		DRM_DEBUG_DRIVER("chan=%d en=%d->%d zpos=%d->%d\n",
> +		       channel, was_enabled, enable, old_zpos, zpos);
> +
>  		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
>  
>  		regmap_update_bits(mixer->engine.regs,
> @@ -69,6 +99,8 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
>  				   SUN8I_MIXER_BLEND_ROUTE(bld_base),
>  				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
>  				   val);
> +
> +		DRM_DEBUG_DRIVER("  enable pipe %d <- ch %d\n", zpos, channel);
>  	}
>  }
>  
> @@ -261,45 +293,43 @@ static int sun8i_ui_layer_atomic_check(struct drm_plane *plane,
>  						   true, true);
>  }
>  
> -static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane,
> -					  struct drm_plane_state *old_state)
> +static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> +					 struct drm_plane_state *old_state)
>  {
>  	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;
> +	bool was_enabled = old_state->crtc && old_state->visible;
> +	bool enable = plane->state->crtc && plane->state->visible;
>  
> -	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false, 0,
> -			      old_zpos);
> +	if (enable) {
> +		sun8i_ui_layer_update_coord(mixer, layer->channel,
> +					    layer->overlay, plane, zpos);
> +		sun8i_ui_layer_update_formats(mixer, layer->channel,
> +					      layer->overlay, plane);
> +		sun8i_ui_layer_update_buffer(mixer, layer->channel,
> +					     layer->overlay, plane);
> +	}
> +
> +	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> +			      was_enabled, enable, zpos, old_zpos);
>  }
>  
> -static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> -					 struct drm_plane_state *old_state)
> +void sun8i_ui_layer_plane_reset(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);
> +	drm_atomic_helper_plane_reset(plane);
> +	if (!plane->state)
>  		return;
> -	}
>  
> -	sun8i_ui_layer_update_coord(mixer, layer->channel,
> -				    layer->overlay, plane, zpos);
> -	sun8i_ui_layer_update_formats(mixer, layer->channel,
> -				      layer->overlay, plane);
> -	sun8i_ui_layer_update_buffer(mixer, layer->channel,
> -				     layer->overlay, plane);
> -	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> -			      true, zpos, old_zpos);
> +	plane->state->zpos = layer->channel;
>  }
>  
>  static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
>  	.prepare_fb	= drm_gem_fb_prepare_fb,
>  	.atomic_check	= sun8i_ui_layer_atomic_check,
> -	.atomic_disable	= sun8i_ui_layer_atomic_disable,
>  	.atomic_update	= sun8i_ui_layer_atomic_update,
>  };
>  
> @@ -308,7 +338,7 @@ static const struct drm_plane_funcs sun8i_ui_layer_funcs = {
>  	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
>  	.destroy		= drm_plane_cleanup,
>  	.disable_plane		= drm_atomic_helper_disable_plane,
> -	.reset			= drm_atomic_helper_plane_reset,
> +	.reset			= sun8i_ui_layer_plane_reset,
>  	.update_plane		= drm_atomic_helper_update_plane,
>  };
>  
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> index bd0e6a52d1d8..675ebcdac00b 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> @@ -18,10 +18,11 @@
>  #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 was_enabled, bool enable,
> +				  unsigned int zpos, unsigned int old_zpos)
>  {
>  	u32 val, bld_base, ch_base;
> +	unsigned int old_pipe_ch;
>  
>  	bld_base = sun8i_blender_base(mixer);
>  	ch_base = sun8i_channel_base(mixer, channel);
> @@ -29,28 +30,57 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
>  	DRM_DEBUG_DRIVER("%sabling VI channel %d overlay %d\n",
>  			 enable ? "En" : "Dis", channel, overlay);
>  
> -	if (enable)
> -		val = SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN;
> -	else
> -		val = 0;
> -
> -	regmap_update_bits(mixer->engine.regs,
> -			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay),
> -			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
> +	if (!was_enabled != !enable) {
> +		val = enable ? SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN : 0;
>  
> -	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);
> +				   SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay),
> +				   SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
> +	}
>  
> -		regmap_update_bits(mixer->engine.regs,
> +	/*
> +	 * If this layer was enabled and is being disabled or if it is
> +	 * enabled and just changing zpos, clear the old route, if it is
> +	 * still configured to this layer in HW.
> +	 */
> +	if ((was_enabled && !enable) || (enable && zpos != old_zpos)) {
> +		/* get channel the pipe for old_zpos is routed to from the HW */
> +		regmap_read(mixer->engine.regs,
>  				   SUN8I_MIXER_BLEND_ROUTE(bld_base),
> -				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> -				   0);
> +				   &old_pipe_ch);
> +		old_pipe_ch &= SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos);
> +		old_pipe_ch >>= SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(old_zpos);
> +
> +		/*
> +		 * Check that pipe for old_zpos is still routed to our layer,
> +		 * and clear/disable it if it is.
> +		 */
> +
> +		if (old_pipe_ch == channel) {
> +			DRM_DEBUG_DRIVER("chan=%d en=%d->%d zpos=%d->%d\n",
> +			       channel, was_enabled, enable, old_zpos, zpos);
> +
> +			DRM_DEBUG_DRIVER("  disable pipe %d\n", old_zpos);
> +
> +			regmap_update_bits(mixer->engine.regs,
> +					   SUN8I_MIXER_BLEND_ROUTE(bld_base),
> +					   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> +					   0);
> +
> +			regmap_update_bits(mixer->engine.regs,
> +					   SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> +					   SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> +					   0);
> +		}
>  	}
>  
> -	if (enable) {
> +	/*
> +	 * If enabling this layer or changin zpos, set route to this layer.
> +	 */
> +	if ((enable && !was_enabled) || (enable && zpos != old_zpos)) {
> +		DRM_DEBUG_DRIVER("chan=%d en=%d->%d zpos=%d->%d\n",
> +		       channel, was_enabled, enable, old_zpos, zpos);
> +
>  		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
>  
>  		regmap_update_bits(mixer->engine.regs,
> @@ -63,6 +93,8 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
>  				   SUN8I_MIXER_BLEND_ROUTE(bld_base),
>  				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
>  				   val);
> +
> +		DRM_DEBUG_DRIVER("  enable pipe %d <- ch %d\n", zpos, channel);
>  	}
>  }
>  
> @@ -345,45 +377,43 @@ static int sun8i_vi_layer_atomic_check(struct drm_plane *plane,
>  						   true, true);
>  }
>  
> -static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane,
> -					  struct drm_plane_state *old_state)
> +static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> +					 struct drm_plane_state *old_state)
>  {
>  	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;
> +	bool was_enabled = old_state->crtc && old_state->visible;
> +	bool enable = plane->state->crtc && plane->state->visible;
>  
> -	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false, 0,
> -			      old_zpos);
> +	if (enable) {
> +		sun8i_vi_layer_update_coord(mixer, layer->channel,
> +					    layer->overlay, plane, zpos);
> +		sun8i_vi_layer_update_formats(mixer, layer->channel,
> +					      layer->overlay, plane);
> +		sun8i_vi_layer_update_buffer(mixer, layer->channel,
> +					     layer->overlay, plane);
> +	}
> +
> +	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> +			      was_enabled, enable, zpos, old_zpos);
>  }
>  
> -static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> -					 struct drm_plane_state *old_state)
> +void sun8i_vi_layer_plane_reset(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);
> +	drm_atomic_helper_plane_reset(plane);
> +	if (!plane->state)
>  		return;
> -	}
>  
> -	sun8i_vi_layer_update_coord(mixer, layer->channel,
> -				    layer->overlay, plane, zpos);
> -	sun8i_vi_layer_update_formats(mixer, layer->channel,
> -				      layer->overlay, plane);
> -	sun8i_vi_layer_update_buffer(mixer, layer->channel,
> -				     layer->overlay, plane);
> -	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> -			      true, zpos, old_zpos);
> +	plane->state->zpos = layer->channel;
>  }
>  
>  static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
>  	.prepare_fb	= drm_gem_fb_prepare_fb,
>  	.atomic_check	= sun8i_vi_layer_atomic_check,
> -	.atomic_disable	= sun8i_vi_layer_atomic_disable,
>  	.atomic_update	= sun8i_vi_layer_atomic_update,
>  };
>  
> @@ -392,7 +422,7 @@ static const struct drm_plane_funcs sun8i_vi_layer_funcs = {
>  	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
>  	.destroy		= drm_plane_cleanup,
>  	.disable_plane		= drm_atomic_helper_disable_plane,
> -	.reset			= drm_atomic_helper_plane_reset,
> +	.reset			= sun8i_vi_layer_plane_reset,
>  	.update_plane		= drm_atomic_helper_update_plane,
>  };
>  
> -- 
> 2.23.0
> 

Re: [PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

[Maxime Ripard]

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

Hi,

On Sun, Sep 15, 2019 at 12:03:37AM +0200, megous@megous.com wrote:
> From: Ondrej Jirman <megous@megous.com>
>
> There are various issues that this re-work of sun8i_[uv]i_layer_enable
> function fixes:
>
> - Make sure that we re-initialize zpos on reset
> - Minimize register updates by doing them only when state changes
> - Fix issue where DE pipe might get disabled even if it is no longer
>   used by the layer that's currently calling sun8i_ui_layer_enable
> - .atomic_disable callback is not really needed because .atomic_update
>   can do the disable too, so drop the duplicate code
>
> Signed-off-by: Ondrej Jirman <megous@megous.com>

It looks like these fixes should be in separate patches. Is there any
reason it's not the case?

Maxime

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

Re: [PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

[Ondřej Jirman]

Hi,

On Wed, Sep 18, 2019 at 04:17:34PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Sun, Sep 15, 2019 at 12:03:37AM +0200, megous@megous.com wrote:
> > From: Ondrej Jirman <megous@megous.com>
> >
> > There are various issues that this re-work of sun8i_[uv]i_layer_enable
> > function fixes:
> >
> > - Make sure that we re-initialize zpos on reset
> > - Minimize register updates by doing them only when state changes
> > - Fix issue where DE pipe might get disabled even if it is no longer
> >   used by the layer that's currently calling sun8i_ui_layer_enable
> > - .atomic_disable callback is not really needed because .atomic_update
> >   can do the disable too, so drop the duplicate code
> >
> > Signed-off-by: Ondrej Jirman <megous@megous.com>
> 
> It looks like these fixes should be in separate patches. Is there any
> reason it's not the case?

Bullet points just describe the resulting effect/benefits of the change to fix
the pipe control register update issue (see the referenced e-mail).

I can maybe split off the first bullet point into a separate patch. But
I can't guarantee it will not make the original issue worse, because it might
have been hiding the other issue with register updates.

The rest is just a result of the single logical change. It doesn't work
individually, it all has the goal of fixing the issue as a whole.

If I were to split it I would have to actually re-implement .atomic_disable
callback only to remove it in the next patch. I don't see the benefit.

regards,
	o.

> Maxime

Re: [PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

[Maxime Ripard]

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

On Wed, Sep 18, 2019 at 05:23:09PM +0200, Ondřej Jirman wrote:
> Hi,
>
> On Wed, Sep 18, 2019 at 04:17:34PM +0200, Maxime Ripard wrote:
> > Hi,
> >
> > On Sun, Sep 15, 2019 at 12:03:37AM +0200, megous@megous.com wrote:
> > > From: Ondrej Jirman <megous@megous.com>
> > >
> > > There are various issues that this re-work of sun8i_[uv]i_layer_enable
> > > function fixes:
> > >
> > > - Make sure that we re-initialize zpos on reset
> > > - Minimize register updates by doing them only when state changes
> > > - Fix issue where DE pipe might get disabled even if it is no longer
> > >   used by the layer that's currently calling sun8i_ui_layer_enable
> > > - .atomic_disable callback is not really needed because .atomic_update
> > >   can do the disable too, so drop the duplicate code
> > >
> > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> >
> > It looks like these fixes should be in separate patches. Is there any
> > reason it's not the case?
>
> Bullet points just describe the resulting effect/benefits of the change to fix
> the pipe control register update issue (see the referenced e-mail).

It's definitely ok to have multiple patches needed to address a single
perceived issue.

A commit is not about what you're fixing but what you're changing. And
the fact that you have tha bullet list in the first place proves that
you have multiple logical changes in your patch.

And even then, your commit log mentions that you're fixing multiple
issues (without explaining them).

> I can maybe split off the first bullet point into a separate patch. But
> I can't guarantee it will not make the original issue worse, because it might
> have been hiding the other issue with register updates.
>
> The rest is just a result of the single logical change. It doesn't work
> individually, it all has the goal of fixing the issue as a whole.
>
> If I were to split it I would have to actually re-implement .atomic_disable
> callback only to remove it in the next patch. I don't see the benefit.

Your commit log says that you remove atomic_disable. Why would you
remove it, to add it back, to remove it again?

Maxime

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

Re: [PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

[Ondřej Jirman]

On Wed, Sep 18, 2019 at 10:16:17PM +0200, Maxime Ripard wrote:
> On Wed, Sep 18, 2019 at 05:23:09PM +0200, Ondřej Jirman wrote:
> > Hi,
> >
> > On Wed, Sep 18, 2019 at 04:17:34PM +0200, Maxime Ripard wrote:
> > > Hi,
> > >
> > > On Sun, Sep 15, 2019 at 12:03:37AM +0200, megous@megous.com wrote:
> > > > From: Ondrej Jirman <megous@megous.com>
> > > >
> > > > There are various issues that this re-work of sun8i_[uv]i_layer_enable
> > > > function fixes:
> > > >
> > > > - Make sure that we re-initialize zpos on reset
> > > > - Minimize register updates by doing them only when state changes
> > > > - Fix issue where DE pipe might get disabled even if it is no longer
> > > >   used by the layer that's currently calling sun8i_ui_layer_enable
> > > > - .atomic_disable callback is not really needed because .atomic_update
> > > >   can do the disable too, so drop the duplicate code
> > > >
> > > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > >
> > > It looks like these fixes should be in separate patches. Is there any
> > > reason it's not the case?
> >
> > Bullet points just describe the resulting effect/benefits of the change to fix
> > the pipe control register update issue (see the referenced e-mail).
> 
> It's definitely ok to have multiple patches needed to address a single
> perceived issue.

Yes, but I can't simply split the patch. In order for each change to work on its
own, they'd have to be done differently than the final result.

I wouldn't mind at all if it was just a simple splitting, but you're asking
for too much work, this time, for no benefit that I can see.

> A commit is not about what you're fixing but what you're changing. And
> the fact that you have tha bullet list in the first place proves that
> you have multiple logical changes in your patch.
> 
> And even then, your commit log mentions that you're fixing multiple
> issues (without explaining them).

I can reword the commit message if that helps, and skip the bullet list if it
is confusing. There's a single core issue and that is that the driver doesn't
update the pipe/channel configuration correctly leading to disabling of
arbitrary layers (not even those being updated - update of UI layer may disable
VI layer as a side effect for example) at wrong times. And only changes
necessary to debug/fix this are included.

I may try generating a nicer patch with a different diff options, if it makes it
more readable for review.

> > I can maybe split off the first bullet point into a separate patch. But
> > I can't guarantee it will not make the original issue worse, because it might
> > have been hiding the other issue with register updates.
> >
> > The rest is just a result of the single logical change. It doesn't work
> > individually, it all has the goal of fixing the issue as a whole.
> >
> > If I were to split it I would have to actually re-implement .atomic_disable
> > callback only to remove it in the next patch. I don't see the benefit.
> 
> Your commit log says that you remove atomic_disable. Why would you
> remove it, to add it back, to remove it again?

Because if I remove it I need to re-implement the functionality in the update
callback. The core will change what is called based on presence of callbacks.
It's not a simple removal.

If I first implement the new sun8i_[uv]i_layer_enable and update callback,
keeping a disable callback would not work, because the new update callback
will only work if disable callback is not defined (because it it is, then
the drm core will not call the update callback in all cases that I need).

regards,
	o.

> Maxime



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

Re: [PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

[Ondřej Jirman]

On Thu, Sep 19, 2019 at 02:20:58PM +0200, megous hlavni wrote:
> On Wed, Sep 18, 2019 at 10:16:17PM +0200, Maxime Ripard wrote:
> > On Wed, Sep 18, 2019 at 05:23:09PM +0200, Ondřej Jirman wrote:
> > > Hi,
> > >
> > > On Wed, Sep 18, 2019 at 04:17:34PM +0200, Maxime Ripard wrote:
> > > > Hi,
> > > >
> > > > On Sun, Sep 15, 2019 at 12:03:37AM +0200, megous@megous.com wrote:
> > > > > From: Ondrej Jirman <megous@megous.com>
> > > > >
> > > > > There are various issues that this re-work of sun8i_[uv]i_layer_enable
> > > > > function fixes:
> > > > >
> > > > > - Make sure that we re-initialize zpos on reset
> > > > > - Minimize register updates by doing them only when state changes
> > > > > - Fix issue where DE pipe might get disabled even if it is no longer
> > > > >   used by the layer that's currently calling sun8i_ui_layer_enable
> > > > > - .atomic_disable callback is not really needed because .atomic_update
> > > > >   can do the disable too, so drop the duplicate code
> > > > >
> > > > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > > >
> > > > It looks like these fixes should be in separate patches. Is there any
> > > > reason it's not the case?
> > >
> > > Bullet points just describe the resulting effect/benefits of the change to fix
> > > the pipe control register update issue (see the referenced e-mail).
> > 
> > It's definitely ok to have multiple patches needed to address a single
> > perceived issue.
> 
> Yes, but I can't simply split the patch. In order for each change to work on its
> own, they'd have to be done differently than the final result.
> 
> I wouldn't mind at all if it was just a simple splitting, but you're asking
> for too much work, this time, for no benefit that I can see.
> 
> > A commit is not about what you're fixing but what you're changing. And
> > the fact that you have tha bullet list in the first place proves that
> > you have multiple logical changes in your patch.
> > 
> > And even then, your commit log mentions that you're fixing multiple
> > issues (without explaining them).
> 
> I can reword the commit message if that helps, and skip the bullet list if it
> is confusing. There's a single core issue and that is that the driver doesn't
> update the pipe/channel configuration correctly leading to disabling of
> arbitrary layers (not even those being updated - update of UI layer may disable
> VI layer as a side effect for example) at wrong times. And only changes
> necessary to debug/fix this are included.

How about this:

 A problem was found where identical configuration of planes leads
 to different register settings at the HW layer when using a X server
 with modesetting driver and one plane marked as a cursor.
  
 On first run of the X server, only the black screen and the layer
 containing the cursor is visible. Switching to console and back
 corrects the situation.
 
 I have dumped registers, and found out this:

 (In both cases there are two enabled planes, plane 1 with zpos 0 and
 plane 3 with zpos 1).
 
 1) First Xorg run:
 
   0x01101000 : 00000201
   0x01101080 : 00000030
 
   BLD_FILL_COLOR_CTL: (aka SUN8I_MIXER_BLEND_PIPE_CTL)
     P1_EN
 
   BLD_CH_RTCTL: (aka SUN8I_MIXER_BLEND_ROUTE)
     P0_RTCTL channel0
     P1_RTCTL channel3
 
 2) After switch to console and back to Xorg:
 
 0x01101000 : 00000301
 0x01101080 : 00000031
 
   BLD_FILL_COLOR_CTL:
     P1_EN and P0_EN
 
   BLD_CH_RTCTL:
     P0_RTCTL channel1
     P1_RTCTL channel3
 
 What happens is that sun8i_ui_layer_enable() function may disable
 blending pipes even if it is no longer assigned to its layer, because
 of incorrect state/zpos tracking in the driver.
 
 In particular, layer 1 is configured to zpos 0 and thus uses pipe 0.
 When layer 3 is enabled by X server, sun8i_ui_layer_enable() will get
 called with old_zpos=0 zpos=1, which will lead to disabling of pipe 0.
 
 In general this issue can happen to any layer during enable or zpos
 changes on multiple layers at once.
 
 To correct this we now pass previous enabled/disabled state of the
 layer, and pass real previous zpos of the layer to
 sun8i_ui_layer_enable() and rework the sun8i_ui_layer_enable() function
 to react to the state changes correctly. In order to not complicate
 the atomic_disable callback with all of the above changes, we simply
 remove it and implement all the chanes as part of atomic_update, which
 also reduces the code duplication.
 
 To make this all work, initial zpos positions of all layers need to be
 restored to initial values on reset.


> I may try generating a nicer patch with a different diff options, if it makes it
> more readable for review.
> 
> > > I can maybe split off the first bullet point into a separate patch. But
> > > I can't guarantee it will not make the original issue worse, because it might
> > > have been hiding the other issue with register updates.
> > >
> > > The rest is just a result of the single logical change. It doesn't work
> > > individually, it all has the goal of fixing the issue as a whole.
> > >
> > > If I were to split it I would have to actually re-implement .atomic_disable
> > > callback only to remove it in the next patch. I don't see the benefit.
> > 
> > Your commit log says that you remove atomic_disable. Why would you
> > remove it, to add it back, to remove it again?
> 
> Because if I remove it I need to re-implement the functionality in the update
> callback. The core will change what is called based on presence of callbacks.
> It's not a simple removal.
> 
> If I first implement the new sun8i_[uv]i_layer_enable and update callback,
> keeping a disable callback would not work, because the new update callback
> will only work if disable callback is not defined (because it it is, then
> the drm core will not call the update callback in all cases that I need).
> 
> regards,
> 	o.
> 
> > Maxime
> 
> 
> 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

[Maxime Ripard]

On Thu, Sep 19, 2019 at 03:12:44PM +0200, Ondřej Jirman wrote:
> On Thu, Sep 19, 2019 at 02:20:58PM +0200, megous hlavni wrote:
> > On Wed, Sep 18, 2019 at 10:16:17PM +0200, Maxime Ripard wrote:
> > > On Wed, Sep 18, 2019 at 05:23:09PM +0200, Ondřej Jirman wrote:
> > > > Hi,
> > > >
> > > > On Wed, Sep 18, 2019 at 04:17:34PM +0200, Maxime Ripard wrote:
> > > > > Hi,
> > > > >
> > > > > On Sun, Sep 15, 2019 at 12:03:37AM +0200, megous@megous.com wrote:
> > > > > > From: Ondrej Jirman <megous@megous.com>
> > > > > >
> > > > > > There are various issues that this re-work of sun8i_[uv]i_layer_enable
> > > > > > function fixes:
> > > > > >
> > > > > > - Make sure that we re-initialize zpos on reset
> > > > > > - Minimize register updates by doing them only when state changes
> > > > > > - Fix issue where DE pipe might get disabled even if it is no longer
> > > > > >   used by the layer that's currently calling sun8i_ui_layer_enable
> > > > > > - .atomic_disable callback is not really needed because .atomic_update
> > > > > >   can do the disable too, so drop the duplicate code
> > > > > >
> > > > > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > > > >
> > > > > It looks like these fixes should be in separate patches. Is there any
> > > > > reason it's not the case?
> > > >
> > > > Bullet points just describe the resulting effect/benefits of the change to fix
> > > > the pipe control register update issue (see the referenced e-mail).
> > >
> > > It's definitely ok to have multiple patches needed to address a single
> > > perceived issue.
> >
> > Yes, but I can't simply split the patch. In order for each change to work on its
> > own, they'd have to be done differently than the final result.
> >
> > I wouldn't mind at all if it was just a simple splitting, but you're asking
> > for too much work, this time, for no benefit that I can see.
> >
> > > A commit is not about what you're fixing but what you're changing. And
> > > the fact that you have tha bullet list in the first place proves that
> > > you have multiple logical changes in your patch.
> > >
> > > And even then, your commit log mentions that you're fixing multiple
> > > issues (without explaining them).
> >
> > I can reword the commit message if that helps, and skip the bullet list if it
> > is confusing. There's a single core issue and that is that the driver doesn't
> > update the pipe/channel configuration correctly leading to disabling of
> > arbitrary layers (not even those being updated - update of UI layer may disable
> > VI layer as a side effect for example) at wrong times. And only changes
> > necessary to debug/fix this are included.
>
> How about this:
>
>  A problem was found where identical configuration of planes leads
>  to different register settings at the HW layer when using a X server
>  with modesetting driver and one plane marked as a cursor.

We don't have a cursor plane.

>  On first run of the X server, only the black screen and the layer
>  containing the cursor is visible. Switching to console and back
>  corrects the situation.
>
>  I have dumped registers, and found out this:
>
>  (In both cases there are two enabled planes, plane 1 with zpos 0 and
>  plane 3 with zpos 1).
>
>  1) First Xorg run:
>
>    0x01101000 : 00000201
>    0x01101080 : 00000030
>
>    BLD_FILL_COLOR_CTL: (aka SUN8I_MIXER_BLEND_PIPE_CTL)
>      P1_EN
>
>    BLD_CH_RTCTL: (aka SUN8I_MIXER_BLEND_ROUTE)
>      P0_RTCTL channel0
>      P1_RTCTL channel3
>
>  2) After switch to console and back to Xorg:
>
>  0x01101000 : 00000301
>  0x01101080 : 00000031
>
>    BLD_FILL_COLOR_CTL:
>      P1_EN and P0_EN
>
>    BLD_CH_RTCTL:
>      P0_RTCTL channel1
>      P1_RTCTL channel3
>
>  What happens is that sun8i_ui_layer_enable() function may disable
>  blending pipes even if it is no longer assigned to its layer, because
>  of incorrect state/zpos tracking in the driver.
>
>  In particular, layer 1 is configured to zpos 0 and thus uses pipe 0.
>  When layer 3 is enabled by X server, sun8i_ui_layer_enable() will get
>  called with old_zpos=0 zpos=1, which will lead to disabling of pipe 0.
>
>  In general this issue can happen to any layer during enable or zpos
>  changes on multiple layers at once.
>
>  To correct this we now pass previous enabled/disabled state of the
>  layer, and pass real previous zpos of the layer to
>  sun8i_ui_layer_enable() and rework the sun8i_ui_layer_enable() function
>  to react to the state changes correctly. In order to not complicate
>  the atomic_disable callback with all of the above changes, we simply
>  remove it and implement all the chanes as part of atomic_update, which
>  also reduces the code duplication.

I'm not even sure why we need that old state. Can't we just reset
completely the whole thing and do the configuration all over again?

That description just looks to me like the reset is not done properly,
and now we have to deal with the fallouts later on.

>  To make this all work, initial zpos positions of all layers need to be
>  restored to initial values on reset.

And this also fixes a whole other bunch of issues, and can be made
very trivially in a separate patch.

Maxime

Re: [PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

[Ondřej Jirman]

On Fri, Sep 20, 2019 at 05:11:42PM +0200, Maxime Ripard wrote:
> On Thu, Sep 19, 2019 at 03:12:44PM +0200, Ondřej Jirman wrote:
> > On Thu, Sep 19, 2019 at 02:20:58PM +0200, megous hlavni wrote:
> > > On Wed, Sep 18, 2019 at 10:16:17PM +0200, Maxime Ripard wrote:
> > > > On Wed, Sep 18, 2019 at 05:23:09PM +0200, Ondřej Jirman wrote:
> > > > > Hi,
> > > > >
> > > > > On Wed, Sep 18, 2019 at 04:17:34PM +0200, Maxime Ripard wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Sun, Sep 15, 2019 at 12:03:37AM +0200, megous@megous.com wrote:
> > > > > > > From: Ondrej Jirman <megous@megous.com>
> > > > > > >
> > > > > > > There are various issues that this re-work of sun8i_[uv]i_layer_enable
> > > > > > > function fixes:
> > > > > > >
> > > > > > > - Make sure that we re-initialize zpos on reset
> > > > > > > - Minimize register updates by doing them only when state changes
> > > > > > > - Fix issue where DE pipe might get disabled even if it is no longer
> > > > > > >   used by the layer that's currently calling sun8i_ui_layer_enable
> > > > > > > - .atomic_disable callback is not really needed because .atomic_update
> > > > > > >   can do the disable too, so drop the duplicate code
> > > > > > >
> > > > > > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > > > > >
> > > > > > It looks like these fixes should be in separate patches. Is there any
> > > > > > reason it's not the case?
> > > > >
> > > > > Bullet points just describe the resulting effect/benefits of the change to fix
> > > > > the pipe control register update issue (see the referenced e-mail).
> > > >
> > > > It's definitely ok to have multiple patches needed to address a single
> > > > perceived issue.
> > >
> > > Yes, but I can't simply split the patch. In order for each change to work on its
> > > own, they'd have to be done differently than the final result.
> > >
> > > I wouldn't mind at all if it was just a simple splitting, but you're asking
> > > for too much work, this time, for no benefit that I can see.
> > >
> > > > A commit is not about what you're fixing but what you're changing. And
> > > > the fact that you have tha bullet list in the first place proves that
> > > > you have multiple logical changes in your patch.
> > > >
> > > > And even then, your commit log mentions that you're fixing multiple
> > > > issues (without explaining them).
> > >
> > > I can reword the commit message if that helps, and skip the bullet list if it
> > > is confusing. There's a single core issue and that is that the driver doesn't
> > > update the pipe/channel configuration correctly leading to disabling of
> > > arbitrary layers (not even those being updated - update of UI layer may disable
> > > VI layer as a side effect for example) at wrong times. And only changes
> > > necessary to debug/fix this are included.
> >
> > How about this:
> >
> >  A problem was found where identical configuration of planes leads
> >  to different register settings at the HW layer when using a X server
> >  with modesetting driver and one plane marked as a cursor.
> 
> We don't have a cursor plane.

Did I say we did? I only wrote that the problem was found this way. It's there
regardless of how it was revealed.

> >  On first run of the X server, only the black screen and the layer
> >  containing the cursor is visible. Switching to console and back
> >  corrects the situation.
> >
> >  I have dumped registers, and found out this:
> >
> >  (In both cases there are two enabled planes, plane 1 with zpos 0 and
> >  plane 3 with zpos 1).
> >
> >  1) First Xorg run:
> >
> >    0x01101000 : 00000201
> >    0x01101080 : 00000030
> >
> >    BLD_FILL_COLOR_CTL: (aka SUN8I_MIXER_BLEND_PIPE_CTL)
> >      P1_EN
> >
> >    BLD_CH_RTCTL: (aka SUN8I_MIXER_BLEND_ROUTE)
> >      P0_RTCTL channel0
> >      P1_RTCTL channel3
> >
> >  2) After switch to console and back to Xorg:
> >
> >  0x01101000 : 00000301
> >  0x01101080 : 00000031
> >
> >    BLD_FILL_COLOR_CTL:
> >      P1_EN and P0_EN
> >
> >    BLD_CH_RTCTL:
> >      P0_RTCTL channel1
> >      P1_RTCTL channel3
> >
> >  What happens is that sun8i_ui_layer_enable() function may disable
> >  blending pipes even if it is no longer assigned to its layer, because
> >  of incorrect state/zpos tracking in the driver.
> >
> >  In particular, layer 1 is configured to zpos 0 and thus uses pipe 0.
> >  When layer 3 is enabled by X server, sun8i_ui_layer_enable() will get
> >  called with old_zpos=0 zpos=1, which will lead to disabling of pipe 0.
> >
> >  In general this issue can happen to any layer during enable or zpos
> >  changes on multiple layers at once.
> >
> >  To correct this we now pass previous enabled/disabled state of the
> >  layer, and pass real previous zpos of the layer to
> >  sun8i_ui_layer_enable() and rework the sun8i_ui_layer_enable() function
> >  to react to the state changes correctly. In order to not complicate
> >  the atomic_disable callback with all of the above changes, we simply
> >  remove it and implement all the chanes as part of atomic_update, which
> >  also reduces the code duplication.
> 
> I'm not even sure why we need that old state. Can't we just reset
> completely the whole thing and do the configuration all over again?

That would be nice from a dev standpoint if we can get a complete state for all
planes at once from DRM core, but how? DRM helper gives callbacks
for updating individual planes with prev and new state. These individual layer
change notifications don't map nicely to how pipes are represented in the mixer
registers.

> That description just looks to me like the reset is not done properly,
> and now we have to deal with the fallouts later on.

What in particular?

> >  To make this all work, initial zpos positions of all layers need to be
> >  restored to initial values on reset.
> 
> And this also fixes a whole other bunch of issues, and can be made
> very trivially in a separate patch.

Maybe reset should also reset registers?

regards,
	o.

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

Re: [PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

[Ondřej Jirman]

Hi,

On Tue, Sep 24, 2019 at 02:40:54PM +0200, megous hlavni wrote:
> > >  On first run of the X server, only the black screen and the layer
> > >  containing the cursor is visible. Switching to console and back
> > >  corrects the situation.
> > >
> > >  I have dumped registers, and found out this:
> > >
> > >  (In both cases there are two enabled planes, plane 1 with zpos 0 and
> > >  plane 3 with zpos 1).
> > >
> > >  1) First Xorg run:
> > >
> > >    0x01101000 : 00000201
> > >    0x01101080 : 00000030
> > >
> > >    BLD_FILL_COLOR_CTL: (aka SUN8I_MIXER_BLEND_PIPE_CTL)
> > >      P1_EN
> > >
> > >    BLD_CH_RTCTL: (aka SUN8I_MIXER_BLEND_ROUTE)
> > >      P0_RTCTL channel0
> > >      P1_RTCTL channel3
> > >
> > >  2) After switch to console and back to Xorg:
> > >
> > >  0x01101000 : 00000301
> > >  0x01101080 : 00000031
> > >
> > >    BLD_FILL_COLOR_CTL:
> > >      P1_EN and P0_EN
> > >
> > >    BLD_CH_RTCTL:
> > >      P0_RTCTL channel1
> > >      P1_RTCTL channel3
> > >
> > >  What happens is that sun8i_ui_layer_enable() function may disable
> > >  blending pipes even if it is no longer assigned to its layer, because
> > >  of incorrect state/zpos tracking in the driver.
> > >
> > >  In particular, layer 1 is configured to zpos 0 and thus uses pipe 0.
> > >  When layer 3 is enabled by X server, sun8i_ui_layer_enable() will get
> > >  called with old_zpos=0 zpos=1, which will lead to disabling of pipe 0.
> > >
> > >  In general this issue can happen to any layer during enable or zpos
> > >  changes on multiple layers at once.
> > >
> > >  To correct this we now pass previous enabled/disabled state of the
> > >  layer, and pass real previous zpos of the layer to
> > >  sun8i_ui_layer_enable() and rework the sun8i_ui_layer_enable() function
> > >  to react to the state changes correctly. In order to not complicate
> > >  the atomic_disable callback with all of the above changes, we simply
> > >  remove it and implement all the chanes as part of atomic_update, which
> > >  also reduces the code duplication.
> > 
> > I'm not even sure why we need that old state. Can't we just reset
> > completely the whole thing and do the configuration all over again?
> 
> That would be nice from a dev standpoint if we can get a complete state for all
> planes at once from DRM core, but how? DRM helper gives callbacks
> for updating individual planes with prev and new state. These individual layer
> change notifications don't map nicely to how pipes are represented in the mixer
> registers.

If anyone wants to pursue this further, feel free to. I'm not planning to
pursue this fix further, at the moment.

regards,
	o.

Re: [PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

[Maxime Ripard]

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

On Tue, Sep 24, 2019 at 02:40:54PM +0200, Ondřej Jirman wrote:
> > >  On first run of the X server, only the black screen and the layer
> > >  containing the cursor is visible. Switching to console and back
> > >  corrects the situation.
> > >
> > >  I have dumped registers, and found out this:
> > >
> > >  (In both cases there are two enabled planes, plane 1 with zpos 0 and
> > >  plane 3 with zpos 1).
> > >
> > >  1) First Xorg run:
> > >
> > >    0x01101000 : 00000201
> > >    0x01101080 : 00000030
> > >
> > >    BLD_FILL_COLOR_CTL: (aka SUN8I_MIXER_BLEND_PIPE_CTL)
> > >      P1_EN
> > >
> > >    BLD_CH_RTCTL: (aka SUN8I_MIXER_BLEND_ROUTE)
> > >      P0_RTCTL channel0
> > >      P1_RTCTL channel3
> > >
> > >  2) After switch to console and back to Xorg:
> > >
> > >  0x01101000 : 00000301
> > >  0x01101080 : 00000031
> > >
> > >    BLD_FILL_COLOR_CTL:
> > >      P1_EN and P0_EN
> > >
> > >    BLD_CH_RTCTL:
> > >      P0_RTCTL channel1
> > >      P1_RTCTL channel3
> > >
> > >  What happens is that sun8i_ui_layer_enable() function may disable
> > >  blending pipes even if it is no longer assigned to its layer, because
> > >  of incorrect state/zpos tracking in the driver.
> > >
> > >  In particular, layer 1 is configured to zpos 0 and thus uses pipe 0.
> > >  When layer 3 is enabled by X server, sun8i_ui_layer_enable() will get
> > >  called with old_zpos=0 zpos=1, which will lead to disabling of pipe 0.
> > >
> > >  In general this issue can happen to any layer during enable or zpos
> > >  changes on multiple layers at once.
> > >
> > >  To correct this we now pass previous enabled/disabled state of the
> > >  layer, and pass real previous zpos of the layer to
> > >  sun8i_ui_layer_enable() and rework the sun8i_ui_layer_enable() function
> > >  to react to the state changes correctly. In order to not complicate
> > >  the atomic_disable callback with all of the above changes, we simply
> > >  remove it and implement all the chanes as part of atomic_update, which
> > >  also reduces the code duplication.
> >
> > I'm not even sure why we need that old state. Can't we just reset
> > completely the whole thing and do the configuration all over again?
>
> That would be nice from a dev standpoint if we can get a complete state for all
> planes at once from DRM core, but how? DRM helper gives callbacks
> for updating individual planes with prev and new state. These individual layer
> change notifications don't map nicely to how pipes are represented in the mixer
> registers.

You have a pointer to the full DRM state in the state field, so you
have that option.

The other option is just to clear everything in atomic_begin, update
each plane in atomic_update, and then trigger the readout of the new
register values in atomic_commit.

> > That description just looks to me like the reset is not done properly,
> > and now we have to deal with the fallouts later on.
>
> What in particular?

Having to care about the old state? If the reset was done properly, we
wouldn't have to care.

> > >  To make this all work, initial zpos positions of all layers need to be
> > >  restored to initial values on reset.
> >
> > And this also fixes a whole other bunch of issues, and can be made
> > very trivially in a separate patch.
>
> Maybe reset should also reset registers?

The reset callback actually does the opposite, it resets a DRM
state. If anything, we want to initialize the state in reset by
reading the registers, not the opposite.

Maxime

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