mbox series

[0/3] drm/exynos: add support for dynamic zpos in DECON and FIMD

Message ID 20181206093857.22324-1-a.hajda@samsung.com (mailing list archive)
Headers show
Series drm/exynos: add support for dynamic zpos in DECON and FIMD | expand

Message

Andrzej Hajda Dec. 6, 2018, 9:38 a.m. UTC
Hi Inki,

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

Regards
Andrzej


Andrzej Hajda (3):
  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(-)

Comments

Andrzej Hajda Dec. 6, 2018, 11:06 a.m. UTC | #1
Hi Inki,

On 06.12.2018 10:38, Andrzej Hajda wrote:
> 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
>
>
> Andrzej Hajda (3):
>   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 Dec. 7, 2018, 1:18 a.m. UTC | #2
Hi Andrzej,

18. 12. 6. 오후 8:06에 Andrzej Hajda 이(가) 쓴 글:
> Hi Inki,
> 
> On 06.12.2018 10:38, Andrzej Hajda wrote:
>> 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

> 
> 
> Regards
> 
> Andrzej
> 
> 
>>
>> Regards
>> Andrzej
>>
>>
>> Andrzej Hajda (3):
>>   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 Dec. 10, 2018, 2:25 a.m. UTC | #3
Hi Andrzej,

18. 12. 6. 오후 6:38에 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

> It was tested on tm2 and trats2.
> 
> Regards
> Andrzej
> 
> 
> Andrzej Hajda (3):
>   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 Dec. 10, 2018, 7:35 a.m. UTC | #4
Hi Inki,

On 10.12.2018 03:25, Inki Dae wrote:
> Hi Andrzej,
>
> 18. 12. 6. 오후 6:38에 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?


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



>
> Thanks,
> Inki Dae
>
>> It was tested on tm2 and trats2.
>>
>> Regards
>> Andrzej
>>
>>
>> Andrzej Hajda (3):
>>   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 Dec. 10, 2018, 11:45 p.m. UTC | #5
Hi Andrzej,

18. 12. 10. 오후 4:35에 Andrzej Hajda 이(가) 쓴 글:
> Hi Inki,
> 
> On 10.12.2018 03:25, Inki Dae wrote:
>> Hi Andrzej,
>>
>> 18. 12. 6. 오후 6:38에 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?
> 
> 
> 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

Plane1 is displayed below Plane3.

> 
> then if you change zorder of planes to: plane3, plane1:
> 
> 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

> 
> 
> 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
> 
> 
> 
>>
>> Thanks,
>> Inki Dae
>>
>>> It was tested on tm2 and trats2.
>>>
>>> Regards
>>> Andrzej
>>>
>>>
>>> Andrzej Hajda (3):
>>>   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 Dec. 11, 2018, 7:49 a.m. UTC | #6
On 11.12.2018 00:45, Inki Dae wrote:
> Hi Andrzej,
>
> 18. 12. 10. 오후 4:35에 Andrzej Hajda 이(가) 쓴 글:
>> Hi Inki,
>>
>> On 10.12.2018 03:25, Inki Dae wrote:
>>> Hi Andrzej,
>>>
>>> 18. 12. 6. 오후 6:38에 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?
>>
>> 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
> 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).


>
>> then if you change zorder of planes to: plane3, plane1:
>>
>> 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.


> 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.


> 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.


> In fact, we supported mutable zpos before but changed it to immutable with same reason.


It was just broken if I remember correctly.


>
> Lastly, your patch made real user broken.


Who? How?


Regards

Andrzej


>
> Thanks,
> Inki Dae
>
>>
>> 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
>>
>>
>>
>>> Thanks,
>>> Inki Dae
>>>
>>>> It was tested on tm2 and trats2.
>>>>
>>>> Regards
>>>> Andrzej
>>>>
>>>>
>>>> Andrzej Hajda (3):
>>>>   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 Dec. 11, 2018, 8:01 a.m. UTC | #7
18. 12. 11. 오후 4:49에 Andrzej Hajda 이(가) 쓴 글:
> On 11.12.2018 00:45, Inki Dae wrote:
>> Hi Andrzej,
>>
>> 18. 12. 10. 오후 4:35에 Andrzej Hajda 이(가) 쓴 글:
>>> Hi Inki,
>>>
>>> On 10.12.2018 03:25, Inki Dae wrote:
>>>> Hi Andrzej,
>>>>
>>>> 18. 12. 6. 오후 6:38에 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?
>>>
>>> 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
>> 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).
> 
> 
>>
>>> then if you change zorder of planes to: plane3, plane1:
>>>
>>> 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.
> 
> 
>> 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.
> 
> 
>> 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?

