[PATCH v2] drm/sun4i: sun8i: Avoid clearing blending order at each atomic commit

[PATCH v2] drm/sun4i: sun8i: Avoid clearing blending order at each atomic commit

[Paul Kocialkowski]

Blending order is set based on the z position of each DRM plane. The
blending order register is currently cleared at each atomic DRM commit,
with the intent that each committed plane will set the appropriate
bits (based on its z-pos) when enabling the plane.

However, it sometimes happens that a particular plane is left unchanged
by an atomic commit and thus will not be configured again. In that
scenario, blending order is cleared and only the bits relevant for the
planes affected by the commit are set. This leaves the planes that did
not change without their blending order set in the register, leading
to that plane not being displayed.

Instead of clearing the blending order register at every atomic commit,
this change moves the register's initial clear at bind time and only
clears the bits for a specific plane when disabling it or changing its
zpos.

This way, planes that are left untouched by a DRM atomic commit are
no longer disabled.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.c    | 15 +++------------
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 24 ++++++++++++++++++++----
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 24 ++++++++++++++++++++----
 3 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index 8e81c24d736e..12cb7183ce51 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -260,17 +260,6 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32 format)
 	return NULL;
 }
 
-static void sun8i_mixer_atomic_begin(struct sunxi_engine *engine,
-				     struct drm_crtc_state *old_state)
-{
-	/*
-	 * Disable all pipes at the beginning. They will be enabled
-	 * again if needed in plane update callback.
-	 */
-	regmap_update_bits(engine->regs, SUN8I_MIXER_BLEND_PIPE_CTL,
-			   SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
-}
-
 static void sun8i_mixer_commit(struct sunxi_engine *engine)
 {
 	DRM_DEBUG_DRIVER("Committing changes\n");
@@ -322,7 +311,6 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
 }
 
 static const struct sunxi_engine_ops sun8i_engine_ops = {
-	.atomic_begin	= sun8i_mixer_atomic_begin,
 	.commit		= sun8i_mixer_commit,
 	.layers_init	= sun8i_layers_init,
 };
@@ -449,6 +437,9 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
 		regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_MODE(i),
 			     SUN8I_MIXER_BLEND_MODE_DEF);
 
+	regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL,
+			   SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
+
 	return 0;
 
 err_disable_bus_clk:
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index 518e1921f47e..28c15c6ef1ef 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -27,7 +27,8 @@
 #include "sun8i_ui_scaler.h"
 
 static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
-				  int overlay, bool enable, unsigned int zpos)
+				  int overlay, bool enable, unsigned int zpos,
+				  unsigned int old_zpos)
 {
 	u32 val;
 
@@ -43,6 +44,18 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
 			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(channel, 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,
+				   SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
+				   0);
+
+		regmap_update_bits(mixer->engine.regs,
+				   SUN8I_MIXER_BLEND_ROUTE,
+				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
+				   0);
+	}
+
 	if (enable) {
 		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
 
@@ -242,9 +255,11 @@ 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);
+	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false, 0,
+			      old_zpos);
 }
 
 static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
@@ -252,11 +267,12 @@ 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);
+				      layer->overlay, false, 0, old_zpos);
 		return;
 	}
 
@@ -267,7 +283,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);
+			      true, zpos, old_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 17e0d00cfd8a..f4fe97813f94 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -21,7 +21,8 @@
 #include "sun8i_vi_scaler.h"
 
 static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
-				  int overlay, bool enable, unsigned int zpos)
+				  int overlay, bool enable, unsigned int zpos,
+				  unsigned int old_zpos)
 {
 	u32 val;
 
@@ -37,6 +38,18 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
 			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR(channel, 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,
+				   SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
+				   0);
+
+		regmap_update_bits(mixer->engine.regs,
+				   SUN8I_MIXER_BLEND_ROUTE,
+				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
+				   0);
+	}
+
 	if (enable) {
 		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
 
@@ -270,9 +283,11 @@ 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);
+	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false, 0,
+			      old_zpos);
 }
 
 static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
@@ -280,11 +295,12 @@ 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);
+				      layer->overlay, false, 0, old_zpos);
 		return;
 	}
 
@@ -295,7 +311,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);
+			      true, zpos, old_zpos);
 }
 
 static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
-- 
2.17.1

Re: [PATCH v2] drm/sun4i: sun8i: Avoid clearing blending order at each atomic commit

[Paul Kocialkowski]

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

On Tue, 2018-07-17 at 14:25 +0200, Paul Kocialkowski wrote:
> Blending order is set based on the z position of each DRM plane. The
> blending order register is currently cleared at each atomic DRM commit,
> with the intent that each committed plane will set the appropriate
> bits (based on its z-pos) when enabling the plane.
> 
> However, it sometimes happens that a particular plane is left unchanged
> by an atomic commit and thus will not be configured again. In that
> scenario, blending order is cleared and only the bits relevant for the
> planes affected by the commit are set. This leaves the planes that did
> not change without their blending order set in the register, leading
> to that plane not being displayed.
> 
> Instead of clearing the blending order register at every atomic commit,
> this change moves the register's initial clear at bind time and only
> clears the bits for a specific plane when disabling it or changing its
> zpos.
> 
> This way, planes that are left untouched by a DRM atomic commit are
> no longer disabled.

