[PATCH 2/3] drm/exynos/fimd: create local helper for disabling hardware window

DECON has fixed hardware window order. To implement dynamic zpos
normalized_zpos of active plane has to be connected to window number, and
remaining windows have to be disabled.

Signed-off-by: Andrzej Hajda <***@samsung.com>
---
drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 23 +++++++++----------
1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index aef487dd8731..6e3e6e970690 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -326,7 +326,7 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
to_exynos_plane_state(plane->base.state);
struct decon_context *ctx = crtc->ctx;
struct drm_framebuffer *fb = state->base.fb;
- unsigned int win = plane->index;
+ unsigned int win = state->base.normalized_zpos + ctx->first_win;
unsigned int cpp = fb->format->cpp[0];
unsigned int pitch = fb->pitches[0];
dma_addr_t dma_addr = exynos_drm_fb_dma_addr(fb, 0);
@@ -376,19 +376,18 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
decon_set_bits(ctx, DECON_WINCONx(win), WINCONx_ENWIN_F, ~0);
}

-static void decon_disable_plane(struct exynos_drm_crtc *crtc,
- struct exynos_drm_plane *plane)
-{
- struct decon_context *ctx = crtc->ctx;
- unsigned int win = plane->index;
-
- decon_set_bits(ctx, DECON_WINCONx(win), WINCONx_ENWIN_F, 0);
-}
-
static void decon_atomic_flush(struct exynos_drm_crtc *crtc)
{
struct decon_context *ctx = crtc->ctx;
unsigned long flags;
+ int win = hweight32(crtc->base.state->plane_mask) + ctx->first_win;
+
+ /* disable windows corresponding to disabled planes */
+ for (; win < WINDOWS_NR; ++win) {
+ if (!readl(ctx->addr + DECON_WINCONx(win)) & WINCONx_ENWIN_F)
+ break;
+ decon_set_bits(ctx, DECON_WINCONx(win), WINCONx_ENWIN_F, 0);
+ }

spin_lock_irqsave(&ctx->vblank_lock, flags);

@@ -462,7 +461,7 @@ static void decon_disable(struct exynos_drm_crtc *crtc)
* a destroyed buffer later.
*/
for (i = ctx->first_win; i < WINDOWS_NR; i++)
- decon_disable_plane(crtc, &ctx->planes[i]);
+ decon_set_bits(ctx, DECON_WINCONx(i), WINCONx_ENWIN_F, 0);

decon_swreset(ctx);

@@ -531,7 +530,6 @@ static const struct exynos_drm_crtc_ops decon_crtc_ops = {
.disable_vblank = decon_disable_vblank,
.atomic_begin = decon_atomic_begin,
.update_plane = decon_update_plane,
- .disable_plane = decon_disable_plane,
.mode_valid = decon_mode_valid,
.atomic_flush = decon_atomic_flush,
};
@@ -552,6 +550,7 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
ctx->configs[win].num_pixel_formats = ARRAY_SIZE(decon_formats);
ctx->configs[win].zpos = win - ctx->first_win;
ctx->configs[win].type = decon_win_types[win];
+ ctx->configs[win].capabilities = EXYNOS_DRM_PLANE_CAP_ZPOS;

ret = exynos_plane_init(drm_dev, &ctx->planes[win], win,
&ctx->configs[win]);

--
2.17.1

Andrzej Hajda

2018-12-06 09:38:57 UTC

FIMD has fixed hardware window order. To implement dynamic zpos
normalized_zpos of active plane has to be connected to window number, and
remaining windows have to be disabled.

Signed-off-by: Andrzej Hajda <***@samsung.com>
---
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 226127f35052..c4e2129e0e3f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -706,6 +706,12 @@ static void fimd_atomic_flush(struct exynos_drm_crtc *crtc)
if (ctx->suspended)
return;

+ for (i = hweight32(crtc->base.state->plane_mask); i < WINDOWS_NR; i++) {
+ if (!(readl(ctx->regs + WINCON(i)) & WINCONx_ENWIN))
+ break;
+ fimd_disable_win(ctx, i);
+ }
+
for (i = 0; i < WINDOWS_NR; i++)
fimd_shadow_protect_win(ctx, i, false);

@@ -722,7 +728,7 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
dma_addr_t dma_addr;
unsigned long val, size, offset;
unsigned int last_x, last_y, buf_offsize, line_size;
- unsigned int win = plane->index;
+ unsigned int win = state->base.normalized_zpos;
unsigned int cpp = fb->format->cpp[0];
unsigned int pitch = fb->pitches[0];

@@ -804,17 +810,6 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
atomic_set(&ctx->win_updated, 1);
}

