weston-simple-egl fullscreen broken?

[Vincent ABRIOU]

On 03/09/2017 04:41 PM, Pekka Paalanen wrote:
> On Thu, 9 Mar 2017 15:01:03 +0000
> Vincent ABRIOU <vincent.abriou at st.com> wrote:
>
>> On 03/09/2017 02:48 PM, Pekka Paalanen wrote:
>>> On Thu, 9 Mar 2017 12:53:35 +0000
>>> Vincent ABRIOU <vincent.abriou at st.com> wrote:
>>>
>>>> Hi Pekka,
>>>>
>>>> On 03/09/2017 11:32 AM, Pekka Paalanen wrote:
>>>>> On Wed, 8 Mar 2017 17:16:31 +0000
>>>>> Vincent ABRIOU <vincent.abriou at st.com> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I have investigate deeper the issue and it comes from the Mali400
>>>>>> wayland library (at least in the r6p1-01rel0 version).
>>>>>>
>>>>>> Actually, the Mali400 wayland library is creating very early the output
>>>>>> surface taking into account the 250x250 default size because fullscreen
>>>>>> information is not yet available at this moment.
>>>>>>
>>>>>> Code from simple-egl.c:
>>>>>>
>>>>>> static void
>>>>>> create_surface(struct window *window)
>>>>>> {
>>>>>> 	struct display *display = window->display;
>>>>>> 	EGLBoolean ret;
>>>>>>
>>>>>> 	window->surface =
>>>>>> 		wl_compositor_create_surface(display->compositor);
>>>>>>
>>>>>> 	window->native =
>>>>>> 		wl_egl_window_create(window->surface,
>>>>>> 				     window->geometry.width,
>>>>>> 				     window->geometry.height);
>>>>>> 	window->egl_surface =
>>>>>> 		weston_platform_create_egl_surface(display->egl.dpy,
>>>>>> 						   display->egl.conf,
>>>>>> 						   window->native,
>>>>>> 						   NULL);
>>>>>> 	/* Mali400 has already created its first 250x250 surface */
>>>>>>
>>>>>> 	...
>>>>>> 	...
>>>>>>
>>>>>> 	/* Full screen information comes too late */
>>>>>> 	if (window->fullscreen)
>>>>>> 		zxdg_toplevel_v6_set_fullscreen(window->xdg_toplevel,
>>>>>> 		NULL);
>>>>>> }
>>>>>>
>>>>>>
>>>>>> The mali400 library lock occurs while the eglSwapBuffer when the 250x250
>>>>>> surface need to be committed whereas a fullscreen surface is expected.
>>>>>>
>>>>>> I fix this issue in the Mali400 wayland library by avoiding to commit
>>>>>> the surface if the size does not match the expected output surface size.
>>>>>
>>>>> Hi,
>>>>>
>>>>> you do know that sometimes not committing from eglSwapBuffers() is
>>>>> going to break applications, right?
>>>>>
>>>>> The commit is used for a lot more than just pushing a buffer.
>>>>>
>>>>> Applications can be expecting other state to be latched in by calling
>>>>> eglSwapBuffers() since it must never return without having sent the
>>>>> wl_surface.commit, and wait for events triggered by that commit before
>>>>> continuing.
>>>>
>>>> Ok I see. So my patch is unsafe.
>>>>
>>>>>
>>>>> In particular, applications that throttle to wl_surface.frame callbacks
>>>>> would freeze forever if any single commit was skipped.
>>>>>
>>>>>> Once the 250x250 surface is skipped during eglSwapBuffer it is then
>>>>>> destroyed and created again with the right fullscreen parameters.
>>>>>
>>>>>>>>>> On 6 February 2017 at 16:56, Fabien DESSENNE <fabien.dessenne at st.com>
>>>>>>>> wrote:
>>>>>>>>>>> I remember I used to get « weston-simple-egl -f » working fine.
>>>>>>>>>>> But it does not work anymore : nothing is displayed. From the logs
>>>>>>>>>>> I can see (among others) zxdg_toplevel_v6.configure and
>>>>>>>>>>> wl_surface.commit Testing with another client works fine:
>>>>>>>>>>> weston-terminal -f -> OK There may be something wrong with my
>>>>>>>>>>> environment since I am testing with the Daniel’s atomic version
>>>>>>>>>>> (forked from weston-1.12.0+), but I guess this is broken also with the official
>>>>>>>> version.
>>>>>
>>>>> I think there are two different things here.
>>>>>
>>>>> First, I believe that simple-egl is maybe a little off when starting
>>>>> with -f. It could negotiate the fullscreen size before creating the
>>>>> wl_egl_window. It actually does already use window->wait_for_configure
>>>>> AFAICT, it would just need to postpone creating the wl_egl_window later
>>>>> (e.g. on-demand just before calling redraw()), which means it needs to
>>>>> postpone also GL init and the other dependent things.
>>>>>
>>>>> The code guarantees, that there are no draw calls before the call to
>>>>> wl_egl_window_resize() setting the size for the first frame. This works
>>>>> fine for EGL implementations that get the buffer on the first draw call
>>>>> or EGL_EXT_buffer_age query, but unfortunately we don't probably have a
>>>>> spec to point to and say the EGL implementation got it wrong.
>>>>>
>>>>> Second, using a wrong sized buffer for the first commit should cause a
>>>>> glitch at most, and start running just fine fullscreen after the first
>>>>> correctly sized buffer has been committed. But it sounds like it just
>>>>> fails to show at all, ever?
>>>>>
>>>>
>>>> I was expecting the first frame is wrong and then others will be good.
>>>> But as you mention, the Mali400 library is stuck and any of the frames
>>>> are shown. The second time the weston-simple-egl redraw function is
>>>> called it is stuck in glClear.
>>>
>>> I wonder if that's actually Weston's problem/feature of not mapping a
>>> fullscreen window with a "wrong" size... Quentin?
>>>
>>> Vincent, would you be able to check what the Mali library is waiting
>>> for? Is it a wl_surface.frame callback or something else?
>>
>> After a check, Mali library is waiting for a wl_surface_frame callback.
>> This callback is used to warn mali that the swap has been completed.
>
> Oh, just occurred to me - if you still have your patch to not issue
> wl_surface.commit from eglSwapBuffers, then the compositor will never
> ack the Mali library's wl_surface.frame either. :-)
>

No, no I test without my patch :). I use the genuine Mali library.

Btw, the path I have done in the Mali library was not setting any 
callback and not committing any surface. It was just returning the 
wayland swap function very early.

Vincent


> Btw. it does not mean "the swap has been completed", or, well, not the
> swap you just posted. The previous swap has been completed. It just
> means the compositor has taken the frame into compositing and it would
> be an appropriate time to start drawing again to *some other* buffer.
>
> Buffer availability is signalled by wl_buffer.release instead. Frame
> callbacks provide throttling, release events tell you which buffer is
> free for re-use.
>
>
> Thanks,
> pq
>