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

From: OndÅej Jirman
Date: Sat Sep 14 2019 - 18:15:18 EST

Next message: Martin Steigerwald: "Re: Linux 5.3-rc8"
Previous message: megous: "[PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting"
In reply to: megous: "[PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Martin Steigerwald: "Re: Linux 5.3-rc8"
Previous message: megous: "[PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting"
In reply to: megous: "[PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]