On Sun, Sep 15, 2019 at 12:03:37AM +0200, megous hlavni wrote: > From: Ondrej Jirman <meg...@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 <meg...@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 >
