<div dir="ltr">On Wed, Jun 12, 2013 at 1:33 PM, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div>Frank Henigman <<a href="mailto:fjhenigman@google.com" target="_blank">fjhenigman@google.com</a>> writes:<br>
<br>
> On Tue, Jun 11, 2013 at 1:10 PM, Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>> wrote:<br>
><br>
>> Frank Henigman <<a href="mailto:fjhenigman@google.com" target="_blank">fjhenigman@google.com</a>> writes:<br>
>><br>
>> > Replace the one texture lock with a lock per texture. This allows<br>
>> > uploading textures from one thread concurrently with drawing in another<br>
>> > thread. _mesa_lock_context_textures() was used to check for texture<br>
>> > updates from other contexts and also to acquire the texture lock.<br>
>> > It's been replaced with _mesa_check_context_textures() which only does<br>
>> > the checking. Code sections that were between<br>
>> > _mesa_lock_context_textures() and _mesa_unlock_context_textures()<br>
>> > have been updated to lock individual textures as needed.<br>
>><br>
>> When someone's doing something like glCopyTexSubImage() from an FBO<br>
>> backed by a texture to another texture, how is the locking supposed to<br>
>> work? How about copies from one texture to the same texture?<br>
>><br>
><br>
> Right now glCopyTexSubImage locks the destination texture before copying<br>
> to it, but doesn't lock the source texture. This was safe because locking<br>
> any<br>
> texture effectively locked them all. With my change that's no longer true<br>
> so<br>
> now we're copying from an unlocked texture. Is that your concern?<br>
> So we just need to have it lock the source texture too?<br>
> We'll have to check for source == destination so we don't try to lock twice.<br>
<br>
</div>That's an example of my concern.<br>
<div><br>
> I'm pretty sure that our current locking doesn't cover nearly as much as<br>
>> it needs to if one wants to make thread-per-context shareCtx support<br>
>> actually work, so I'm really concerned that this change may make the<br>
>> locking unfixable.<br>
>><br>
><br>
> I agree there probably are problems with locking currently, and there seems<br>
> to be zero coverage for context sharing in piglit. But I don't understand<br>
> how<br>
> my change makes anything unfixable. Can you elaborate?<br>
<br>
</div>Basic ABBA locking problems. Someone does a copyteximage from texture A<br>
to fbo-wrapped texture B, at the same time someone does copytexsubimage<br>
>From texture B to fbo-wrapped texture A.<br>
<br>
The timeline for ABBA failure is:<br>
<br>
thread 1: thread 2:<br>
lock texture A<br>
lock texture B<br>
block locking texture B<br>
block locking texture A<br>
<br>
(You may object that "who would copy from A to B at the same time as<br>
copying from B to A?", Well, it's legal, and you can probably find some<br>
other lock-two-textures operation to replace one of these copyteximages<br>
with. Generally when I say "that would be insane, I can't imagine<br>
anyone doing that", I just have too weak of an imagination).<br></blockquote><div><br></div><div>This occurred to me too, but I thought it too insane to mention. (^:</div><div>I think it suffices to acquire locks in the same order. Seems like</div>
<div>texture id could be used for ordering.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div>
> The idea behind the test was to mimic an actual use case and show<br>
> that my patch increases concurrency. I had thought to actually measure<br>
> concurrency in the test (maybe by timing how long the threads wait)<br>
> but realized this would fail on single core machines so I left that out,<br>
> though I can still tell the patch helps because the test completes<br>
> significantly<br>
> faster with it.<br>
<br>
</div>The goal with piglit is to find bugs in drivers and demonstrate them as<br>
simply as possible, as opposed to demoing features or testing<br>
performance.<br></blockquote><div><br></div><div>The (arguable) bug I set out to demonstrate was that the threads don't work</div><div>concurrently, but the test falls short of that so I'll change it to simultaneously</div>
<div>hit the same texture.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div>> use of shared contexts will probably include some synchronization between<br>
> the users. If you think there's value in a test of concurrent access to a<br>
> texture I'll write one (including your glCopyTexSubImage scenarios).<br>
> I'm interested in any suggestions as to the most useful tests to have at<br>
> this point. Thanks.<br>
<br>
</div>Things I'm afraid of with shareCtx are basically everything with<br>
not-externally-mutexed concurrent access:<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
- MSAA window system buffer resolves.<br>
- Concurrent operations on something after a depth or a color clear<br>
to it.<br>
- Reference counting races around intel_miptree_reference/release()<br>
- Reference counting races around intel_region_reference/release()<br>
(check out EGLImage hooks for totally unprotected cases!)<br>
- Concurrent intel_finalize_mipmap_tree().<br>
- Concurrent brw_workaround_depthstencil_alignment()<br>
- While there's no zero-copy PBO support any more, we need to<br>
reintroduce it, so concurrent texture operations that might reference<br>
or COW from a PBO.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
glCopyTexSubImage was just an example of "reads from texture and writes<br>
to texture, thanks to FBOs" -- you've also got the normal drawing path,<br>
glCopyPixels, glDrawPixels, glBitmap, glBlitFramebuffer. If zero-copy<br>
PBOs are reintroduced, then glReadPixels() and glGetTexImage() are<br>
concerns.<br>
</blockquote></div><br></div><div class="gmail_extra">There seem to be two categories here: operations on one texture and</div><div class="gmail_extra">and operations on multiple textures. For the former, I don't think my patch</div>
<div class="gmail_extra">does any harm. There may be places where locking is lacking but those</div><div class="gmail_extra">have been there all along and should be fixable by adding locking.</div><div class="gmail_extra">
In the second category there's work to do for glCopyTexSubImage() and the</div><div class="gmail_extra">other functions you mention, but they seem fixable, even for insane usage.</div><div class="gmail_extra">As for "the normal drawing path" I don't think I've harmed that either.</div>
<div class="gmail_extra">It doesn't look like any driver holds the lock during rendering (except the</div><div class="gmail_extra">intel 915 driver does "lock all textures" in intelRunPipeline() for reasons</div>
<div class="gmail_extra">I don't understand) therefore they don't become less safe in the way</div><div class="gmail_extra">glCopyTexSubImage() did.<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">
Thanks for your input. Looking forward to hearing how I'm all wrong. (^:</div>
</div>