[Mesa-dev] [PATCH 4/4] i965: Use prepare_external instead of make_shareable in setTexBuffer2
Chad Versace
chadversary at chromium.org
Wed Nov 22 01:43:29 UTC 2017
On Wed 08 Nov 2017, Jason Ekstrand wrote:
> 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.
That's fair compromise of performance vs risk. The risk of a GLX pixmap
being used as a storage image is low enough to fall into the category
"don't care until real-life counter-example is found (and maybe we won't
care even then)". Thankfully, in Vulkan we can prohibit
VK_IMAGE_USAGE_STORAGE_BIT for WSI images.
> 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
Yes, the terms "finish/prepare", as used in i965, continually confuse me.
> 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.
>
> Cc: "17.3" <mesa-stable at lists.freedesktop.org>
> Cc: Chad Versace <chadversary at chromium.org>
> Cc: Daniel Stone <daniels at collabora.com>
> Cc: Louis-Francis Ratté-Boulianne <lfrb at collabora.com>
> Cc: Adam Jackson <ajax at redhat.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Keith Packard <keithp at keithp.com>
> Cc: Eric Anholt <eric at anholt.net>
> ---
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 18 ++++++++
> src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 3 ++
> src/mesa/drivers/dri/i965/intel_screen.c | 2 +-
> src/mesa/drivers/dri/i965/intel_tex.h | 2 +
> src/mesa/drivers/dri/i965/intel_tex_image.c | 61 ++++++++++++++++++++++++++-
> 5 files changed, 84 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 47cfccc..a95b67c 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -2791,6 +2791,24 @@ 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;
> +
> + /* Otherwise, reset the aux state to the least common denominator for the
> + * image's modifier because we don't know how it's been edited prior to our
> + * seeing it here.
> + */
I want to say something stronger here, strong than what the comment
suggests.
If "we don't know how it's been edited prior to our seeing it here",
then the image's contents (where image := color_surface + aux_surface)
are undefined at this point. If we truly don't know the state of the
aux surface, as given us to by the previous owner, then the aux surface
can have any isl_aux_state except ISL_AUX_STATE_INVALID. And, if the aux
surface's state, as given to us by the previous owner, is not
ISL_AUX_STATE_COMPRESSED_NO_CLEAR, then we must give up here
and declare that all is lost, because the previous owner has no
available mechanism to give us RENDER_SURFACE_STATE::ClearColor.
So... when I read the above comment, I interpret it as saying the below:
We don't know the aux state of the aux surface. The previous owner
could have given it to us in any state. Since we don't receive the
clear color from the previous owner, we fallback to
ISL_AUX_STATE_COMPRESSED_NO_CLEAR and hope for the best, because we
can do no better.
When, in fact, the comment should say something like below:
The image's previous owner, before releasing the image to us, must
transfer the image to the aux state defined by its
DRM format modifier. In other words, the aux state of the incoming
image is uniquely determined by its DRM format modifier.
I hate to be language lawyer here, but, as we've discovered over the
years, the clarity and precision of comments are significant when
working with aux surfaces and shared images.
Also, maybe in a follow-up patch, maybe in this patch, maybe in isl.h,
maybe in this function, we should probably add a comment that expounds
on the subset relation, with respect to external ownership transfers,
between ISL_AUX_STATE_PASS_THROUGH and all other valid states. Roughly,
I want a comment like below somehwhere. I'll add it as a follow-up patch
if you don't want to do it now.
Aside: From the perspective of ownership transfers of external
images, ISL_AUX_STATE_PASS_THROUGH is a special case of each aux
state state (except ISL_AUX_STATE_INVALID). As a consequence, prior
to releasing ownership of the image, the owner may transfer the
image's aux surface to ISL_AUX_STATE_PASS_THROUGH regardless of the
particular aux state defined by the DRM format modifier.
> + 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 5b7d7ef..08bd446 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -676,6 +676,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 56560bd..bf4603f 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -126,7 +126,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 42565ba..abd055c 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 28800f6..f841f4b 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
> @@ -479,7 +479,7 @@ intelSetTexBuffer2(__DRIcontext *pDRICtx, GLint target,
> */
> texFormat = _mesa_get_srgb_format_linear(intel_rb_format(rb));
>
> - intel_miptree_make_shareable(brw, rb->mt);
Ouch. If I understand this rat's nest, then presence of
intel_miptree_make_shareable here [which discards any aux surfaces] made
it impossible to use I915_FORMAT_MOD_Y_TILED_CCS with
GLX_EXT_texture_from_pixmap.
> + intel_miptree_finish_external(brw, rb->mt);
[chadv talks to himself]
In isolation, replacing intel_miptree_make_shareable with
intel_miptree_finish_external looks safe. But does it look safe when
viewed globally?
[chadv continues reading...]
>
> _mesa_lock_texture(&brw->ctx, texObj);
> texImage = _mesa_get_tex_image(ctx, texObj, target, 0);
> @@ -488,6 +488,65 @@ 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 is needed in this error path.
> + 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 came context as we have it bound as
s/came/same/
> + * 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 time texture to a framebuffer but it says the
"prevent binding time texture"?
> + * 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.");
> + }
> +
Here's another good place for a comment that says:
/* The image's owner (that's us), before releasing ownership of the
* image, must transfer the image to the aux state defined by its
* DRM format modifier. Blah blah blah.
/
> + intel_miptree_prepare_external(brw, intel_tex->mt);
I'm feeling distrustful of all code today. So I did not trust
intel_miptree_prepare_external to do the right thing. There are exactly
two valid aux states to which intel_miptree_prepare_external may
transfer a CCS_E image: ISL_AUX_STATE_PASS_THROUGH and
ISL_AUX_STATE_COMPRESSED_NO_CLEAR. I descended to the depths of
intel_miptree_prepare_external, and confirmed by code inspection that it
really does transfer to ISL_AUX_STATE_COMPRESSED_NO_CLEAR.
> +
> + _mesa_unlock_texture(&brw->ctx, tex_obj);
> +}
With the wishy-washy comment fixed in intel_miptree_finish_external(),
this patch is
Reviewed-by: Chad Versace <chadversary at chromium.org>
Thanks for untangling this bug.
> +
> static GLboolean
> intel_bind_renderbuffer_tex_image(struct gl_context *ctx,
> struct gl_renderbuffer *rb,
> --
> 2.5.0.400.gff86faf
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list