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

[Lucas Stach]

Am Dienstag, den 18.12.2018, 17:43 +0000 schrieb 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.

No it isn't. It's exactly what it says in the commit log. It's keeping
the tripple buffer around for a bit, even if we don't strictly need it
when the client is currently doing double buffering.

When things are on the edge between double buffering being enough and
sometimes a third buffer being needed to avoid stalling we would
otherwise bounce rapidly between allocating and disposing the third
buffer.

The DRM platform has no such optimization and just keeps the third
buffer around forever. This patch keeps the optimization in the Wayland
platform, but adds a bit of hysteresis before disposing the buffer when
going from tripple to double buffering to see if things are settling on
double buffering.

> 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?

A hysteresis value of 18 is just something that worked well in
practice. It didn't appear to defer the buffer destruction for too long
 while keeping the feedback loop well under control.

Regards,
Lucas