[PATCH v2] drm/sun4i: Enable output signal premultiplication for DE2/DE3

[PATCH v2] drm/sun4i: Enable output signal premultiplication for DE2/DE3

[Roman Stratiienko]

Otherwise alpha value is discarded, resulting incorrect pixel
apperance on the display.

This also fixes missing transparency for the most bottom layer.

Test applications and videos w/ w/o this patch are available at [1].

[1]: https://github.com/GloDroid/glodroid_tests/issues/1
Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>

---
Changelog:

V2: Added code hunk missing in v1
---
 drivers/gpu/drm/sun4i/sun8i_mixer.c | 5 +++--
 drivers/gpu/drm/sun4i/sun8i_mixer.h | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index 6b1711a9a71f..ba2932aaed08 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -320,8 +320,9 @@ static void sun8i_mixer_mode_set(struct sunxi_engine *engine,
 	else
 		val = 0;
 
-	regmap_update_bits(engine->regs, SUN8I_MIXER_BLEND_OUTCTL(bld_base),
-			   SUN8I_MIXER_BLEND_OUTCTL_INTERLACED, val);
+	val |= SUN8I_MIXER_BLEND_OUTCTL_PREMULTIPLY;
+
+	regmap_write(engine->regs, SUN8I_MIXER_BLEND_OUTCTL(bld_base), val);
 
 	DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
 			 interlaced ? "on" : "off");
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
index ebfc276b2464..bc12c95af6f3 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
@@ -65,6 +65,7 @@
 #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_OUTCTL_PREMULTIPLY	BIT(0)
 #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED	BIT(1)
 
 #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)	BIT(ch)
-- 
2.30.2

Re: [PATCH v2] drm/sun4i: Enable output signal premultiplication for DE2/DE3

[Jernej Škrabec]

Dne nedelja, 05. junij 2022 ob 11:40:18 CEST je Roman Stratiienko napisal(a):
> Otherwise alpha value is discarded, resulting incorrect pixel
> apperance on the display.
> 
> This also fixes missing transparency for the most bottom layer.

Can you explain that a bit more? Also, BSP driver never enables this bit. What 
are we doing differently?

> 
> Test applications and videos w/ w/o this patch are available at [1].
> 
> [1]: https://github.com/GloDroid/glodroid_tests/issues/1

As stated in other emails, commit messages should not contain external links 
(per patch rules).

Best regards,
Jernej

> Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
> 
> ---
> Changelog:
> 
> V2: Added code hunk missing in v1
> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.c | 5 +++--
>  drivers/gpu/drm/sun4i/sun8i_mixer.h | 1 +
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 6b1711a9a71f..ba2932aaed08
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -320,8 +320,9 @@ static void sun8i_mixer_mode_set(struct sunxi_engine
> *engine, else
>  		val = 0;
> 
> -	regmap_update_bits(engine->regs, 
SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> -			   SUN8I_MIXER_BLEND_OUTCTL_INTERLACED, 
val);
> +	val |= SUN8I_MIXER_BLEND_OUTCTL_PREMULTIPLY;
> +
> +	regmap_write(engine->regs, SUN8I_MIXER_BLEND_OUTCTL(bld_base), 
val);
> 
>  	DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
>  			 interlaced ? "on" : "off");
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> b/drivers/gpu/drm/sun4i/sun8i_mixer.h index ebfc276b2464..bc12c95af6f3
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -65,6 +65,7 @@
>  #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_OUTCTL_PREMULTIPLY	BIT(0)
>  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED	BIT(1)
> 
>  #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)	BIT(ch)

Re: [PATCH v2] drm/sun4i: Enable output signal premultiplication for DE2/DE3

[Roman Stratiienko]

Hi,

вс, 5 июн. 2022 г. в 23:23, Jernej Škrabec <jernej.skrabec@gmail.com>:
>
> Dne nedelja, 05. junij 2022 ob 11:40:18 CEST je Roman Stratiienko napisal(a):
> > Otherwise alpha value is discarded, resulting incorrect pixel
> > apperance on the display.
> >
> > This also fixes missing transparency for the most bottom layer.
>
> Can you explain that a bit more?

Well... I would recommend reading Bartosz Ciechanowski's blog
https://ciechanow.ski/alpha-compositing/ or the Porter-Duff's 1984
whitepaper itself.

HINT: That magic numbers from sun8i_mixer.h ( 0x03010301 ) corresponds
to SOURCE OVER mode.

