Re: [PATCH 08/30] drm/sun4i: ui layer: Write attributes in one go

["Jernej Škrabec" <jernej.skrabec@gmail.com>]

Dne ponedeljek, 27. oktober 2025 ob 17:04:38 Srednjeevropski standardni čas je Chen-Yu Tsai napisal(a):
> On Mon, Oct 13, 2025 at 3:23 AM Jernej Skrabec <jernej.skrabec@gmail.com> wrote:
> >
> > It turns out that none of the UI channel registers were meant to be
> > read. Mostly it works fine but sometimes it returns incorrect values.
> >
> > Rework UI layer code to write all registers in one go to avoid reads.
> >
> > This rework will also allow proper code separation.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> 
> Reviewed-by: Chen-Yu Tsai <wens@kernel.org>
> 
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 50 +++++++++-----------------
> >  1 file changed, 16 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > index 12c83c54f9bc..8634d2ee613a 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > @@ -25,25 +25,27 @@
> >  #include "sun8i_ui_scaler.h"
> >  #include "sun8i_vi_scaler.h"
> >
> > -static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int channel,
> > -                                       int overlay, struct drm_plane *plane)
> > +static void sun8i_ui_layer_update_attributes(struct sun8i_mixer *mixer,
> > +                                            int channel, int overlay,
> > +                                            struct drm_plane *plane)
> >  {
> > -       u32 mask, val, ch_base;
> > +       struct drm_plane_state *state = plane->state;
> > +       const struct drm_format_info *fmt;
> > +       u32 val, ch_base, hw_fmt;
> >
> >         ch_base = sun8i_channel_base(mixer, channel);
> > +       fmt = state->fb->format;
> > +       sun8i_mixer_drm_format_to_hw(fmt->format, &hw_fmt);
> >
> > -       mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK |
> > -               SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK;
> > -
> > -       val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(plane->state->alpha >> 8);
> > -
> > -       val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
> > +       val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(state->alpha >> 8);
> > +       val |= (state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
> 
> Changing "plane->state" to "state" made the diff somewhat harder to read.

Do you prefer that I change that in next revision?

Best regards,
Jernej

> 
> >                 SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> >                 SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +       val |= hw_fmt << SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_OFFSET;
> > +       val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN;
> >
> > -       regmap_update_bits(mixer->engine.regs,
> > -                          SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
> > -                          mask, val);
> > +       regmap_write(mixer->engine.regs,
> > +                    SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay), val);
> >  }
> >
> >  static void sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel,
> > @@ -111,24 +113,6 @@ static void sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel,
> >         }
> >  }
> >
> > -static void sun8i_ui_layer_update_formats(struct sun8i_mixer *mixer, int channel,
> > -                                         int overlay, struct drm_plane *plane)
> > -{
> > -       struct drm_plane_state *state = plane->state;
> > -       const struct drm_format_info *fmt;
> > -       u32 val, ch_base, hw_fmt;
> > -
> > -       ch_base = sun8i_channel_base(mixer, channel);
> > -
> > -       fmt = state->fb->format;
> > -       sun8i_mixer_drm_format_to_hw(fmt->format, &hw_fmt);
> > -
> > -       val = hw_fmt << SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_OFFSET;
> > -       regmap_update_bits(mixer->engine.regs,
> > -                          SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
> > -                          SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_MASK, val);
> > -}
> > -
> >  static void sun8i_ui_layer_update_buffer(struct sun8i_mixer *mixer, int channel,
> >                                          int overlay, struct drm_plane *plane)
> >  {
> > @@ -220,12 +204,10 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> >         if (!new_state->crtc || !new_state->visible)
> >                 return;
> >
> > +       sun8i_ui_layer_update_attributes(mixer, layer->channel,
> > +                                        layer->overlay, plane);
> >         sun8i_ui_layer_update_coord(mixer, layer->channel,
> >                                     layer->overlay, plane);
> > -       sun8i_ui_layer_update_alpha(mixer, layer->channel,
> > -                                   layer->overlay, plane);
> > -       sun8i_ui_layer_update_formats(mixer, layer->channel,
> > -                                     layer->overlay, plane);
> >         sun8i_ui_layer_update_buffer(mixer, layer->channel,
> >                                      layer->overlay, plane);
> >  }
> > --
> > 2.51.0
> >
> >
>