[Mesa-dev] [PATCH] egl/wayland: break double/tripple buffering feedback loops

[Emil Velikov]

On Tue, 18 Dec 2018 at 11:16, Lucas Stach <l.stach at pengutronix.de> wrote:
>
> Currently we dispose any unneeded color buffers immediately if we detect that
> there are more unlocked buffers than we need. This can lead to feedback loops
> between the compositor and the application causing rapid toggling between
> double and tripple buffering. Scenario: 2 buffers already qeued to the
> compositor, egl/wayland allocates a new back buffer to avoid trottling,
> slowing down the frame, this allows the compositor to catch up and unlock
> both buffers, then EGL detects that there are more buffers than currently
> need, freeing the buffer, restartig the loop shortly after.
>
> To avoid wasting CPU time on rapidly freeing and reallocating color buffers
> break those feedback loops by letting the unneeded buffers sit around for a
> short while before disposing them.
>
> Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
> ---
>  src/egl/drivers/dri2/platform_wayland.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
> index 34e09d7ec167..3fa08c1639d1 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -474,15 +474,17 @@ get_back_bo(struct dri2_egl_surface *dri2_surf)
>     wl_display_dispatch_queue_pending(dri2_dpy->wl_dpy, dri2_surf->wl_queue);
>
>     while (dri2_surf->back == NULL) {
> +      int age = 0;
>        for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
>           /* Get an unlocked buffer, preferably one with a dri_buffer
>            * already allocated. */
> -         if (dri2_surf->color_buffers[i].locked)
> +         if (dri2_surf->color_buffers[i].locked || dri2_surf->color_buffers[i].age < age)
>              continue;
>           if (dri2_surf->back == NULL)
>              dri2_surf->back = &dri2_surf->color_buffers[i];
> -         else if (dri2_surf->back->dri_image == NULL)
> +         else if (dri2_surf->back->dri_image == NULL && dri2_surf->color_buffers[i].dri_image)
>              dri2_surf->back = &dri2_surf->color_buffers[i];
> +         age = dri2_surf->back->age;
>        }
>
AFAICT this is the wayland equivalent of
4f1d27a406478d405eac6f9894ccc46a80034adb
Where the exact same logic/commit message applies.

Can you please split that up? I'd imagine this isn't enough to for the
usecase you had in mind?

>        if (dri2_surf->back)
> @@ -615,10 +617,13 @@ update_buffers(struct dri2_egl_surface *dri2_surf)
>
>     /* If we have an extra unlocked buffer at this point, we had to do triple
>      * buffering for a while, but now can go back to just double buffering.
> -    * That means we can free any unlocked buffer now. */
> +    * That means we can free any unlocked buffer now. To avoid toggling between
> +    * going back to double buffering and needing to allocate third buffer too
> +    * fast we let the unneeded buffer sit around for a short while. */
>     for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
>        if (!dri2_surf->color_buffers[i].locked &&
> -          dri2_surf->color_buffers[i].wl_buffer) {
> +          dri2_surf->color_buffers[i].wl_buffer &&
> +          dri2_surf->color_buffers[i].age > 18) {

The age check here seems strange - both number used and it's relation
to double/triple buffering.
Have you considered tracking/checking how many buffers we have?

HTH
Emil