As you can see from the blending equation it outputs both pixel value
and alpha value (non-premultiplied data mode).

Also single-layer non-premultiplied buffers may have for example
(R255,G255,B255,A2) pixel value, which should be sent as {R2, G2, B2}
through the physical display interface.

When OUTCTL.PREMULTI disabled pixel, the RGB values passes as is, and
even 100% transparent data {R255, G255, B255, A0} will appear as 100%
opaque white.

>Also, BSP driver never enables this bit. What are we doing differently?

Are you sure the BSP does not have an issues with presenting
transparent buffers?
Does the sunxi even have a customer-feedback mechanism and publicly
available stable BSP with all the fixes?

Regards,
Roman

>
> >
> > Test applications and videos w/ w/o this patch are available at [1].
> >
> > [1]: https://github.com/GloDroid/glodroid_tests/issues/1
>
> As stated in other emails, commit messages should not contain external links
> (per patch rules).
>
> Best regards,
> Jernej
>
> > Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
> >
> > ---
> > Changelog:
> >
> > V2: Added code hunk missing in v1
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_mixer.c | 5 +++--
> >  drivers/gpu/drm/sun4i/sun8i_mixer.h | 1 +
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 6b1711a9a71f..ba2932aaed08
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > @@ -320,8 +320,9 @@ static void sun8i_mixer_mode_set(struct sunxi_engine
> > *engine, else
> >               val = 0;
> >
> > -     regmap_update_bits(engine->regs,
> SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> > -                        SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
> val);
> > +     val |= SUN8I_MIXER_BLEND_OUTCTL_PREMULTIPLY;
> > +
> > +     regmap_write(engine->regs, SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> val);
> >
> >       DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
> >                        interlaced ? "on" : "off");
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index ebfc276b2464..bc12c95af6f3
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > @@ -65,6 +65,7 @@
> >  #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_OUTCTL_PREMULTIPLY BIT(0)
> >  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED  BIT(1)
> >
> >  #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)    BIT(ch)
>
>
>
>

Re: [PATCH v2] drm/sun4i: Enable output signal premultiplication for DE2/DE3

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1381 bytes --]

On Mon, Jun 06, 2022 at 01:16:06PM +0300, Roman Stratiienko wrote:
> вс, 5 июн. 2022 г. в 23:23, Jernej Škrabec <jernej.skrabec@gmail.com>:
> >
> > Dne nedelja, 05. junij 2022 ob 11:40:18 CEST je Roman Stratiienko napisal(a):
> > > Otherwise alpha value is discarded, resulting incorrect pixel
> > > apperance on the display.
> > >
> > > This also fixes missing transparency for the most bottom layer.
> >
> > Can you explain that a bit more?
> 
> Well... I would recommend reading Bartosz Ciechanowski's blog
> https://ciechanow.ski/alpha-compositing/ or the Porter-Duff's 1984
> whitepaper itself.
> 
> HINT: That magic numbers from sun8i_mixer.h ( 0x03010301 ) corresponds
> to SOURCE OVER mode.
> 
> As you can see from the blending equation it outputs both pixel value
> and alpha value (non-premultiplied data mode).
> 
> Also single-layer non-premultiplied buffers may have for example
> (R255,G255,B255,A2) pixel value, which should be sent as {R2, G2, B2}
> through the physical display interface.
> 
> When OUTCTL.PREMULTI disabled pixel, the RGB values passes as is, and
> even 100% transparent data {R255, G255, B255, A0} will appear as 100%
> opaque white.

Without going into the full explanation about what alpha is, your commit
log must describe what the bug is exactly, and most importantly how do
you trigger it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] drm/sun4i: Enable output signal premultiplication for DE2/DE3

[Roman Stratiienko]

