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

From: Roman Stratiienko
Date: Mon Jun 06 2022 - 04:17:44 EST

Next message: Sameer Pujar: "Re: [PATCH v3 1/6] ASoC: tegra: Add binding doc for OPE module"
Previous message: Aneesh Kumar K V: "Re: [RFC PATCH v4 1/7] mm/demotion: Add support for explicit memory tiers"
In reply to: Jernej Škrabec: "Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property"
Next in thread: Maxime Ripard: "Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

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

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: Sameer Pujar: "Re: [PATCH v3 1/6] ASoC: tegra: Add binding doc for OPE module"
Previous message: Aneesh Kumar K V: "Re: [RFC PATCH v4 1/7] mm/demotion: Add support for explicit memory tiers"
In reply to: Jernej Škrabec: "Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property"
Next in thread: Maxime Ripard: "Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]