Recover mojo frame source after a GPU crash or startup deadlock [chromium/src : main]
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
Hi @kyle...@chromium.org. For some context: I've been working with @fang...@chromium.org on a BFS for Wayland using the mojo impl, and it's hit some subtle timing issues when testing in the wild. I've addressed these on the source side wherever possible but I believe this one needs to be solved on the viz side. (Not sure what the driver can do if a frame issued with `force: true` is never acked.) Appreciate your review and please let me know if this isn't the best approach.
Related details
- Kramer Ge
- Kyle Charbonneau
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 1 comment![]( "Open in Gerrit")
last_begin_frame_args_ = BeginFrameArgs();What is the flow of events that caused this deadlock?
From my understanding a newly connected frame_sink gets issued a `last_begin_frame_args_` after `OnDisplayDidFinishFrame`. so we missed the opportunity to dispatch frame callback.
But does new frame_sinks never `DidFinishFrame`? If it does, `ExternalBeginFrameSourceMojo::OnFrameSinkDidFinishFrame` will call `MaybeProduceFrameCallback`.
Related details
- Kramer Ge
- Kyle Charbonneau
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
last_begin_frame_args_ = BeginFrameArgs();What is the flow of events that caused this deadlock?
From my understanding a newly connected frame_sink gets issued a `last_begin_frame_args_` after `OnDisplayDidFinishFrame`. so we missed the opportunity to dispatch frame callback.
But does new frame_sinks never `DidFinishFrame`? If it does, `ExternalBeginFrameSourceMojo::OnFrameSinkDidFinishFrame` will call `MaybeProduceFrameCallback`.
The deadlock happens if a new sink starts observing after a frame is displayed in `OnDisplayDidFinishFrame` but before the ack callback is dispatched in `MaybeProduceFrameCallback`.
The late sink is sent a missed begin frame and gets added to `pending_frame_sinks_`. But it never reaches `DidFinishFrame`. (It never hears back from its client.) So the sink gets stuck in the list and `MaybeProduceFrameCallback` exits early.
The fix avoids sending frames to new sinks if they have already been displayed.
Related details
- Kramer Ge
- Kramer Ge
- Kyle Charbonneau
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kyle Charbonneau (Gerrit)
Kyle Charbonneau added 1 comment![]( "Open in Gerrit")
last_begin_frame_args_ = BeginFrameArgs();Mitchell CohenWhat is the flow of events that caused this deadlock?
From my understanding a newly connected frame_sink gets issued a `last_begin_frame_args_` after `OnDisplayDidFinishFrame`. so we missed the opportunity to dispatch frame callback.
But does new frame_sinks never `DidFinishFrame`? If it does, `ExternalBeginFrameSourceMojo::OnFrameSinkDidFinishFrame` will call `MaybeProduceFrameCallback`.
The deadlock happens if a new sink starts observing after a frame is displayed in `OnDisplayDidFinishFrame` but before the ack callback is dispatched in `MaybeProduceFrameCallback`.
The late sink is sent a missed begin frame and gets added to `pending_frame_sinks_`. But it never reaches `DidFinishFrame`. (It never hears back from its client.) So the sink gets stuck in the list and `MaybeProduceFrameCallback` exits early.
The fix avoids sending frames to new sinks if they have already been displayed.
The deadlock happens if a new sink starts observing after a frame is displayed in OnDisplayDidFinishFrame
If the new sink starts observing after OnDisplayDidFinishFrame() then the ack callback should have already fired. I guess if there is also a sink that is already observing but didn't produce a frame, so pending_frame_sinks isn't empty, that could happen.
The same problem you describe seems like it would happen if any client is sent a begin frame, so it's put in pending_frame_sinks, but never responds. This part of the change doesn't fix that.
In general this logic here to not send an ack back to the other end of the mojo interface was designed for headless mode where it [needs to wait](https://crrev.com/c/3354381) for all clients to respond. That ensures no overlapping frames. That doesn't seem like appropriate behavior for use of this class with wayland?
Related details
- Kramer Ge
- Kramer Ge
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
last_begin_frame_args_ = BeginFrameArgs();Mitchell CohenWhat is the flow of events that caused this deadlock?
From my understanding a newly connected frame_sink gets issued a `last_begin_frame_args_` after `OnDisplayDidFinishFrame`. so we missed the opportunity to dispatch frame callback.
But does new frame_sinks never `DidFinishFrame`? If it does, `ExternalBeginFrameSourceMojo::OnFrameSinkDidFinishFrame` will call `MaybeProduceFrameCallback`.
Kyle CharbonneauThe deadlock happens if a new sink starts observing after a frame is displayed in `OnDisplayDidFinishFrame` but before the ack callback is dispatched in `MaybeProduceFrameCallback`.
The late sink is sent a missed begin frame and gets added to `pending_frame_sinks_`. But it never reaches `DidFinishFrame`. (It never hears back from its client.) So the sink gets stuck in the list and `MaybeProduceFrameCallback` exits early.
The fix avoids sending frames to new sinks if they have already been displayed.
The deadlock happens if a new sink starts observing after a frame is displayed in OnDisplayDidFinishFrame
If the new sink starts observing after OnDisplayDidFinishFrame() then the ack callback should have already fired. I guess if there is also a sink that is already observing but didn't produce a frame, so pending_frame_sinks isn't empty, that could happen.
The same problem you describe seems like it would happen if any client is sent a begin frame, so it's put in pending_frame_sinks, but never responds. This part of the change doesn't fix that.
In general this logic here to not send an ack back to the other end of the mojo interface was designed for headless mode where it [needs to wait](https://crrev.com/c/3354381) for all clients to respond. That ensures no overlapping frames. That doesn't seem like appropriate behavior for use of this class with wayland?
Yes, the repro requires two sinks to deadlock.
Wayland always needs the ack to come after the frame is displayed (especially if issued with `force: true`) and doesn't want to wait for all the clients. Another option I've considered is making sure OnDisplayDidFinishFrame always calls dispatch when it's not in headless mode.
Related details
- Kramer Ge
- Kramer Ge
- Kyle Charbonneau
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kyle Charbonneau (Gerrit)
Kyle Charbonneau added 1 comment![]( "Open in Gerrit")
last_begin_frame_args_ = BeginFrameArgs();Mitchell CohenWhat is the flow of events that caused this deadlock?
From my understanding a newly connected frame_sink gets issued a `last_begin_frame_args_` after `OnDisplayDidFinishFrame`. so we missed the opportunity to dispatch frame callback.
But does new frame_sinks never `DidFinishFrame`? If it does, `ExternalBeginFrameSourceMojo::OnFrameSinkDidFinishFrame` will call `MaybeProduceFrameCallback`.
Kyle CharbonneauThe deadlock happens if a new sink starts observing after a frame is displayed in `OnDisplayDidFinishFrame` but before the ack callback is dispatched in `MaybeProduceFrameCallback`.
The late sink is sent a missed begin frame and gets added to `pending_frame_sinks_`. But it never reaches `DidFinishFrame`. (It never hears back from its client.) So the sink gets stuck in the list and `MaybeProduceFrameCallback` exits early.
The fix avoids sending frames to new sinks if they have already been displayed.
Mitchell CohenThe deadlock happens if a new sink starts observing after a frame is displayed in OnDisplayDidFinishFrame
If the new sink starts observing after OnDisplayDidFinishFrame() then the ack callback should have already fired. I guess if there is also a sink that is already observing but didn't produce a frame, so pending_frame_sinks isn't empty, that could happen.
The same problem you describe seems like it would happen if any client is sent a begin frame, so it's put in pending_frame_sinks, but never responds. This part of the change doesn't fix that.
In general this logic here to not send an ack back to the other end of the mojo interface was designed for headless mode where it [needs to wait](https://crrev.com/c/3354381) for all clients to respond. That ensures no overlapping frames. That doesn't seem like appropriate behavior for use of this class with wayland?
Yes, the repro requires two sinks to deadlock.
Wayland always needs the ack to come after the frame is displayed (especially if issued with `force: true`) and doesn't want to wait for all the clients. Another option I've considered is making sure OnDisplayDidFinishFrame always calls dispatch when it's not in headless mode.
Another option I've considered is making sure OnDisplayDidFinishFrame always calls dispatch when it's not in headless mode.
Let's hear from Kramer but I'm surprised it doesn't work this way now. On Wayland if there is a slow viz client (eg. a slow OOPIF) that often delivers OnBeginFrame response late it's going to throttle the next begin frame for all clients in the same window. That seems undesirable. I'm not sure of the context here and if there is a good reason for this or not.
Related details
- Kramer Ge
- Kramer Ge
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 1 comment![]( "Open in Gerrit")
last_begin_frame_args_ = BeginFrameArgs();Mitchell CohenWhat is the flow of events that caused this deadlock?
From my understanding a newly connected frame_sink gets issued a `last_begin_frame_args_` after `OnDisplayDidFinishFrame`. so we missed the opportunity to dispatch frame callback.
But does new frame_sinks never `DidFinishFrame`? If it does, `ExternalBeginFrameSourceMojo::OnFrameSinkDidFinishFrame` will call `MaybeProduceFrameCallback`.
Kyle CharbonneauThe deadlock happens if a new sink starts observing after a frame is displayed in `OnDisplayDidFinishFrame` but before the ack callback is dispatched in `MaybeProduceFrameCallback`.
The late sink is sent a missed begin frame and gets added to `pending_frame_sinks_`. But it never reaches `DidFinishFrame`. (It never hears back from its client.) So the sink gets stuck in the list and `MaybeProduceFrameCallback` exits early.
The fix avoids sending frames to new sinks if they have already been displayed.
The deadlock happens if a new sink starts observing after a frame is displayed in OnDisplayDidFinishFrame
If the new sink starts observing after OnDisplayDidFinishFrame() then the ack callback should have already fired. I guess if there is also a sink that is already observing but didn't produce a frame, so pending_frame_sinks isn't empty, that could happen.
The same problem you describe seems like it would happen if any client is sent a begin frame, so it's put in pending_frame_sinks, but never responds. This part of the change doesn't fix that.
In general this logic here to not send an ack back to the other end of the mojo interface was designed for headless mode where it [needs to wait](https://crrev.com/c/3354381) for all clients to respond. That ensures no overlapping frames. That doesn't seem like appropriate behavior for use of this class with wayland?
That doesn't seem like appropriate behavior for use of this class with wayland?
Indeed no. By reading this code I assumed that deadlock can happen other than startup too. It leaves opportunity for a renderer to freeze frame production in viz. We didn't realize it's for headless.
I think we need to use a different mode for `ExternalBeginFrameSourceMojo`, i.e. one that doesn't unnecessarily block on a slow renderer,.
Related details
- Kramer Ge
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
last_begin_frame_args_ = BeginFrameArgs();Mitchell CohenWhat is the flow of events that caused this deadlock?
From my understanding a newly connected frame_sink gets issued a `last_begin_frame_args_` after `OnDisplayDidFinishFrame`. so we missed the opportunity to dispatch frame callback.
But does new frame_sinks never `DidFinishFrame`? If it does, `ExternalBeginFrameSourceMojo::OnFrameSinkDidFinishFrame` will call `MaybeProduceFrameCallback`.
Kyle CharbonneauThe deadlock happens if a new sink starts observing after a frame is displayed in `OnDisplayDidFinishFrame` but before the ack callback is dispatched in `MaybeProduceFrameCallback`.
The late sink is sent a missed begin frame and gets added to `pending_frame_sinks_`. But it never reaches `DidFinishFrame`. (It never hears back from its client.) So the sink gets stuck in the list and `MaybeProduceFrameCallback` exits early.
The fix avoids sending frames to new sinks if they have already been displayed.
Kramer GeThe deadlock happens if a new sink starts observing after a frame is displayed in OnDisplayDidFinishFrame
If the new sink starts observing after OnDisplayDidFinishFrame() then the ack callback should have already fired. I guess if there is also a sink that is already observing but didn't produce a frame, so pending_frame_sinks isn't empty, that could happen.
The same problem you describe seems like it would happen if any client is sent a begin frame, so it's put in pending_frame_sinks, but never responds. This part of the change doesn't fix that.
In general this logic here to not send an ack back to the other end of the mojo interface was designed for headless mode where it [needs to wait](https://crrev.com/c/3354381) for all clients to respond. That ensures no overlapping frames. That doesn't seem like appropriate behavior for use of this class with wayland?
That doesn't seem like appropriate behavior for use of this class with wayland?
Indeed no. By reading this code I assumed that deadlock can happen other than startup too. It leaves opportunity for a renderer to freeze frame production in viz. We didn't realize it's for headless.
I think we need to use a different mode for `ExternalBeginFrameSourceMojo`, i.e. one that doesn't unnecessarily block on a slow renderer,.
Is the "mode" essentially `frame_sink_manager_->run_all_compositor_stages_before_draw()`? Should the callback always be dispatched when it is false even if there are pending sinks?
Related details
- Kramer Ge
- Kramer Ge
- Kyle Charbonneau
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 1 comment![]( "Open in Gerrit")
last_begin_frame_args_ = BeginFrameArgs();Mitchell CohenWhat is the flow of events that caused this deadlock?
From my understanding a newly connected frame_sink gets issued a `last_begin_frame_args_` after `OnDisplayDidFinishFrame`. so we missed the opportunity to dispatch frame callback.
But does new frame_sinks never `DidFinishFrame`? If it does, `ExternalBeginFrameSourceMojo::OnFrameSinkDidFinishFrame` will call `MaybeProduceFrameCallback`.
Kyle CharbonneauThe deadlock happens if a new sink starts observing after a frame is displayed in `OnDisplayDidFinishFrame` but before the ack callback is dispatched in `MaybeProduceFrameCallback`.
The late sink is sent a missed begin frame and gets added to `pending_frame_sinks_`. But it never reaches `DidFinishFrame`. (It never hears back from its client.) So the sink gets stuck in the list and `MaybeProduceFrameCallback` exits early.
The fix avoids sending frames to new sinks if they have already been displayed.
Kramer GeThe deadlock happens if a new sink starts observing after a frame is displayed in OnDisplayDidFinishFrame
If the new sink starts observing after OnDisplayDidFinishFrame() then the ack callback should have already fired. I guess if there is also a sink that is already observing but didn't produce a frame, so pending_frame_sinks isn't empty, that could happen.
The same problem you describe seems like it would happen if any client is sent a begin frame, so it's put in pending_frame_sinks, but never responds. This part of the change doesn't fix that.
In general this logic here to not send an ack back to the other end of the mojo interface was designed for headless mode where it [needs to wait](https://crrev.com/c/3354381) for all clients to respond. That ensures no overlapping frames. That doesn't seem like appropriate behavior for use of this class with wayland?
Mitchell CohenThat doesn't seem like appropriate behavior for use of this class with wayland?
Indeed no. By reading this code I assumed that deadlock can happen other than startup too. It leaves opportunity for a renderer to freeze frame production in viz. We didn't realize it's for headless.
I think we need to use a different mode for `ExternalBeginFrameSourceMojo`, i.e. one that doesn't unnecessarily block on a slow renderer,.
Is the "mode" essentially `frame_sink_manager_->run_all_compositor_stages_before_draw()`? Should the callback always be dispatched when it is false even if there are pending sinks?
`run_all_compositor_stages_before_draw` is blocking a draw, here it's blocking the next beginframe. I think we should refer to other `external_beginframe_sources`, such as `components/viz/service/frame_sinks/external_begin_frame_source_mojo_ios.h`, They do not wait for frame_sinks to finish, nor do they wait for `OnDisplayDidFinishFrame`.
That would make `begin_frame_source_wayland` simpler. Default logic of `ExternalBeginFrameSourceMojo` is for headless and AR/XR
Related details
- Kramer Ge
- Kyle Charbonneau
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kyle Charbonneau (Gerrit)
Kyle Charbonneau added 1 comment![]( "Open in Gerrit")
I'm going to be OOO so removing myself from reviewer.
Related details
- Kramer Ge
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen abandoned this change.
Mitchell Cohen abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |