Re: [Mesa-dev] [PATCH 4/4] i965: Use prepare_external instead of make_shareable in setTexBuffer2

[Chad Versace]

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" 
> Cc: Chad Versace 
> Cc: Daniel Stone 
> Cc: Louis-Francis Ratté-Boulianne 
> Cc: Adam Jackson 
> Cc: Chris Wilson 
> Cc: Keith Packard 
> Cc: Eric Anholt 
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 18 
>  

Re: [Mesa-dev] [PATCH 4/4] i965: Use prepare_external instead of make_shareable in setTexBuffer2

[Kenneth Graunke]

On Wednesday, November 8, 2017 3:20:47 PM PST 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.
> 
> 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.
> 
> Cc: "17.3" 
> Cc: Chad Versace 
> Cc: Daniel Stone 
> Cc: Louis-Francis Ratté-Boulianne 
> Cc: Adam Jackson 
> Cc: Chris Wilson 
> Cc: Keith Packard 
> Cc: Eric Anholt 

I'm going to go out on a limb and offer you a
Reviewed-by: Kenneth Graunke 
for the series (assuming you fix the issue Emil caught).

Hopefully some of the other fine people in the CC list have looked
at it and are happy with it as well.


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org

Re: [Mesa-dev] [PATCH 4/4] i965: Use prepare_external instead of make_shareable in setTexBuffer2

[Jason Ekstrand]

On Mon, Nov 13, 2017 at 7:33 AM, Emil Velikov 
wrote:

> Hi Jason,
>
> I'm not too familiar with the spec in question, so just a minor
> question/suggestion below.
>
> On 8 November 2017 at 23:20, Jason Ekstrand  wrote:
>
> > +void
> > +intelReleaseTexBuffer(__DRIcontext *pDRICtx, GLint target,
> > +  __DRIdrawable *dPriv)
> > +{
> > +   struct brw_context *brw = pDRICtx->driverPrivate;
> > +   struct gl_context *ctx = >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(>ctx, tex_obj);
> > +
> > +   intel_tex = intel_texture_object(tex_obj);
> > +   if (!intel_tex->mt)
> > +  return;
> Here we should be unlocking the texobj, right?
>

You are correct, sir.  Fixed locally.

--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] i965: Use prepare_external instead of make_shareable in setTexBuffer2

[Emil Velikov]

Hi Jason,

I'm not too familiar with the spec in question, so just a minor
question/suggestion below.

On 8 November 2017 at 23:20, Jason Ekstrand  wrote:

> +void
> +intelReleaseTexBuffer(__DRIcontext *pDRICtx, GLint target,
> +  __DRIdrawable *dPriv)
> +{
> +   struct brw_context *brw = pDRICtx->driverPrivate;
> +   struct gl_context *ctx = >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(>ctx, tex_obj);
> +
> +   intel_tex = intel_texture_object(tex_obj);
> +   if (!intel_tex->mt)
> +  return;
Here we should be unlocking the texobj, right?

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] i965: Use prepare_external instead of make_shareable in setTexBuffer2

[Jason Ekstrand]

On Mon, Sep 18, 2017 at 10:12 AM, Eric Anholt  wrote:

> Jason Ekstrand  writes:
>
> > On Mon, Sep 18, 2017 at 9:20 AM, Chad Versace 
> > wrote:
> >
> >> On Thu 14 Sep 2017, Eric Anholt wrote:
> >> > Jason Ekstrand  writes:
> >> >
> >> > > 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.  This means that we can do whatever
> aux
> >> > > tracking we want between glxBindTexImageEXT and
> glxReleaseTexImageEXT
> >> so
> >> > > long as we always transition from external in Bind and to external
> in
> >> > > Release.
> >> >
> >> > The intent of the spec was to get at the hard-to-define "you get
> pixels
> >> > at least as new as the outstanding X11 rendering when you called
> >> > glxBindTexImageEXT(), but if X11 keeps on rendering to the thing then
> >> > you may get newer pixels, too."  With your CCS plan and X11 rendering
> in
> >> > parallel with you GL texturing from the X11 pixmap, will we always see
> >> > either old or new pixels but not anything else?
> >>
> >
> > That gets interesting... Yes, reading and writing to a CCS image at the
> > same time is problematic.  However, the spec does say "undefined" and not
> > "either new or old pixels".  :-)  I'm not sure how this translates into
> the
> > real world though.
>
> It would translate into "I see the random garbage on my screen when
> running a compositor."  X11 does not stop rendering to the pixmap when
> the compositor is trying to texture from it.
>

