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

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