Closed Bug 1088414 Opened 5 years ago Closed 5 years ago

Use a single synchronized texture for d3d11

Categories

(Core :: Graphics: Layers, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: mattwoodrow, Assigned: bas.schouten)

References

(Blocks 2 open bugs)

Details

(Whiteboard: sketchy)

Attachments

(1 file, 2 obsolete files)

Attached patch sync-hack (obsolete) — Splinter Review
Calling AcquireSync on textures created with D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX appears to be really slow, this really bites us with the number of textures involved when tiling.

I've also tested using ID3D11Query to check when work is completed, this also appears to be pretty slow.

This instead just uses a single texture with a keyedmutex, and makes sure we draw to it last and lock it first. It's pretty hacky, but seems to work really well so far.
Attachment #8510725 - Flags: review?(bas)
Blocks: 1088423
Comment on attachment 8510725 [details] [diff] [review]
sync-hack

Review of attachment 8510725 [details] [diff] [review]:
-----------------------------------------------------------------

This is really really dangerous. But let's do it :P.
Attachment #8510725 - Flags: review?(bas)
Attachment #8510725 - Flags: review+
Attachment #8510725 - Flags: feedback?(jmuizelaar)
Comment on attachment 8510725 [details] [diff] [review]
sync-hack

Review of attachment 8510725 [details] [diff] [review]:
-----------------------------------------------------------------

I guess we'll get more information having this in the tree than having it as a patch...
Attachment #8510725 - Flags: feedback?(jmuizelaar) → feedback+
Comment on attachment 8510725 [details] [diff] [review]
sync-hack

Actually, I'd be a lot happier taking this if we hand a stand alone test case showing the performance problem and this fixing it.
Attachment #8510725 - Flags: feedback+ → feedback-
Let's do it!
https://hg.mozilla.org/mozilla-central/rev/e2efedec2d60
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1100413
Comment on attachment 8510725 [details] [diff] [review]
sync-hack

Review of attachment 8510725 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +1488,5 @@
> +  RefPtr<IDXGIKeyedMutex> mutex;
> +  sentinelTexture->QueryInterface((IDXGIKeyedMutex**)byRef(mutex));
> +  mutex->AcquireSync(0, INFINITE);
> +  mutex->ReleaseSync(0);
> +}

I'm not really convinced that this code is thread safe....

Also, FenceContentDrawing looks pretty sketchy to me. Shouldn't we AcquireSync at the beginning of drawing and ReleaseSync at the end instead of allocating a texture?
Comment on attachment 8510725 [details] [diff] [review]
sync-hack

Review of attachment 8510725 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +1488,5 @@
> +  RefPtr<IDXGIKeyedMutex> mutex;
> +  sentinelTexture->QueryInterface((IDXGIKeyedMutex**)byRef(mutex));
> +  mutex->AcquireSync(0, INFINITE);
> +  mutex->ReleaseSync(0);
> +}

I misread CreateDrawTargetForD3D11Texture as CreateDrawTarget.
Comment on attachment 8510725 [details] [diff] [review]
sync-hack

Review of attachment 8510725 [details] [diff] [review]:
-----------------------------------------------------------------

This also seems broken with e10s
(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> I backed this out because of its brokeness:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c4897f9ffb

Hopefully, we'll be able to fix it up and reland soon.
We also get crashes here: https://crash-stats.mozilla.com/report/index/df756757-95df-4315-b64e-559572141116. Presumably because the draw target is no good.
Depends on: 1104147
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: matt.woodrow → bas
Attachment #8510725 - Attachment is obsolete: true
Attachment #8533916 - Flags: review?(jmuizelaar)
Comment on attachment 8533916 [details] [diff] [review]
Use a single sync texture for D3D11

Review of attachment 8533916 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Hopefully it works out.

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +223,5 @@
> +
> +  MOZ_ASSERT(aSyncObject->GetSyncType() == SyncObject::SyncType::D3D11);
> +
> +  SyncObjectD3D11* sync = static_cast<SyncObjectD3D11*>(aSyncObject);
> +

Add a comment like:

// We copy into the Sync texture as a precaution to ensure ordering in the queue. Perhaps we can drop this if we can become confident of the queue ordering.
Attachment #8533916 - Flags: review?(jmuizelaar) → review+
Attachment #8533916 - Attachment is obsolete: true
Attachment #8535639 - Flags: review?(jmuizelaar)
Attachment #8535639 - Flags: review?(jmuizelaar) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dadec018f807 for breaking all non-Windows builds:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4614692&repo=mozilla-inbound

I know Bas mentioned a fix in #developers, but it was taking too long and I need to step out for the night. Feel free to immediately reland with the fix applied, though.
https://hg.mozilla.org/mozilla-central/rev/6d1f7a6e90a4
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 1131370
Whiteboard: sketchy
See Also: → 1254560
You need to log in before you can comment on or make changes to this bug.