Closed Bug 1066312 Opened 5 years ago Closed 5 years ago

Use IDXGIKeyedMutex for synchronization with D3D11 angle

Categories

(Core :: Canvas: WebGL, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Depends on: 912196
This might be the only part of angle that we need to patch.
Summary: Use IXDGIKeyedMutex for synchronization with D3D11 angle → Use IDXGIKeyedMutex for synchronization with D3D11 angle
Duplicate of this bug: 912191
OS: Mac OS X → Windows 7
I was able to get something working yesterday however performance is greatly reduced due to what looks like contention spread all over d3d11. I'll look into this on Monday.
It looks like the performance problem was not in fact caused by this synchronization strategy but is actually caused by the ANGLE D3D11 backend. I'll look into why next. The same content runs fine in IE11 so the problem doesn't seem foundational to D3D11.
The ANGLE guys say that the D3D11 perf issues are largely related to texSubImage/bufferSubData calls being slow. Maybe other things, but those are the main points they mentioned.
To be clear, they have solutions/are solving these, but we certainly don't have the fixes in our ANGLE yet.

Also, specifically, Intel D3D11 drivers are supposedly much slower for these cases. I was hearing that D3D11 perf was as bad as 25% of the D3D9 perf. (75% worse)
I've filed bug 1068169 about the problem I was seeing with BenWa's test case.
Depends on: 1068169
Attached patch webgl-fence2 (obsolete) — Splinter Review
A working version
Assignee: nobody → jmuizelaar
Attachment #8488236 - Attachment is obsolete: true
Depends on: 1070308
Attached patch Updated on top of ShSurf (obsolete) — Splinter Review
Attachment #8491586 - Attachment is obsolete: true
Attached patch Current version (obsolete) — Splinter Review
Attachment #8505558 - Attachment is obsolete: true
Attachment #8510700 - Flags: feedback?(jgilbert)
Comment on attachment 8510700 [details] [diff] [review]
Current version

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

::: gfx/gl/SharedSurface.h
@@ +117,4 @@
>      virtual void Fence() = 0;
>      virtual bool WaitSync() = 0;
>      virtual bool PollSync() = 0;
> +    virtual void AcquireProducer() {}

What's this for?

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +67,4 @@
>          egl->fDestroySurface(egl->Display(), pbuffer);
>          return nullptr;
>      }
> +    void *keyedMutex = nullptr;

Star to against type, not identifier.

@@ +157,5 @@
>  void
> +SharedSurface_ANGLEShareHandle::ProducerAcquireImpl()
> +{
> +  if (mKeyedMutex)
> +    mKeyedMutex->AcquireSync(0, INFINITE);

4-space

@@ +172,5 @@
> +        mKeyedMutex->ReleaseSync(0);
> +        return;
> +    } else if (mFence) {
> +        MOZ_ASSERT(mGL->IsExtensionSupported(GLContext::NV_fence));
> +        mGL->fSetFence(mFence, LOCAL_GL_ALL_COMPLETED_NV);

Why do we set the fence if we don't wait on it?

::: gfx/gl/SharedSurfaceANGLE.h
@@ +52,4 @@
>                                     EGLContext context,
>                                     EGLSurface pbuffer,
>                                     HANDLE shareHandle,
> +                                   IDXGIKeyedMutex* keyedMutex,

const RefPtr<IDXGIKeyedMutex>& keyedMutex,

@@ +79,5 @@
>      HANDLE GetShareHandle() {
>          return mShareHandle;
>      }
> +
> +    RefPtr<ID3D11Texture2D> GetConsumerTexture() {

const
Attachment #8510700 - Flags: feedback?(jgilbert) → feedback+
The required ANGLE patch is here:
https://chromium-review.googlesource.com/#/c/225642/
Pretty similar to the previous patch with feedback addressed.
Attachment #8510700 - Attachment is obsolete: true
Attachment #8512190 - Flags: review?(jgilbert)
Comment on attachment 8512190 [details] [diff] [review]
keyed-mutex-clean.patch

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

You have a bunch of unnecessary copies going on here, for some reason.

::: gfx/gl/GLConsts.h
@@ +5188,4 @@
>  #define LOCAL_EGL_COVERAGE_SAMPLE_RESOLVE_NV                 0x3131
>  #define LOCAL_EGL_D3D_TEXTURE_2D_SHARE_HANDLE_ANGLE          0x3200
>  #define LOCAL_EGL_D3D_TEXTURE_2D_KEYED_MUTEX_ANGLE           0x3209
> +#define LOCAL_EGL_KEYED_MUTEX_ANGLE                          0x3202

It looks like we don't need this.

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +68,4 @@
>          egl->fDestroySurface(egl->Display(), pbuffer);
>          return nullptr;
>      }
> +    void* keyedMutex = nullptr;

If we're going to hold a ref to this, we should hold it as early as possible.

@@ +99,4 @@
>                                                                 EGLContext context,
>                                                                 EGLSurface pbuffer,
>                                                                 HANDLE shareHandle,
> +                                                               const RefPtr<IDXGIKeyedMutex> keyedMutex,

const Foo&

@@ +180,5 @@
> +SharedSurface_ANGLEShareHandle::ConsumerAcquireImpl()
> +{
> +    if (!mConsumerTexture) {
> +        RefPtr<ID3D11Texture2D> tex;
> +        HRESULT hr = gfxWindowsPlatform::GetPlatform()->GetD3D11Device()->OpenSharedResource(mShareHandle,

Consider using an `auto` to reduce how long this line is.
Also we should really not be pulling the d3d device like this, but we can deal with this later maybe.

::: gfx/gl/SharedSurfaceANGLE.h
@@ +52,4 @@
>                                     EGLContext context,
>                                     EGLSurface pbuffer,
>                                     HANDLE shareHandle,
> +                                   const RefPtr<IDXGIKeyedMutex> keyedMutex,

const Foo&

@@ +79,5 @@
>      HANDLE GetShareHandle() {
>          return mShareHandle;
>      }
> +
> +    const RefPtr<ID3D11Texture2D> GetConsumerTexture() const {

const Foo&
Attachment #8512190 - Flags: review?(jgilbert) → review+
Technically we should also make an extension for this, but since this is all ANGLE-specific, I'm not sure how worth-it it is.
(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> 
> ::: gfx/gl/SharedSurfaceANGLE.h
> @@ +52,4 @@
> >                                     EGLContext context,
> >                                     EGLSurface pbuffer,
> >                                     HANDLE shareHandle,
> > +                                   const RefPtr<IDXGIKeyedMutex> keyedMutex,
> 
> const Foo&

This can't be const reference because we're taking ownership of it.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> (In reply to Jeff Gilbert [:jgilbert] from comment #14)
> > 
> > ::: gfx/gl/SharedSurfaceANGLE.h
> > @@ +52,4 @@
> > >                                     EGLContext context,
> > >                                     EGLSurface pbuffer,
> > >                                     HANDLE shareHandle,
> > > +                                   const RefPtr<IDXGIKeyedMutex> keyedMutex,
> > 
> > const Foo&
> 
> This can't be const reference because we're taking ownership of it.

We should be able to. We don't modify the RefPtr object, we modify the underlying object. The only thing that can't be a reference is the member var.
(In reply to Jeff Gilbert [:jgilbert] from comment #17)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #14)
> > This can't be const reference because we're taking ownership of it.
> 
> We should be able to. We don't modify the RefPtr object, we modify the
> underlying object. The only thing that can't be a reference is the member
> var.

True.
https://hg.mozilla.org/mozilla-central/rev/aede4ed28fd0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.