[PATCH v10 00/17] drm/exynos: atomic modesetting support

[Inki Dae]

On 2015년 06월 17일 23:33, Gustavo Padovan wrote:
> Hi Inki,
> 
> 2015-06-17 Inki Dae <inki.dae at samsung.com>:
> 
>> Hi Gustavo,
>>
>> On 2015년 06월 17일 05:35, Gustavo Padovan wrote:
>>> HI Inki,
>>>
>>> 2015-06-15 Inki Dae <inki.dae at samsung.com>:
>>>
>>>> Hi Gustavo,
>>>>
>>>> On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
>>>>> From: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
>>>>>
>>>>> Hi,
>>>>>
>>>>> Here goes the full support for atomic modesetting on exynos. I've
>>>>> split the patches in the various phases of atomic support.
>>>>>
>>>>> v2: fixes comments by Joonyoung
>>>>>         - remove unused var in patch 09
>>>>>         - use ->disable instead of outdated ->dpms in hdmi code
>>>>>         - remove WARN_ON from crtc enable/disable
>>>>>
>>>>> v3: fixes comment by Joonyoung
>>>>>         - move the removal of drm_helper_disable_unused_functions() to
>>>>>         separated patch
>>>>>
>>>>> v4: add patches that remove unnecessary calls to disable_plane()
>>>>>
>>>>> v5: fixes NULL CRTC crash on planes updates (reported by Inki and Tobias)
>>>>>
>>>>> v6: rebase on latest exynos_drm_next
>>>>>
>>>>> v7: fix comments by Joonyoung
>>>>>         - fix two checkpatch errors
>>>>>         - remove extra crtc->commit() call
>>>>>         - check for null fb on exynos_check_plane()
>>>>>
>>>>> v8: fix comments by Joonyoung
>>>>>         - fix merge error
>>>>>         - move drm_crtc_vblank_get to the commit that introduces atomic pageflip
>>>>>         - remove .prepare() in the apropiated patch
>>>>>         - add a new patch to move exynos_drm_crtc_disable()
>>>>>
>>>>> v9:  * fix comments by Joonyoung
>>>>>         - also remove encoder .prepare()
>>>>>         - do not shift exynos_update_plane() parameters
>>>>>         - remove unused .mode_set() and .mode_set_base()
>>>>>      * add specific exynos .atomic_commit()
>>>>>      * add split of exynos_crtc->ops->dpms() into enable() and disable()
>>>>>      * add other atomic clean ups
>>>>>
>>>>> v10: * fix comments by Joonyoung
>>>>> 	- add more comments on exynos_atomic_commit()
>>>>> 	- make exynos_crtc's .enable and .disable void
>>>>
>>>> I found out one issue that refresh rate gets low with display extension
>>>> mode test.
>>>>
>>>> I tested it with two crtc drivers - vidi and fimd on Trats2 board. Here
>>>> is how to test it,
>>>>
>>>> #echo 1 > /sys/devices/platform/exynos-drm-vidi/connection
>>>> # modetest -M exynos -v -s 31 at 29:720x1280 -s 23 at 21:640x480
>>>> setting mode 720x1280-60Hz at XR24 on connectors 31, crtc 29
>>>> setting mode 640x480-75Hz at XR24 on connectors 23, crtc 21
>>>> freq: 20.00Hz
>>>> freq: 20.00Hz
>>>>
>>>> As you can see, refresh rate is 20.
>>>>
>>>> Below is the result without atomic patch series,
>>>> # modetest -M exynos -v -s 31 at 29:720x1280 -s 23 at 21:640x480
>>>> setting mode 720x1280-60Hz at XR24 on connectors 31, crtc 29
>>>> setting mode 640x480-75Hz at XR24 on connectors 23, crtc 21
>>>> freq: 60.18Hz
>>>> freq: 49.09Hz
>>>>
>>>> I can see 60Hz for FIMD and 49Hz for vidi.
>>>
>>> I gave this a try and figured out that this might be a vidi specific
>>> problem. If I try VIDI and FIMD I get the same results as you but with
>>> Mixer and FIMD(the setup I actually use to test) everything works fine.
>>
>> Hm... Did Mixer and FIMD combination really work correctly with
>> extension mode?
> 
> Yes.
> 
> collabora at singularity:~$ modetest -M exynos -s 31:1920x1080 -v -s
> 25:1366x768
> setting mode 1920x1080-60Hz at XR24 on connectors 31, crtc 29
> setting mode 1366x768-60Hz at XR24 on connectors 25, crtc 23
> freq: 55.82Hz
> freq: 55.38Hz
> freq: 55.30Hz
> freq: 55.38Hz
> freq: 55.30Hz
> freq: 55.38Hz
> freq: 55.30Hz
> freq: 55.38Hz
> freq: 55.30Hz
> freq: 55.38Hz

Your patch series had one bug that the vblank operation was duplicated.
As you may know, the page flip operation already contains vblank
functionality but your patch made atomic_commit callback wait for the
completion of vblank again.

See the below patch I posted,
[PATCH] drm/exynos: do not wait for vblank at atomic operation

That was why I asked for that mixer and fimd combination worked
correctly with extension mode. Don't you think above frequency is
strange? The frequency should be 60Hz, not 55Hz.

We would need to enhance the atomic feature for Exynos drm later.
Current atomic feature doesn't really do atomic operation, and is
required for more cleanups. Anyway, I have already merged it and will
have a pull-request soon.

Thanks for your contributions,
Inki Dae

> 
>>
>>> So somehow my patches caused a regression on vidi that I'm still
>>> ivestigating.
>>
>> I think this issue is because page flip operations are performed in
>> atomic: if I see correctly, when page flip is requested by modetest, the
>> call flow is as follows,
>>
>> drm_atomic_helper_page_flip
>> 	drm_atomic_async_commit
>> 		exynos_atomic_commit
>> 			...
>> 			drm_atomic_helper_wait_for_vblanks
>> 			...
>>
>> However, the function, drm_atomic_helper_wait_for_vblanks called by
>> exynos_atomic_commit, waits for the vblank completion of each crtc
>> driver . See wait_event_timeout function call in
>> drm_atomic_helper_wait_for_vblanks function.
>>
>> This means that a page flip request of a crtc driver cannot be performed
>> until the vblank completion of another crtc driver. I think you should
>> have implemented exynos_atomic_commit function asynchronously, not
>> synchronously like you did. So it seems that this function should be
>> re-implemented.
> 
> I have a patch for it already. I'll resend a v2 of my last series. 
> 
>>
>> If my analysis is correct and you cannot post the change set within this
>> week, I'm not sure whether the atomic patch series should go to
>> mainline. Anyway, I will decide it and have a pull-request at the end of
>> this week at least.
> 
> We have about 3 months to fix this, until v4.2 is out, not one week. And
> it is working for FIMD and Mixer.
> 
> 	Gustavo
>