renderer: return to original context before deleting a VA element

Submitted by ramin.azarmehr@gmail.com on June 15, 2018, 6:21 p.m.

Details

Message ID 20180615182146.8962-1-ramin.azarmehr@gmail.com
State New
Headers show
Series "renderer: return to original context before deleting a VA element" ( rev: 1 ) in Virgil 3D

Not browsing as part of any series.

Commit Message

ramin.azarmehr@gmail.com June 15, 2018, 6:21 p.m.
From: Ramin Azarmehr <ramin.azarmehr@gmail.com>

Bug (GL or GLES): deleting a VA element from a wrong current-context causes rendering/freezing problems.

Reason: at the time of deleting the VA element, the original context that generated it is not current, causing 
a "va->id" of another context to be deleted. This problem can also occur for sampler view/state and other objects. 

Fix: check if the correct EGL context is "current" (if not, switch to it) before deleting VAs. Calling
vrend_hw_switch_context() is cheap if the correct context is already current.

Reproducible example: run Android Nougat with virgl, then launch Google Maps (creates lots of VAs in its context). 
Then, press Square button of Android to tile the active applications (context switches to SurfaceFlinger), 
and close Google Maps window from there. The rendering will freeze (VA from SurfaceFlinger will be deleted instead). 
On Mesa-based drivers (e.g., Intel i915) an error like below will be generated by glBindVertexArray() 
in vrend_draw_bind_vertex_binding():

"Mesa: User error: GL_INVALID_OPERATION in glBindVertexArray(non-gen name)"

---
 src/vrend_renderer.c | 3 +++
 1 file changed, 3 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
index d6dee8b..85b68c1 100644
--- a/src/vrend_renderer.c
+++ b/src/vrend_renderer.c
@@ -252,6 +252,7 @@  struct vrend_vertex_element_array {
    unsigned count;
    struct vrend_vertex_element elements[PIPE_MAX_ATTRIBS];
    GLuint id;
+   struct vrend_context *ctx;
 };
 
 struct vrend_constants {
@@ -1276,6 +1277,7 @@  static void vrend_destroy_vertex_elements_object(void *obj_ptr)
    struct vrend_vertex_element_array *v = obj_ptr;
 
    if (vrend_state.have_gles31_vertex_attrib_binding) {
+      vrend_hw_switch_context(v->ctx, true);
       glDeleteVertexArrays(1, &v->id);
    }
    FREE(v);
@@ -1823,6 +1825,7 @@  int vrend_create_vertex_elements_state(struct vrend_context *ctx,
    if (!v)
       return ENOMEM;
 
+   v->ctx = ctx;
    v->count = num_elements;
    for (i = 0; i < num_elements; i++) {
       memcpy(&v->elements[i].base, &elements[i], sizeof(struct pipe_vertex_element));

Comments

On 16 June 2018 at 04:21,  <ramin.azarmehr@gmail.com> wrote:
> From: Ramin Azarmehr <ramin.azarmehr@gmail.com>
>
> Bug (GL or GLES): deleting a VA element from a wrong current-context causes rendering/freezing problems.
>
> Reason: at the time of deleting the VA element, the original context that generated it is not current, causing
> a "va->id" of another context to be deleted. This problem can also occur for sampler view/state and other objects.
>
> Fix: check if the correct EGL context is "current" (if not, switch to it) before deleting VAs. Calling
> vrend_hw_switch_context() is cheap if the correct context is already current.
>
> Reproducible example: run Android Nougat with virgl, then launch Google Maps (creates lots of VAs in its context).
> Then, press Square button of Android to tile the active applications (context switches to SurfaceFlinger),
> and close Google Maps window from there. The rendering will freeze (VA from SurfaceFlinger will be deleted instead).
> On Mesa-based drivers (e.g., Intel i915) an error like below will be generated by glBindVertexArray()
> in vrend_draw_bind_vertex_binding():
>
> "Mesa: User error: GL_INVALID_OPERATION in glBindVertexArray(non-gen name)"
>

This seems indicative of a bigger problem, there may be a bunch of
object destruction paths which rely on the context
being correct, we probably need to audit them all, I don't think we
should need to store the ctx like here, we should
have switched ctx before getting this far.

Dave.