On Tue, 18 Dec 2018 at 17:59, Lucas Stach <l.st...@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.st...@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.st...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev