Bug 225956 – Red flashes zooming on Google Maps when using external monitor on multi-gpu systems

• Component: New Bugs
• Status: RESOLVED FIXED

[Kyle Piddington]

Red flashes zooming on Google Maps when using external monitor on multi-gpu systems

[Kyle Piddington]

Created attachment 429021 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Kyle Piddington]

Created attachment 429023 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 429023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429023&action=review

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:229
> +    mCmdBuffer.waitUntilScheduled();

1. The fix is now missing from other callers of the flushCommandBufer (sic, typo buffer vs bufer)

2. Do we need to account for mCmdBuffer.isValid() ?

3. The structure is also strange wrt finishCommandBuffer():
 - flushCommandBufer is always called from the payload methods, except for finish()
 - For finish(), surprisingly it calls finishCommandBuffer() but then that function exists only for the reason of finish() and nothing else?


One option would be to make all functions normal clients of flushCommandBufer(), e.g. to remove finishCommandBuffer()
Then it'd be easier to understand the mistake in 1., e.g. that after flushCommandBufer(), something else needs to happen also (e.g. waitUntilScheduled or waitUntilFinished())

Other option would be to add function scheduleCommandBuffer().
Then it'd be easier to understand the mistake in 1. e.g. all functions should just call scheduleCommandBuffer() or finishCommandBuffer() but never flushCommandBufer directly.
In this case flushCommandBufer could be removed and the 1-liner contents could be moved to scheduleCommandBuffer and finishCommandBuffer()
scheduleCommandBuffer and finishCommandBuffer should have the same return type, e.g. if and only if one can fail then the other can fail too ?

[Kimmo Kinnunen]

Other option would be to add a enum parameter to flushCommandBufer() (WaitForScheduled, WaitForFinished)

[Kyle Piddington]

