[Mesa-dev] [PATCH] mesa: per-texture locking

[Frank Henigman]

On Wed, Jun 12, 2013 at 1:33 PM, Eric Anholt <eric at anholt.net> wrote:

> Frank Henigman <fjhenigman at google.com> writes:
>
> > On Tue, Jun 11, 2013 at 1:10 PM, Eric Anholt <eric at anholt.net> wrote:
> >
> >> Frank Henigman <fjhenigman at google.com> writes:
> >>
> >> > Replace the one texture lock with a lock per texture.  This allows
> >> > uploading textures from one thread concurrently with drawing in
> another
> >> > thread.  _mesa_lock_context_textures() was used to check for texture
> >> > updates from other contexts and also to acquire the texture lock.
> >> > It's been replaced with _mesa_check_context_textures() which only does
> >> > the checking.  Code sections that were between
> >> > _mesa_lock_context_textures() and _mesa_unlock_context_textures()
> >> > have been updated to lock individual textures as needed.
> >>
> >> When someone's doing something like glCopyTexSubImage() from an FBO
> >> backed by a texture to another texture, how is the locking supposed to
> >> work?  How about copies from one texture to the same texture?
> >>
> >
> > Right now glCopyTexSubImage locks the destination texture before copying
> > to it, but doesn't lock the source texture.  This was safe because
> locking
> > any
> > texture effectively locked them all.  With my change that's no longer
> true
> > so
> > now we're copying from an unlocked texture.  Is that your concern?
> > So we just need to have it lock the source texture too?
> > We'll have to check for source == destination so we don't try to lock
> twice.
>
> That's an example of my concern.
>
> > I'm pretty sure that our current locking doesn't cover nearly as much as
> >> it needs to if one wants to make thread-per-context shareCtx support
> >> actually work, so I'm really concerned that this change may make the
> >> locking unfixable.
> >>
> >
> > I agree there probably are problems with locking currently, and there
> seems
> > to be zero coverage for context sharing in piglit.  But I don't
> understand
> > how
> > my change makes anything unfixable.  Can you elaborate?
>
> Basic ABBA locking problems.  Someone does a copyteximage from texture A
> to fbo-wrapped texture B, at the same time someone does copytexsubimage
> From texture B to fbo-wrapped texture A.
>
> The timeline for ABBA failure is:
>
> thread 1:                  thread 2:
> lock texture A
>                            lock texture B
> block locking texture B
>                            block locking texture A
>
> (You may object that "who would copy from A to B at the same time as
> copying from B to A?", Well, it's legal, and you can probably find some
> other lock-two-textures operation to replace one of these copyteximages
> with.  Generally when I say "that would be insane, I can't imagine
> anyone doing that", I just have too weak of an imagination).
>

This occurred to me too, but I thought it too insane to mention.  (^:
I think it suffices to acquire locks in the same order.  Seems like
texture id could be used for ordering.

 > The idea behind the test was to mimic an actual use case and show
> > that my patch increases concurrency.  I had thought to actually measure
> > concurrency in the test (maybe by timing how long the threads wait)
> > but realized this would fail on single core machines so I left that out,
> > though I can still tell the patch helps because the test completes
> > significantly
> > faster with it.
>
> The goal with piglit is to find bugs in drivers and demonstrate them as
> simply as possible, as opposed to demoing features or testing
> performance.
>

The (arguable) bug I set out to demonstrate was that the threads don't work
concurrently, but the test falls short of that so I'll change it to
simultaneously
hit the same texture.

> use of shared contexts will probably include some synchronization between
> > the users.  If you think there's value in a test of concurrent access to
> a
> > texture I'll write one (including your glCopyTexSubImage scenarios).
> > I'm interested in any suggestions as to the most useful tests to have at
> > this point.   Thanks.
>
> Things I'm afraid of with shareCtx are basically everything with
> not-externally-mutexed concurrent access:
>

> - MSAA window system buffer resolves.
> - Concurrent operations on something after a depth or a color clear
>   to it.
> - Reference counting races around intel_miptree_reference/release()
> - Reference counting races around intel_region_reference/release()
>   (check out EGLImage hooks for totally unprotected cases!)
> - Concurrent intel_finalize_mipmap_tree().
> - Concurrent brw_workaround_depthstencil_alignment()
> - While there's no zero-copy PBO support any more, we need to
>   reintroduce it, so concurrent texture operations that might reference
>   or COW from a PBO.
>

> glCopyTexSubImage was just an example of "reads from texture and writes
> to texture, thanks to FBOs" -- you've also got the normal drawing path,
> glCopyPixels, glDrawPixels, glBitmap, glBlitFramebuffer.  If zero-copy
> PBOs are reintroduced, then glReadPixels() and glGetTexImage() are
> concerns.
>

There seem to be two categories here: operations on one texture and
and operations on multiple textures.  For the former, I don't think my patch
does any harm.  There may be places where locking is lacking but those
have been there all along and should be fixable by adding locking.
In the second category there's work to do for glCopyTexSubImage() and the
other functions you mention, but they seem fixable, even for insane usage.
As for "the normal drawing path" I don't think I've harmed that either.
It doesn't look like any driver holds the lock during rendering (except the
intel 915 driver does "lock all textures" in intelRunPipeline() for reasons
I don't understand) therefore they don't become less safe in the way
glCopyTexSubImage() did.

Thanks for your input.  Looking forward to hearing how I'm all wrong.  (^:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130613/00629173/attachment.html>