Re: Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property

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

Dne ponedeljek, 06. junij 2022 ob 10:17:20 CEST je Roman Stratiienko 
napisal(a):
> Hello Jernej,
> 
> Thank you for having a look.
> 
> вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> >
> > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko 
napisal(a):
> > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > "None", "Pre-multiplied", "Coverage"
> > >
> > > Add the blend mode property and route it to the appropriate registers.
> > >
> > > Note:
> > > "force_premulti" parameter was added to handle multi-overlay channel
> > > cases in future changes. It must be set to true for cases when more
> > > than 1 overlay layer is used within a channel and at least one of the
> > > overlay layers within a group uses premultiplied blending mode.
> >
> > Please remove this parameter. It's nothing special, so it can be easily 
added
> > once it's actually needed. For now, it only complicates code.
> 
> I would prefer keeping it if you do not have any strong opinion against it.

Actually I do. Patch will be smaller and easier to follow if there is no extra 
variables with fixed values in it.

> 
> I am working now on exposing all overlays, so it will be needed soon anyway.

Well, it will just be one patch more there, if at all.

Regards,
Jernej

> 
> Also it helps to better understand the COV2PREMULT mode which has not
> the best description in the datasheet. Only after testing this
> register using devmem I became confident on its purpose.
> 
> >
> > >
> > > Test:
> > > Manually tested all the modes using kmsxx python wrapper with and
> > > without 'force_premulti' flag enabled.
> > >
> > > Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>
> > > ---
> > >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  2 ++
> > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 48 ++++++++++++++++++++-----
> > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.h |  5 +++
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 49 ++++++++++++++++++++++----
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.h |  5 +++
> > >  5 files changed, 94 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index ebfc276b2464..5c05907e26fb
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > @@ -65,6 +65,8 @@
> > >  #define SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(n)  (0xf << ((n) << 2))
> > >  #define SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(n)        ((n) << 2)
> > >
> > > +#define SUN8I_MIXER_BLEND_PREMULTIPLY_EN(pipe)       BIT(pipe)
> > > +
> > >  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED  BIT(1)
> > >
> > >  #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)    BIT(ch)
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index 6ccbbca3176d..
29c0d9cca19a
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > @@ -58,24 +58,46 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> > > *mixer, int channel, }
> > >
> > >  static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int
> > > channel, -                                    int overlay, struct
> > drm_plane *plane)
> > > +                                     int overlay, struct
> > drm_plane *plane,
> > > +                                     unsigned int zpos,
> > bool force_premulti)
> > >  {
> > > -     u32 mask, val, ch_base;
> > > +     u32 mask, val, ch_base, bld_base;
> > > +     bool in_premulti, out_premulti;
> > >
> > > +     bld_base = sun8i_blender_base(mixer);
> > >       ch_base = sun8i_channel_base(mixer, channel);
> > >
> > > +     in_premulti = plane->state->pixel_blend_mode ==
> > DRM_MODE_BLEND_PREMULTI;
> > > +     out_premulti = force_premulti || in_premulti;
> > > +
> > >       mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK |
> > > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK;
> > > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK |
> > > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK;
> > >
> > >       val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(plane->state->alpha >>
> > 8);
> > >
> > > -     val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
> > > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +     if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
> > > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > > +     } else {
> > > +             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> > ?
> > > +
> > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > +
> > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +
> > > +             if (in_premulti)
> > > +                     val |=
> > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI;
> > > +     }
> > > +
> > > +     if (!in_premulti && out_premulti)
> > > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM;
> > >
> > >       regmap_update_bits(mixer->engine.regs,
> > >                          SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base,
> > overlay),
> > >                          mask, val);
> > > +
> > > +     regmap_update_bits(
> > > +             mixer->engine.regs,
> > SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> > 0);
> > >  }
> > >
> > >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> > > channel, @@ -274,7 +296,7 @@ static void
> > > sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > > sun8i_ui_layer_update_coord(mixer, layer->channel,
> > >                                   layer->overlay, plane, zpos);
> > >       sun8i_ui_layer_update_alpha(mixer, layer->channel,
> > > -                                 layer->overlay, plane);
> > > +                                 layer->overlay, plane, zpos,
> > false);
> > >       sun8i_ui_layer_update_formats(mixer, layer->channel,
> > >                                     layer->overlay, plane);
> > >       sun8i_ui_layer_update_buffer(mixer, layer->channel,
> > > @@ -332,8 +354,8 @@ struct sun8i_ui_layer 
*sun8i_ui_layer_init_one(struct
> > > drm_device *drm, {
> > >       enum drm_plane_type type = DRM_PLANE_TYPE_OVERLAY;
> > >       int channel = mixer->cfg->vi_num + index;
> > > +     unsigned int plane_cnt, blend_modes;
> > >       struct sun8i_ui_layer *layer;
> > > -     unsigned int plane_cnt;
> > >       int ret;
> > >
> > >       layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
> > > @@ -362,6 +384,16 @@ struct sun8i_ui_layer 
*sun8i_ui_layer_init_one(struct
> > > drm_device *drm, return ERR_PTR(ret);
> > >       }
> > >
> > > +     blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
> > > +                   BIT(DRM_MODE_BLEND_COVERAGE) |
> > > +                   BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > > +
> > > +     ret = drm_plane_create_blend_mode_property(&layer->plane,
> > blend_modes);
> > > +     if (ret) {
> > > +             dev_err(drm->dev, "Couldn't add blend mode
> > property\n");
> > > +             return ERR_PTR(ret);
> > > +     }
> > > +
> > >       ret = drm_plane_create_zpos_property(&layer->plane, channel,
> > >                                            0, plane_cnt -
> > 1);
> > >       if (ret) {
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h index 
43c48cf7bc51..bd3c30e8058d
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > > @@ -46,6 +46,11 @@
> > >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER              
((1) << 1)
> > >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED   ((2) << 1)
> > >
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK    GENMASK(17, 16)
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COVERAGE        ((0) << 16)
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM        ((1) << 16)
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI        ((2) << 16)
> > > +
> > >  struct sun8i_mixer;
> > >
> > >  struct sun8i_ui_layer {
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 
662ba1018cc4..6581fc7d9668
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > @@ -52,21 +52,38 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> > > *mixer, int channel, }
> > >
> > >  static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int
> > > channel, -                                    int overlay, struct
> > drm_plane *plane)
> > > +                                     int overlay, struct
> > drm_plane *plane,
> > > +                                     unsigned int zpos,
> > bool force_premulti)
> > >  {
> > > -     u32 mask, val, ch_base;
> > > +     u32 mask, val, ch_base, bld_base;
> > > +     bool in_premulti = false, out_premulti = false;
> > >
> > >       ch_base = sun8i_channel_base(mixer, channel);
> > > +     bld_base = sun8i_blender_base(mixer);
> > >
> > >       if (mixer->cfg->is_de3) {
> > > +             in_premulti = plane->state->pixel_blend_mode ==
> > DRM_MODE_BLEND_PREMULTI;
> > > +             out_premulti = force_premulti || in_premulti;
> > > +
> > >               mask = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK |
> > > -                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK;
> > > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK
> > |
> > > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK;
> > >               val = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA
> > >                       (plane->state->alpha >> 8);
> > >
> > > -             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> > ?
> > > -
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > -
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +             if (plane->state->pixel_blend_mode ==
> > DRM_MODE_BLEND_PIXEL_NONE) {
> > > +                     val |=
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > > +             } else {
> > > +                     val |= (plane->state->alpha ==
> > DRM_BLEND_ALPHA_OPAQUE) ?
> > > +
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > +
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +
> > > +                     if (in_premulti)
> > > +                             val |=
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI;
> > > +             }
> > > +
> > > +             if (!in_premulti && out_premulti)
> > > +                     val |=
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM;
> > >
> > >               regmap_update_bits(mixer->engine.regs,
> > >
> > SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
> > > @@ -79,6 +96,11 @@ static void sun8i_vi_layer_update_alpha(struct
> > > sun8i_mixer *mixer, int channel, SUN8I_MIXER_FCC_GLOBAL_ALPHA
> > >                                       (plane->state->alpha
> > >> 8));
> > >       }
> > > +
> > > +     regmap_update_bits(
> > > +             mixer->engine.regs,
> > SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> > 0);
> >
> > Shouldn't be above block inside DE3 if block?
> 
> No, we have to update the value to non-premulti in case it was set to
> premulti by another layer.
> 
> 
> Regards,
> Roman.
> 
> >
> > Looks good otherwise.
> >
> > Best regards,
> > Jernej
> >
> > >  }
> > >
> > >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> > > channel, @@ -408,7 +430,7 @@ static void
> > > sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > > sun8i_vi_layer_update_coord(mixer, layer->channel,
> > >                                   layer->overlay, plane, zpos);
> > >       sun8i_vi_layer_update_alpha(mixer, layer->channel,
> > > -                                 layer->overlay, plane);
> > > +                                 layer->overlay, plane, zpos,
> > false);
> > >       sun8i_vi_layer_update_formats(mixer, layer->channel,
> > >                                     layer->overlay, plane);
> > >       sun8i_vi_layer_update_buffer(mixer, layer->channel,
> > > @@ -563,6 +585,19 @@ struct sun8i_vi_layer 
*sun8i_vi_layer_init_one(struct
> > > drm_device *drm, }
> > >       }
> > >
> > > +     if (mixer->cfg->is_de3) {
> > > +             unsigned int blend_modes = BIT(DRM_MODE_BLEND_PREMULTI)
> > |
> > > +
> > BIT(DRM_MODE_BLEND_COVERAGE) |
> > > +
> > BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > > +
> > > +             ret = drm_plane_create_blend_mode_property(&layer-
> > >plane,
> > > +
> > blend_modes);
> > > +             if (ret) {
> > > +                     dev_err(drm->dev, "Couldn't add blend mode
> > property\n");
> > > +                     return ERR_PTR(ret);
> > > +             }
> > > +     }
> > > +
> > >       ret = drm_plane_create_zpos_property(&layer->plane, index,
> > >                                            0, plane_cnt -
> > 1);
> > >       if (ret) {
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 
9939a4cc7a52..ccf91f09f1fe
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > @@ -44,6 +44,11 @@
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK   GENMASK(31, 24)
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)     ((x) << 24)
> > >
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK   GENMASK(17, 16)
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COVERAGE       ((0) << 16)
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM       ((1) << 16)
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI       ((2) << 16)
> > > +
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL     ((0) <<
> > 1)
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER     ((1) <<
> > 1)
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED  ((2) << 1)
> >
> >
> >
> >
>