[PATCH libdrm] tests/modetest: Add modetest_atomic tool

[Emil Velikov]

On 25 July 2018 at 13:37, Benjamin Gaignard
<benjamin.gaignard at linaro.org> wrote:
> 2018-07-25 14:05 GMT+02:00 Emil Velikov <emil.l.velikov at gmail.com>:
>> On 25 July 2018 at 12:27, Benjamin Gaignard
>> <benjamin.gaignard at linaro.org> wrote:
>>> 2018-07-25 12:29 GMT+02:00 Emil Velikov <emil.l.velikov at gmail.com>:
>>>> On 20 July 2018 at 12:33, Benjamin Gaignard
>>>> <benjamin.gaignard at linaro.org> wrote:
>>>>> This is a modetest like tool but using atomic API.
>>>>>
>>>>> With modetest_atomic it is mandatory to specify a mode ("-s")
>>>>> and a plane ("-P") to display a pattern on screen.
>>>>>
>>>>> "-v" does a loop swapping between two framebuffers for each
>>>>> active planes.
>>>>>
>>>>> modetest_atomic doesn't offer cursor support
>>>>>
>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard at linaro.org>
>>>>> ---
>>>>>
>>>>> The code is based on modetest and keep most of it infrastructure
>>>>> like arguments parsing, finding properties id from their name,
>>>>> resources dumping or the general way of working.
>>>>> It duplicates modetest code but adding compilation flags or
>>>>> conditional tests everywhere in modetest would have made it
>>>>> more complex and unreadable.
>>>>>
>>>>> Creating modetest_atomic could allow to test atomic API without
>>>>> need to use "big" frameworks like weston, drm_hwcomposer or igt
>>>>> with all their dependencies.
>>>>> kmscube could also be used to test atomic API but it need EGL.
>>>>>
>>>>> It have been tested (only) on stm driver with one or two planes
>>>>> actived.
>>>>>
>>>>>  tests/modetest/Makefile.am       |   13 +-
>>>>>  tests/modetest/Makefile.sources  |    7 +
>>>>>  tests/modetest/modetest_atomic.c | 1546 ++++++++++++++++++++++++++++++++++++++
>>>>
>>>> Quick diff/grep/wc exercise shows that ~10% of modetest_atomic.c is new code.
>>>> I agree that modetest.c is getting kind of big and messy. How about
>>>> instead of duplicating it, we flesh out the legacy vs atomic codepaths
>>>> into helpers in separate files?
>>>>
>>>> It will result in [c]leaner code, only one executable
>>>> installed/packaged and less build system changes.
>>>>
>>>> What do you think?
>>>> Emil
>>>
>>> It was my initial thought too: I have tried to add "-a" option to
>>> enable atomic way
>>> of working but their is small changes to do everywhere and that add
>>> lot of conditional tests.
>> Can you please try beating something into shape? It's fairly hard to
>> judge without anything to look at.
>
> After dump_resource  code in main function all the logic is different
> between the two
> modetest because, with atomic you to build a request and commit it
> while, with legacy API,
> you could set mode and after set a plane.
> In addition with atomic version you have set a mode and a plane to be
> able to display something.
> That explain why the end of main function is completely different.
>
> Another example in the set_mode function: in legacy API you have
> create a FB and reference/use it
> with calls to bo_create, drmModeAddFB2 and drmModeSetCrtc while for
> atomic you have to set
> properties on connectors, create and set a blob for the mode before
> active the crct.
> The same diff exist for set_plane.
>
> I can add big if/else blocks but I do think that will make the code
> more readable.

Guessing you meant "...I do not think..."

What is the blocker of moving the atomic specifics into a few functions?
As said before legacy.c and atomic.c would be great.

That will about 5(?) cases of

if (atomic)
  atomic_foo(...)
else
  legacy_foo(...)

I can see that 5 (10 even) cases of if (atomic) atomic_foo(...) else
legacy_foo(...) can be fiddly. Yet it fades in the context of 1.5kloc.

Thanks
Emil