> 
>> 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.

> 
>>
>> 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

> 
> 
> Regards
> 
> Andrzej
> 
> 
>>
>> Thanks,
>> Inki Dae
>>
>>>
>>> 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
>>>
>>>
>>>
>>>> Thanks,
>>>> Inki Dae
>>>>
>>>>> It was tested on tm2 and trats2.
>>>>>
>>>>> Regards
>>>>> Andrzej
>>>>>
>>>>>
>>>>> Andrzej Hajda (3):
>>>>>   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 Dec. 11, 2018, 8:36 a.m. UTC | #8
On 11.12.2018 09:01, Inki Dae wrote:
>
> 18. 12. 11. 오후 4:49에 Andrzej Hajda 이(가) 쓴 글:
>> On 11.12.2018 00:45, Inki Dae wrote:
>>> Hi Andrzej,
>>>
>>> 18. 12. 10. 오후 4:35에 Andrzej Hajda 이(가) 쓴 글:
>>>> Hi Inki,
>>>>
>>>> On 10.12.2018 03:25, Inki Dae wrote:
>>>>> Hi Andrzej,
>>>>>
>>>>> 18. 12. 6. 오후 6:38에 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?
>>>> 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
>>> 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).
>>
>>
>>>> then if you change zorder of planes to: plane3, plane1:
>>>>
>>>> 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.
>>
>>
>>> 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.
>>
>>
>>> 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?
>
>>> 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.
>
>>> 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/


Are you saying it works with latest mainline without my patches?


Regards

Andrzej


>
> Thanks,
> Inki Dae
>
>>
>> Regards
>>
>> Andrzej
>>
>>
>>> Thanks,
>>> Inki Dae
>>>
>>>> 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
>>>>
>>>>
>>>>
>>>>> Thanks,
>>>>> Inki Dae
>>>>>
>>>>>> It was tested on tm2 and trats2.
>>>>>>
>>>>>> Regards
>>>>>> Andrzej
>>>>>>
>>>>>>
>>>>>> Andrzej Hajda (3):
>>>>>>   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 Dec. 11, 2018, 1:20 p.m. UTC | #9
Hi again,

It seems I have missed two questions:

On 11.12.2018 09:36, Andrzej Hajda wrote:
> On 11.12.2018 09:01, Inki Dae wrote:
>> 18. 12. 11. 오후 4:49에 Andrzej Hajda 이(가) 쓴 글:
>>> On 11.12.2018 00:45, Inki Dae wrote:
>>>> Hi Andrzej,
>>>>
>>>> 18. 12. 10. 오후 4:35에 Andrzej Hajda 이(가) 쓴 글:
>>>>> Hi Inki,
>>>>>
>>>>> On 10.12.2018 03:25, Inki Dae wrote:
>>>>>> Hi Andrzej,
>>>>>>
>>>>>> 18. 12. 6. 오후 6:38에 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?
>>>>> 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
>>>> 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).
>>>
>>>
>>>>> then if you change zorder of planes to: plane3, plane1:
>>>>>
>>>>> 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.
>>>
>>>
>>>> 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.
>>>
>>>
>>>> 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?


I didn't say it is required, but usually it is better if the driver
supports more features than less.


Regards

Andrzej


>>
>>>> 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.


I tried to find the code in git history, I have found some pre-atomic
patches for zpos. It is not clear how it worked and why mutable zpos has
been removed. Do you remember reasons?


Regards

Andrzej



