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

From: Jernej Škrabec
Date: Mon Jun 06 2022 - 17:27:14 EST

Next message: Peter Xu: "Re: [PATCH AUTOSEL 5.16 07/28] x86/kvm/fpu: Limit guest user_xfeatures to supported bits of XCR0"
Previous message: Rob Clark: "Re: [PATCH] drm/msm: Fix double pm_runtime_disable() call"
In reply to: Roman Stratiienko: "Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>:
> >
> > 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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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)
> >
> >
> >
> >
>

Next message: Peter Xu: "Re: [PATCH AUTOSEL 5.16 07/28] x86/kvm/fpu: Limit guest user_xfeatures to supported bits of XCR0"
Previous message: Rob Clark: "Re: [PATCH] drm/msm: Fix double pm_runtime_disable() call"
In reply to: Roman Stratiienko: "Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]