-static void fimd_disable_plane(struct exynos_drm_crtc *crtc,
- struct exynos_drm_plane *plane)
-{
- struct fimd_context *ctx = crtc->ctx;
-
- if (ctx->suspended)
- return;
-
- fimd_disable_win(ctx, plane->index);
-}
-
static void fimd_enable(struct exynos_drm_crtc *crtc)
{
struct fimd_context *ctx = crtc->ctx;
@@ -933,7 +928,6 @@ static const struct exynos_drm_crtc_ops fimd_crtc_ops = {
.disable_vblank = fimd_disable_vblank,
.atomic_begin = fimd_atomic_begin,
.update_plane = fimd_update_plane,
- .disable_plane = fimd_disable_plane,
.atomic_flush = fimd_atomic_flush,
.atomic_check = fimd_atomic_check,
.te_handler = fimd_te_handler,
@@ -987,6 +981,7 @@ static int fimd_bind(struct device *dev, struct device *master, void *data)
ctx->configs[i].num_pixel_formats = ARRAY_SIZE(fimd_formats);
ctx->configs[i].zpos = i;
ctx->configs[i].type = fimd_win_types[i];
+ ctx->configs[i].capabilities = EXYNOS_DRM_PLANE_CAP_ZPOS;
ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
&ctx->configs[i]);
if (ret)

--
2.17.1

Andrzej Hajda

2018-12-06 11:06:19 UTC

Hi Inki,

Hi Inki,
This small patchset adds dynamic zpos support for DECON and FIMD.
It was tested on tm2 and trats2.

I have realized that this patchset interferes with Christoph's exynos
plane patches for fimd and decon, but in drm-exynos/exynos_drm_next I
have found only patches for decon. Do you plan to get Christpoph's
alpha/blending patches for fimd?

Should I rebase my patches on current exynos-drm-next, or on all
Christoph's patches?

Regards

Andrzej

Regards
Andrzej
drm/exynos/decon5433: add dynamic zpos support
drm/exynos/fimd: create local helper for disabling hardware window
drm/exynos/fimd: add dynamic zpos support
drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 23 +++++-----
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 42 ++++++++-----------
2 files changed, 29 insertions(+), 36 deletions(-)

Inki Dae

2018-12-07 01:18:45 UTC

Hi Andrzej,

Post by Andrzej Hajda
Hi Inki,

Post by Andrzej Hajda
Hi Inki,
This small patchset adds dynamic zpos support for DECON and FIMD.
It was tested on tm2 and trats2.

I have realized that this patchset interferes with Christoph's exynos
plane patches for fimd and decon, but in drm-exynos/exynos_drm_next I
have found only patches for decon. Do you plan to get Christpoph's
alpha/blending patches for fimd?
Should I rebase my patches on current exynos-drm-next, or on all
Christoph's patches?

I missed to merge Christoph's patches. Thanks for notice.
Now you can rebase your patch on top of exynos-drm-next.

Thanks,
Inki Dae

Post by Andrzej Hajda
Regards
Andrzej

Post by Andrzej Hajda
Regards
Andrzej
drm/exynos/decon5433: add dynamic zpos support
drm/exynos/fimd: create local helper for disabling hardware window
drm/exynos/fimd: add dynamic zpos support
drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 23 +++++-----
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 42 ++++++++-----------
2 files changed, 29 insertions(+), 36 deletions(-)

Inki Dae

2018-12-10 02:25:05 UTC

Hi Andrzej,

Post by Andrzej Hajda
Hi Inki,
This small patchset adds dynamic zpos support for DECON and FIMD.

This patch will allow user space to change zpos. However, DECON and FIMD devices have fixed priority of HW overlays.
This would mean that zpos change by user space doesn't guarantee correct HW overlay priority.

Why do you want to support mutable zpos?

Thanks,
Inki Dae

Post by Andrzej Hajda
It was tested on tm2 and trats2.
Regards
Andrzej
drm/exynos/decon5433: add dynamic zpos support
drm/exynos/fimd: create local helper for disabling hardware window
drm/exynos/fimd: add dynamic zpos support
drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 23 +++++-----
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 42 ++++++++-----------
2 files changed, 29 insertions(+), 36 deletions(-)

Andrzej Hajda

2018-12-10 07:35:22 UTC

Hi Inki,

Post by Inki Dae
Hi Andrzej,

Post by Andrzej Hajda
Hi Inki,
This small patchset adds dynamic zpos support for DECON and FIMD.

Practically you have patches which proves it works, you can see that
changing zpos value results in adequate change in displayed image.

Conceptually it is just a matter of disconnecting hardware windows
present in DECON and FIMD from DRM planes which are software entities.

You can reason about it this way:

- drm plane is a framebuffer plus informations how it should be
transformed/displayed on DECON/FIMD,

- hw window in DECON/FIMD is just a channel through which plane is send
to the display.

DECON and FIMD has fixed hw windows order - windows with lower numbers
are displayed below windows with higher number. To display planes in
given z-order you just need to send them via windows with appropriate
index - farthest plane should go always via win0, closer one via win1,
..., till the last plane.

So for example if you have three planes and want to display them in
following order (first one farthest, last one closest):

plane2, plane1, plane3

you should map them to planes as follow:

plane2 -> win0, plane1 -> win1, plane3 -> win2

then for example plane2 is disabled, we will have following mapping:

plane1 -> win0, plane3 -> win1, win2 - disabled

then if you change zorder of planes to: plane3, plane1:

plane3 -> win0, plane1 -> win1

As you see there is no relation between plane number and window number,
but window number is equal to plane's position in zpos-ordered list of
planes and this is exact value of normalized_zpos field in drm_plane_state.

I hope this extended explanation will clarify things.

Regards

Andrzej

Post by Inki Dae
Thanks,
Inki Dae

Inki Dae

2018-12-10 23:45:49 UTC

Hi Andrzej,

Post by Andrzej Hajda
Hi Inki,

Post by Inki Dae
Hi Andrzej,

Post by Andrzej Hajda
Hi Inki,
This small patchset adds dynamic zpos support for DECON and FIMD.

Plane1 is displayed below Plane3.

Post by Andrzej Hajda
plane3 -> win0, plane1 -> win1

Plane3 is displayed below Plane1 because DECON/FIMD HW aren't able to change HW overlay priroty.
However, user space wanted that Plane1 is displayed below Plane3. Like this, zpos change by user space doesn't guarantee correct HW overlay priority.
And there is no any reason to change zpos in user space excepting Mixer device which supports HW overlay priority change.
In fact, we supported mutable zpos before but changed it to immutable with same reason.

Lastly, your patch made real user broken.

Thanks,
Inki Dae

Post by Andrzej Hajda
As you see there is no relation between plane number and window number,
but window number is equal to plane's position in zpos-ordered list of
planes and this is exact value of normalized_zpos field in drm_plane_state.
I hope this extended explanation will clarify things.
Regards
Andrzej

Post by Inki Dae
Thanks,
Inki Dae

Andrzej Hajda

2018-12-11 07:49:11 UTC

Post by Inki Dae
Hi Andrzej,

Post by Andrzej Hajda
Hi Inki,

Post by Inki Dae
Hi Andrzej,

Post by Andrzej Hajda
Hi Inki,
This small patchset adds dynamic zpos support for DECON and FIMD.

Plane1 is displayed below Plane3.

And this is what we wanted, the initial order was: plane2, plane1,
plane3, first farthest (or lowest if you prefer this naming schema).

Post by Andrzej Hajda
plane3 -> win0, plane1 -> win1

Plane3 is displayed below Plane1 because DECON/FIMD HW aren't able to change HW overlay priroty.

No, plane3 is displayed below plane1 because we sent it via win0, if we
want inverse we can send plane3 via win1 and plane1 via win0.

Post by Inki Dae
However, user space wanted that Plane1 is displayed below Plane3. Like this, zpos change by user space doesn't guarantee correct HW overlay priority.

As I said before in this example userspace wanted plane3 below plane1,
and as I wrote in comment above any order of planes user want driver is
able to realize with this patch.

Post by Inki Dae
And there is no any reason to change zpos in user space excepting Mixer device which supports HW overlay priority change.

Lack of special registry for manipulating windows order does not mean it
cannot support dynamic zpos.

Post by Inki Dae
In fact, we supported mutable zpos before but changed it to immutable with same reason.

It was just broken if I remember correctly.

Post by Inki Dae
Lastly, your patch made real user broken.

Who? How?

Regards

Andrzej

Post by Inki Dae
Thanks,
Inki Dae

Inki Dae

2018-12-11 08:01:20 UTC

Post by Inki Dae
Hi Andrzej,

Post by Andrzej Hajda
Hi Inki,

Post by Inki Dae
Hi Andrzej,

Post by Andrzej Hajda
Hi Inki,
This small patchset adds dynamic zpos support for DECON and FIMD.

Plane1 is displayed below Plane3.

And this is what we wanted, the initial order was: plane2, plane1,
plane3, first farthest (or lowest if you prefer this naming schema).

Post by Andrzej Hajda
plane3 -> win0, plane1 -> win1

Plane3 is displayed below Plane1 because DECON/FIMD HW aren't able to change HW overlay priroty.

No, plane3 is displayed below plane1 because we sent it via win0, if we
want inverse we can send plane3 via win1 and plane1 via win0.

Post by Inki Dae
However, user space wanted that Plane1 is displayed below Plane3. Like this, zpos change by user space doesn't guarantee correct HW overlay priority.

As I said before in this example userspace wanted plane3 below plane1,
and as I wrote in comment above any order of planes user want driver is
able to realize with this patch.

Post by Inki Dae
And there is no any reason to change zpos in user space excepting Mixer device which supports HW overlay priority change.

Lack of special registry for manipulating windows order does not mean it
cannot support dynamic zpos.

Why changing zpos in user space is required?

Post by Inki Dae
In fact, we supported mutable zpos before but changed it to immutable with same reason.

It was just broken if I remember correctly.

No, I worked well and real user used it I remember.

Post by Inki Dae
Lastly, your patch made real user broken.

Who? How?

Window server of Tizen didn't work and you can test it with below image - I didn't check why it doesn't. Anyway, we don't have to break real user.
http://download.tizen.org/snapshots/tizen/unified/latest/images/standard/mobile-wayland-armv7l-tm2/

Thanks,
Inki Dae

Post by Andrzej Hajda
Regards
Andrzej

Post by Inki Dae
Thanks,
Inki Dae

Andrzej Hajda

2018-12-11 08:36:51 UTC

Post by Inki Dae
Hi Andrzej,

Post by Andrzej Hajda
Hi Inki,

Post by Inki Dae
Hi Andrzej,

Post by Andrzej Hajda
Hi Inki,
This small patchset adds dynamic zpos support for DECON and FIMD.

Plane1 is displayed below Plane3.

And this is what we wanted, the initial order was: plane2, plane1,
plane3, first farthest (or lowest if you prefer this naming schema).

Post by Andrzej Hajda
plane3 -> win0, plane1 -> win1

Plane3 is displayed below Plane1 because DECON/FIMD HW aren't able to change HW overlay priroty.

No, plane3 is displayed below plane1 because we sent it via win0, if we
want inverse we can send plane3 via win1 and plane1 via win0.

Post by Inki Dae
However, user space wanted that Plane1 is displayed below Plane3. Like this, zpos change by user space doesn't guarantee correct HW overlay priority.

As I said before in this example userspace wanted plane3 below plane1,
and as I wrote in comment above any order of planes user want driver is
able to realize with this patch.

Post by Inki Dae
And there is no any reason to change zpos in user space excepting Mixer device which supports HW overlay priority change.

Lack of special registry for manipulating windows order does not mean it
cannot support dynamic zpos.

Why changing zpos in user space is required?

Post by Inki Dae
In fact, we supported mutable zpos before but changed it to immutable with same reason.

It was just broken if I remember correctly.

No, I worked well and real user used it I remember.