>>
>>>> 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/
>
> Are you saying it works with latest mainline without my patches?
>
>
> Regards
>
> Andrzej
>
>
>> Thanks,
>> Inki Dae
>>
>>> Regards
>>>
>>> Andrzej
>>>
>>>
>>>> Thanks,
>>>> Inki Dae
>>>>
>>>>> 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
>>>>>
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Inki Dae
>>>>>>
>>>>>>> It was tested on tm2 and trats2.
>>>>>>>
>>>>>>> Regards
>>>>>>> Andrzej
>>>>>>>
>>>>>>>
>>>>>>> Andrzej Hajda (3):
>>>>>>>   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 Dec. 11, 2018, 11:26 p.m. UTC | #10
18. 12. 11. 오후 5:36에 Andrzej Hajda 이(가) 쓴 글:
> On 11.12.2018 09:01, Inki Dae wrote:
>>
>> 18. 12. 11. 오후 4:49에 Andrzej Hajda 이(가) 쓴 글:
>>> On 11.12.2018 00:45, Inki Dae wrote:
>>>> Hi Andrzej,
>>>>
>>>> 18. 12. 10. 오후 4:35에 Andrzej Hajda 이(가) 쓴 글:
>>>>> Hi Inki,
>>>>>
>>>>> On 10.12.2018 03:25, Inki Dae wrote:
>>>>>> Hi Andrzej,
>>>>>>
>>>>>> 18. 12. 6. 오후 6:38에 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?
>>>>> 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
>>>> 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).
>>>
>>>
>>>>> then if you change zorder of planes to: plane3, plane1:
>>>>>
>>>>> 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.
>>>
>>>
>>>> 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.
>>>
>>>
>>>> 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?
>>
>>>> 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.
>>
>>>> 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/
> 
> 
> Are you saying it works with latest mainline without my patches?

Right. actually, I checked your patch with Tizen image released in Sept and it worked well without your patch but with your patch it didn't work.
With latest Tizen image Display is keeping black screen without your patch but window server worked well. Seems latest Tizen image has some problem.

Thanks,
Inki Dae

> 
> 
> Regards
> 
> Andrzej
> 
> 
>>
>> Thanks,
>> Inki Dae
>>
>>>
>>> Regards
>>>
>>> Andrzej
>>>
>>>
>>>> Thanks,
>>>> Inki Dae
>>>>
>>>>> 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
>>>>>
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Inki Dae
>>>>>>
>>>>>>> It was tested on tm2 and trats2.
>>>>>>>
>>>>>>> Regards
>>>>>>> Andrzej
>>>>>>>
>>>>>>>
>>>>>>> Andrzej Hajda (3):
>>>>>>>   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 Dec. 11, 2018, 11:48 p.m. UTC | #11
18. 12. 11. 오후 10:20에 Andrzej Hajda 이(가) 쓴 글:
> Hi again,
> 
> It seems I have missed two questions:
> 
> On 11.12.2018 09:36, Andrzej Hajda wrote:
>> On 11.12.2018 09:01, Inki Dae wrote:
>>> 18. 12. 11. 오후 4:49에 Andrzej Hajda 이(가) 쓴 글:
>>>> On 11.12.2018 00:45, Inki Dae wrote:
>>>>> Hi Andrzej,
>>>>>
>>>>> 18. 12. 10. 오후 4:35에 Andrzej Hajda 이(가) 쓴 글:
>>>>>> Hi Inki,
>>>>>>
>>>>>> On 10.12.2018 03:25, Inki Dae wrote:
>>>>>>> Hi Andrzej,
>>>>>>>
>>>>>>> 18. 12. 6. 오후 6:38에 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?
>>>>>> 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
>>>>> 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).
>>>>
>>>>
>>>>>> then if you change zorder of planes to: plane3, plane1:
>>>>>>
>>>>>> 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.
>>>>
>>>>
>>>>> 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.
>>>>
>>>>
>>>>> 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?
> 
> 
> I didn't say it is required, but usually it is better if the driver
> supports more features than less.

Supporting more feature is ok but without HW support I don't see what benefit there is excepting making user space confusing.

> 
> 
> Regards
> 
> Andrzej
> 
> 
>>>
>>>>> 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.
> 
> 
> I tried to find the code in git history, I have found some pre-atomic
> patches for zpos. It is not clear how it worked and why mutable zpos has
> been removed. Do you remember reasons?
> 

