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

[Emil Velikov]

On Tue, 18 Dec 2018 at 17:59, Lucas Stach <l.stach at pengutronix.de> wrote:
>
> 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.
>
Indeed, I misread things. Thanks for the correction.

> > 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.
>
As Pekka pointed out, please document how the number was chosen.
Otherwise any change could lead to a regression.

Thanks
Emil