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

[Derek Foreman]

On 1/15/19 8:02 AM, Daniel Stone wrote:
> 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.

I'm having a bit of a hard time following the logic in this first hunk
myself...

The dri2_surf->color_buffers[i].age < age check looks like it's intended
to skip buffers younger than the one currently in hand - ie) pick the
oldest buffer.  But doing so would break the second hunk because we'd
never end up with a very old buffer to trim.  (It doesn't actually cause
the oldest buffer to be picked though, because of the other tests involved)

I would like to at least see a comment explaining what's going on,
because it looks kind of like a bug on a casual read.

I think I side with Emil that this is an independent functional change.
 The first hunk should be able to stand on its own, and having a commit
log for it explaining what it does to the selection process would be
helpful.

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

Yes, it's a bit more annoying to track how long a buffer is "unneeded"
for when you always pick the oldest, since age becomes useless for that
purpose.

For this patch to work, we need a buffer to be unused until it ages
beyond a threshold - intuitively it seems to me this will just naturally
happen to the last buffer in the array without the first hunk at all?

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

Right - to be clear, I think this is a really good idea. :)
I'm just having a little trouble with the details of the implementation.

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