(In reply to Kimmo Kinnunen from comment #4)
> Comment on attachment 429023 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429023&action=review
> 
> > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:229
> > +    mCmdBuffer.waitUntilScheduled();
> 
> 1. The fix is now missing from other callers of the flushCommandBufer (sic,
> typo buffer vs bufer)
Flushing a command buffer commits it to the command queue. We don't need to wait synchronously on every commit, only when we explicitly need to ensure work is on the GPU. (Ie. for glFlush())

> 
> 2. Do we need to account for mCmdBuffer.isValid() ?
No, isValid checks if a command buffer is not committed. 
> 
> 3. The structure is also strange wrt finishCommandBuffer():
>  - flushCommandBufer is always called from the payload methods, except for
> finish()
FinishCommandBuffer uses waitUntilCompleted, which will block the thread until all GPU work finishes. 

>  - For finish(), surprisingly it calls finishCommandBuffer() but then that
> function exists only for the reason of finish() and nothing else?
> 
I would assume this is mirrored from the Vulkan implementation. 
> 
> One option would be to make all functions normal clients of
> flushCommandBufer(), e.g. to remove finishCommandBuffer()
This won't work, as glFlush() and glFinish() have different behavior. Flush() ensures work is committed to the GPU. finish() ensures it completes.

> Then it'd be easier to understand the mistake in 1., e.g. that after
> flushCommandBufer(), something else needs to happen also (e.g.
> waitUntilScheduled or waitUntilFinished())
> 
> Other option would be to add function scheduleCommandBuffer().
> Then it'd be easier to understand the mistake in 1. e.g. all functions
> should just call scheduleCommandBuffer() or finishCommandBuffer() but never
> flushCommandBufer directly.
> In this case flushCommandBufer could be removed and the 1-liner contents
> could be moved to scheduleCommandBuffer and finishCommandBuffer()
> scheduleCommandBuffer and finishCommandBuffer should have the same return
> type, e.g. if and only if one can fail then the other can fail too ?

[Kimmo Kinnunen]

(In reply to Kyle Piddington from comment #6)
> > > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:229
> > > +    mCmdBuffer.waitUntilScheduled();
> > 
> > 1. The fix is now missing from other callers of the flushCommandBufer (sic,
> > typo buffer vs bufer)
> Flushing a command buffer commits it to the command queue. We don't need to
> wait synchronously on every commit, only when we explicitly need to ensure
> work is on the GPU. (Ie. for glFlush())

Sure, I was already discounting finish off from "other callers".
Let me rephrase.

The fix is now missing from other callers of the flushCommandBufer that call the function for the purpose of inducing a GL-visible flush.

In concrete terms: You still have the same error in unMakeCurrent.
EGLMakeCurrent should induce equivalent of GL flush.


> > 2. Do we need to account for mCmdBuffer.isValid() ?
> No, isValid checks if a command buffer is not committed. 
> > 
> > 3. The structure is also strange wrt finishCommandBuffer():
> >  - flushCommandBufer is always called from the payload methods, except for
> > finish()
> FinishCommandBuffer uses waitUntilCompleted, which will block the thread
> until all GPU work finishes.
> 
 
> > One option would be to make all functions normal clients of
> > flushCommandBufer(), e.g. to remove finishCommandBuffer()
> This won't work, as glFlush() and glFinish() have different behavior.
> Flush() ensures work is committed to the GPU. finish() ensures it completes.


No, I did not mean that. What I meant is that current structure is a bit weird.
You have:

unMakeCurrent:
  flushCommandBufer
  waitUntilScheduled

flush:
  flushCommandBufer
  waitUntilScheduled

finish:
  finishCommandBuffer

finishCommandBuffer:
  flushCommandBufer
  waitUntilFinished

So the reader asks:
- "What's the point of having helper finishCommandBuffer which is only called at one call site"

- "What's the point of NOT having helper function scheduleCommandBuffer that would compare to finishCommandBuffer" (Current flushCommandBufer does not compare to finishCommandBuffer).

- "What's the point of one helper function needing a wait after it (flushCommandBuffer), where as the other comparable helper function having the wait inside the helper (finishCommandBuffer)".


So you'd expect structure to be:
  
unMakeCurrent:
  scheduleCommandBuffer

flush:
  scheduleCommandBuffer

finish:
  finishCommandBuffer

scheduleCommandBuffer:
  flushCommandBufer
  waitUntilScheduled

finishCommandBuffer:
  flushCommandBufer
  waitUntilFinished

OR

unMakeCurrent:
  flushCommandBufer
  waitUntilScheduled

flush:
  flushCommandBufer
  waitUntilScheduled

finish:
  flushCommandBufer
  waitUntilFinished

OR

unMakeCurrent:
  flushCommandBufer(WaitUntilScheduled)


flush:
  flushCommandBufer(WaitUntilScheduled)

finish:
  flushCommandBufer(WaitUntilFinished)


And the significance of this, which is very low, is to avoid the bug here being discussed, e.g. that the same use case (gl induced flush) would have different behavior in different places (e.g buggy behavior and correct behavior) unless the code would be made more obviously error-proof.

[Kyle Piddington]

(In reply to Kimmo Kinnunen from comment #7)
> (In reply to Kyle Piddington from comment #6)
> > > > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:229
> > > > +    mCmdBuffer.waitUntilScheduled();
> > > 
> > > 1. The fix is now missing from other callers of the flushCommandBufer (sic,
> > > typo buffer vs bufer)
> > Flushing a command buffer commits it to the command queue. We don't need to
> > wait synchronously on every commit, only when we explicitly need to ensure
> > work is on the GPU. (Ie. for glFlush())
> 
> Sure, I was already discounting finish off from "other callers".
> Let me rephrase.
> 
> The fix is now missing from other callers of the flushCommandBufer that call
> the function for the purpose of inducing a GL-visible flush.
> 
> In concrete terms: You still have the same error in unMakeCurrent.
> EGLMakeCurrent should induce equivalent of GL flush.
This makes sense. Apologies for misunderstanding.
> 
> 
> > > 2. Do we need to account for mCmdBuffer.isValid() ?
> > No, isValid checks if a command buffer is not committed. 
> > > 
> > > 3. The structure is also strange wrt finishCommandBuffer():
> > >  - flushCommandBufer is always called from the payload methods, except for
> > > finish()
> > FinishCommandBuffer uses waitUntilCompleted, which will block the thread
> > until all GPU work finishes.
> > 
>  
> > > One option would be to make all functions normal clients of
> > > flushCommandBufer(), e.g. to remove finishCommandBuffer()
> > This won't work, as glFlush() and glFinish() have different behavior.
> > Flush() ensures work is committed to the GPU. finish() ensures it completes.
> 
> 
> No, I did not mean that. What I meant is that current structure is a bit
> weird.
> You have:
> 
> unMakeCurrent:
>   flushCommandBufer
>   waitUntilScheduled
> 
> flush:
>   flushCommandBufer
>   waitUntilScheduled
> 
> finish:
>   finishCommandBuffer
> 
> finishCommandBuffer:
>   flushCommandBufer
>   waitUntilFinished
> 
> So the reader asks:
> - "What's the point of having helper finishCommandBuffer which is only
> called at one call site"
> 
> - "What's the point of NOT having helper function scheduleCommandBuffer that
> would compare to finishCommandBuffer" (Current flushCommandBufer does not
> compare to finishCommandBuffer).
> 
> - "What's the point of one helper function needing a wait after it
> (flushCommandBuffer), where as the other comparable helper function having
> the wait inside the helper (finishCommandBuffer)".
> 
> 
> So you'd expect structure to be:
>   
> unMakeCurrent:
>   scheduleCommandBuffer
> 
> flush:
>   scheduleCommandBuffer
> 
> finish:
>   finishCommandBuffer
> 
> scheduleCommandBuffer:
>   flushCommandBufer
>   waitUntilScheduled
> 
> finishCommandBuffer:
>   flushCommandBufer
>   waitUntilFinished
> 
> OR
> 
> unMakeCurrent:
>   flushCommandBufer
>   waitUntilScheduled
> 
> flush:
>   flushCommandBufer
>   waitUntilScheduled
> 
> finish:
>   flushCommandBufer
>   waitUntilFinished
> 
> OR
> 
> unMakeCurrent:
>   flushCommandBufer(WaitUntilScheduled)
> 
> 
> flush:
>   flushCommandBufer(WaitUntilScheduled)
> 
> finish:
>   flushCommandBufer(WaitUntilFinished)
> 
> 
> And the significance of this, which is very low, is to avoid the bug here
> being discussed, e.g. that the same use case (gl induced flush) would have
> different behavior in different places (e.g buggy behavior and correct
> behavior) unless the code would be made more obviously error-proof.

I'll do a few updates, thanks for the tips.

[Kyle Piddington]

Created attachment 429192 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 429192 [details]
Patch

looks good to me

[EWS]

Committed r277824 (237970@main): <https://commits.webkit.org/237970@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429192 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78277460>