Adding Daniel and LFRB.  I don't know if the current X code for modifiers
usess them when creating surfaces for X to render into.  I think it
probably only uses modifiers for surfaces that come in through DRI3 +
present in which case there shouldn't be X rendering to buffers with CCS.
If there is, I think we should consider that a bug just as much as doing
front-buffer rendering to CCS.

--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] i965: Use prepare_external instead of make_shareable in setTexBuffer2

[Eric Anholt]

Jason Ekstrand  writes:

> On Mon, Sep 18, 2017 at 9:20 AM, Chad Versace 
> wrote:
>
>> On Thu 14 Sep 2017, Eric Anholt wrote:
>> > Jason Ekstrand  writes:
>> >
>> > > 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.  This means that we can do whatever aux
>> > > tracking we want between glxBindTexImageEXT and glxReleaseTexImageEXT
>> so
>> > > long as we always transition from external in Bind and to external in
>> > > Release.
>> >
>> > The intent of the spec was to get at the hard-to-define "you get pixels
>> > at least as new as the outstanding X11 rendering when you called
>> > glxBindTexImageEXT(), but if X11 keeps on rendering to the thing then
>> > you may get newer pixels, too."  With your CCS plan and X11 rendering in
>> > parallel with you GL texturing from the X11 pixmap, will we always see
>> > either old or new pixels but not anything else?
>>
>
> That gets interesting... Yes, reading and writing to a CCS image at the
> same time is problematic.  However, the spec does say "undefined" and not
> "either new or old pixels".  :-)  I'm not sure how this translates into the
> real world though.

It would translate into "I see the random garbage on my screen when
running a compositor."  X11 does not stop rendering to the pixmap when
the compositor is trying to texture from it.


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] i965: Use prepare_external instead of make_shareable in setTexBuffer2

[Jason Ekstrand]

On Mon, Sep 18, 2017 at 9:20 AM, Chad Versace 
wrote:

> On Thu 14 Sep 2017, Eric Anholt wrote:
> > Jason Ekstrand  writes:
> >
> > > 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.  This means that we can do whatever aux
> > > tracking we want between glxBindTexImageEXT and glxReleaseTexImageEXT
> so
> > > long as we always transition from external in Bind and to external in
> > > Release.
> >
> > The intent of the spec was to get at the hard-to-define "you get pixels
> > at least as new as the outstanding X11 rendering when you called
> > glxBindTexImageEXT(), but if X11 keeps on rendering to the thing then
> > you may get newer pixels, too."  With your CCS plan and X11 rendering in
> > parallel with you GL texturing from the X11 pixmap, will we always see
> > either old or new pixels but not anything else?
>

That gets interesting... Yes, reading and writing to a CCS image at the
same time is problematic.  However, the spec does say "undefined" and not
"either new or old pixels".  :-)  I'm not sure how this translates into the
real world though.


> We have bigger problems if X11 today renders to any surface that has
> been externally shared with I915_FORMAT_MOD_Y_TILED_CCS. If I ran
> git-grep correctly, then xf86-video-intel nor xf86-video-modesetting is
> unaware of modifiers and hence unaware of CCS aux state.
>

you're not grepping the right branches. :-)  We do have code floating
around that makes modesetting modifiers-aware.  It's not aware of what the
modifiers mean other than that multi-planar modifiers have racing trouble
and it avoids using them for front-buffer rendering.  That said, I've run a
variety of examples, and I have yet to see any corruption issues.