ср, 8 июн. 2022 г. в 11:17, Maxime Ripard <maxime@cerno.tech>:
>
> On Mon, Jun 06, 2022 at 01:16:06PM +0300, Roman Stratiienko wrote:
> > вс, 5 июн. 2022 г. в 23:23, Jernej Škrabec <jernej.skrabec@gmail.com>:
> > >
> > > Dne nedelja, 05. junij 2022 ob 11:40:18 CEST je Roman Stratiienko napisal(a):
> > > > Otherwise alpha value is discarded, resulting incorrect pixel
> > > > apperance on the display.
> > > >
> > > > This also fixes missing transparency for the most bottom layer.
> > >
> > > Can you explain that a bit more?
> >
> > Well... I would recommend reading Bartosz Ciechanowski's blog
> > https://ciechanow.ski/alpha-compositing/ or the Porter-Duff's 1984
> > whitepaper itself.
> >
> > HINT: That magic numbers from sun8i_mixer.h ( 0x03010301 ) corresponds
> > to SOURCE OVER mode.
> >
> > As you can see from the blending equation it outputs both pixel value
> > and alpha value (non-premultiplied data mode).
> >
> > Also single-layer non-premultiplied buffers may have for example
> > (R255,G255,B255,A2) pixel value, which should be sent as {R2, G2, B2}
> > through the physical display interface.
> >
> > When OUTCTL.PREMULTI disabled pixel, the RGB values passes as is, and
> > even 100% transparent data {R255, G255, B255, A0} will appear as 100%
> > opaque white.
>
> Without going into the full explanation about what alpha is, your commit
> log must describe what the bug is exactly, and most importantly how do
> you trigger it.

I do not understand what you want me to add. I checked alpha
appearance manually by
preparing framebuffers with data and presenting it on the display in
various combinations.

I attached the videos and tests as a proof. If you don't believe me
you can always check.

If you find something missing in the commit message or don't like to
see external links feel
free to amend it. From my point of view the patch is complete.

>
> Maxime

Roman

Re: [PATCH v2] drm/sun4i: Enable output signal premultiplication for DE2/DE3

[Maxime Ripard]

On Wed, Jun 08, 2022 at 07:44:03PM +0300, Roman Stratiienko wrote:
> ср, 8 июн. 2022 г. в 11:17, Maxime Ripard <maxime@cerno.tech>:
> >
> > On Mon, Jun 06, 2022 at 01:16:06PM +0300, Roman Stratiienko wrote:
> > > вс, 5 июн. 2022 г. в 23:23, Jernej Škrabec <jernej.skrabec@gmail.com>:
> > > >
> > > > Dne nedelja, 05. junij 2022 ob 11:40:18 CEST je Roman Stratiienko napisal(a):
> > > > > Otherwise alpha value is discarded, resulting incorrect pixel
> > > > > apperance on the display.
> > > > >
> > > > > This also fixes missing transparency for the most bottom layer.
> > > >
> > > > Can you explain that a bit more?
> > >
> > > Well... I would recommend reading Bartosz Ciechanowski's blog
> > > https://ciechanow.ski/alpha-compositing/ or the Porter-Duff's 1984
> > > whitepaper itself.
> > >
> > > HINT: That magic numbers from sun8i_mixer.h ( 0x03010301 ) corresponds
> > > to SOURCE OVER mode.
> > >
> > > As you can see from the blending equation it outputs both pixel value
> > > and alpha value (non-premultiplied data mode).
> > >
> > > Also single-layer non-premultiplied buffers may have for example
> > > (R255,G255,B255,A2) pixel value, which should be sent as {R2, G2, B2}
> > > through the physical display interface.
> > >
> > > When OUTCTL.PREMULTI disabled pixel, the RGB values passes as is, and
> > > even 100% transparent data {R255, G255, B255, A0} will appear as 100%
> > > opaque white.
> >
> > Without going into the full explanation about what alpha is, your commit
> > log must describe what the bug is exactly, and most importantly how do
> > you trigger it.
> 
> I do not understand what you want me to add.

Context.

Again, what the bug is exactly and how it fails, so things like what
setup triggers it, what exactly is wrong with it (is the layer not
displayed at all, corrupted, something else?), etc.

> I checked alpha appearance manually by preparing framebuffers with
> data and presenting it on the display in various combinations.
> 
> I attached the videos and tests as a proof. If you don't believe me
> you can always check.

It's not about believing you.

Let's say that in a couple of years it turns out that this patch
introduced a regression, or something just isn't clear about it.

We go have a look at the commit message, and it just points to a video
that isn't hosted anywhere anymore, because you closed your hosting
account, removed it, whatever.

Then what?

That's why we ask to have the whole context in your message, because
otherwise you introduce a dependency on something Linux as a whole has
no control over. And if it's not there anymore, your current commit
message is useless.

> If you find something missing in the commit message or don't like to
> see external links feel free to amend it. From my point of view the
> patch is complete.

I'm sorry you feel that way, but I don't really get your attitude there.
What did you expect from reviews in general if you're just going to
ignore anything we might say?

Maxime