Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903353002:
Make ozone/drm/mojo more immune to startup races (Closed)
DescriptionMake ozone/drm/mojo more immune to startup races
The mojo implementation of ozone/drm would crash if selected host
entry points were invoked before the DRM thread successfully started.
This change modifies the host code to tolerate the DRM thread not yet
being available.
Tested by deferring the GPU service startup by 2 seconds. Note that
mushrome and mash configurations have different timing requirements
that impact ozone/drm startup order.
BUG=725659
Review-Url: https://codereview.chromium.org/2903353002
Cr-Commit-Position: refs/heads/master@{#477118}
Committed: https://chromium.googlesource.com/chromium/src/+/f14aff33e90b3b078dd060567c20a3f9ed692983
Patch Set 1 #
Total comments: 5
Patch Set 2 : added todo #
Total comments: 3
Patch Set 3 : make CursorProxyMojo a GpuThreadObserver #
Total comments: 7
Patch Set 4 : review comments #Patch Set 5 : cursor setup independent of evdev / drm startup and timing of PlatformWindow creation. #
Total comments: 5
Patch Set 6 : fixed nits #Patch Set 7 : simpler patch #
Messages
Total messages: 42 (10 generated)
rjkroege@chromium.org changed reviewers: + sadrul@chromium.org
ptal
https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... ui/ozone/platform/drm/ozone_platform_gbm.cc:180: cursor_->SetDrmCursorProxy(new CursorProxyMojo(args.connector)); Since we already know that this |mus_thread_proxy_| is going to be used in the gpu thread too, can we just 'StartDrmThread()' here? That would mean we don't need the unfortunate checks in MusThreadProxy.
https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... ui/ozone/platform/drm/ozone_platform_gbm.cc:180: cursor_->SetDrmCursorProxy(new CursorProxyMojo(args.connector)); On 2017/05/25 22:37:00, sadrul wrote: > Since we already know that this |mus_thread_proxy_| is going to be used in the > gpu thread too, can we just 'StartDrmThread()' here? That would mean we don't > need the unfortunate checks in MusThreadProxy. I'd prefer not to do this because: * the "unfortunate checks" are needed in split-process mode regardless. :-( * doing that would increase the divergence between what we have now and what we need for multi-process mode * more divergence between param traits IPC and mojo versions of the code.
https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... ui/ozone/platform/drm/ozone_platform_gbm.cc:180: cursor_->SetDrmCursorProxy(new CursorProxyMojo(args.connector)); On 2017/05/25 22:52:49, rjkroege wrote: > On 2017/05/25 22:37:00, sadrul wrote: > > Since we already know that this |mus_thread_proxy_| is going to be used in the > > gpu thread too, can we just 'StartDrmThread()' here? That would mean we don't > > need the unfortunate checks in MusThreadProxy. > > I'd prefer not to do this because: > * the "unfortunate checks" are needed in split-process mode regardless. :-( > * doing that would increase the divergence between what we have now and what we > need for multi-process mode > * more divergence between param traits IPC and mojo versions of the code. In split-process mode, the MusThreadProxy created here in InitializeUI() is not the one that will be used in InitializeGpu() below (~line 214), because these will be called from different processes. Presumably*, we would create a new instance of MusThreadProxy in InitializeGpu() at that time, and immediately call StartDrmThread() on that (as we now do, in ~line 230). Which means the checks in MusThreadProxy will not be needed, because it will always have a Start()'ed |drm_thread_|. That's why I was thinking the checks added in MusThreadProxy in this CL will not be needed in that config. Do I have this wrong? In the viz/gpu process, will the creation of MusThreadProxy happen in somewhere other than InitializeGpu()? [*] It's not really clear to me what your plans are regarding the split mode.
dnicoara@chromium.org changed reviewers: + dnicoara@chromium.org
Drive-by questions ... https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/gpu/d... File ui/ozone/platform/drm/gpu/drm_thread.cc (right): https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_thread.cc:210: auto window = screen_manager_->GetWindow(widget); Why is this needed? The window lifetime is "managed" by the browser process via CreateWindow()/DestroyWindow(). It should never be the case that a window event arrives before the window is created or after the window is destroyed. https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... ui/ozone/platform/drm/ozone_platform_gbm.cc:180: cursor_->SetDrmCursorProxy(new CursorProxyMojo(args.connector)); On 2017/05/26 01:46:56, sadrul wrote: > On 2017/05/25 22:52:49, rjkroege wrote: > > On 2017/05/25 22:37:00, sadrul wrote: > > > Since we already know that this |mus_thread_proxy_| is going to be used in > the > > > gpu thread too, can we just 'StartDrmThread()' here? That would mean we > don't > > > need the unfortunate checks in MusThreadProxy. > > > > I'd prefer not to do this because: > > * the "unfortunate checks" are needed in split-process mode regardless. :-( > > * doing that would increase the divergence between what we have now and what > we > > need for multi-process mode > > * more divergence between param traits IPC and mojo versions of the code. > > In split-process mode, the MusThreadProxy created here in InitializeUI() is not > the one that will be used in InitializeGpu() below (~line 214), because these > will be called from different processes. Presumably*, we would create a new > instance of MusThreadProxy in InitializeGpu() at that time, and immediately call > StartDrmThread() on that (as we now do, in ~line 230). Which means the checks in > MusThreadProxy will not be needed, because it will always have a Start()'ed > |drm_thread_|. That's why I was thinking the checks added in MusThreadProxy in > this CL will not be needed in that config. > > Do I have this wrong? In the viz/gpu process, will the creation of > MusThreadProxy happen in somewhere other than InitializeGpu()? > > [*] It's not really clear to me what your plans are regarding the split mode. I share Sadrul's concern. It feels like you're running into a threading issue where the OzonePlatform is called while InitializeGPU() is running.
On 2017/05/26 15:51:04, dnicoara wrote: > Drive-by questions ... > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/gpu/d... > File ui/ozone/platform/drm/gpu/drm_thread.cc (right): > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/gpu/d... > ui/ozone/platform/drm/gpu/drm_thread.cc:210: auto window = > screen_manager_->GetWindow(widget); > Why is this needed? The window lifetime is "managed" by the browser process via > CreateWindow()/DestroyWindow(). It should never be the case that a window event > arrives before the window is created or after the window is destroyed. > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... > File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... > ui/ozone/platform/drm/ozone_platform_gbm.cc:180: cursor_->SetDrmCursorProxy(new > CursorProxyMojo(args.connector)); > On 2017/05/26 01:46:56, sadrul wrote: > > On 2017/05/25 22:52:49, rjkroege wrote: > > > On 2017/05/25 22:37:00, sadrul wrote: > > > > Since we already know that this |mus_thread_proxy_| is going to be used in > > the > > > > gpu thread too, can we just 'StartDrmThread()' here? That would mean we > > don't > > > > need the unfortunate checks in MusThreadProxy. > > > > > > I'd prefer not to do this because: > > > * the "unfortunate checks" are needed in split-process mode regardless. :-( > > > * doing that would increase the divergence between what we have now and what > > we > > > need for multi-process mode > > > * more divergence between param traits IPC and mojo versions of the code. > > > > In split-process mode, the MusThreadProxy created here in InitializeUI() is > not > > the one that will be used in InitializeGpu() below (~line 214), because these > > will be called from different processes. Presumably*, we would create a new > > instance of MusThreadProxy in InitializeGpu() at that time, and immediately > call > > StartDrmThread() on that (as we now do, in ~line 230). Which means the checks > in > > MusThreadProxy will not be needed, because it will always have a Start()'ed > > |drm_thread_|. That's why I was thinking the checks added in MusThreadProxy in > > this CL will not be needed in that config. > > > > Do I have this wrong? In the viz/gpu process, will the creation of > > MusThreadProxy happen in somewhere other than InitializeGpu()? > > > > [*] It's not really clear to me what your plans are regarding the split mode. > > I share Sadrul's concern. It feels like you're running into a threading issue > where the OzonePlatform is called while InitializeGPU() is running. This is the crux of the bug that I'm trying to fix. InitializeGPU is getting invoked late. Now, one could argue that the correct fix is to go change why that's happening. But that would appear to be a large refactoring in how mushrome will do display management. (Which will happen the right way as part of the next milestone.) How about I add a TODO?
On 2017/05/25 22:37:01, sadrul wrote: > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... > File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... > ui/ozone/platform/drm/ozone_platform_gbm.cc:180: cursor_->SetDrmCursorProxy(new > CursorProxyMojo(args.connector)); > Since we already know that this |mus_thread_proxy_| is going to be used in the > gpu thread too, can we just 'StartDrmThread()' here? That would mean we don't > need the unfortunate checks in MusThreadProxy. I attempted to code this up and it gets tedious because the drm thread and the gpu thread need to have each other's task runner so I need to change how the drm thread is introduced to the gpu thread. I'd rather the checks in code that's going to get altered anyway than changing the startup code shared between cash and mustash. wdyt?
On 2017/05/29 15:32:14, rjkroege wrote: > On 2017/05/25 22:37:01, sadrul wrote: > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... > > File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... > > ui/ozone/platform/drm/ozone_platform_gbm.cc:180: > cursor_->SetDrmCursorProxy(new > > CursorProxyMojo(args.connector)); > > Since we already know that this |mus_thread_proxy_| is going to be used in the > > gpu thread too, can we just 'StartDrmThread()' here? That would mean we don't > > need the unfortunate checks in MusThreadProxy. > > I attempted to code this up and it gets tedious because the drm thread and the > gpu thread need to have each other's task runner so I need to change how the drm > thread is introduced to the gpu thread. I'd rather the checks in code that's > going to get altered anyway than changing the startup code shared between cash > and mustash. wdyt? lgtm https://codereview.chromium.org/2903353002/diff/20001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_thread.cc (right): https://codereview.chromium.org/2903353002/diff/20001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_thread.cc:210: auto window = screen_manager_->GetWindow(widget); I think we suggest using 'auto*' here.
https://codereview.chromium.org/2903353002/diff/20001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_thread.cc (right): https://codereview.chromium.org/2903353002/diff/20001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_thread.cc:211: // TODO(rjkroege): This check is needed only to address mushrome startup Why isn't the |drm_thread_| check enough for this case? I'm more concerned by MoveCursor() since that skips the UI thread. Wouldn't all window operations need this check?
On 2017/05/29 15:28:12, rjkroege wrote: > On 2017/05/26 15:51:04, dnicoara wrote: > > Drive-by questions ... > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/gpu/d... > > File ui/ozone/platform/drm/gpu/drm_thread.cc (right): > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/gpu/d... > > ui/ozone/platform/drm/gpu/drm_thread.cc:210: auto window = > > screen_manager_->GetWindow(widget); > > Why is this needed? The window lifetime is "managed" by the browser process > via > > CreateWindow()/DestroyWindow(). It should never be the case that a window > event > > arrives before the window is created or after the window is destroyed. > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... > > File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... > > ui/ozone/platform/drm/ozone_platform_gbm.cc:180: > cursor_->SetDrmCursorProxy(new > > CursorProxyMojo(args.connector)); > > On 2017/05/26 01:46:56, sadrul wrote: > > > On 2017/05/25 22:52:49, rjkroege wrote: > > > > On 2017/05/25 22:37:00, sadrul wrote: > > > > > Since we already know that this |mus_thread_proxy_| is going to be used > in > > > the > > > > > gpu thread too, can we just 'StartDrmThread()' here? That would mean we > > > don't > > > > > need the unfortunate checks in MusThreadProxy. > > > > > > > > I'd prefer not to do this because: > > > > * the "unfortunate checks" are needed in split-process mode regardless. > :-( > > > > * doing that would increase the divergence between what we have now and > what > > > we > > > > need for multi-process mode > > > > * more divergence between param traits IPC and mojo versions of the code. > > > > > > In split-process mode, the MusThreadProxy created here in InitializeUI() is > > not > > > the one that will be used in InitializeGpu() below (~line 214), because > these > > > will be called from different processes. Presumably*, we would create a new > > > instance of MusThreadProxy in InitializeGpu() at that time, and immediately > > call > > > StartDrmThread() on that (as we now do, in ~line 230). Which means the > checks > > in > > > MusThreadProxy will not be needed, because it will always have a Start()'ed > > > |drm_thread_|. That's why I was thinking the checks added in MusThreadProxy > in > > > this CL will not be needed in that config. > > > > > > Do I have this wrong? In the viz/gpu process, will the creation of > > > MusThreadProxy happen in somewhere other than InitializeGpu()? > > > > > > [*] It's not really clear to me what your plans are regarding the split > mode. > > > > I share Sadrul's concern. It feels like you're running into a threading issue > > where the OzonePlatform is called while InitializeGPU() is running. > > This is the crux of the bug that I'm trying to fix. InitializeGPU is getting > invoked late. Now, one could argue that the correct fix is to go change why > that's happening. But that would appear to be a large refactoring in how > mushrome will do display management. (Which will happen the right way as part of > the next milestone.) How about I add a TODO? Wondering, does the GPU thread exist when InitializeUI() is called? (Or is there even a GPU thread?) The only major difference between the old path and the MUS path seems to be that in the old path any call to the DrmGpuPlatformSupportHost (the equivalent to MusThreadProxy) goes through an IPC and is executed on the GPU main thread before being posted on the DRM thread. In the Mus case, it seems like MusThreadProxy queues commands directly to the DRM thread (effectively skipping one hop). A quick solution would be to just post the calls to the GPU thread. I believe that should guarantee proper call order since the GPU thread runner runs after InitializeGPU() is called.
kylechar@chromium.org changed reviewers: + kylechar@chromium.org
Other than the cursor stuff (which I don't understand) you're adding the same logic that DrmGpuPlatformSupportHost uses. If the GPU connection isn't ready then DrmGpuPlatformSupportHost::Send() returns false early. https://codereview.chromium.org/2903353002/diff/20001/ui/ozone/platform/drm/m... File ui/ozone/platform/drm/mus_thread_proxy.cc (right): https://codereview.chromium.org/2903353002/diff/20001/ui/ozone/platform/drm/m... ui/ozone/platform/drm/mus_thread_proxy.cc:126: if (!drm_thread_ || !drm_thread_->IsRunning()) Maybe add MusThreadProxy::IsConnected() method with this logic just to avoid repeating it?
On 2017/05/29 22:31:52, kylechar wrote: > Other than the cursor stuff (which I don't understand) you're adding the same > logic that DrmGpuPlatformSupportHost uses. If the GPU connection isn't ready > then DrmGpuPlatformSupportHost::Send() returns false early. > Ahh, good point!
On 2017/05/29 17:01:54, dnicoara wrote: > https://codereview.chromium.org/2903353002/diff/20001/ui/ozone/platform/drm/g... > File ui/ozone/platform/drm/gpu/drm_thread.cc (right): > > https://codereview.chromium.org/2903353002/diff/20001/ui/ozone/platform/drm/g... > ui/ozone/platform/drm/gpu/drm_thread.cc:211: // TODO(rjkroege): This check is > needed only to address mushrome startup > Why isn't the |drm_thread_| check enough for this case? > > I'm more concerned by MoveCursor() since that skips the UI thread. > > Wouldn't all window operations need this check? Yes, as you observed, there is an issue here.
On 2017/05/29 17:19:03, dnicoara wrote: > On 2017/05/29 15:28:12, rjkroege wrote: > > On 2017/05/26 15:51:04, dnicoara wrote: > > > Drive-by questions ... > > > > > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/gpu/d... > > > File ui/ozone/platform/drm/gpu/drm_thread.cc (right): > > > > > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/gpu/d... > > > ui/ozone/platform/drm/gpu/drm_thread.cc:210: auto window = > > > screen_manager_->GetWindow(widget); > > > Why is this needed? The window lifetime is "managed" by the browser process > > via > > > CreateWindow()/DestroyWindow(). It should never be the case that a window > > event > > > arrives before the window is created or after the window is destroyed. > > > > > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... > > > File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): > > > > > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... > > > ui/ozone/platform/drm/ozone_platform_gbm.cc:180: > > cursor_->SetDrmCursorProxy(new > > > CursorProxyMojo(args.connector)); > > > On 2017/05/26 01:46:56, sadrul wrote: > > > > On 2017/05/25 22:52:49, rjkroege wrote: > > > > > On 2017/05/25 22:37:00, sadrul wrote: > > > > > > Since we already know that this |mus_thread_proxy_| is going to be > used > > in > > > > the > > > > > > gpu thread too, can we just 'StartDrmThread()' here? That would mean > we > > > > don't > > > > > > need the unfortunate checks in MusThreadProxy. > > > > > > > > > > I'd prefer not to do this because: > > > > > * the "unfortunate checks" are needed in split-process mode regardless. > > :-( > > > > > * doing that would increase the divergence between what we have now and > > what > > > > we > > > > > need for multi-process mode > > > > > * more divergence between param traits IPC and mojo versions of the > code. > > > > > > > > In split-process mode, the MusThreadProxy created here in InitializeUI() > is > > > not > > > > the one that will be used in InitializeGpu() below (~line 214), because > > these > > > > will be called from different processes. Presumably*, we would create a > new > > > > instance of MusThreadProxy in InitializeGpu() at that time, and > immediately > > > call > > > > StartDrmThread() on that (as we now do, in ~line 230). Which means the > > checks > > > in > > > > MusThreadProxy will not be needed, because it will always have a > Start()'ed > > > > |drm_thread_|. That's why I was thinking the checks added in > MusThreadProxy > > in > > > > this CL will not be needed in that config. > > > > > > > > Do I have this wrong? In the viz/gpu process, will the creation of > > > > MusThreadProxy happen in somewhere other than InitializeGpu()? > > > > > > > > [*] It's not really clear to me what your plans are regarding the split > > mode. > > > > > > I share Sadrul's concern. It feels like you're running into a threading > issue > > > where the OzonePlatform is called while InitializeGPU() is running. > > > > This is the crux of the bug that I'm trying to fix. InitializeGPU is getting > > invoked late. Now, one could argue that the correct fix is to go change why > > that's happening. But that would appear to be a large refactoring in how > > mushrome will do display management. (Which will happen the right way as part > of > > the next milestone.) How about I add a TODO? > > Wondering, does the GPU thread exist when InitializeUI() is called? (Or is there > even a GPU thread?) The GPU thread doesn't exist when InitializeUI is called. > > The only major difference between the old path and the MUS path seems to be that > in the old path any call to the DrmGpuPlatformSupportHost (the equivalent to > MusThreadProxy) goes through an IPC and is executed on the GPU main thread > before being posted on the DRM thread. In the Mus case, it seems like > MusThreadProxy queues commands directly to the DRM thread (effectively skipping > one hop). A quick solution would be to just post the calls to the GPU thread. I > believe that should guarantee proper call order since the GPU thread runner runs > after InitializeGPU() is called. My hope in the introduction of mojo is to remove the extra hop: DrmThread directly implements the Mojo interface (i.e. the pattern used by Cursor interface) but as you pointed out, there is a issue here with the cursor handling. I will prepare a modified patch that does something better with the setting up of the mojo bindings. The original bug here is consequence of the way that the mushrome introduces an additional level of thread/process hopping between display::Display* and the NDD implementation. I spent a day looking at the display code and have reluctantly concluded that fixing it for 725659 is probably a quarter's work. :-(
On 2017/05/30 14:39:12, dnicoara wrote: > On 2017/05/29 22:31:52, kylechar wrote: > > Other than the cursor stuff (which I don't understand) you're adding the same > > logic that DrmGpuPlatformSupportHost uses. If the GPU connection isn't ready > > then DrmGpuPlatformSupportHost::Send() returns false early. > > > > Ahh, good point! This was why I went with the if (!drm_thread_ || !drm_thread_->IsRunning()) thingers.
On 2017/05/30 15:29:39, rjkroege wrote: > On 2017/05/29 17:19:03, dnicoara wrote: > > On 2017/05/29 15:28:12, rjkroege wrote: > > > On 2017/05/26 15:51:04, dnicoara wrote: > > > > Drive-by questions ... > > > > > > > > > > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/gpu/d... > > > > File ui/ozone/platform/drm/gpu/drm_thread.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/gpu/d... > > > > ui/ozone/platform/drm/gpu/drm_thread.cc:210: auto window = > > > > screen_manager_->GetWindow(widget); > > > > Why is this needed? The window lifetime is "managed" by the browser > process > > > via > > > > CreateWindow()/DestroyWindow(). It should never be the case that a window > > > event > > > > arrives before the window is created or after the window is destroyed. > > > > > > > > > > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... > > > > File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2903353002/diff/1/ui/ozone/platform/drm/ozone... > > > > ui/ozone/platform/drm/ozone_platform_gbm.cc:180: > > > cursor_->SetDrmCursorProxy(new > > > > CursorProxyMojo(args.connector)); > > > > On 2017/05/26 01:46:56, sadrul wrote: > > > > > On 2017/05/25 22:52:49, rjkroege wrote: > > > > > > On 2017/05/25 22:37:00, sadrul wrote: > > > > > > > Since we already know that this |mus_thread_proxy_| is going to be > > used > > > in > > > > > the > > > > > > > gpu thread too, can we just 'StartDrmThread()' here? That would mean > > we > > > > > don't > > > > > > > need the unfortunate checks in MusThreadProxy. > > > > > > > > > > > > I'd prefer not to do this because: > > > > > > * the "unfortunate checks" are needed in split-process mode > regardless. > > > :-( > > > > > > * doing that would increase the divergence between what we have now > and > > > what > > > > > we > > > > > > need for multi-process mode > > > > > > * more divergence between param traits IPC and mojo versions of the > > code. > > > > > > > > > > In split-process mode, the MusThreadProxy created here in InitializeUI() > > is > > > > not > > > > > the one that will be used in InitializeGpu() below (~line 214), because > > > these > > > > > will be called from different processes. Presumably*, we would create a > > new > > > > > instance of MusThreadProxy in InitializeGpu() at that time, and > > immediately > > > > call > > > > > StartDrmThread() on that (as we now do, in ~line 230). Which means the > > > checks > > > > in > > > > > MusThreadProxy will not be needed, because it will always have a > > Start()'ed > > > > > |drm_thread_|. That's why I was thinking the checks added in > > MusThreadProxy > > > in > > > > > this CL will not be needed in that config. > > > > > > > > > > Do I have this wrong? In the viz/gpu process, will the creation of > > > > > MusThreadProxy happen in somewhere other than InitializeGpu()? > > > > > > > > > > [*] It's not really clear to me what your plans are regarding the split > > > mode. > > > > > > > > I share Sadrul's concern. It feels like you're running into a threading > > issue > > > > where the OzonePlatform is called while InitializeGPU() is running. > > > > > > This is the crux of the bug that I'm trying to fix. InitializeGPU is getting > > > invoked late. Now, one could argue that the correct fix is to go change why > > > that's happening. But that would appear to be a large refactoring in how > > > mushrome will do display management. (Which will happen the right way as > part > > of > > > the next milestone.) How about I add a TODO? > > > > Wondering, does the GPU thread exist when InitializeUI() is called? (Or is > there > > even a GPU thread?) > > The GPU thread doesn't exist when InitializeUI is called. > > > > > The only major difference between the old path and the MUS path seems to be > that > > in the old path any call to the DrmGpuPlatformSupportHost (the equivalent to > > MusThreadProxy) goes through an IPC and is executed on the GPU main thread > > before being posted on the DRM thread. In the Mus case, it seems like > > MusThreadProxy queues commands directly to the DRM thread (effectively > skipping > > one hop). A quick solution would be to just post the calls to the GPU thread. > I > > believe that should guarantee proper call order since the GPU thread runner > runs > > after InitializeGPU() is called. > > My hope in the introduction of mojo is to remove the extra hop: DrmThread > directly implements the Mojo interface (i.e. the pattern used by Cursor > interface) but as you pointed out, there is a issue here with the cursor > handling. I will prepare a modified patch that does something better with the > setting up of the mojo bindings. > Sounds good! > The original bug here is consequence of the way that the mushrome introduces an > additional level of thread/process hopping between display::Display* and the NDD > implementation. I spent a day looking at the display code and have reluctantly > concluded that fixing it for 725659 is probably a quarter's work. :-( Sorry, I didn't mean to imply that fixing the display code needs to be done now. I am scared by the drm_thread.cc change since window messaging order was very clear and having the if-statement sounded to me like dropping state and leaving UI/GPU out-of-sync. Hence my try to suggest simpler workarounds in the mean time. But it sounds like the mojo changes you are making will solve this issue. :)
PTAL. No changes to DrmThread, instead replicate the suppression of DrmThread actions from the host proxy in a manner identical to the pre-existing Send behaviour.
https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... ui/ozone/platform/drm/ozone_platform_gbm.cc:181: mus_thread_proxy_->AddGpuThreadObserver(cursor_proxy); I think this will result in a use after free? |cursor_| and the CursorProxyMojo it owns will be destroyed before |mus_thread_proxy_| is destroyed. When MusThreadProxy is destroyed it will notify all of the observers that are still registered including CursorProxy. https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... ui/ozone/platform/drm/ozone_platform_gbm.cc:182: cursor_->SetDrmCursorProxy(cursor_proxy); Can this be changed to take a unqiue_ptr?
https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... ui/ozone/platform/drm/ozone_platform_gbm.cc:181: mus_thread_proxy_->AddGpuThreadObserver(cursor_proxy); On the old path the cursor is special cased and notified last otherwise window creation events (scheduled off the UI thread) may race with cursor input events (scheduled off the event thread). Wouldn't this be the case in MUS as well?
ptal https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... ui/ozone/platform/drm/ozone_platform_gbm.cc:181: mus_thread_proxy_->AddGpuThreadObserver(cursor_proxy); On 2017/05/30 20:35:17, dnicoara wrote: > On the old path the cursor is special cased and notified last otherwise window > creation events (scheduled off the UI thread) may race with cursor input events > (scheduled off the event thread). Wouldn't this be the case in MUS as well? I don't understand. The creation of the CursorProxyMojo can safely happen here. And the cursor is still special cased (see pending_cursor_requests_). https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... ui/ozone/platform/drm/ozone_platform_gbm.cc:181: mus_thread_proxy_->AddGpuThreadObserver(cursor_proxy); On 2017/05/30 19:46:22, kylechar wrote: > I think this will result in a use after free? > > |cursor_| and the CursorProxyMojo it owns will be destroyed before > |mus_thread_proxy_| is destroyed. When MusThreadProxy is destroyed it will > notify all of the observers that are still registered including CursorProxy. Done. https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... ui/ozone/platform/drm/ozone_platform_gbm.cc:182: cursor_->SetDrmCursorProxy(cursor_proxy); On 2017/05/30 19:46:22, kylechar wrote: > Can this be changed to take a unqiue_ptr? Done.
lgtm
https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... ui/ozone/platform/drm/ozone_platform_gbm.cc:181: mus_thread_proxy_->AddGpuThreadObserver(cursor_proxy); On 2017/05/31 00:39:14, rjkroege wrote: > On 2017/05/30 20:35:17, dnicoara wrote: > > On the old path the cursor is special cased and notified last otherwise window > > creation events (scheduled off the UI thread) may race with cursor input > events > > (scheduled off the event thread). Wouldn't this be the case in MUS as well? > > I don't understand. The creation of the CursorProxyMojo can safely happen here. > And the cursor is still special cased (see pending_cursor_requests_). Cursor creation is fine. I'm referring to the order in which the GpuThreadObservers are called. Perhaps I'm misunderstanding what the mojo binding is doing, but this is the case I'm thinking of: 1. mus_thread_proxy_->AddGpuThreadObserver(cursor_proxy) 2. CreatePlatformWindow(...) 3. mus_thread_proxy_->StartDrmThread() // This calls the observers a. cursor_proxy->OnGpuThreadReady() b. DrmWindowHost::OnGpuThreadReady() // This is the window created in step 2 The cursor_proxy may start sending cursor events before the window is created on the GPU side since it has been notified first. So DrmThread::MoveCursor() may be called before DrmThread::CreateWindow(). If (a) is the last notification, then the above case would never happen since DrmWindowHost::OnGpuThreadReady() would queue the window creation message before cursor_proxy is notified of GPU startup.
On 2017/05/31 15:01:14, dnicoara wrote: > https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... > File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): > > https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... > ui/ozone/platform/drm/ozone_platform_gbm.cc:181: > mus_thread_proxy_->AddGpuThreadObserver(cursor_proxy); > On 2017/05/31 00:39:14, rjkroege wrote: > > On 2017/05/30 20:35:17, dnicoara wrote: > > > On the old path the cursor is special cased and notified last otherwise > window > > > creation events (scheduled off the UI thread) may race with cursor input > > events > > > (scheduled off the event thread). Wouldn't this be the case in MUS as well? > > > > I don't understand. The creation of the CursorProxyMojo can safely happen > here. > > And the cursor is still special cased (see pending_cursor_requests_). > > Cursor creation is fine. I'm referring to the order in which the > GpuThreadObservers are called. Perhaps I'm misunderstanding what the mojo > binding is doing, but this is the case I'm thinking of: > > 1. mus_thread_proxy_->AddGpuThreadObserver(cursor_proxy) > 2. CreatePlatformWindow(...) > 3. mus_thread_proxy_->StartDrmThread() // This calls the observers > a. cursor_proxy->OnGpuThreadReady() > b. DrmWindowHost::OnGpuThreadReady() // This is the window created in step 2 > > The cursor_proxy may start sending cursor events before the window is created on > the GPU side since it has been notified first. So DrmThread::MoveCursor() may be > called before DrmThread::CreateWindow(). > > If (a) is the last notification, then the above case would never happen since > DrmWindowHost::OnGpuThreadReady() would queue the window creation message before > cursor_proxy is notified of GPU startup. And I don't think |pending_cursor_requests_| are helping since they're flushing all pending requests right after MusThreadProxy::StartDrmThread() is called but before MusThreadProxy::RunObservers() happens which means these queued events will be executed before the window is created on the GPU thread.
On 2017/05/31 15:01:14, dnicoara wrote: > https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... > File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): > > https://codereview.chromium.org/2903353002/diff/40001/ui/ozone/platform/drm/o... > ui/ozone/platform/drm/ozone_platform_gbm.cc:181: > mus_thread_proxy_->AddGpuThreadObserver(cursor_proxy); > On 2017/05/31 00:39:14, rjkroege wrote: > > On 2017/05/30 20:35:17, dnicoara wrote: > > > On the old path the cursor is special cased and notified last otherwise > window > > > creation events (scheduled off the UI thread) may race with cursor input > > events > > > (scheduled off the event thread). Wouldn't this be the case in MUS as well? > > > > I don't understand. The creation of the CursorProxyMojo can safely happen > here. > > And the cursor is still special cased (see pending_cursor_requests_). > > Cursor creation is fine. I'm referring to the order in which the > GpuThreadObservers are called. Perhaps I'm misunderstanding what the mojo > binding is doing, but this is the case I'm thinking of: > > 1. mus_thread_proxy_->AddGpuThreadObserver(cursor_proxy) > 2. CreatePlatformWindow(...) > 3. mus_thread_proxy_->StartDrmThread() // This calls the observers > a. cursor_proxy->OnGpuThreadReady() > b. DrmWindowHost::OnGpuThreadReady() // This is the window created in step 2 > > The cursor_proxy may start sending cursor events before the window is created on > the GPU side since it has been notified first. So DrmThread::MoveCursor() may be > called before DrmThread::CreateWindow(). You're right. Thanks for the explanation as it made it (comparatively) easy to replicate. I have fixed in this revision. PTAL. > > If (a) is the last notification, then the above case would never happen since > DrmWindowHost::OnGpuThreadReady() would queue the window creation message before > cursor_proxy is notified of GPU startup.
lgtm with a question: https://codereview.chromium.org/2903353002/diff/80001/ui/ozone/platform/drm/c... File ui/ozone/platform/drm/cursor_proxy_mojo.h (right): https://codereview.chromium.org/2903353002/diff/80001/ui/ozone/platform/drm/c... ui/ozone/platform/drm/cursor_proxy_mojo.h:37: void SendToDelegate(DrmCursorProxy* delegate); override https://codereview.chromium.org/2903353002/diff/80001/ui/ozone/platform/drm/m... File ui/ozone/platform/drm/mus_thread_proxy.cc (right): https://codereview.chromium.org/2903353002/diff/80001/ui/ozone/platform/drm/m... ui/ozone/platform/drm/mus_thread_proxy.cc:62: // cursor requests until the DRM thread launches. Unfortunately, this includes Isn't cursor updates on GPU start covered by DrmWindowHost::OnGpuThreadReady()? The callstack is: DrmWindowHost::OnGpuThreadReady() DrmWindowHost::SendBoundsChange() DrmCursor::CommitBoundsChange() DrmCursor::SendCursorShowLocked()
https://codereview.chromium.org/2903353002/diff/80001/ui/ozone/platform/drm/c... File ui/ozone/platform/drm/cursor_proxy_mojo.h (right): https://codereview.chromium.org/2903353002/diff/80001/ui/ozone/platform/drm/c... ui/ozone/platform/drm/cursor_proxy_mojo.h:37: void SendToDelegate(DrmCursorProxy* delegate); On 2017/06/05 15:04:43, dnicoara wrote: > override Done. https://codereview.chromium.org/2903353002/diff/80001/ui/ozone/platform/drm/m... File ui/ozone/platform/drm/mus_thread_proxy.cc (right): https://codereview.chromium.org/2903353002/diff/80001/ui/ozone/platform/drm/m... ui/ozone/platform/drm/mus_thread_proxy.cc:62: // cursor requests until the DRM thread launches. Unfortunately, this includes On 2017/06/05 15:04:43, dnicoara wrote: > Isn't cursor updates on GPU start covered by DrmWindowHost::OnGpuThreadReady()? > > The callstack is: > DrmWindowHost::OnGpuThreadReady() > DrmWindowHost::SendBoundsChange() > DrmCursor::CommitBoundsChange() > DrmCursor::SendCursorShowLocked() No. Mushrome sets the cursor (once) when it sets up its WindowTreeClient which is before the GPU start. This then arrives at ui::DrmCursor before the GPU process starts.
The CQ bit was checked by rjkroege@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, kylechar@chromium.org, dnicoara@chromium.org Link to the patchset: https://codereview.chromium.org/2903353002/#ps100001 (title: "fixed nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2903353002/diff/80001/ui/ozone/platform/drm/m... File ui/ozone/platform/drm/mus_thread_proxy.cc (right): https://codereview.chromium.org/2903353002/diff/80001/ui/ozone/platform/drm/m... ui/ozone/platform/drm/mus_thread_proxy.cc:62: // cursor requests until the DRM thread launches. Unfortunately, this includes On 2017/06/05 20:58:27, rjkroege wrote: > On 2017/06/05 15:04:43, dnicoara wrote: > > Isn't cursor updates on GPU start covered by > DrmWindowHost::OnGpuThreadReady()? > > > > The callstack is: > > DrmWindowHost::OnGpuThreadReady() > > DrmWindowHost::SendBoundsChange() > > DrmCursor::CommitBoundsChange() > > DrmCursor::SendCursorShowLocked() > > No. Mushrome sets the cursor (once) when it sets up its WindowTreeClient which > is before the GPU start. This then arrives at ui::DrmCursor before the GPU > process starts. But ui::DrmCursor is a host side object, not GPU. It should exist when Mushrome sets the cursor (and it caches the bitmap).
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/06/05 21:10:54, dnicoara wrote: > https://codereview.chromium.org/2903353002/diff/80001/ui/ozone/platform/drm/m... > File ui/ozone/platform/drm/mus_thread_proxy.cc (right): > > https://codereview.chromium.org/2903353002/diff/80001/ui/ozone/platform/drm/m... > ui/ozone/platform/drm/mus_thread_proxy.cc:62: // cursor requests until the DRM > thread launches. Unfortunately, this includes > On 2017/06/05 20:58:27, rjkroege wrote: > > On 2017/06/05 15:04:43, dnicoara wrote: > > > Isn't cursor updates on GPU start covered by > > DrmWindowHost::OnGpuThreadReady()? > > > > > > The callstack is: > > > DrmWindowHost::OnGpuThreadReady() > > > DrmWindowHost::SendBoundsChange() > > > DrmCursor::CommitBoundsChange() > > > DrmCursor::SendCursorShowLocked() > > > > No. Mushrome sets the cursor (once) when it sets up its WindowTreeClient which > > is before the GPU start. This then arrives at ui::DrmCursor before the GPU > > process starts. > > But ui::DrmCursor is a host side object, not GPU. It should exist when Mushrome > sets the cursor (and it caches the bitmap). You're right. I simplified the patch. The cursor stashing change is working around a bug better fixed elsewhere.
The CQ bit was checked by rjkroege@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dnicoara@chromium.org, sadrul@chromium.org, kylechar@chromium.org Link to the patchset: https://codereview.chromium.org/2903353002/#ps120001 (title: "simpler patch")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496703647649050,
"parent_rev": "fb2595ceb3789b97653b6e11f894d08ff8f9e23b", "commit_rev":
"f14aff33e90b3b078dd060567c20a3f9ed692983"}
Message was sent while issue was closed.
Description was changed from ========== Make ozone/drm/mojo more immune to startup races The mojo implementation of ozone/drm would crash if selected host entry points were invoked before the DRM thread successfully started. This change modifies the host code to tolerate the DRM thread not yet being available. Tested by deferring the GPU service startup by 2 seconds. Note that mushrome and mash configurations have different timing requirements that impact ozone/drm startup order. BUG=725659 ========== to ========== Make ozone/drm/mojo more immune to startup races The mojo implementation of ozone/drm would crash if selected host entry points were invoked before the DRM thread successfully started. This change modifies the host code to tolerate the DRM thread not yet being available. Tested by deferring the GPU service startup by 2 seconds. Note that mushrome and mash configurations have different timing requirements that impact ozone/drm startup order. BUG=725659 Review-Url: https://codereview.chromium.org/2903353002 Cr-Commit-Position: refs/heads/master@{#477118} Committed: https://chromium.googlesource.com/chromium/src/+/f14aff33e90b3b078dd060567c20... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f14aff33e90b3b078dd060567c20... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
