From: Roman Stratiienko <[email protected]>
DE2.0 blender does not take into the account alpha channel of vi layer.
Thus makes overlaying of this layer totally opaque.
Using vi layer as bottom solves this issue.
Tested on Android.
Signed-off-by: Roman Stratiienko <[email protected]>
---
drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 33 -----------------------
drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 36 +++++++++++++++++++++++++-
2 files changed, 35 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index dd2a1c851939..25183badc85f 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -99,36 +99,6 @@ static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel,
insize = SUN8I_MIXER_SIZE(src_w, src_h);
outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
- if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
- bool interlaced = false;
- u32 val;
-
- DRM_DEBUG_DRIVER("Primary layer, updating global size W: %u H: %u\n",
- dst_w, dst_h);
- regmap_write(mixer->engine.regs,
- SUN8I_MIXER_GLOBAL_SIZE,
- outsize);
- regmap_write(mixer->engine.regs,
- SUN8I_MIXER_BLEND_OUTSIZE(bld_base), outsize);
-
- if (state->crtc)
- interlaced = state->crtc->state->adjusted_mode.flags
- & DRM_MODE_FLAG_INTERLACE;
-
- if (interlaced)
- val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
- else
- val = 0;
-
- regmap_update_bits(mixer->engine.regs,
- SUN8I_MIXER_BLEND_OUTCTL(bld_base),
- SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
- val);
-
- DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
- interlaced ? "on" : "off");
- }
-
/* Set height and width */
DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n",
state->src.x1 >> 16, state->src.y1 >> 16);
@@ -349,9 +319,6 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
if (!layer)
return ERR_PTR(-ENOMEM);
- if (index == 0)
- type = DRM_PLANE_TYPE_PRIMARY;
-
/* possible crtcs are set later */
ret = drm_universal_plane_init(drm, &layer->plane, 0,
&sun8i_ui_layer_funcs,
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index 07c27e6a4b77..49c4074e164f 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -116,6 +116,36 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
insize = SUN8I_MIXER_SIZE(src_w, src_h);
outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
+ if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
+ bool interlaced = false;
+ u32 val;
+
+ DRM_DEBUG_DRIVER("Primary layer, updating global size W: %u H: %u\n",
+ dst_w, dst_h);
+ regmap_write(mixer->engine.regs,
+ SUN8I_MIXER_GLOBAL_SIZE,
+ outsize);
+ regmap_write(mixer->engine.regs,
+ SUN8I_MIXER_BLEND_OUTSIZE(bld_base), outsize);
+
+ if (state->crtc)
+ interlaced = state->crtc->state->adjusted_mode.flags
+ & DRM_MODE_FLAG_INTERLACE;
+
+ if (interlaced)
+ val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
+ else
+ val = 0;
+
+ regmap_update_bits(mixer->engine.regs,
+ SUN8I_MIXER_BLEND_OUTCTL(bld_base),
+ SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
+ val);
+
+ DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
+ interlaced ? "on" : "off");
+ }
+
/* Set height and width */
DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n",
(state->src.x1 >> 16) & ~(format->hsub - 1),
@@ -445,6 +475,7 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
struct sun8i_mixer *mixer,
int index)
{
+ enum drm_plane_type type = DRM_PLANE_TYPE_OVERLAY;
struct sun8i_vi_layer *layer;
unsigned int plane_cnt;
int ret;
@@ -453,12 +484,15 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
if (!layer)
return ERR_PTR(-ENOMEM);
+ if (index == 0)
+ type = DRM_PLANE_TYPE_PRIMARY;
+
/* possible crtcs are set later */
ret = drm_universal_plane_init(drm, &layer->plane, 0,
&sun8i_vi_layer_funcs,
sun8i_vi_layer_formats,
ARRAY_SIZE(sun8i_vi_layer_formats),
- NULL, DRM_PLANE_TYPE_OVERLAY, NULL);
+ NULL, type, NULL);
if (ret) {
dev_err(drm->dev, "Couldn't initialize layer\n");
return ERR_PTR(ret);
--
2.17.1
Dne četrtek, 19. september 2019 ob 22:03:26 CEST je Roman Stratiienko
napisal(a):
> Hello guys,
>
> Actually, I beleive this is True solution, and current one is wrong. Let
> me explain why.
>
> De2. 0 was designed to match Android hwcomposer hal requirements IMO.
> You can easily agree with this conclusion by comparing Composer HAL and
> De2. 0 hardware manuals.
>
> I faced at least 4 issues when try to run Android using the mainline kernel
> sun8i mixer implementation. Current one, missing pixel formats (my previous
> patch), missing plane alpha and rotation properties. I plan to fix it and
> also send appropriate solution to the upstream.
Android and mainline Linux don't have necessarily same view how things should
work. Check how different Android version of Linux kernel was in the past.
Fortunately, they are converging now.
While I agree that HW was probably designed with Android in mind, that doesn't
mean that Android way is the best or only way of doing things. Android can
afford a lot of non-intuitive things because it's closed system and all IP
cores are used only in certain ways. You can't say that for general Linux
distro.
Would you say that advertising formats with alpha support is correct thing to
do if alpha blending is not supported? Put aside the fact that it makes it
easier to implement Android features for you and imagine some app which finds
first available overlay plane with ARGB8888 support. It puts it on top with
zpos property and gives it some transparent image. If that plane is VI, it
would look wrong and users would complain. At the end, it's not apps fault to
expect that if plane advertises format with alpha channel actually supports
transparency.
Regarding rotation, that core is not part of display pipeline. It takes one
buffer and writes transformed (rotated in this case) image to output buffer.
This principle is very definition of V4L2 M2M framework and should be handled
by it.
>
> To achieve optimal UI performance Android requires at least 4 ui layers to
> make screen composition. Current patch enables 4th plane usable.
Ah, your idea is to pretend that VI planes supports all features of UI planes
while in fact they not?
>
> As for using vi plane to display video. I assume that some of current users
> may have regression in their software, but it could be easily fixed. For
> example if vi layer isn't fullscreen and should be on top of the other
> layers, it can actually be placed on the bottom and overlayed with pictures
> with transparent rectangles in video region.
This idea is imposible to implement if VI plane becomes primary plane. Apps
wouldn't find overlay plane which suports YUV buffers and only one which does,
it's already taken by window manager for rendering windows.
Best regards,
Jernej
> But I assume most of users such as browser etc. uses GPU for that.
>
> And if you are watching fullscreen video, I can imagine only subtitles
> layer and advertizing layers on top of the video layers.
>
> чт, 19 сент. 2019 г., 21:15 Jernej Škrabec <[email protected]>:
> > Dne četrtek, 19. september 2019 ob 19:17:54 CEST je Maxime Ripard
> >
> > napisal(a):
> > > Hi,
> > >
> > > On Thu, Sep 19, 2019 at 03:37:03PM +0300,
> >
> > [email protected]
> >
> > wrote:
> > > > From: Roman Stratiienko <[email protected]>
> > > >
> > > > DE2.0 blender does not take into the account alpha channel of vi
> > > > layer.
> > > > Thus makes overlaying of this layer totally opaque.
> > > > Using vi layer as bottom solves this issue.
> >
> > What issue? Overlays don't have to be "full screen", thus missing support
> > for
> > alpha blending doesn't make it less valuable. And VI planes are already
> > placed
> > at the bottom (zpos = 0).
> >
> > > > Tested on Android.
> > > >
> > > > Signed-off-by: Roman Stratiienko <[email protected]>
> > >
> > > It sounds like a workaround more than an actual fix.
> > >
> > > If the VI planes can't use the alpha, then we should just stop
> > > reporting that format.
> > >
> > > Jernej, what do you think?
> >
> > Commit message is misleading. What this commit actually does is moving
> > primary
> > plane from first UI plane to bottom most plane, i.e. first VI plane.
> > However, VI
> > planes are scarce resource, almost all mixers have only one. I wouldn't
> > set it
> > as primary, because it's the only one which provide support for YUV
> > formats.
> > That could be used for example by video player for zero-copy rendering.
> > Probably most apps wouldn't touch it if it was primary (that's usually
> > reserved for window manager, if used).
> >
> > I left few formats with alpha channel exposed by VI planes, just because
> > they
> > don't have equivalent format without alpha. But I'm fine with removing
> > them if
> > you all agree on that.
> >
> > Best regards,
> > Jernej
> >
> > > Maxime
> > >
> > > > ---
> > > >
> > > > drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 33 -----------------------
> > > > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 36
> > > > +++++++++++++++++++++++++-
> > > > 2 files changed, 35 insertions(+), 34 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index
> >
> > dd2a1c851939..25183badc85f
> >
> > > > 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > @@ -99,36 +99,6 @@ static int sun8i_ui_layer_update_coord(struct
> > > > sun8i_mixer *mixer, int channel,>
> > > >
> > > > insize = SUN8I_MIXER_SIZE(src_w, src_h);
> > > > outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
> > > >
> > > > - if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > > > - bool interlaced = false;
> > > > - u32 val;
> > > > -
> > > > - DRM_DEBUG_DRIVER("Primary layer, updating global size
> >
> > W: %u H: %u\n",
> >
> > > > - dst_w, dst_h);
> > > > - regmap_write(mixer->engine.regs,
> > > > - SUN8I_MIXER_GLOBAL_SIZE,
> > > > - outsize);
> > > > - regmap_write(mixer->engine.regs,
> > > > - SUN8I_MIXER_BLEND_OUTSIZE(bld_base),
> >
> > outsize);
> >
> > > > -
> > > > - if (state->crtc)
> > > > - interlaced = state->crtc->state-
> > >
> > >adjusted_mode.flags
> > >
> > > > - & DRM_MODE_FLAG_INTERLACE;
> > > > -
> > > > - if (interlaced)
> > > > - val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
> > > > - else
> > > > - val = 0;
> > > > -
> > > > - regmap_update_bits(mixer->engine.regs,
> > > > -
> >
> > SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> >
> > > > -
> >
> > SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
> >
> > > > - val);
> > > > -
> > > > - DRM_DEBUG_DRIVER("Switching display mixer interlaced
> >
> > mode %s\n",
> >
> > > > - interlaced ? "on" : "off");
> > > > - }
> > > > -
> > > >
> > > > /* Set height and width */
> > > > DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n",
> > > >
> > > > state->src.x1 >> 16, state->src.y1 >> 16);
> > > >
> > > > @@ -349,9 +319,6 @@ struct sun8i_ui_layer
> >
> > *sun8i_ui_layer_init_one(struct
> >
> > > > drm_device *drm,>
> > > >
> > > > if (!layer)
> > > >
> > > > return ERR_PTR(-ENOMEM);
> > > >
> > > > - if (index == 0)
> > > > - type = DRM_PLANE_TYPE_PRIMARY;
> > > > -
> > > >
> > > > /* possible crtcs are set later */
> > > > ret = drm_universal_plane_init(drm, &layer->plane, 0,
> > > >
> > > > &sun8i_ui_layer_funcs,
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index
> >
> > 07c27e6a4b77..49c4074e164f
> >
> > > > 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > @@ -116,6 +116,36 @@ static int sun8i_vi_layer_update_coord(struct
> > > > sun8i_mixer *mixer, int channel,>
> > > >
> > > > insize = SUN8I_MIXER_SIZE(src_w, src_h);
> > > > outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
> > > >
> > > > + if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > > > + bool interlaced = false;
> > > > + u32 val;
> > > > +
> > > > + DRM_DEBUG_DRIVER("Primary layer, updating global size
> >
> > W: %u H: %u\n",
> >
> > > > + dst_w, dst_h);
> > > > + regmap_write(mixer->engine.regs,
> > > > + SUN8I_MIXER_GLOBAL_SIZE,
> > > > + outsize);
> > > > + regmap_write(mixer->engine.regs,
> > > > + SUN8I_MIXER_BLEND_OUTSIZE(bld_base),
> >
> > outsize);
> >
> > > > +
> > > > + if (state->crtc)
> > > > + interlaced = state->crtc->state-
> > >
> > >adjusted_mode.flags
> > >
> > > > + & DRM_MODE_FLAG_INTERLACE;
> > > > +
> > > > + if (interlaced)
> > > > + val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
> > > > + else
> > > > + val = 0;
> > > > +
> > > > + regmap_update_bits(mixer->engine.regs,
> > > > +
> >
> > SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> >
> > > > +
> >
> > SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
> >
> > > > + val);
> > > > +
> > > > + DRM_DEBUG_DRIVER("Switching display mixer interlaced
> >
> > mode %s\n",
> >
> > > > + interlaced ? "on" : "off");
> > > > + }
> > > > +
> > > >
> > > > /* Set height and width */
> > > > DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n",
> > > >
> > > > (state->src.x1 >> 16) & ~(format->hsub -
> >
> > 1),
> >
> > > > @@ -445,6 +475,7 @@ struct sun8i_vi_layer
> >
> > *sun8i_vi_layer_init_one(struct
> >
> > > > drm_device *drm,>
> > > >
> > > > struct
> >
> > sun8i_mixer *mixer,
> >
> > > > int index)
> > > >
> > > > {
> > > >
> > > > + enum drm_plane_type type = DRM_PLANE_TYPE_OVERLAY;
> > > >
> > > > struct sun8i_vi_layer *layer;
> > > > unsigned int plane_cnt;
> > > > int ret;
> > > >
> > > > @@ -453,12 +484,15 @@ struct sun8i_vi_layer
> > > > *sun8i_vi_layer_init_one(struct drm_device *drm,>
> > > >
> > > > if (!layer)
> > > >
> > > > return ERR_PTR(-ENOMEM);
> > > >
> > > > + if (index == 0)
> > > > + type = DRM_PLANE_TYPE_PRIMARY;
> > > > +
> > > >
> > > > /* possible crtcs are set later */
> > > > ret = drm_universal_plane_init(drm, &layer->plane, 0,
> > > >
> > > > &sun8i_vi_layer_funcs,
> > > > sun8i_vi_layer_formats,
> >
> > ARRAY_SIZE(sun8i_vi_layer_formats),
> >
> > > > - NULL,
> >
> > DRM_PLANE_TYPE_OVERLAY, NULL);
> >
> > > > + NULL, type, NULL);
> > > >
> > > > if (ret) {
> > > >
> > > > dev_err(drm->dev, "Couldn't initialize layer\n");
> > > > return ERR_PTR(ret);
> > > >
> > > > --
> > > > 2.17.1
Hi,
On Thu, Sep 19, 2019 at 03:37:03PM +0300, [email protected] wrote:
> From: Roman Stratiienko <[email protected]>
>
> DE2.0 blender does not take into the account alpha channel of vi layer.
> Thus makes overlaying of this layer totally opaque.
> Using vi layer as bottom solves this issue.
>
> Tested on Android.
>
> Signed-off-by: Roman Stratiienko <[email protected]>
It sounds like a workaround more than an actual fix.
If the VI planes can't use the alpha, then we should just stop
reporting that format.
Jernej, what do you think?
Maxime
> ---
> drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 33 -----------------------
> drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 36 +++++++++++++++++++++++++-
> 2 files changed, 35 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> index dd2a1c851939..25183badc85f 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> @@ -99,36 +99,6 @@ static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel,
> insize = SUN8I_MIXER_SIZE(src_w, src_h);
> outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
>
> - if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> - bool interlaced = false;
> - u32 val;
> -
> - DRM_DEBUG_DRIVER("Primary layer, updating global size W: %u H: %u\n",
> - dst_w, dst_h);
> - regmap_write(mixer->engine.regs,
> - SUN8I_MIXER_GLOBAL_SIZE,
> - outsize);
> - regmap_write(mixer->engine.regs,
> - SUN8I_MIXER_BLEND_OUTSIZE(bld_base), outsize);
> -
> - if (state->crtc)
> - interlaced = state->crtc->state->adjusted_mode.flags
> - & DRM_MODE_FLAG_INTERLACE;
> -
> - if (interlaced)
> - val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
> - else
> - val = 0;
> -
> - regmap_update_bits(mixer->engine.regs,
> - SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> - SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
> - val);
> -
> - DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
> - interlaced ? "on" : "off");
> - }
> -
> /* Set height and width */
> DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n",
> state->src.x1 >> 16, state->src.y1 >> 16);
> @@ -349,9 +319,6 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
> if (!layer)
> return ERR_PTR(-ENOMEM);
>
> - if (index == 0)
> - type = DRM_PLANE_TYPE_PRIMARY;
> -
> /* possible crtcs are set later */
> ret = drm_universal_plane_init(drm, &layer->plane, 0,
> &sun8i_ui_layer_funcs,
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> index 07c27e6a4b77..49c4074e164f 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> @@ -116,6 +116,36 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
> insize = SUN8I_MIXER_SIZE(src_w, src_h);
> outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
>
> + if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> + bool interlaced = false;
> + u32 val;
> +
> + DRM_DEBUG_DRIVER("Primary layer, updating global size W: %u H: %u\n",
> + dst_w, dst_h);
> + regmap_write(mixer->engine.regs,
> + SUN8I_MIXER_GLOBAL_SIZE,
> + outsize);
> + regmap_write(mixer->engine.regs,
> + SUN8I_MIXER_BLEND_OUTSIZE(bld_base), outsize);
> +
> + if (state->crtc)
> + interlaced = state->crtc->state->adjusted_mode.flags
> + & DRM_MODE_FLAG_INTERLACE;
> +
> + if (interlaced)
> + val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
> + else
> + val = 0;
> +
> + regmap_update_bits(mixer->engine.regs,
> + SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> + SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
> + val);
> +
> + DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
> + interlaced ? "on" : "off");
> + }
> +
> /* Set height and width */
> DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n",
> (state->src.x1 >> 16) & ~(format->hsub - 1),
> @@ -445,6 +475,7 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
> struct sun8i_mixer *mixer,
> int index)
> {
> + enum drm_plane_type type = DRM_PLANE_TYPE_OVERLAY;
> struct sun8i_vi_layer *layer;
> unsigned int plane_cnt;
> int ret;
> @@ -453,12 +484,15 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
> if (!layer)
> return ERR_PTR(-ENOMEM);
>
> + if (index == 0)
> + type = DRM_PLANE_TYPE_PRIMARY;
> +
> /* possible crtcs are set later */
> ret = drm_universal_plane_init(drm, &layer->plane, 0,
> &sun8i_vi_layer_funcs,
> sun8i_vi_layer_formats,
> ARRAY_SIZE(sun8i_vi_layer_formats),
> - NULL, DRM_PLANE_TYPE_OVERLAY, NULL);
> + NULL, type, NULL);
> if (ret) {
> dev_err(drm->dev, "Couldn't initialize layer\n");
> return ERR_PTR(ret);
> --
> 2.17.1
>
Dne četrtek, 19. september 2019 ob 19:17:54 CEST je Maxime Ripard napisal(a):
> Hi,
>
> On Thu, Sep 19, 2019 at 03:37:03PM +0300, [email protected]
wrote:
> > From: Roman Stratiienko <[email protected]>
> >
> > DE2.0 blender does not take into the account alpha channel of vi layer.
> > Thus makes overlaying of this layer totally opaque.
> > Using vi layer as bottom solves this issue.
What issue? Overlays don't have to be "full screen", thus missing support for
alpha blending doesn't make it less valuable. And VI planes are already placed
at the bottom (zpos = 0).
> >
> > Tested on Android.
> >
> > Signed-off-by: Roman Stratiienko <[email protected]>
>
> It sounds like a workaround more than an actual fix.
>
> If the VI planes can't use the alpha, then we should just stop
> reporting that format.
>
> Jernej, what do you think?
Commit message is misleading. What this commit actually does is moving primary
plane from first UI plane to bottom most plane, i.e. first VI plane. However, VI
planes are scarce resource, almost all mixers have only one. I wouldn't set it
as primary, because it's the only one which provide support for YUV formats.
That could be used for example by video player for zero-copy rendering.
Probably most apps wouldn't touch it if it was primary (that's usually
reserved for window manager, if used).
I left few formats with alpha channel exposed by VI planes, just because they
don't have equivalent format without alpha. But I'm fine with removing them if
you all agree on that.
Best regards,
Jernej
>
> Maxime
>
> > ---
> >
> > drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 33 -----------------------
> > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 36 +++++++++++++++++++++++++-
> > 2 files changed, 35 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index dd2a1c851939..25183badc85f
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > @@ -99,36 +99,6 @@ static int sun8i_ui_layer_update_coord(struct
> > sun8i_mixer *mixer, int channel,>
> > insize = SUN8I_MIXER_SIZE(src_w, src_h);
> > outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
> >
> > - if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > - bool interlaced = false;
> > - u32 val;
> > -
> > - DRM_DEBUG_DRIVER("Primary layer, updating global size
W: %u H: %u\n",
> > - dst_w, dst_h);
> > - regmap_write(mixer->engine.regs,
> > - SUN8I_MIXER_GLOBAL_SIZE,
> > - outsize);
> > - regmap_write(mixer->engine.regs,
> > - SUN8I_MIXER_BLEND_OUTSIZE(bld_base),
outsize);
> > -
> > - if (state->crtc)
> > - interlaced = state->crtc->state-
>adjusted_mode.flags
> > - & DRM_MODE_FLAG_INTERLACE;
> > -
> > - if (interlaced)
> > - val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
> > - else
> > - val = 0;
> > -
> > - regmap_update_bits(mixer->engine.regs,
> > -
SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> > -
SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
> > - val);
> > -
> > - DRM_DEBUG_DRIVER("Switching display mixer interlaced
mode %s\n",
> > - interlaced ? "on" : "off");
> > - }
> > -
> >
> > /* Set height and width */
> > DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n",
> >
> > state->src.x1 >> 16, state->src.y1 >> 16);
> >
> > @@ -349,9 +319,6 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct
> > drm_device *drm,>
> > if (!layer)
> >
> > return ERR_PTR(-ENOMEM);
> >
> > - if (index == 0)
> > - type = DRM_PLANE_TYPE_PRIMARY;
> > -
> >
> > /* possible crtcs are set later */
> > ret = drm_universal_plane_init(drm, &layer->plane, 0,
> >
> > &sun8i_ui_layer_funcs,
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 07c27e6a4b77..49c4074e164f
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > @@ -116,6 +116,36 @@ static int sun8i_vi_layer_update_coord(struct
> > sun8i_mixer *mixer, int channel,>
> > insize = SUN8I_MIXER_SIZE(src_w, src_h);
> > outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
> >
> > + if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > + bool interlaced = false;
> > + u32 val;
> > +
> > + DRM_DEBUG_DRIVER("Primary layer, updating global size
W: %u H: %u\n",
> > + dst_w, dst_h);
> > + regmap_write(mixer->engine.regs,
> > + SUN8I_MIXER_GLOBAL_SIZE,
> > + outsize);
> > + regmap_write(mixer->engine.regs,
> > + SUN8I_MIXER_BLEND_OUTSIZE(bld_base),
outsize);
> > +
> > + if (state->crtc)
> > + interlaced = state->crtc->state-
>adjusted_mode.flags
> > + & DRM_MODE_FLAG_INTERLACE;
> > +
> > + if (interlaced)
> > + val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
> > + else
> > + val = 0;
> > +
> > + regmap_update_bits(mixer->engine.regs,
> > +
SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> > +
SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
> > + val);
> > +
> > + DRM_DEBUG_DRIVER("Switching display mixer interlaced
mode %s\n",
> > + interlaced ? "on" : "off");
> > + }
> > +
> >
> > /* Set height and width */
> > DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n",
> >
> > (state->src.x1 >> 16) & ~(format->hsub -
1),
> >
> > @@ -445,6 +475,7 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct
> > drm_device *drm,>
> > struct
sun8i_mixer *mixer,
> > int index)
> >
> > {
> >
> > + enum drm_plane_type type = DRM_PLANE_TYPE_OVERLAY;
> >
> > struct sun8i_vi_layer *layer;
> > unsigned int plane_cnt;
> > int ret;
> >
> > @@ -453,12 +484,15 @@ struct sun8i_vi_layer
> > *sun8i_vi_layer_init_one(struct drm_device *drm,>
> > if (!layer)
> >
> > return ERR_PTR(-ENOMEM);
> >
> > + if (index == 0)
> > + type = DRM_PLANE_TYPE_PRIMARY;
> > +
> >
> > /* possible crtcs are set later */
> > ret = drm_universal_plane_init(drm, &layer->plane, 0,
> >
> > &sun8i_vi_layer_funcs,
> > sun8i_vi_layer_formats,
> >
ARRAY_SIZE(sun8i_vi_layer_formats),
> >
> > - NULL,
DRM_PLANE_TYPE_OVERLAY, NULL);
> > + NULL, type, NULL);
> >
> > if (ret) {
> >
> > dev_err(drm->dev, "Couldn't initialize layer\n");
> > return ERR_PTR(ret);
> >
> > --
> > 2.17.1
On Thu, Sep 19, 2019 at 08:15:49PM +0200, Jernej Škrabec wrote:
> Dne četrtek, 19. september 2019 ob 19:17:54 CEST je Maxime Ripard napisal(a):
> > >
> > > Tested on Android.
> > >
> > > Signed-off-by: Roman Stratiienko <[email protected]>
> >
> > It sounds like a workaround more than an actual fix.
> >
> > If the VI planes can't use the alpha, then we should just stop
> > reporting that format.
> >
> > Jernej, what do you think?
>
> Commit message is misleading. What this commit actually does is moving primary
> plane from first UI plane to bottom most plane, i.e. first VI plane. However, VI
> planes are scarce resource, almost all mixers have only one. I wouldn't set it
> as primary, because it's the only one which provide support for YUV formats.
> That could be used for example by video player for zero-copy rendering.
> Probably most apps wouldn't touch it if it was primary (that's usually
> reserved for window manager, if used).
Yeah, we definitely don't want to use it as primary and prevent the
video display.
> I left few formats with alpha channel exposed by VI planes, just because they
> don't have equivalent format without alpha. But I'm fine with removing them if
> you all agree on that.
If there's no alpha support, then yeah, we shouldn't expose the format
at all, and then we can either add the new formats, or just not expose
them if they are exotic enough.
Maxime
[ Resending message in plain mode ]
Hello guys,
Actually, I believe this is True solution, and current one is wrong.
Let me explain why.
De2. 0 was designed to match Android hwcomposer hal requirements IMO.
You can easily agree with this conclusion by comparing Composer HAL
and De2. 0 hardware manuals.
I faced at least 4 issues when trying to run Android using the
mainline kernel sun8i mixer implementation. Current one, missing pixel
formats (my previous patch), missing plane alpha and rotation
properties. I plan to fix it and also send appropriate solution to the
upstream.
To achieve optimal UI performance Android requires at least 4 ui
layers to make screen composition. Current patch enables 4th plane
usable.
As for using vi plane to display video. I assume that some of the
current users may have regression in their software, but it could be
easily fixed. For example if vi layer isn't fullscreen and should be
on top of the other layers, it can actually be placed on the bottom
and overlayed with pictures with transparent rectangles in video
region.
But I assume most of users such as browser etc. uses GPU for that.
And if you are watching fullscreen video, I can imagine only subtitles
layer and advertising layers on top of the video layers.
On Thu, Sep 19, 2019 at 9:15 PM Jernej Škrabec <[email protected]> wrote:
>
> Dne četrtek, 19. september 2019 ob 19:17:54 CEST je Maxime Ripard napisal(a):
> > Hi,
> >
> > On Thu, Sep 19, 2019 at 03:37:03PM +0300, [email protected]
> wrote:
> > > From: Roman Stratiienko <[email protected]>
> > >
> > > DE2.0 blender does not take into the account alpha channel of vi layer.
> > > Thus makes overlaying of this layer totally opaque.
> > > Using vi layer as bottom solves this issue.
>
> What issue? Overlays don't have to be "full screen", thus missing support for
> alpha blending doesn't make it less valuable. And VI planes are already placed
> at the bottom (zpos = 0).
>
> > >
> > > Tested on Android.
> > >
> > > Signed-off-by: Roman Stratiienko <[email protected]>
> >
> > It sounds like a workaround more than an actual fix.
> >
> > If the VI planes can't use the alpha, then we should just stop
> > reporting that format.
> >
> > Jernej, what do you think?
>
> Commit message is misleading. What this commit actually does is moving primary
> plane from first UI plane to bottom most plane, i.e. first VI plane. However, VI
> planes are scarce resource, almost all mixers have only one. I wouldn't set it
> as primary, because it's the only one which provide support for YUV formats.
> That could be used for example by video player for zero-copy rendering.
> Probably most apps wouldn't touch it if it was primary (that's usually
> reserved for window manager, if used).
>
> I left few formats with alpha channel exposed by VI planes, just because they
> don't have equivalent format without alpha. But I'm fine with removing them if
> you all agree on that.
>
> Best regards,
> Jernej
>
> >
> > Maxime
> >
> > > ---
> > >
> > > drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 33 -----------------------
> > > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 36 +++++++++++++++++++++++++-
> > > 2 files changed, 35 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index dd2a1c851939..25183badc85f
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > @@ -99,36 +99,6 @@ static int sun8i_ui_layer_update_coord(struct
> > > sun8i_mixer *mixer, int channel,>
> > > insize = SUN8I_MIXER_SIZE(src_w, src_h);
> > > outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
> > >
> > > - if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > > - bool interlaced = false;
> > > - u32 val;
> > > -
> > > - DRM_DEBUG_DRIVER("Primary layer, updating global size
> W: %u H: %u\n",
> > > - dst_w, dst_h);
> > > - regmap_write(mixer->engine.regs,
> > > - SUN8I_MIXER_GLOBAL_SIZE,
> > > - outsize);
> > > - regmap_write(mixer->engine.regs,
> > > - SUN8I_MIXER_BLEND_OUTSIZE(bld_base),
> outsize);
> > > -
> > > - if (state->crtc)
> > > - interlaced = state->crtc->state-
> >adjusted_mode.flags
> > > - & DRM_MODE_FLAG_INTERLACE;
> > > -
> > > - if (interlaced)
> > > - val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
> > > - else
> > > - val = 0;
> > > -
> > > - regmap_update_bits(mixer->engine.regs,
> > > -
> SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> > > -
> SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
> > > - val);
> > > -
> > > - DRM_DEBUG_DRIVER("Switching display mixer interlaced
> mode %s\n",
> > > - interlaced ? "on" : "off");
> > > - }
> > > -
> > >
> > > /* Set height and width */
> > > DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n",
> > >
> > > state->src.x1 >> 16, state->src.y1 >> 16);
> > >
> > > @@ -349,9 +319,6 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct
> > > drm_device *drm,>
> > > if (!layer)
> > >
> > > return ERR_PTR(-ENOMEM);
> > >
> > > - if (index == 0)
> > > - type = DRM_PLANE_TYPE_PRIMARY;
> > > -
> > >
> > > /* possible crtcs are set later */
> > > ret = drm_universal_plane_init(drm, &layer->plane, 0,
> > >
> > > &sun8i_ui_layer_funcs,
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 07c27e6a4b77..49c4074e164f
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > @@ -116,6 +116,36 @@ static int sun8i_vi_layer_update_coord(struct
> > > sun8i_mixer *mixer, int channel,>
> > > insize = SUN8I_MIXER_SIZE(src_w, src_h);
> > > outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
> > >
> > > + if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > > + bool interlaced = false;
> > > + u32 val;
> > > +
> > > + DRM_DEBUG_DRIVER("Primary layer, updating global size
> W: %u H: %u\n",
> > > + dst_w, dst_h);
> > > + regmap_write(mixer->engine.regs,
> > > + SUN8I_MIXER_GLOBAL_SIZE,
> > > + outsize);
> > > + regmap_write(mixer->engine.regs,
> > > + SUN8I_MIXER_BLEND_OUTSIZE(bld_base),
> outsize);
> > > +
> > > + if (state->crtc)
> > > + interlaced = state->crtc->state-
> >adjusted_mode.flags
> > > + & DRM_MODE_FLAG_INTERLACE;
> > > +
> > > + if (interlaced)
> > > + val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
> > > + else
> > > + val = 0;
> > > +
> > > + regmap_update_bits(mixer->engine.regs,
> > > +
> SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> > > +
> SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
> > > + val);
> > > +
> > > + DRM_DEBUG_DRIVER("Switching display mixer interlaced
> mode %s\n",
> > > + interlaced ? "on" : "off");
> > > + }
> > > +
> > >
> > > /* Set height and width */
> > > DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n",
> > >
> > > (state->src.x1 >> 16) & ~(format->hsub -
> 1),
> > >
> > > @@ -445,6 +475,7 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct
> > > drm_device *drm,>
> > > struct
> sun8i_mixer *mixer,
> > > int index)
> > >
> > > {
> > >
> > > + enum drm_plane_type type = DRM_PLANE_TYPE_OVERLAY;
> > >
> > > struct sun8i_vi_layer *layer;
> > > unsigned int plane_cnt;
> > > int ret;
> > >
> > > @@ -453,12 +484,15 @@ struct sun8i_vi_layer
> > > *sun8i_vi_layer_init_one(struct drm_device *drm,>
> > > if (!layer)
> > >
> > > return ERR_PTR(-ENOMEM);
> > >
> > > + if (index == 0)
> > > + type = DRM_PLANE_TYPE_PRIMARY;
> > > +
> > >
> > > /* possible crtcs are set later */
> > > ret = drm_universal_plane_init(drm, &layer->plane, 0,
> > >
> > > &sun8i_vi_layer_funcs,
> > > sun8i_vi_layer_formats,
> > >
> ARRAY_SIZE(sun8i_vi_layer_formats),
> > >
> > > - NULL,
> DRM_PLANE_TYPE_OVERLAY, NULL);
> > > + NULL, type, NULL);
> > >
> > > if (ret) {
> > >
> > > dev_err(drm->dev, "Couldn't initialize layer\n");
> > > return ERR_PTR(ret);
> > >
> > > --
> > > 2.17.1
>
>
>
>
--
Best regards,
Roman Stratiienko
Global Logic
ADIT Team
Hi,
On Thu, Sep 19, 2019 at 11:03:26PM +0300, Roman Stratiienko wrote:
> Actually, I beleive this is True solution, and current one is wrong. Let
> me explain why.
>
> De2. 0 was designed to match Android hwcomposer hal requirements IMO.
> You can easily agree with this conclusion by comparing Composer HAL and
> De2. 0 hardware manuals.
>
> I faced at least 4 issues when try to run Android using the mainline kernel
> sun8i mixer implementation. Current one, missing pixel formats (my previous
> patch), missing plane alpha and rotation properties. I plan to fix it and
> also send appropriate solution to the upstream.
>
> To achieve optimal UI performance Android requires at least 4 ui layers to
> make screen composition. Current patch enables 4th plane usable.
Note that you can also get 4 UI planes by enabling more than one UI
layer per channel. You wouldn't be able to use alpha between each
plane of a given channel, but we can use a similar trick than what we
did for the pipes in the sun4i backend.
Maxime
@Jernej Škrabec @Maxime Ripard
Thanks for your time and valuable suggestions.
Currently I would have to go away from mainline KMS to solve my
(Android) issues, and I hope to get back when I
find mainline-friendly solution for it.
Having no primary layer or zero-buffer primary layer and 4 overlays
could be a universal solution, but I have not sufficient knowledge in
KMS to bring-up this idea.
On Fri, Sep 20, 2019 at 9:18 AM Maxime Ripard <[email protected]> wrote:
>
> Hi,
>
> On Thu, Sep 19, 2019 at 11:03:26PM +0300, Roman Stratiienko wrote:
> > Actually, I beleive this is True solution, and current one is wrong. Let
> > me explain why.
> >
> > De2. 0 was designed to match Android hwcomposer hal requirements IMO.
> > You can easily agree with this conclusion by comparing Composer HAL and
> > De2. 0 hardware manuals.
> >
> > I faced at least 4 issues when try to run Android using the mainline kernel
> > sun8i mixer implementation. Current one, missing pixel formats (my previous
> > patch), missing plane alpha and rotation properties. I plan to fix it and
> > also send appropriate solution to the upstream.
> >
> > To achieve optimal UI performance Android requires at least 4 ui layers to
> > make screen composition. Current patch enables 4th plane usable.
>
> Note that you can also get 4 UI planes by enabling more than one UI
> layer per channel. You wouldn't be able to use alpha between each
> plane of a given channel, but we can use a similar trick than what we
> did for the pipes in the sun4i backend.
>
> Maxime
--
Best regards,
Roman Stratiienko
Global Logic Inc.