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

[Daniel Stone]

Hi,

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:
> > >           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.

Right - the crucial part in Derek's GBM commit was removing the
'break' and adding the extra conditional on age.

Derek's patch stabilises the age of buffers handed back to the user,
by always returning the oldest available buffer. That slightly
pessimises rendering if there is a 'free' buffer in the queue: if four
buffers are allocated, then we will always return a buffer from three
flips ago, maybe meaning more rendering work. It means that, barring
the client holding on to one buffer for unexpectedly long, the age of
the oldest buffer in the queue will never be greater than the queue
depth.

This patch instead relies on unbalanced ages, where older buffers in
the queue are allowed to age far beyond the queue depth if not used
during normal rendering.

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

Ideally we'd have globally optimal behaviour for both platforms, but
that doesn't really seem doable for now. I think this is a good
balance though. There will only be one GBM user at a time, so having
that allocate excessive buffers doesn't seem too bad, and the penalty
for doing so is your entire system stuttering as the compositor
becomes blocked. Given the general stability of compositors, if they
need a larger queue depth at some point, they are likely to need it
again in the near future.

Conversely, there may be a great many Wayland clients, and these
clients may bounce between overlay and GPU composition. Given that, it
seems reasonable to opportunistically free up buffers, to make sure we
have enough memory available across the system.

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

Yeah, having this #defined with a comment above it would be nice.

With that, this patch is:
Reviewed-by: Daniel Stone <daniels at collabora.com>

Cheers,
Daniel