> So, with respect to reviewing this patch, I think it's safe to assume
> that X11 never renders to a CCS-enabled surface, and that this patch is
> the correct fix. If X11 does render to CCS-enabled surfaces, then we
> have more invasive problems that need fixing.
>

Yes.  In any case, I think the old code was decidedly wrong in the face of
modifiers.


> Reviewed-by: Chad Versace 
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] i965: Use prepare_external instead of make_shareable in setTexBuffer2

[Chad Versace]

On Thu 14 Sep 2017, Eric Anholt wrote:
> Jason Ekstrand  writes:
> 
> > 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.  This means that we can do whatever aux
> > tracking we want between glxBindTexImageEXT and glxReleaseTexImageEXT so
> > long as we always transition from external in Bind and to external in
> > Release.
> 
> The intent of the spec was to get at the hard-to-define "you get pixels
> at least as new as the outstanding X11 rendering when you called
> glxBindTexImageEXT(), but if X11 keeps on rendering to the thing then
> you may get newer pixels, too."  With your CCS plan and X11 rendering in
> parallel with you GL texturing from the X11 pixmap, will we always see
> either old or new pixels but not anything else?

We have bigger problems if X11 today renders to any surface that has
been externally shared with I915_FORMAT_MOD_Y_TILED_CCS. If I ran
git-grep correctly, then xf86-video-intel nor xf86-video-modesetting is
unaware of modifiers and hence unaware of CCS aux state.

So, with respect to reviewing this patch, I think it's safe to assume
that X11 never renders to a CCS-enabled surface, and that this patch is
the correct fix. If X11 does render to CCS-enabled surfaces, then we
have more invasive problems that need fixing.

Reviewed-by: Chad Versace 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] i965: Use prepare_external instead of make_shareable in setTexBuffer2

[Eric Anholt]

Jason Ekstrand  writes:

> 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.  This means that we can do whatever aux
> tracking we want between glxBindTexImageEXT and glxReleaseTexImageEXT so
> long as we always transition from external in Bind and to external in
> Release.

The intent of the spec was to get at the hard-to-define "you get pixels
at least as new as the outstanding X11 rendering when you called
glxBindTexImageEXT(), but if X11 keeps on rendering to the thing then
you may get newer pixels, too."  With your CCS plan and X11 rendering in
parallel with you GL texturing from the X11 pixmap, will we always see
either old or new pixels but not anything else?


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] i965: Use prepare_external instead of make_shareable in setTexBuffer2

[Jason Ekstrand]

Adding people who may have some shot at understanding this stuff

On Tue, Sep 12, 2017 at 4:23 PM, 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.  This means that we can do whatever aux
> tracking we want between glxBindTexImageEXT and glxReleaseTexImageEXT so
> long as we always transition from external in Bind and to external in
> Release.
>
> The fact that we were using make_shareable before was a problem because
> it would resolve away 100% of the aux data and then throw away our
> reference to the aux buffer.  If the aux data was shared with some other
> application (i.e. if we're using I915_FORMAT_MOD_Y_TILED_CCS) then we
> would forget that the aux data even existed for the rest of eternity.
> This is fine for the first frame but any subsequent calls to
> glxBindTexImageEXT would bind the texture as if it has no aux
> whatsoever and no resolves would happen and texturing would happen as if
> there is no aux.  This was causing rendering corruption in mutter when
> running on top of X11 with modifiers.
> ---
>  src/mesa/drivers/dri/i965/intel_tex_image.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c
> b/src/mesa/drivers/dri/i965/intel_tex_image.c
> index 09ff287..0e8a947 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
> @@ -251,7 +251,7 @@ intelSetTexBuffer2(__DRIcontext *pDRICtx, GLint
> target,
>internal_format = GL_RGB;
> }
>
> -   intel_miptree_make_shareable(brw, rb->mt);
> +   intel_miptree_prepare_external(brw, rb->mt);
>
> _mesa_lock_texture(>ctx, texObj);
> texImage = _mesa_get_tex_image(ctx, texObj, target, 0);
> --
> 2.5.0.400.gff86faf
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev