i965: Use finish_external instead of make_shareable in setTexBuffer2

The setTexBuffer2 hook from GLX is used to implement glxBindTexImageEXT
which has tighter restrictions than just "it's shared".  In particular,
it says that any rendering to the image while it is bound causes the
contents to become undefined.

The GLX_EXT_texture_from_pixmap extension provides us with an acquire
and release in the form of glXBindTexImageEXT and glXReleaseTexImageEXT.
The extension spec says,

    "Rendering to the drawable while it is bound to a texture will leave
    the contents of the texture in an undefined state.  However, no
    synchronization between rendering and texturing is done by GLX.  It
    is the application's responsibility to implement any synchronization
    required."

From the EGL 1.4 spec for eglBindTexImage:

    "After eglBindTexImage is called, the specified surface is no longer
    available for reading or writing.  Any read operation, such as
    glReadPixels or eglCopyBuffers, which reads values from any of the
    surface’s color buffers or ancillary buffers will produce
    indeterminate results.  In addition, draw operations that are done
    to the surface before its color buffer is released from the texture
    produce indeterminate results

In other words, between the bind and release calls, we effectively own
those pixels and can assume, so long as we don't crash, that no one else
is reading from/writing to the surface.  The GLX and EGL implementations
call the setTexBuffer2 and releaseTexBuffer function pointers that the
driver can hook.

In theory, this means that, between BindTexImage and ReleaseTexImage, we
own the pixels and it should be safe to track aux usage so we
can avoid redundant resolves so long as we start off with the right
assumption at the start of the bind/release pair.

In practice, however, X11 has slightly different expectations.  It's
expected that the server may be drawing to the image at the same time as
the compositor is texturing from it.  In that case, the worst expected
outcome should be tearing or partial rendering and not random corruption
like we see when rendering races with scanout with CCS.  Fortunately,
the GEM rules about texture/render dependencies save us here.  If X11
submits work to write to a pixmap after the compositor has submitted
work to texture from it, GEM inserts a dependency between the compositor
and X11.  If X11 is using a high-priority context, this will cause the
compositor to get a temporarily boosted priority while the batch from
X11 is waiting on it.  This means that we will never have an actual race
between X11 and the compositor so no corruption can happen.

Unfortunately, however, this means that X11 will likely be rendering to it
between the compositor's BindTexImage and ReleaseTexImage calls.  If we
want to avoid strange issues, we need to be a bit careful about
resolves because we can't really transition it away from the "default"
aux usage.  The only case where this would practically be a problem is
with image_load_store where we have to do a full resolve in order to use
the image via the data port.  Even there it would only be a problem if
batches were split such that X11's rendering happens between the resolve
and the use of it as a storage image.  However, the chances of this
happening are very slim so we just emit a warning and hope for the best.

This commit adds a new helper intel_miptree_finish_external which resets
all aux state to whatever ISL says is the right worst-case "default" for
the given modifier.  It feels a little awkward to call it "finish"
because it's actually an acquire from the perspective of the driver, but
it matches the semantics of the other prepare/finish functions.  This
new helper gets called in intelSetTexBuffer2 instead of make_shareable.
We also add an intelReleaseTexBuffer (we passed NULL to releaseTexBuffer
before) and call intel_miptree_prepare_external in it.  This probably
does nothing most of the time but it means that the prepare/finish calls
are properly matched.

Reviewed-by: Kenneth Graunke <[email protected]>
Reviewed-by: Chad Versace <[email protected]>

5 files changed, 87 insertions, 2 deletions

diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 819c36b2135..c6213b21629 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -2796,6 +2796,25 @@ intel_miptree_prepare_external(struct brw_context *brw,
                                 aux_usage, supports_fast_clear);
 }
 