We was able to configure zpos before with below patch and real user used it for long time,
commit 00ae67cf26fad3889e71e3bdbec012b1f938dc0e
Author: Joonyoung Shim <jy0922.shim@samsung.com>
Date:   Wed Jun 27 14:27:06 2012 +0900

    drm/exynos: add property for plane zpos
    
    The exynos drm driver used a specific ioctl - DRM_EXYNOS_PLANE_SET_ZPOS
    to set zpos of plane. It can be substitute to property of plane. This
    patch adds a property for plane zpos and removes
    DRM_EXYNOS_PLANE_SET_ZPOS ioctl.
    
    Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
    Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
    Signed-off-by: Inki Dae <inki.dae@samsung.com>

So you said it was just broken wouldn't be true.
Since then, I don't remember whether zpos feature was broken.
But we made decon/fimd's planes to be immutable because they didn't support priority change of HW overlays.

And Marek added mutable zpos support only for Mixer device. As I mentioned already Mixer device is able to change HW overlay priority so it is reasonable,
commit a2cb911eb663b5820dab89f21ce698d68e7cc568
Author: Marek Szyprowski <m.szyprowski@samsung.com>
Date:   Wed Dec 16 13:21:44 2015 +0100

    drm/exynos: mixer: set window priority based on zpos
    
    'zpos' plane property is configurable, so adjust hardware layers
    priority based on the zpos value. 'zpos' value shifted by one can be
    used directly as hw priority value and stored to the registers, because
    mixer accepts priority values from 1 to 15 (0 means that layer is
    disabled).
    
    This patch also changes the default layer priority to match already
    exposed initial zpos values. The initial configuration is now:
    [top] video > gfx layer1 > gfx layer0 [bottom].
    
    Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
    Signed-off-by: Inki Dae <inki.dae@samsung.com> 

Thanks,
Inki Dae

> 
> Regards
> 
> Andrzej
> 
> 
> 
>>>
>>>>> 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/
>>
>> Are you saying it works with latest mainline without my patches?
>>
>>
>> Regards
>>
>> Andrzej
>>
>>
>>> Thanks,
>>> Inki Dae
>>>
>>>> Regards
>>>>
>>>> Andrzej
>>>>
>>>>
>>>>> Thanks,
>>>>> Inki Dae
>>>>>
>>>>>> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> Inki Dae
>>>>>>>
>>>>>>>> It was tested on tm2 and trats2.
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Andrzej
>>>>>>>>
>>>>>>>>
>>>>>>>> Andrzej Hajda (3):
>>>>>>>>   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 Dec. 12, 2018, 8:30 a.m. UTC | #12
On 12.12.2018 00:48, Inki Dae wrote:
>
> 18. 12. 11. 오후 10:20에 Andrzej Hajda 이(가) 쓴 글:
>> Hi again,
>>
>> It seems I have missed two questions:
>>
>> On 11.12.2018 09:36, Andrzej Hajda wrote:
>>> On 11.12.2018 09:01, Inki Dae wrote:
>>>> 18. 12. 11. 오후 4:49에 Andrzej Hajda 이(가) 쓴 글:
>>>>> On 11.12.2018 00:45, Inki Dae wrote:
>>>>>> Hi Andrzej,
>>>>>>
>>>>>> 18. 12. 10. 오후 4:35에 Andrzej Hajda 이(가) 쓴 글:
>>>>>>> Hi Inki,
>>>>>>>
>>>>>>> On 10.12.2018 03:25, Inki Dae wrote:
>>>>>>>> Hi Andrzej,
>>>>>>>>
>>>>>>>> 18. 12. 6. 오후 6:38에 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?
>>>>>>> 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
>>>>>> 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).
>>>>>
>>>>>
>>>>>>> then if you change zorder of planes to: plane3, plane1:
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>>
>>>>>> 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.
>>>>>
>>>>>
>>>>>> 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?
>>
>> I didn't say it is required, but usually it is better if the driver
>> supports more features than less.
> Supporting more feature is ok but without HW support I don't see what benefit there is excepting making user space confusing.


But I want to emphasize again, this patch adds real support for dynamic
zpos on planes, it is not fake, why it should be confusing for userspace?

It will make exynos_drm planes consistent (all real planes will support
dynamic zpos) and flexible - userspace will be able to change plane
order with simple property adjustment.

