gles: Sync texture uploads for shared contexts

[Drakulix]

Attempt to solve #1615.

This solution isn't perfect, most notably it doesn't synchronize across the access of Bind<GlesTexture>. To do that we would need to be able to store the lock, which means moving the GlesTarget into the GlesFrame and making Bind return a Frame. I think we talked about this, so we should fix this at some point, but I didn't want to do that refactor now.

Fixing Bind would automatically solve this for exporting and other things as well, as all of these rely to binding a framebuffer container object to our texture.

(Also binding a texture to write to it and sampling from it in a separate thread is probably a operation not as common.)

Additionally we keep a lock to the surface-data, so threads should not be able to race imports to the same texture any more.

In an attempt to not punish GL ES 2.0 contexts too much (which don't support fencing at all), we do try to track whether a context is shared or not in the first place, before we fall back to glFinish. This means writes to a texture while a shared context is constructed could potentially be still racy, but I doubt this is a real problem.

[ids1024]

Hm, a glFinish call for each shm buffer upload (even though most won't be shown on other outputs) sounds like it could be rough on performance, but necessary without proper synchronization mechanisms. Though only GL ES 2.0 being available shouldn't be too common. I guess it's not hard to force an ES 2.0 context and test this, though any GPU that only supports 2.0 will also probably be lower performance to start with.

Might be best just to disable shared contexts on GL ES 2.0 then, though that would be handled by the compositor and not Smithay (should it be documented somewhere?).

[Drakulix]

Hm, a glFinish call for each shm buffer upload (even though most won't be shown on other outputs) sounds like it could be rough on performance, but necessary without proper synchronization mechanisms. Though only GL ES 2.0 being available shouldn't be too common. I guess it's not hard to force an ES 2.0 context and test this, though any GPU that only supports 2.0 will also probably be lower performance to start with.

Might be best just to disable shared contexts on GL ES 2.0 then, though that would be handled by the compositor and not Smithay (should it be documented somewhere?).

Yeah, that is what I was thinking for cosmic-comp. Disable sharing, if only 2.0 is available. I struggled with finding a place to document this. It really belongs to the ImportMemWl implementation, but there aren't doc-comments for implementation blocks of traits.

[ids1024]

Sounds good.

but there aren't doc-comments for implementation blocks of traits.

Actually there are; if you add a doc comment to the impl block it will show up at https://smithay.github.io/smithay/smithay/backend/renderer/gles/struct.GlesRenderer.html#impl-ImportMemWl-for-GlesRenderer

Though it's not emphasized much in the generated documentation, so it's rather easy to miss any information there.

[Drakulix]

@ids1024 Should both be addressed.

[ids1024]

This generally looks right. surface_lock does seem like it should prevent races in this function, and the calls to wait_for_upload should address reading before the draw call is completed, as well as starting another write first. Using ClientWaitSync to remove already signaled fences seems good too.

When a surface rendered on multiple outputs, do multiple threads end up calling import_shm_buffer with the same damage regions? And the second thread will wait_for_upload on the fence created by the first thread, but then still try to render? There's no harm other than performance in copying the same damage regions multiple times, but I guess that could also leave the first thread still reading from the texture when the second thread tries to draw to it. (Which is probably technically undefined even if the draw won't change the texture.)

[Drakulix]

When a surface rendered on multiple outputs, do multiple threads end up calling import_shm_buffer with the same damage regions?

Only if they race the import. The last imported commit is only updated after the import has taken place. I guess we could change that now that we are synchronizing, but I am not sure this is correct for other renderers, which might then access the old or partially updated texture during the second import.

If we wanted to address that, we probably need to document the expected synchronization/multi-threading properties and then carefully check every renderer.

And the second thread will wait_for_upload on the fence created by the first thread, but then still try to render?

Yes, to fix that we would need to pass in some serial of the import, so we would know, if the wait_for_upload-fence was for the same commit we are trying to process. (It could be from a previous import.)

There's no harm other than performance in copying the same damage regions multiple times, but I guess that could also leave the first thread still reading from the texture when the second thread tries to draw to it. (Which is probably technically undefined even if the draw won't change the texture.)

Rendering the texture also briefly locks the upload sync-point, so as long as an import is still in progress we will block there. The only issue here is, when we are already drawing, thus the sync-lock is released and then the import-call happens. To fix that we would have to keep the lock during the render and generate a new sync-point.. Sigh

[ids1024]

Only if they race the import. The last imported commit is only updated after the import has taken place

I guess this shouldn't race at the moment (at least as this is usually used), since import_surface holds a lock on data before it reads renderer_seen, and doesn't release that lock until it's updated that.

Though RendererSurfaceState is not specific to a renderer? So could this cause problems if import_surface is being called with different renderers?

[Drakulix]

Only if they race the import. The last imported commit is only updated after the import has taken place

I guess this shouldn't race at the moment (at least as this is usually used), since import_surface holds a lock on data before it reads renderer_seen, and doesn't release that lock until it's updated that.

True.

Though RendererSurfaceState is not specific to a renderer? So could this cause problems if import_surface is being called with different renderers?

No, since the texture storage is specific to the renderer.

[ids1024]

Oh right, renderer_seen is indexed by texture_id which depends on renderer.id() which is unique for renderers that don't share contexts. So that part is fine as well.

So yeah, as far as I can tell this should synchronize things correctly with the way compositors normally use these APIs, though a compositor doing something more unusual may still be able to do something that doesn't synchronize properly. So this seems like a good improvement, though it could be worth thinking about how to better design some of these APIs in the future.