+void
+intel_miptree_finish_external(struct brw_context *brw,
+                              struct intel_mipmap_tree *mt)
+{
+   if (!mt->mcs_buf)
+      return;
+
+   /* We don't know the actual aux state of the aux surface.  The previous
+    * owner could have given it to us in a number of different states.
+    * Because we don't know the aux state, we reset the aux state to the
+    * least common denominator of possible valid states.
+    */
+   enum isl_aux_state default_aux_state =
+      isl_drm_modifier_get_default_aux_state(mt->drm_modifier);
+   assert(mt->last_level == mt->first_level);
+   intel_miptree_set_aux_state(brw, mt, 0, 0, INTEL_REMAINING_LAYERS,
+                               default_aux_state);
+}
+
 /**
  * Make it possible to share the BO backing the given miptree with another
  * process or another miptree.
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
index 7fcf09f118a..07c85807e80 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
@@ -675,6 +675,9 @@ intel_miptree_finish_depth(struct brw_context *brw,
 void
 intel_miptree_prepare_external(struct brw_context *brw,
                                struct intel_mipmap_tree *mt);
+void
+intel_miptree_finish_external(struct brw_context *brw,
+                              struct intel_mipmap_tree *mt);
 
 void
 intel_miptree_make_shareable(struct brw_context *brw,
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
index ef5aee894fa..1f9b0efa42f 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -130,7 +130,7 @@ static const __DRItexBufferExtension intelTexBufferExtension = {
 
    .setTexBuffer        = intelSetTexBuffer,
    .setTexBuffer2       = intelSetTexBuffer2,
-   .releaseTexBuffer    = NULL,
+   .releaseTexBuffer    = intelReleaseTexBuffer,
 };
 
 static void
diff --git a/src/mesa/drivers/dri/i965/intel_tex.h b/src/mesa/drivers/dri/i965/intel_tex.h
index 9fb1b3ffbc4..4c48875f820 100644
--- a/src/mesa/drivers/dri/i965/intel_tex.h
+++ b/src/mesa/drivers/dri/i965/intel_tex.h
@@ -43,6 +43,8 @@ void intelSetTexBuffer(__DRIcontext *pDRICtx,
 		       GLint target, __DRIdrawable *pDraw);
 void intelSetTexBuffer2(__DRIcontext *pDRICtx,
 			GLint target, GLint format, __DRIdrawable *pDraw);
+void intelReleaseTexBuffer(__DRIcontext *pDRICtx, GLint target,
+                           __DRIdrawable *dPriv);
 
 struct intel_mipmap_tree *
 intel_miptree_create_for_teximage(struct brw_context *brw,
diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c b/src/mesa/drivers/dri/i965/intel_tex_image.c
index 8c4e0da5a05..e25bc9a0c08 100644
--- a/src/mesa/drivers/dri/i965/intel_tex_image.c
+++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
@@ -487,7 +487,7 @@ intelSetTexBuffer2(__DRIcontext *pDRICtx, GLint target,
       internal_format = GL_RGB;
    }
 
-   intel_miptree_make_shareable(brw, rb->mt);
+   intel_miptree_finish_external(brw, rb->mt);
 
    _mesa_lock_texture(&brw->ctx, texObj);
    texImage = _mesa_get_tex_image(ctx, texObj, target, 0);
@@ -496,6 +496,67 @@ intelSetTexBuffer2(__DRIcontext *pDRICtx, GLint target,
    _mesa_unlock_texture(&brw->ctx, texObj);
 }
 
+void
+intelReleaseTexBuffer(__DRIcontext *pDRICtx, GLint target,
+                      __DRIdrawable *dPriv)
+{
+   struct brw_context *brw = pDRICtx->driverPrivate;
+   struct gl_context *ctx = &brw->ctx;
+   struct gl_texture_object *tex_obj;
+   struct intel_texture_object *intel_tex;
+
+   tex_obj = _mesa_get_current_tex_object(ctx, target);
+   if (!tex_obj)
+      return;
+
+   _mesa_lock_texture(&brw->ctx, tex_obj);
+
+   intel_tex = intel_texture_object(tex_obj);
+   if (!intel_tex->mt) {
+      _mesa_unlock_texture(&brw->ctx, tex_obj);
+      return;
+   }
+
+   /* The intel_miptree_prepare_external below as well as the finish_external
+    * above in intelSetTexBuffer2 *should* do nothing.  The BindTexImage call
+    * from both GLX and EGL has TexImage2D and not TexSubImage2D semantics so
+    * the texture is not immutable.  This means that the user cannot create a
+    * texture view of the image with a different format.  Since the only three
+    * formats available when using BindTexImage are all UNORM, we can never
+    * end up with an sRGB format being used for texturing and so we shouldn't
+    * get any format-related resolves when texturing from it.
+    *
+    * While very unlikely, it is possible that the client could use the bound
+    * texture with GL_ARB_image_load_store.  In that case, we'll do a resolve
+    * but that's not actually a problem as it just means that we lose
+    * compression on this texture until the next time it's used as a render
+    * target.
+    *
+    * The only other way we could end up with an unexpected aux usage would be
+    * if we rendered to the image from the same context as we have it bound as
+    * a texture between BindTexImage and ReleaseTexImage.  However, the spec
+    * clearly calls this case out and says you shouldn't do that.  It doesn't
+    * explicitly prevent binding the texture to a framebuffer but it says the
+    * results of trying to render to it while bound are undefined.
+    *
+    * Just to keep everything safe and sane, we do a prepare_external but it
+    * should be a no-op in almost all cases.  On the off chance that someone
+    * ever triggers this, we should at least warn them.
+    */
+   if (intel_tex->mt->mcs_buf &&
+       intel_miptree_get_aux_state(intel_tex->mt, 0, 0) !=
+       isl_drm_modifier_get_default_aux_state(intel_tex->mt->drm_modifier)) {
+      _mesa_warning(ctx, "Aux state changed between BindTexImage and "
+                         "ReleaseTexImage.  Most likely someone tried to draw "
+                         "to the pixmap bound in BindTexImage or used it with "
+                         "image_load_store.");
+   }
+
+   intel_miptree_prepare_external(brw, intel_tex->mt);
+
+   _mesa_unlock_texture(&brw->ctx, tex_obj);
+}
+
 static GLboolean
 intel_bind_renderbuffer_tex_image(struct gl_context *ctx,
                                   struct gl_renderbuffer *rb,