Other argument for it is that most drivers having zpos use dynamic zpos
(msm, omap, rcar, tegra, sun4i, sti), immutable_zpos is used only by sti
and only for cursor.


>
>>
>> Regards
>>
>> Andrzej
>>
>>
>>>>>> 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.
>>
>> I tried to find the code in git history, I have found some pre-atomic
>> patches for zpos. It is not clear how it worked and why mutable zpos has
>> been removed. Do you remember reasons?
>>
> We was able to configure zpos before with below patch and real user used it for long time,
> commit 00ae67cf26fad3889e71e3bdbec012b1f938dc0e
> Author: Joonyoung Shim <jy0922.shim@samsung.com>
> Date:   Wed Jun 27 14:27:06 2012 +0900
>
>     drm/exynos: add property for plane zpos
>     
>     The exynos drm driver used a specific ioctl - DRM_EXYNOS_PLANE_SET_ZPOS
>     to set zpos of plane. It can be substitute to property of plane. This
>     patch adds a property for plane zpos and removes
>     DRM_EXYNOS_PLANE_SET_ZPOS ioctl.
>     
>     Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>     Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>     Signed-off-by: Inki Dae <inki.dae@samsung.com>
>
> So you said it was just broken wouldn't be true.
> Since then, I don't remember whether zpos feature was broken.
> But we made decon/fimd's planes to be immutable because they didn't support priority change of HW overlays.


I have found this patch, but I was lost in figuring out how it exactly
worked in pre-atomic world, but it looks suspicious to me - property
change is neither protected from concurrent access neither followed by
hw update. And more importantly I have not spotted code for zpos
normalization, so I guess it was left to userspace to figure it out.

Summarizing, current zpos implementation is:

- done in drm core, driver just does minimal work to translate it to hw,

- atomic,

- consistent across all drm drivers,

- well documented.

All this makes it appealing feature for compositors.


Regards

Andrzej


>
> And Marek added mutable zpos support only for Mixer device. As I mentioned already Mixer device is able to change HW overlay priority so it is reasonable,
> commit a2cb911eb663b5820dab89f21ce698d68e7cc568
> Author: Marek Szyprowski <m.szyprowski@samsung.com>
> Date:   Wed Dec 16 13:21:44 2015 +0100
>
>     drm/exynos: mixer: set window priority based on zpos
>     
>     'zpos' plane property is configurable, so adjust hardware layers
>     priority based on the zpos value. 'zpos' value shifted by one can be
>     used directly as hw priority value and stored to the registers, because
>     mixer accepts priority values from 1 to 15 (0 means that layer is
>     disabled).
>     
>     This patch also changes the default layer priority to match already
>     exposed initial zpos values. The initial configuration is now:
>     [top] video > gfx layer1 > gfx layer0 [bottom].
>     
>     Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>     Signed-off-by: Inki Dae <inki.dae@samsung.com> 
>
> Thanks,
> Inki Dae
>
>> Regards
>>
>> Andrzej
>>
>>
>>
>>>>>> 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/
>>> Are you saying it works with latest mainline without my patches?
>>>
>>>
>>> Regards
>>>
>>> Andrzej
>>>
>>>
>>>> Thanks,
>>>> Inki Dae
>>>>
>>>>> Regards
>>>>>
>>>>> Andrzej
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Inki Dae
>>>>>>
>>>>>>> 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Inki Dae
>>>>>>>>
>>>>>>>>> It was tested on tm2 and trats2.
>>>>>>>>>
>>>>>>>>> Regards
>>>>>>>>> Andrzej
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Andrzej Hajda (3):
>>>>>>>>>   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 Dec. 12, 2018, 8:43 a.m. UTC | #13
On 12.12.2018 00:26, Inki Dae wrote:
>
> 18. 12. 11. 오후 5:36에 Andrzej Hajda 이(가) 쓴 글:
>> On 11.12.2018 09:01, Inki Dae wrote:
>>> 18. 12. 11. 오후 4:49에 Andrzej Hajda 이(가) 쓴 글:
>>>> On 11.12.2018 00:45, Inki Dae wrote:
>>>>> Hi Andrzej,
>>>>>
>>>>> 18. 12. 10. 오후 4:35에 Andrzej Hajda 이(가) 쓴 글:
>>>>>> Hi Inki,
>>>>>>
>>>>>> On 10.12.2018 03:25, Inki Dae wrote:
>>>>>>> Hi Andrzej,
>>>>>>>
>>>>>>> 18. 12. 6. 오후 6:38에 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?
>>>>>> 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
>>>>> 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).
>>>>
>>>>
>>>>>> then if you change zorder of planes to: plane3, plane1:
>>>>>>
>>>>>> 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.
>>>>
>>>>
>>>>> 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.
>>>>
>>>>
>>>>> 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?
>>>
>>>>> 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.
>>>
>>>>> 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/
>>
>> Are you saying it works with latest mainline without my patches?
> Right. actually, I checked your patch with Tizen image released in Sept and it worked well without your patch but with your patch it didn't work.


