1694788 - Don't bind framebuffers just so we can invalidate them on Mali devices

Status: RESOLVED INVALID

(Core :: Graphics: WebRender, task)

Description

[Jamie Nicol [:jnicol]]

Currently we call invalidate_render_target() at the end of each render pass, and invalidate the framebuffers which are no longer required. invalidate_render_target() iterates through each of the texture's framebuffers, binds it, and calls glInvalidateFramebuffer.

I believe this is the desirable behaviour on some platforms, certainly on Angle according to past discussions I've had with Dzmitry. On Mali, however, this is too late to call glInvalidateFramebuffer for it to be helpful [1].

We only benefit from invalidating the framebuffers before they are unbound after rendering to them, as it allows the driver to omit the tile resolve step. Binding the framebuffer only to invalidate it is at best worthless, but in fact we have seen cases where it is actively harmful [2]. You would think the driver would be smart enough to avoid reading the framebuffer when it's bound if its only use is to be invalidated, but that doesn't appear to always be the case.

[2] I've seen less extreme cases of this in several local profiles. But in this bug there are lots of performance issues, and some of the profiles show many long calls to glInvalidateFramebuffer. The patch I'm about to upload significantly improved things on at least one of the sites.

Comment 1

[Jamie Nicol [:jnicol]]

Attachment: Bug 1694788 - Don't bind framebuffers just to invalidate them on Mali. r?gw

Currently we call invalidate_render_target() at the end of each render
pass, and invalidate the framebuffers which are no longer
required. invalidate_render_target() iterates through each of the
texture's framebuffers, binds it, and calls glInvalidateFramebuffer.

While this is the desirable behaviour on some platforms, on Mali this
is too late to call glInvalidateFramebuffer for it to be helpful.

We only benefit from invalidating the framebuffers before they are
unbound after rendering to them, as it allows the driver to omit the
tile resolve step. Binding the framebuffer only to invalidate it is at
best worthless, but in fact we have seen cases where it is actively
harmful.

This patch adds a RenderTargetInvalidationMode enum which controls the
desired behaviour regarding invalidating framebuffers. On other
platforms we continue to bind and invalidate framebuffers when they
are no longer required. On Mali, we only invalidate framebuffers if
they are already bound.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jnicol, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 3

[Glenn Watson [:gw]]

Not sure what's up with this one, leaving for jnicol to follow up.

Comment 4

[Jamie Nicol [:jnicol]]

This didn't actually seem to help much - the time in InvalidateFramebuffers just indicated the GPU was overwhelmed. In theory it may still be a worthwhile change but we can always resurrect the patch later if required