[PATCH 0/3] drm/exynos: add support for dynamic zpos in DECON and FIMD
Inki Dae
inki.dae at samsung.com
Thu Dec 13 00:04:05 UTC 2018
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 at 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 at samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
>> Signed-off-by: Inki Dae <inki.dae at 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 at 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 at samsung.com>
>> Signed-off-by: Inki Dae <inki.dae at 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(-)
>>>>>>>>>>
>>>>
>>>
>>>
>>
>
>
>
More information about the dri-devel
mailing list