This patch was rebased to apply on top of DRM misc. V1 had been based on
the first revision of the DE2 z-pos support patch, while a subsequent
revision of the patch made it to the kernel tree.

Cheers!

Paul

> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.c    | 15 +++------------
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 24 ++++++++++++++++++++----
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 24 ++++++++++++++++++++----
>  3 files changed, 43 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index 8e81c24d736e..12cb7183ce51 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -260,17 +260,6 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32 format)
>  	return NULL;
>  }
>  
> -static void sun8i_mixer_atomic_begin(struct sunxi_engine *engine,
> -				     struct drm_crtc_state *old_state)
> -{
> -	/*
> -	 * Disable all pipes at the beginning. They will be enabled
> -	 * again if needed in plane update callback.
> -	 */
> -	regmap_update_bits(engine->regs, SUN8I_MIXER_BLEND_PIPE_CTL,
> -			   SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
> -}
> -
>  static void sun8i_mixer_commit(struct sunxi_engine *engine)
>  {
>  	DRM_DEBUG_DRIVER("Committing changes\n");
> @@ -322,7 +311,6 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
>  }
>  
>  static const struct sunxi_engine_ops sun8i_engine_ops = {
> -	.atomic_begin	= sun8i_mixer_atomic_begin,
>  	.commit		= sun8i_mixer_commit,
>  	.layers_init	= sun8i_layers_init,
>  };
> @@ -449,6 +437,9 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>  		regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_MODE(i),
>  			     SUN8I_MIXER_BLEND_MODE_DEF);
>  
> +	regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL,
> +			   SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
> +
>  	return 0;
>  
>  err_disable_bus_clk:
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> index 518e1921f47e..28c15c6ef1ef 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> @@ -27,7 +27,8 @@
>  #include "sun8i_ui_scaler.h"
>  
>  static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
> -				  int overlay, bool enable, unsigned int zpos)
> +				  int overlay, bool enable, unsigned int zpos,
> +				  unsigned int old_zpos)
>  {
>  	u32 val;
>  
> @@ -43,6 +44,18 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
>  			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(channel, 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,
> +				   SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> +				   0);
> +
> +		regmap_update_bits(mixer->engine.regs,
> +				   SUN8I_MIXER_BLEND_ROUTE,
> +				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> +				   0);
> +	}
> +
>  	if (enable) {
>  		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
>  
> @@ -242,9 +255,11 @@ 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);
> +	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false, 0,
> +			      old_zpos);
>  }
>  
>  static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> @@ -252,11 +267,12 @@ 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);
> +				      layer->overlay, false, 0, old_zpos);
>  		return;
>  	}
>  
> @@ -267,7 +283,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);
> +			      true, zpos, old_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 17e0d00cfd8a..f4fe97813f94 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> @@ -21,7 +21,8 @@
>  #include "sun8i_vi_scaler.h"
>  
>  static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
> -				  int overlay, bool enable, unsigned int zpos)
> +				  int overlay, bool enable, unsigned int zpos,
> +				  unsigned int old_zpos)
>  {
>  	u32 val;
>  
> @@ -37,6 +38,18 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
>  			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR(channel, 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,
> +				   SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> +				   0);
> +
> +		regmap_update_bits(mixer->engine.regs,
> +				   SUN8I_MIXER_BLEND_ROUTE,
> +				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> +				   0);
> +	}
> +
>  	if (enable) {
>  		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
>  
> @@ -270,9 +283,11 @@ 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);
> +	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false, 0,
> +			      old_zpos);
>  }
>  
>  static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> @@ -280,11 +295,12 @@ 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);
> +				      layer->overlay, false, 0, old_zpos);
>  		return;
>  	}
>  
> @@ -295,7 +311,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);
> +			      true, zpos, old_zpos);
>  }
>  
>  static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: [PATCH v2] drm/sun4i: sun8i: Avoid clearing blending order at each atomic commit

[Maxime Ripard]

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

On Tue, Jul 17, 2018 at 02:25:22PM +0200, Paul Kocialkowski wrote:
> Blending order is set based on the z position of each DRM plane. The
> blending order register is currently cleared at each atomic DRM commit,
> with the intent that each committed plane will set the appropriate
> bits (based on its z-pos) when enabling the plane.
> 
> However, it sometimes happens that a particular plane is left unchanged
> by an atomic commit and thus will not be configured again. In that
> scenario, blending order is cleared and only the bits relevant for the
> planes affected by the commit are set. This leaves the planes that did
> not change without their blending order set in the register, leading
> to that plane not being displayed.
> 
> Instead of clearing the blending order register at every atomic commit,
> this change moves the register's initial clear at bind time and only
> clears the bits for a specific plane when disabling it or changing its
> zpos.
> 
> This way, planes that are left untouched by a DRM atomic commit are
> no longer disabled.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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