Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2459973002:
Expedite discovery of primary DRM device. (Closed)
DescriptionExpedite discovery of primary DRM device.
Previously the DrmDeviceHandle and fd for the primary device is sent to the GPU process on the UI thread in DrmDisplayHostManager::OnGpuThreadReady(), which is invoked from the constructor of GpuProcessHostUIShim. If a buffer allocation call arrives immediately after a GPU process crash, it is possible that primary DRM device has not yet been sent to the new GPU process by the time the allocation occurs.
This CL moves part of the logic in DrmDisplayHostManager::OnGpuThreadReady() that sends the primary DRM device into a separate function, to be invoked directly by GpuProcessHost constructor on the IO thread.
BUG=chromium:608839
TEST=kill GPU process and confirm no crash in GbmBuffer::CreateBuffer().
Committed: https://crrev.com/5798066b00f2b7824ba0c476cc12736dfbce87f1
Cr-Commit-Position: refs/heads/master@{#429173}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Special-case the AddGraphicsDevice message but require OnChannelEstablished for all other messages. #Patch Set 3 : Fix unused-function warning on non-ozone platforms. #
Total comments: 5
Patch Set 4 : Move OnGpuProcessLaunched to GpuProcessHost::Init #Patch Set 5 : Send the first AddGraphicsDevice IPC synchronously on IO thread. #
Total comments: 7
Patch Set 6 : Explicitly document the thread on which each method is expected to be called. #
Total comments: 11
Patch Set 7 : Add one more comment per rjkroege #Patch Set 8 : Add more comments. #Messages
Total messages: 58 (33 generated)
hshi@chromium.org changed reviewers: + dnicoara@chromium.org
dnicoara@ -- per our offline chat, I've uploaded this patch which adds an "OnGpuProcessLaunched" method in GpuPlatformSupportHost that is supposed to run on IO thread and only takes care of primary device discovery. Unfortunately I'm still encountering exceptions, so I must be still accessing things in a thread-unsafe manner, but it is not obvious to me what DrmDisplayHostManager cannot touch on the IO thread. The exception message is vague and not helpful and so far I haven't figured out how to get a stacktrace. terminate called after throwing an instance of 'std::out_of_range' what(): vector::_M_range_check: __n (which is 0) >= this->size() (which is 0) terminate called after throwing an instance of 'std::out_of_range' what(): vector::_M_range_check: __n (which is 0) >= this->size() (which is 0)
Here's a couple of things I've noticed. Hopefully they help. https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right): https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... ui/ozone/platform/drm/host/drm_display_host_manager.cc:316: drm_devices_[primary_graphics_card_path_] = |drm_devices_| can't be accessed on multiple threads. This is likely one place you might get exceptions from. https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... ui/ozone/platform/drm/host/drm_display_host_manager.cc:329: proxy_->GpuAddGraphicsDevice(drm_devices_[primary_graphics_card_path_], You may need to check how the buffer allocation is posted, but be aware that |proxy_| will post a task to the IO task runner. You may need something better to just invoke the Send() callback rather than posting it. https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:112: host_id_ = host_id; One of the odd things about GpuProcessHost[UIShim] is that OnChannelDestroyed() may be called after the new GPU process is launched. So you can't update these on IO without some kind of locking. https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:116: for (GpuThreadObserver& observer : gpu_thread_observers_) This is another place where the observer vector is accessed from multiple threads. It could be problematic.
On 2016/10/28 21:42:54, dnicoara wrote: > Here's a couple of things I've noticed. Hopefully they help. > > https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... > File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right): > > https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... > ui/ozone/platform/drm/host/drm_display_host_manager.cc:316: > drm_devices_[primary_graphics_card_path_] = > |drm_devices_| can't be accessed on multiple threads. This is likely one place > you might get exceptions from. > > https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... > ui/ozone/platform/drm/host/drm_display_host_manager.cc:329: > proxy_->GpuAddGraphicsDevice(drm_devices_[primary_graphics_card_path_], > You may need to check how the buffer allocation is posted, but be aware that > |proxy_| will post a task to the IO task runner. You may need something better > to just invoke the Send() callback rather than posting it. > > https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... > File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): > > https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... > ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:112: host_id_ = > host_id; > One of the odd things about GpuProcessHost[UIShim] is that OnChannelDestroyed() > may be called after the new GPU process is launched. So you can't update these > on IO without some kind of locking. > > https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... > ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:116: for > (GpuThreadObserver& observer : gpu_thread_observers_) > This is another place where the observer vector is accessed from multiple > threads. It could be problematic. Thanks for the suggestions! Unfortunately it looks like the std::vector access exceptions are due to neither the drm_devices_ nor the gpu_thread_observers_, as it is not fixed after I added autolock's at all relevant function entry points. I'll dig further...
https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): https://codereview.chromium.org/2459973002/diff/1/ui/ozone/platform/drm/host/... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:112: host_id_ = host_id; On 2016/10/28 21:42:54, dnicoara wrote: > One of the odd things about GpuProcessHost[UIShim] is that OnChannelDestroyed() > may be called after the new GPU process is launched. So you can't update these > on IO without some kind of locking. Setting the |host_id_|, |send_runner_| and |send_callback_| from IO is causing your issues. What you want to do is to make sure these are only set from the UI process. As a first pass I'd suggest passing in |send_callback_| to |observer.OnGpuProcessLaunched()|. That way DrmDisplayHostManager may call the callback directly. This will need to change to accommodate for MUS code, but at least will get you a step closer.
The CQ bit was checked by hshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Can you PTAL patch set #2? I propose that we special-case sending AddGraphicsDevice message by adding a flag for OnChannelEstablished and require it for all messages except AddGraphicsDevice.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Expedite discovery of primary device. BUG= ========== to ========== Expedite discovery of primary device. BUG=chromium:608839 TEST=kill GPU process and confirm no crash in GbmBuffer::CreateBuffer ==========
Description was changed from ========== Expedite discovery of primary device. BUG=chromium:608839 TEST=kill GPU process and confirm no crash in GbmBuffer::CreateBuffer ========== to ========== Expedite discovery of primary DRM device. Previously the DrmDeviceHandle and fd for the primary device is sent to the GPU process on the UI thread in DrmDisplayHostManager::OnGpuThreadReady(), which is invoked from the constructor of GpuProcessHostUIShim. If a buffer allocation call arrives immediately after a GPU process crash, it is possible that primary DRM device has not yet been sent to the new GPU process by the time the allocation occurs. This CL moves part of the logic in DrmDisplayHostManager::OnGpuThreadReady() that sends the primary DRM device into a separate function, to be invoked directly by GpuProcessHost constructor on the IO thread. BUG=chromium:608839 TEST=kill GPU process and confirm no crash in GbmBuffer::CreateBuffer(). ==========
hshi@chromium.org changed reviewers: + kbr@chromium.org, piman@chromium.org
+kbr@, +piman@ (OWNERS content/browser/gpu) thanks
https://codereview.chromium.org/2459973002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2459973002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_process_host.cc:474: ->OnGpuProcessLaunched( The GPU process hasn't been launched yet, though... Did you mean to do this from GpuProcessHost::Init ?
https://codereview.chromium.org/2459973002/diff/40001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): https://codereview.chromium.org/2459973002/diff/40001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:165: send_runner_->PostTask(FROM_HERE, base::Bind(send_callback_, message))) Note, because you use PostTask here, it is possible that the OzoneGpuMsg_AddGraphicsDevice will still be happening "too late".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2459973002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2459973002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_process_host.cc:474: ->OnGpuProcessLaunched( On 2016/10/31 23:41:49, piman wrote: > The GPU process hasn't been launched yet, though... Did you mean to do this from > GpuProcessHost::Init ? Done. https://codereview.chromium.org/2459973002/diff/40001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): https://codereview.chromium.org/2459973002/diff/40001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:165: send_runner_->PostTask(FROM_HERE, base::Bind(send_callback_, message))) On 2016/10/31 23:45:46, piman wrote: > Note, because you use PostTask here, it is possible that the > OzoneGpuMsg_AddGraphicsDevice will still be happening "too late". The GpuProcessHost ctor is called from BrowserGpuMemoryBufferManager::CreateGpuMemoryBufferOnIO() which does not appear to synchronously send an IPC to GPU process to allocate the buffer. I believe it queues the IPC on the IO thread, so any IPC queued earlier on the same thread should precede it.
https://codereview.chromium.org/2459973002/diff/40001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): https://codereview.chromium.org/2459973002/diff/40001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:165: send_runner_->PostTask(FROM_HERE, base::Bind(send_callback_, message))) To further clarify on this: at the time of GpuProcessHost::Init() the IPC channel to the GPU process is actually still pending open -- i.e. GpuProcessHost::Send() would not send the message right away, but queue it in |queued_messages_|. This would guarantee that the AddGraphics message inserted here is always first in the queue. Even if we immediately perform the buffer allocation after this, the IPC message containing the DRM device would still be sent before everything else at GpuProcessHost::OnChannelConnected().
The CQ bit was checked by hshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2459973002/diff/80001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): https://codereview.chromium.org/2459973002/diff/80001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:246: send_callback_.Run(message); Done (addressing the comment on posting the Send). Sadly it turns out that GpuAddGraphicsDevice may be invoked synchronously on IO thread (as the result of GpuProcessHost::Init()) or on the UI thread (for subsequent add device events). Ugh!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hshi@chromium.org changed reviewers: + rjkroege@chromium.org
+rjkroege@ (OWNERS ui/ozone)
lgtm
https://codereview.chromium.org/2459973002/diff/80001/content/browser/gpu/gpu... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2459973002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_process_host.cc:613: ui::OzonePlatform::GetInstance() Is this guaranteed to run before GpuProcessHostUiShim is created? https://codereview.chromium.org/2459973002/diff/80001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): https://codereview.chromium.org/2459973002/diff/80001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:106: void DrmGpuPlatformSupportHost::OnGpuProcessLaunched( Is it guaranteed that this will execute before OnChannelEstablished() (especially since they are running on different threads)? https://codereview.chromium.org/2459973002/diff/80001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/gpu_thread_observer.h (right): https://codereview.chromium.org/2459973002/diff/80001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/gpu_thread_observer.h:16: virtual void OnGpuProcessLaunched() = 0; You'll want to document that this is called from browser IO while the rest are called from browser UI.
https://codereview.chromium.org/2459973002/diff/80001/content/browser/gpu/gpu... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2459973002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_process_host.cc:613: ui::OzonePlatform::GetInstance() On 2016/11/01 19:00:40, dnicoara wrote: > Is this guaranteed to run before GpuProcessHostUiShim is created? Yes because GpuProcessHostUiShim::Create() is posted on the UI thread from the ctor of GpuProcessHost, while GpuProcessHost::Init() is called synchronously on the IO thread immediately following the ctor of GpuProcessHost in GpuProcessHost::Get(). https://codereview.chromium.org/2459973002/diff/80001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): https://codereview.chromium.org/2459973002/diff/80001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:106: void DrmGpuPlatformSupportHost::OnGpuProcessLaunched( On 2016/11/01 19:00:40, dnicoara wrote: > Is it guaranteed that this will execute before OnChannelEstablished() > (especially since they are running on different threads)? Yes because OnGpuProcessLaunched is only invoked synchronously on the IO thread from GpuProcessHost::Init() and therefore precedes OnChannelEstablished() which comes from the ctor of GpuProcessHostUiShim.
On 2016/11/01 19:06:01, hshi1 wrote: > https://codereview.chromium.org/2459973002/diff/80001/content/browser/gpu/gpu... > File content/browser/gpu/gpu_process_host.cc (right): > > https://codereview.chromium.org/2459973002/diff/80001/content/browser/gpu/gpu... > content/browser/gpu/gpu_process_host.cc:613: ui::OzonePlatform::GetInstance() > On 2016/11/01 19:00:40, dnicoara wrote: > > Is this guaranteed to run before GpuProcessHostUiShim is created? > > Yes because GpuProcessHostUiShim::Create() is posted on the UI thread from the > ctor of GpuProcessHost, while GpuProcessHost::Init() is called synchronously on > the IO thread immediately following the ctor of GpuProcessHost in > GpuProcessHost::Get(). > > https://codereview.chromium.org/2459973002/diff/80001/ui/ozone/platform/drm/h... > File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): > > https://codereview.chromium.org/2459973002/diff/80001/ui/ozone/platform/drm/h... > ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:106: void > DrmGpuPlatformSupportHost::OnGpuProcessLaunched( > On 2016/11/01 19:00:40, dnicoara wrote: > > Is it guaranteed that this will execute before OnChannelEstablished() > > (especially since they are running on different threads)? > > Yes because OnGpuProcessLaunched is only invoked synchronously on the IO thread > from GpuProcessHost::Init() and therefore precedes OnChannelEstablished() which > comes from the ctor of GpuProcessHostUiShim. Thank you for confirming!
The CQ bit was checked by hshi@chromium.org to run a CQ dry run
https://codereview.chromium.org/2459973002/diff/80001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/gpu_thread_observer.h (right): https://codereview.chromium.org/2459973002/diff/80001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/gpu_thread_observer.h:16: virtual void OnGpuProcessLaunched() = 0; On 2016/11/01 19:00:40, dnicoara wrote: > You'll want to document that this is called from browser IO while the rest are > called from browser UI. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Expedite discovery of primary DRM device. Previously the DrmDeviceHandle and fd for the primary device is sent to the GPU process on the UI thread in DrmDisplayHostManager::OnGpuThreadReady(), which is invoked from the constructor of GpuProcessHostUIShim. If a buffer allocation call arrives immediately after a GPU process crash, it is possible that primary DRM device has not yet been sent to the new GPU process by the time the allocation occurs. This CL moves part of the logic in DrmDisplayHostManager::OnGpuThreadReady() that sends the primary DRM device into a separate function, to be invoked directly by GpuProcessHost constructor on the IO thread. BUG=chromium:608839 TEST=kill GPU process and confirm no crash in GbmBuffer::CreateBuffer(). ========== to ========== Expedite discovery of primary DRM device. Previously the DrmDeviceHandle and fd for the primary device is sent to the GPU process on the UI thread in DrmDisplayHostManager::OnGpuThreadReady(), which is invoked from the constructor of GpuProcessHostUIShim. If a buffer allocation call arrives immediately after a GPU process crash, it is possible that primary DRM device has not yet been sent to the new GPU process by the time the allocation occurs. This CL moves part of the logic in DrmDisplayHostManager::OnGpuThreadReady() that sends the primary DRM device into a separate function, to be invoked directly by GpuProcessHost constructor on the IO thread. BUG=chromium:608839 TEST=kill GPU process and confirm no crash in GbmBuffer::CreateBuffer(). ==========
Description was changed from ========== Expedite discovery of primary DRM device. Previously the DrmDeviceHandle and fd for the primary device is sent to the GPU process on the UI thread in DrmDisplayHostManager::OnGpuThreadReady(), which is invoked from the constructor of GpuProcessHostUIShim. If a buffer allocation call arrives immediately after a GPU process crash, it is possible that primary DRM device has not yet been sent to the new GPU process by the time the allocation occurs. This CL moves part of the logic in DrmDisplayHostManager::OnGpuThreadReady() that sends the primary DRM device into a separate function, to be invoked directly by GpuProcessHost constructor on the IO thread. BUG=chromium:608839 TEST=kill GPU process and confirm no crash in GbmBuffer::CreateBuffer(). ========== to ========== Expedite discovery of primary DRM device. Previously the DrmDeviceHandle and fd for the primary device is sent to the GPU process on the UI thread in DrmDisplayHostManager::OnGpuThreadReady(), which is invoked from the constructor of GpuProcessHostUIShim. If a buffer allocation call arrives immediately after a GPU process crash, it is possible that primary DRM device has not yet been sent to the new GPU process by the time the allocation occurs. This CL moves part of the logic in DrmDisplayHostManager::OnGpuThreadReady() that sends the primary DRM device into a separate function, to be invoked directly by GpuProcessHost constructor on the IO thread. BUG=chromium:608839 TEST=kill GPU process and confirm no crash in GbmBuffer::CreateBuffer(). ==========
https://codereview.chromium.org/2459973002/diff/100001/content/browser/gpu/gp... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2459973002/diff/100001/content/browser/gpu/gp... content/browser/gpu/gpu_process_host.cc:191: void SendGpuProcessMessageByHostId(int host_id, IPC::Message* message) { This is used only in this file? Perhaps it should be in an anonymous namespace? https://codereview.chromium.org/2459973002/diff/100001/content/browser/gpu/gp... content/browser/gpu/gpu_process_host.cc:612: #if defined(USE_OZONE) For the benefit of us who need to update the mus GpuService code to not regress your change, can you add an explanation here. Maybe a link to the bug? https://codereview.chromium.org/2459973002/diff/100001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): https://codereview.chromium.org/2459973002/diff/100001/ui/ozone/platform/drm/... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:244: // Synchronously send IPC if we are on the same thread. Can you expand on when we would be on the same thread? https://codereview.chromium.org/2459973002/diff/100001/ui/ozone/platform/drm/... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:247: return true; I'm probably missing something but where does message get freed in this case?
https://codereview.chromium.org/2459973002/diff/100001/content/browser/gpu/gp... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2459973002/diff/100001/content/browser/gpu/gp... content/browser/gpu/gpu_process_host.cc:191: void SendGpuProcessMessageByHostId(int host_id, IPC::Message* message) { On 2016/11/01 22:54:54, rjkroege wrote: > This is used only in this file? Perhaps it should be in an anonymous namespace? Correct, and it is indeed already in an anonymous namespace. https://codereview.chromium.org/2459973002/diff/100001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): https://codereview.chromium.org/2459973002/diff/100001/ui/ozone/platform/drm/... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:244: // Synchronously send IPC if we are on the same thread. On 2016/11/01 22:54:54, rjkroege wrote: > Can you expand on when we would be on the same thread? DrmGpuPlatformSupportHost::GpuAddGraphicsDevice() is called from two places, (1) on the IO thread in DrmDisplayHostManager::OnGpuProcessLaunched, and (2) on the UI thread in DrmDisplayHostManager::OnAddGraphicsDevice https://codereview.chromium.org/2459973002/diff/100001/ui/ozone/platform/drm/... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:247: return true; On 2016/11/01 22:54:54, rjkroege wrote: > I'm probably missing something but where does message get freed in this case? The convention is that the callee (see also gpu_process_host.cc: send_callback_ which is base::Bind(&SendGpuProcessMessageByHostId, host_id) in this case) takes care of freeing the message. Specifically: - if GpuProcessHost::Send(message) fails, it is freed immediately - otherwise, the IPCSender takes over the ownership of the message
https://codereview.chromium.org/2459973002/diff/100001/content/browser/gpu/gp... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2459973002/diff/100001/content/browser/gpu/gp... content/browser/gpu/gpu_process_host.cc:612: #if defined(USE_OZONE) On 2016/11/01 22:54:54, rjkroege wrote: > For the benefit of us who need to update the mus GpuService code to not regress > your change, can you add an explanation here. Maybe a link to the bug? Done.
The CQ bit was checked by hshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2459973002/diff/100001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): https://codereview.chromium.org/2459973002/diff/100001/ui/ozone/platform/drm/... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:244: // Synchronously send IPC if we are on the same thread. On 2016/11/01 23:02:32, hshi1 wrote: > On 2016/11/01 22:54:54, rjkroege wrote: > > Can you expand on when we would be on the same thread? > > DrmGpuPlatformSupportHost::GpuAddGraphicsDevice() is called from two places, (1) > on the IO thread in DrmDisplayHostManager::OnGpuProcessLaunched, and (2) on the > UI thread in DrmDisplayHostManager::OnAddGraphicsDevice You could add to comment? https://codereview.chromium.org/2459973002/diff/100001/ui/ozone/platform/drm/... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:247: return true; On 2016/11/01 23:02:32, hshi1 wrote: > On 2016/11/01 22:54:54, rjkroege wrote: > > I'm probably missing something but where does message get freed in this case? > > The convention is that the callee (see also gpu_process_host.cc: send_callback_ > which is base::Bind(&SendGpuProcessMessageByHostId, host_id) in this case) takes > care of freeing the message. Specifically: > > - if GpuProcessHost::Send(message) fails, it is freed immediately > - otherwise, the IPCSender takes over the ownership of the message Righto. Sorry that I missed that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2459973002/diff/100001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc (right): https://codereview.chromium.org/2459973002/diff/100001/ui/ozone/platform/drm/... ui/ozone/platform/drm/host/drm_gpu_platform_support_host.cc:244: // Synchronously send IPC if we are on the same thread. On 2016/11/01 23:41:08, rjkroege wrote: > On 2016/11/01 23:02:32, hshi1 wrote: > > On 2016/11/01 22:54:54, rjkroege wrote: > > > Can you expand on when we would be on the same thread? > > > > DrmGpuPlatformSupportHost::GpuAddGraphicsDevice() is called from two places, > (1) > > on the IO thread in DrmDisplayHostManager::OnGpuProcessLaunched, and (2) on > the > > UI thread in DrmDisplayHostManager::OnAddGraphicsDevice > > You could add to comment? Done.
The CQ bit was checked by hshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, rjkroege@chromium.org Link to the patchset: https://codereview.chromium.org/2459973002/#ps140001 (title: "Add more comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Expedite discovery of primary DRM device. Previously the DrmDeviceHandle and fd for the primary device is sent to the GPU process on the UI thread in DrmDisplayHostManager::OnGpuThreadReady(), which is invoked from the constructor of GpuProcessHostUIShim. If a buffer allocation call arrives immediately after a GPU process crash, it is possible that primary DRM device has not yet been sent to the new GPU process by the time the allocation occurs. This CL moves part of the logic in DrmDisplayHostManager::OnGpuThreadReady() that sends the primary DRM device into a separate function, to be invoked directly by GpuProcessHost constructor on the IO thread. BUG=chromium:608839 TEST=kill GPU process and confirm no crash in GbmBuffer::CreateBuffer(). ========== to ========== Expedite discovery of primary DRM device. Previously the DrmDeviceHandle and fd for the primary device is sent to the GPU process on the UI thread in DrmDisplayHostManager::OnGpuThreadReady(), which is invoked from the constructor of GpuProcessHostUIShim. If a buffer allocation call arrives immediately after a GPU process crash, it is possible that primary DRM device has not yet been sent to the new GPU process by the time the allocation occurs. This CL moves part of the logic in DrmDisplayHostManager::OnGpuThreadReady() that sends the primary DRM device into a separate function, to be invoked directly by GpuProcessHost constructor on the IO thread. BUG=chromium:608839 TEST=kill GPU process and confirm no crash in GbmBuffer::CreateBuffer(). ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Expedite discovery of primary DRM device. Previously the DrmDeviceHandle and fd for the primary device is sent to the GPU process on the UI thread in DrmDisplayHostManager::OnGpuThreadReady(), which is invoked from the constructor of GpuProcessHostUIShim. If a buffer allocation call arrives immediately after a GPU process crash, it is possible that primary DRM device has not yet been sent to the new GPU process by the time the allocation occurs. This CL moves part of the logic in DrmDisplayHostManager::OnGpuThreadReady() that sends the primary DRM device into a separate function, to be invoked directly by GpuProcessHost constructor on the IO thread. BUG=chromium:608839 TEST=kill GPU process and confirm no crash in GbmBuffer::CreateBuffer(). ========== to ========== Expedite discovery of primary DRM device. Previously the DrmDeviceHandle and fd for the primary device is sent to the GPU process on the UI thread in DrmDisplayHostManager::OnGpuThreadReady(), which is invoked from the constructor of GpuProcessHostUIShim. If a buffer allocation call arrives immediately after a GPU process crash, it is possible that primary DRM device has not yet been sent to the new GPU process by the time the allocation occurs. This CL moves part of the logic in DrmDisplayHostManager::OnGpuThreadReady() that sends the primary DRM device into a separate function, to be invoked directly by GpuProcessHost constructor on the IO thread. BUG=chromium:608839 TEST=kill GPU process and confirm no crash in GbmBuffer::CreateBuffer(). Committed: https://crrev.com/5798066b00f2b7824ba0c476cc12736dfbce87f1 Cr-Commit-Position: refs/heads/master@{#429173} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/5798066b00f2b7824ba0c476cc12736dfbce87f1 Cr-Commit-Position: refs/heads/master@{#429173} |
