Re: Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
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 :
> >
> > 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
> > > ---
> > > 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 |=
Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
Hi Maxime, пн, 6 июн. 2022 г. в 12:22, Maxime Ripard : > > On Mon, Jun 06, 2022 at 11:20:06AM +0200, Maxime Ripard wrote: > > On Mon, Jun 06, 2022 at 11:17:20AM +0300, Roman Stratiienko wrote: > > > Hello Jernej, > > > > > > Thank you for having a look. > > > > > > вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec : > > > > > > > > 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. > > > > KMS assumes pre-multiplied alpha anyway, so we shouldn't need a > > parameter to force it: we're always going to force it. > > My bad, I got confused with your other patch. > > Still, I agree with Jernej, if it's not needed it only complicates the > code for no particular reason. If you need it at some point in the > future, add it then. Otherwise, it's hard to reason about, since we > don't know what are the expectations that those future patches will > bring. Well, existing code already has some sort of support for the sub-overlays: For example 'int overlay, ' parameter is always 0, but it still passed through the call stack. Therefore this patch is just aligned with already existing traditions to keep sub-overlays in mind. > > Maxime Regards, Roman
Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
On Mon, Jun 06, 2022 at 11:20:06AM +0200, Maxime Ripard wrote: > On Mon, Jun 06, 2022 at 11:17:20AM +0300, Roman Stratiienko wrote: > > Hello Jernej, > > > > Thank you for having a look. > > > > вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec : > > > > > > 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. > > KMS assumes pre-multiplied alpha anyway, so we shouldn't need a > parameter to force it: we're always going to force it. My bad, I got confused with your other patch. Still, I agree with Jernej, if it's not needed it only complicates the code for no particular reason. If you need it at some point in the future, add it then. Otherwise, it's hard to reason about, since we don't know what are the expectations that those future patches will bring. Maxime signature.asc Description: PGP signature
Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
On Mon, Jun 06, 2022 at 11:17:20AM +0300, Roman Stratiienko wrote: > Hello Jernej, > > Thank you for having a look. > > вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec : > > > > 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. KMS assumes pre-multiplied alpha anyway, so we shouldn't need a parameter to force it: we're always going to force it. > 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. Sounds like a good job for a comment in the source code. Maxime signature.asc Description: PGP signature
Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
Hello Jernej,
Thank you for having a look.
вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec :
>
> 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
> > ---
> > 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
Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
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.
>
> Test:
> Manually tested all the modes using kmsxx python wrapper with and
> without 'force_premulti' flag enabled.
>
> Signed-off-by: Roman Stratiienko
> ---
> 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);
>