No surprise, it did not work with mixer for the same reason (broken
dynamic zpos handling), userspace code was written for and tested on
specific version of hardware.

Beside userspace there could be some differences on kernel side
regarding zpos, I will look and test tizen kernel and let you know.


> With latest Tizen image Display is keeping black screen without your patch but window server worked well. Seems latest Tizen image has some problem.


Here is sample script for modetest on tm2 with mainline kernel, which
shows how dynamic zpos works:

----

#!/bin/bash -x

sleep 3600 | modetest -Mexynos -P55:512x512+10+10 -P55:512x512+210+210
-w30:alpha:30000 -w35:alpha:50000 -w40:zpos:0 -w30:zpos:1 -w35:zpos:2 -d &
while read; do
    modetest -Mexynos -w30:zpos:2 -w35:zpos:1
    read
    modetest -Mexynos -w30:zpos:1 -w35:zpos:2
done
-----

Of course CRTC/PLANE ids can differs on your build, so they should be
adjusted.


Regards

Andrzej


>
> Thanks,
> Inki Dae
>
>>
>> Regards
>>
>> Andrzej
>>
>>
>>> Thanks,
>>> Inki Dae
>>>
>>>> Regards
>>>>
>>>> Andrzej
>>>>
>>>>
>>>>> Thanks,
>>>>> Inki Dae
>>>>>
>>>>>> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> Inki Dae
>>>>>>>
>>>>>>>> It was tested on tm2 and trats2.
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Andrzej
>>>>>>>>
>>>>>>>>
>>>>>>>> Andrzej Hajda (3):
>>>>>>>>   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 Dec. 13, 2018, 12:04 a.m. UTC | #14
18. 12. 12. 오후 5:30에 Andrzej Hajda 이(가) 쓴 글:
> On 12.12.2018 00:48, Inki Dae wrote:
>>
>> 18. 12. 11. 오후 10:20에 Andrzej Hajda 이(가) 쓴 글:
>>> Hi again,
>>>
>>> It seems I have missed two questions:
>>>
>>> On 11.12.2018 09:36, Andrzej Hajda wrote:
>>>> On 11.12.2018 09:01, Inki Dae wrote:
>>>>> 18. 12. 11. 오후 4:49에 Andrzej Hajda 이(가) 쓴 글:
>>>>>> On 11.12.2018 00:45, Inki Dae wrote:
>>>>>>> Hi Andrzej,
>>>>>>>
>>>>>>> 18. 12. 10. 오후 4:35에 Andrzej Hajda 이(가) 쓴 글:
>>>>>>>> Hi Inki,
>>>>>>>>
>>>>>>>> On 10.12.2018 03:25, Inki Dae wrote:
>>>>>>>>> Hi Andrzej,
>>>>>>>>>
>>>>>>>>> 18. 12. 6. 오후 6:38에 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?
>>>>>>>> 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
>>>>>>> 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).
>>>>>>
>>>>>>
>>>>>>>> then if you change zorder of planes to: plane3, plane1:
>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>>
>>>>>>> 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?
>>>
>>> I didn't say it is required, but usually it is better if the driver
>>> supports more features than less.
>> Supporting more feature is ok but without HW support I don't see what benefit there is excepting making user space confusing.
> 
> 
> But I want to emphasize again, this patch adds real support for dynamic
> zpos on planes, it is not fake, why it should be confusing for userspace?
It's because user space already know that each HW overlay of DECON and FIMD device has a fixed priority, actually they have definitely fixed HW overlays.

