On 1/15/19 8:02 AM, Daniel Stone wrote: > Hi, > > On Tue, 18 Dec 2018 at 17:59, Lucas Stach <[email protected]> wrote: >> Am Dienstag, den 18.12.2018, 17:43 +0000 schrieb Emil Velikov: >>>> On Tue, 18 Dec 2018 at 11:16, Lucas Stach <[email protected]> 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 <[email protected]> > > Cheers, > Daniel > _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