> 
> It will make exynos_drm planes consistent (all real planes will support
> dynamic zpos) and flexible - userspace will be able to change plane
> order with simple property adjustment.
> 
> Other argument for it is that most drivers having zpos use dynamic zpos
> (msm, omap, rcar, tegra, sun4i, sti), immutable_zpos is used only by sti
> and only for cursor.

Right, they support mutable zpos. In addition, seems they also support priority change of HW overlay - I just checked omap and sun4i drivers,
omap case:
dispc_ovl_setup_common(...u8 zorder, ...)
...
        dispc_ovl_set_zorder(..., zorder);

sun4i case:
sun4i_backend_layer_atomic_update(...)
...
	sun4i_backend_update_layer_zpos(...);
 

And Exynos also supports mutable zpos in case of Mixer device because this HW is able to configure the HW overlay priority like other drm drivers did.

Thanks,
Inki Dae

> 
> 
>>
>>>
>>> Regards
>>>
>>> Andrzej
>>>
>>>
>>>>>>> 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.
>>>
>>> I tried to find the code in git history, I have found some pre-atomic
>>> patches for zpos. It is not clear how it worked and why mutable zpos has
>>> been removed. Do you remember reasons?
>>>
>> We was able to configure zpos before with below patch and real user used it for long time,
>> commit 00ae67cf26fad3889e71e3bdbec012b1f938dc0e
>> Author: Joonyoung Shim <jy0922.shim@samsung.com>
>> Date:   Wed Jun 27 14:27:06 2012 +0900
>>
>>     drm/exynos: add property for plane zpos
>>     
>>     The exynos drm driver used a specific ioctl - DRM_EXYNOS_PLANE_SET_ZPOS
>>     to set zpos of plane. It can be substitute to property of plane. This
>>     patch adds a property for plane zpos and removes
>>     DRM_EXYNOS_PLANE_SET_ZPOS ioctl.
>>     
>>     Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>>     Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>     Signed-off-by: Inki Dae <inki.dae@samsung.com>
>>
>> So you said it was just broken wouldn't be true.
>> Since then, I don't remember whether zpos feature was broken.
>> But we made decon/fimd's planes to be immutable because they didn't support priority change of HW overlays.
> 
> 
> I have found this patch, but I was lost in figuring out how it exactly
> worked in pre-atomic world, but it looks suspicious to me - property
> change is neither protected from concurrent access neither followed by
> hw update. And more importantly I have not spotted code for zpos
> normalization, so I guess it was left to userspace to figure it out.
> 
> Summarizing, current zpos implementation is:
> 
> - done in drm core, driver just does minimal work to translate it to hw,
> 
> - atomic,
> 
> - consistent across all drm drivers,
> 
> - well documented.
> 
> All this makes it appealing feature for compositors.
> 
> 
> Regards
> 
> Andrzej
> 
> 
>>
>> And Marek added mutable zpos support only for Mixer device. As I mentioned already Mixer device is able to change HW overlay priority so it is reasonable,
>> commit a2cb911eb663b5820dab89f21ce698d68e7cc568
>> Author: Marek Szyprowski <m.szyprowski@samsung.com>
>> Date:   Wed Dec 16 13:21:44 2015 +0100
>>
>>     drm/exynos: mixer: set window priority based on zpos
>>     
>>     'zpos' plane property is configurable, so adjust hardware layers
>>     priority based on the zpos value. 'zpos' value shifted by one can be
>>     used directly as hw priority value and stored to the registers, because
>>     mixer accepts priority values from 1 to 15 (0 means that layer is
>>     disabled).
>>     
>>     This patch also changes the default layer priority to match already
>>     exposed initial zpos values. The initial configuration is now:
>>     [top] video > gfx layer1 > gfx layer0 [bottom].
>>     
>>     Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>     Signed-off-by: Inki Dae <inki.dae@samsung.com> 
>>
>> Thanks,
>> Inki Dae
>>
>>> Regards
>>>
>>> Andrzej
>>>
>>>
>>>
>>>>>>> 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/
>>>> Are you saying it works with latest mainline without my patches?
>>>>
>>>>
>>>> Regards
>>>>
>>>> Andrzej
>>>>
>>>>
>>>>> Thanks,
>>>>> Inki Dae
>>>>>
>>>>>> Regards
>>>>>>
>>>>>> Andrzej
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> Inki Dae
>>>>>>>
>>>>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Inki Dae
>>>>>>>>>
>>>>>>>>>> It was tested on tm2 and trats2.
>>>>>>>>>>
>>>>>>>>>> Regards
>>>>>>>>>> Andrzej
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Andrzej Hajda (3):
>>>>>>>>>>   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 Dec. 13, 2018, 12:50 a.m. UTC | #15
18. 12. 12. 오후 5:43에 Andrzej Hajda 이(가) 쓴 글:
> On 12.12.2018 00:26, Inki Dae wrote:
>>
>> 18. 12. 11. 오후 5:36에 Andrzej Hajda 이(가) 쓴 글:
>>> On 11.12.2018 09:01, Inki Dae wrote:
>>>> 18. 12. 11. 오후 4:49에 Andrzej Hajda 이(가) 쓴 글:
>>>>> On 11.12.2018 00:45, Inki Dae wrote:
>>>>>> Hi Andrzej,
>>>>>>
>>>>>> 18. 12. 10. 오후 4:35에 Andrzej Hajda 이(가) 쓴 글:
>>>>>>> Hi Inki,
>>>>>>>
>>>>>>> On 10.12.2018 03:25, Inki Dae wrote:
>>>>>>>> Hi Andrzej,
>>>>>>>>
>>>>>>>> 18. 12. 6. 오후 6:38에 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?
>>>>>>> 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
>>>>>> 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).
>>>>>
>>>>>
>>>>>>> then if you change zorder of planes to: plane3, plane1:
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>>
>>>>>> 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.
>>>>>
>>>>>
>>>>>> 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?
>>>>
>>>>>> 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.
>>>>
>>>>>> 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/
>>>
>>> Are you saying it works with latest mainline without my patches?
>> Right. actually, I checked your patch with Tizen image released in Sept and it worked well without your patch but with your patch it didn't work.
> 
> 
> No surprise, it did not work with mixer for the same reason (broken
> dynamic zpos handling), userspace code was written for and tested on
> specific version of hardware.
> 
> Beside userspace there could be some differences on kernel side
> regarding zpos, I will look and test tizen kernel and let you know.
> 
> 
>> With latest Tizen image Display is keeping black screen without your patch but window server worked well. Seems latest Tizen image has some problem.
> 
> 
> Here is sample script for modetest on tm2 with mainline kernel, which
> shows how dynamic zpos works:
> 
> ----
> 
> #!/bin/bash -x
> 
> sleep 3600 | modetest -Mexynos -P55:512x512+10+10 -P55:512x512+210+210
> -w30:alpha:30000 -w35:alpha:50000 -w40:zpos:0 -w30:zpos:1 -w35:zpos:2 -d &
> while read; do
>     modetest -Mexynos -w30:zpos:2 -w35:zpos:1
>     read
>     modetest -Mexynos -w30:zpos:1 -w35:zpos:2
> done
> -----
> 
> Of course CRTC/PLANE ids can differs on your build, so they should be
> adjusted.

I know. Your patch will show that plane order is changed by changing mapping of plane from one window to another window regardless of overlay priority change support. 
I'm not clear but seems maybe we could do more something with your patch even though DECON/FIMD have limit. So I will enqueue your patch until real user is fixed.

Thanks,
Inki Dae

> 
> 
> Regards
> 
> Andrzej
> 
> 
>>
>> Thanks,
>> Inki Dae
>>
>>>
>>> Regards
>>>
>>> Andrzej
>>>
>>>
>>>> Thanks,
>>>> Inki Dae
>>>>
>>>>> Regards
>>>>>
>>>>> Andrzej
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Inki Dae
>>>>>>
>>>>>>> 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Inki Dae
>>>>>>>>
>>>>>>>>> It was tested on tm2 and trats2.
>>>>>>>>>
>>>>>>>>> Regards
>>>>>>>>> Andrzej
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Andrzej Hajda (3):
>>>>>>>>>   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(-)
>>>>>>>>>
>>>>>
>>>
>>>
>>
> 
> 
>