Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
712343003:
Infrastructure for temportarily relinquishing GPU resources. (Closed)
|
Created:
6 years, 1 month ago by GusFernandez Modified:
6 years ago CC:
chromium-reviews, darin-cc_chromium.org, jam, kalyank, ozone-reviews_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionInfrastructure for temporarily relinquishing GPU resources.
Chromecast needs to release all OpenGL/EGL resources before
launching an external app which will own the screen and GPU.
This includes the EGL display type and window as well as the
default off-screen pbuffer which were previously retained until the GPU process exits.
BUG=432268
Committed: https://crrev.com/44a73aac041d87ae260a46f8c8e917af0e3a29ce
Cr-Commit-Position: refs/heads/master@{#305331}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Clean-up based on feedback from spang and jbauman #
Total comments: 6
Patch Set 3 : Make callback optional. Other changes suggested by jbauman and spang #
Total comments: 10
Patch Set 4 : Changes suggested by piman #Patch Set 5 : Changes suggested by spang #
Total comments: 6
Patch Set 6 : Move RelinquishGpuResources stub implementation to StubGpuPlatformSupport #Patch Set 7 : changes suggested by piman #Patch Set 8 : Add a new DestroyAndTerminateDisplay method to avoid Win unit test failures #Patch Set 9 : Run trybots again. Maybe now I have trybot access. #Patch Set 10 : Try trybots again. Maybe now I have access #
Total comments: 20
Patch Set 11 : changes suggested by dcheng #
Messages
Total messages: 45 (10 generated)
gusfernandez@chromium.org changed reviewers: + gunsch@chromium.org, jbauman@chromium.org, lcwu@google.com
The GpuProcessHost stuff would probably make more sense in GpuProcessHostUIShim if it's going to be executed on the UI thread. https://codereview.chromium.org/712343003/diff/1/content/browser/gpu/gpu_proc... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/712343003/diff/1/content/browser/gpu/gpu_proc... content/browser/gpu/gpu_process_host.cc:365: base::Unretained(host), I don't think this use of base::Unretained is safe. Instead just call host->RelinquishResourcesInternal directly. https://codereview.chromium.org/712343003/diff/1/content/browser/gpu/gpu_proc... content/browser/gpu/gpu_process_host.cc:374: relinquish_callback_ = callback; DCHECK(relinquish_callback_.is_null()); before this. https://codereview.chromium.org/712343003/diff/1/content/browser/gpu/gpu_proc... content/browser/gpu/gpu_process_host.cc:401: } reset relinquish_callback_ here. Also you should do this in GpuProcessHost::~GpuProcessHost as well. https://codereview.chromium.org/712343003/diff/1/content/common/gpu/gpu_chann... File content/common/gpu/gpu_channel_manager.cc (right): https://codereview.chromium.org/712343003/diff/1/content/common/gpu/gpu_chann... content/common/gpu/gpu_channel_manager.cc:106: OnDeleteDefaultOffscreenSurface(); This shouldn't be necessary, as it'll be deleted automatically. https://codereview.chromium.org/712343003/diff/1/content/common/gpu/gpu_chann... content/common/gpu/gpu_channel_manager.cc:327: default_offscreen_surface_->Destroy(); This Destroy should be unnecessary, as it'll be called in the destructor. https://codereview.chromium.org/712343003/diff/1/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/712343003/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:398: CHECK(g_num_surfaces > 0) << "Bad surface count"; DCHECK probably makes more sense here.
This is a significant clean-up and simplifaction based on feedback from Spang and jbauman https://codereview.chromium.org/712343003/diff/1/content/browser/gpu/gpu_proc... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/712343003/diff/1/content/browser/gpu/gpu_proc... content/browser/gpu/gpu_process_host.cc:365: base::Unretained(host), On 2014/11/12 01:47:28, jbauman wrote: > I don't think this use of base::Unretained is safe. Instead just call > host->RelinquishResourcesInternal directly. This is now completely re-written and simplified in gpu_process_host_ui_shim.cc https://codereview.chromium.org/712343003/diff/1/content/browser/gpu/gpu_proc... content/browser/gpu/gpu_process_host.cc:374: relinquish_callback_ = callback; On 2014/11/12 01:47:28, jbauman wrote: > DCHECK(relinquish_callback_.is_null()); before this. Done. https://codereview.chromium.org/712343003/diff/1/content/browser/gpu/gpu_proc... content/browser/gpu/gpu_process_host.cc:401: } On 2014/11/12 01:47:28, jbauman wrote: > reset relinquish_callback_ here. Also you should do this in > GpuProcessHost::~GpuProcessHost as well. Done. https://codereview.chromium.org/712343003/diff/1/content/common/gpu/gpu_chann... File content/common/gpu/gpu_channel_manager.cc (right): https://codereview.chromium.org/712343003/diff/1/content/common/gpu/gpu_chann... content/common/gpu/gpu_channel_manager.cc:106: OnDeleteDefaultOffscreenSurface(); On 2014/11/12 01:47:28, jbauman wrote: > This shouldn't be necessary, as it'll be deleted automatically. See below. https://codereview.chromium.org/712343003/diff/1/content/common/gpu/gpu_chann... content/common/gpu/gpu_channel_manager.cc:327: default_offscreen_surface_->Destroy(); On 2014/11/12 01:47:28, jbauman wrote: > This Destroy should be unnecessary, as it'll be called in the destructor. The whole point is to release the default_offscreen_surface temporarily without having to tear down the whole GPU channel. https://codereview.chromium.org/712343003/diff/1/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/712343003/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:398: CHECK(g_num_surfaces > 0) << "Bad surface count"; On 2014/11/12 01:47:28, jbauman wrote: > DCHECK probably makes more sense here. Done.
spang@chromium.org changed reviewers: + spang@chromium.org
this needs attention from a ui/gl owner https://codereview.chromium.org/712343003/diff/20001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/712343003/diff/20001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:235: g_initialized = false; should this call eglTerminate? https://codereview.chromium.org/712343003/diff/20001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:667: if (--g_num_surfaces == 0) { This seems fairly magic.. shouldn't this be explict? If I write a chromium app using GL, do I necessarily want to destroy the EGL display when I destroy the last surface?
lgtm, with one nit. https://codereview.chromium.org/712343003/diff/20001/content/common/gpu/gpu_m... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/712343003/diff/20001/content/common/gpu/gpu_m... content/common/gpu/gpu_messages.h:306: IPC_MESSAGE_CONTROL0(GpuMsg_RelinquishDisplay) This isn't used in this patch, so remove it.
gusfernandez@chromium.org changed reviewers: + piman@chromium.org, piman@google.com
Patch Set 3 contains Changes suggested by spang and jbauman https://codereview.chromium.org/712343003/diff/20001/content/common/gpu/gpu_m... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/712343003/diff/20001/content/common/gpu/gpu_m... content/common/gpu/gpu_messages.h:306: IPC_MESSAGE_CONTROL0(GpuMsg_RelinquishDisplay) On 2014/11/18 00:30:03, jbauman wrote: > This isn't used in this patch, so remove it. This will be moved to ozone_gpu_messages.cc in the next patch set. https://codereview.chromium.org/712343003/diff/20001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/712343003/diff/20001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:235: g_initialized = false; On 2014/11/17 21:49:12, spang wrote: > should this call eglTerminate? Not strictly necessary with our platform, but it is a good idea anyway. https://codereview.chromium.org/712343003/diff/20001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:667: if (--g_num_surfaces == 0) { On 2014/11/17 21:49:12, spang wrote: > This seems fairly magic.. shouldn't this be explict? > > If I write a chromium app using GL, do I necessarily want to destroy the EGL > display when I destroy the last surface? I considered an explicit call here, but that got messier and it looked like the reference count approach was best. The display is automatically recreated once the first new surface (either native or pbuffer) is created.
https://codereview.chromium.org/712343003/diff/40001/content/browser/gpu/gpu_... File content/browser/gpu/gpu_process_host_ui_shim.cc (right): https://codereview.chromium.org/712343003/diff/40001/content/browser/gpu/gpu_... content/browser/gpu/gpu_process_host_ui_shim.cc:192: Send(new GpuMsg_DeleteDefaultOffscreenSurface()); Would you not want to wait for an ack before calling the callback? https://codereview.chromium.org/712343003/diff/40001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/712343003/diff/40001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:390: ++g_num_surfaces; Could that tracking be done in the constructor/destructor of GLSurfaceEGL? That way it'd avoid duplication. https://codereview.chromium.org/712343003/diff/40001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:669: CHECK_GT(g_num_surfaces, 0) << "Bad surface count"; nit: DCHECK_GT https://codereview.chromium.org/712343003/diff/40001/ui/ozone/common/gpu/ozon... File ui/ozone/common/gpu/ozone_gpu_messages.h (right): https://codereview.chromium.org/712343003/diff/40001/ui/ozone/common/gpu/ozon... ui/ozone/common/gpu/ozone_gpu_messages.h:104: IPC_MESSAGE_CONTROL0(OzoneGpuMsg_RelinquishDisplay) Where is the code that uses this new message?
https://codereview.chromium.org/712343003/diff/40001/content/browser/gpu/gpu_... File content/browser/gpu/gpu_process_host_ui_shim.cc (right): https://codereview.chromium.org/712343003/diff/40001/content/browser/gpu/gpu_... content/browser/gpu/gpu_process_host_ui_shim.cc:192: Send(new GpuMsg_DeleteDefaultOffscreenSurface()); On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > Would you not want to wait for an ack before calling the callback? This is done effectively by the next call. We only need to be synchronous with the second (RelinquishGpuResources) API. https://codereview.chromium.org/712343003/diff/40001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/712343003/diff/40001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:390: ++g_num_surfaces; On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > Could that tracking be done in the constructor/destructor of GLSurfaceEGL? That > way it'd avoid duplication. Good idea. This simplified the code somewhat. https://codereview.chromium.org/712343003/diff/40001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:669: CHECK_GT(g_num_surfaces, 0) << "Bad surface count"; On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > nit: DCHECK_GT Irrelevant after the change above. https://codereview.chromium.org/712343003/diff/40001/ui/ozone/common/gpu/ozon... File ui/ozone/common/gpu/ozone_gpu_messages.h (right): https://codereview.chromium.org/712343003/diff/40001/ui/ozone/common/gpu/ozon... ui/ozone/common/gpu/ozone_gpu_messages.h:104: IPC_MESSAGE_CONTROL0(OzoneGpuMsg_RelinquishDisplay) On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > Where is the code that uses this new message? This and OzoneHostMsg_ResourcesRelinquished below are used to communicate between the browser and GPU processes by the internal Cast ozone_platform which has vendor-specific code which we cannot open-source.
On 2014/11/19 18:50:13, gusfernandez wrote: > https://codereview.chromium.org/712343003/diff/40001/content/browser/gpu/gpu_... > File content/browser/gpu/gpu_process_host_ui_shim.cc (right): > > https://codereview.chromium.org/712343003/diff/40001/content/browser/gpu/gpu_... > content/browser/gpu/gpu_process_host_ui_shim.cc:192: Send(new > GpuMsg_DeleteDefaultOffscreenSurface()); > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > > Would you not want to wait for an ack before calling the callback? > > This is done effectively by the next call. We only need to be synchronous with > the second (RelinquishGpuResources) API. > > https://codereview.chromium.org/712343003/diff/40001/ui/gl/gl_surface_egl.cc > File ui/gl/gl_surface_egl.cc (right): > > https://codereview.chromium.org/712343003/diff/40001/ui/gl/gl_surface_egl.cc#... > ui/gl/gl_surface_egl.cc:390: ++g_num_surfaces; > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > > Could that tracking be done in the constructor/destructor of GLSurfaceEGL? > That > > way it'd avoid duplication. > > Good idea. This simplified the code somewhat. > > https://codereview.chromium.org/712343003/diff/40001/ui/gl/gl_surface_egl.cc#... > ui/gl/gl_surface_egl.cc:669: CHECK_GT(g_num_surfaces, 0) << "Bad surface count"; > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > > nit: DCHECK_GT > > Irrelevant after the change above. > > https://codereview.chromium.org/712343003/diff/40001/ui/ozone/common/gpu/ozon... > File ui/ozone/common/gpu/ozone_gpu_messages.h (right): > > https://codereview.chromium.org/712343003/diff/40001/ui/ozone/common/gpu/ozon... > ui/ozone/common/gpu/ozone_gpu_messages.h:104: > IPC_MESSAGE_CONTROL0(OzoneGpuMsg_RelinquishDisplay) > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > > Where is the code that uses this new message? > > This and OzoneHostMsg_ResourcesRelinquished below are used to communicate > between the browser and GPU processes by the internal Cast ozone_platform which > has vendor-specific code which we cannot open-source. FWIW there is not any requirement for messages to be in this file (or the equivalent file in content). You could make a new file in the vendor repository (e.g. chromecast_vendor_gpu_messages.h). The one thing is that you'll want to reserve an id in ipc/ipc_message_start.h in the main tree because of the way chrome IPC handles message classes.
On 2014/11/19 18:55:41, spang wrote: > On 2014/11/19 18:50:13, gusfernandez wrote: > > > https://codereview.chromium.org/712343003/diff/40001/content/browser/gpu/gpu_... > > File content/browser/gpu/gpu_process_host_ui_shim.cc (right): > > > > > https://codereview.chromium.org/712343003/diff/40001/content/browser/gpu/gpu_... > > content/browser/gpu/gpu_process_host_ui_shim.cc:192: Send(new > > GpuMsg_DeleteDefaultOffscreenSurface()); > > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > > > Would you not want to wait for an ack before calling the callback? > > > > This is done effectively by the next call. We only need to be synchronous with > > the second (RelinquishGpuResources) API. > > > > https://codereview.chromium.org/712343003/diff/40001/ui/gl/gl_surface_egl.cc > > File ui/gl/gl_surface_egl.cc (right): > > > > > https://codereview.chromium.org/712343003/diff/40001/ui/gl/gl_surface_egl.cc#... > > ui/gl/gl_surface_egl.cc:390: ++g_num_surfaces; > > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > > > Could that tracking be done in the constructor/destructor of GLSurfaceEGL? > > That > > > way it'd avoid duplication. > > > > Good idea. This simplified the code somewhat. > > > > > https://codereview.chromium.org/712343003/diff/40001/ui/gl/gl_surface_egl.cc#... > > ui/gl/gl_surface_egl.cc:669: CHECK_GT(g_num_surfaces, 0) << "Bad surface > count"; > > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > > > nit: DCHECK_GT > > > > Irrelevant after the change above. > > > > > https://codereview.chromium.org/712343003/diff/40001/ui/ozone/common/gpu/ozon... > > File ui/ozone/common/gpu/ozone_gpu_messages.h (right): > > > > > https://codereview.chromium.org/712343003/diff/40001/ui/ozone/common/gpu/ozon... > > ui/ozone/common/gpu/ozone_gpu_messages.h:104: > > IPC_MESSAGE_CONTROL0(OzoneGpuMsg_RelinquishDisplay) > > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > > > Where is the code that uses this new message? > > > > This and OzoneHostMsg_ResourcesRelinquished below are used to communicate > > between the browser and GPU processes by the internal Cast ozone_platform > which > > has vendor-specific code which we cannot open-source. > > FWIW there is not any requirement for messages to be in this file (or the > equivalent file in content). You could make a new file in the vendor repository > (e.g. chromecast_vendor_gpu_messages.h). > > The one thing is that you'll want to reserve an id in ipc/ipc_message_start.h in > the main tree because of the way chrome IPC handles message classes. I have a larger design question, though. Why send a message to the GPU to clean up the default surface, then call into the platform which sends another message to the GPU to do more clean up? What I'm wondering is, why not just do: - Add a ShutdownGpu(callback) call to GpuProcessHostUIShim that you use to park the GPU. - Add just one IPC pair to the main gpu_messages.h, GpuMsg_ShutdownGpu + GpuHostMsg_OnGpuShutdown. - In the gpu process upon receiving GpuMsg_ShudownGpu, delete the default offscreen surface, then do #ifdef USE_OZONE ui::OzonePlatform::GetInstance()->GetGpuPlatformSupport()->OnGpuShutdown(); #endif for any vendor-specific cleanup that you want triggered by this type of shutdown.
https://codereview.chromium.org/712343003/diff/40001/content/browser/gpu/gpu_... File content/browser/gpu/gpu_process_host_ui_shim.cc (right): https://codereview.chromium.org/712343003/diff/40001/content/browser/gpu/gpu_... content/browser/gpu/gpu_process_host_ui_shim.cc:192: Send(new GpuMsg_DeleteDefaultOffscreenSurface()); On 2014/11/19 18:50:13, gusfernandez wrote: > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > > Would you not want to wait for an ack before calling the callback? > > This is done effectively by the next call. We only need to be synchronous with > the second (RelinquishGpuResources) API. But that's not true on !OZONE https://codereview.chromium.org/712343003/diff/40001/ui/ozone/common/gpu/ozon... File ui/ozone/common/gpu/ozone_gpu_messages.h (right): https://codereview.chromium.org/712343003/diff/40001/ui/ozone/common/gpu/ozon... ui/ozone/common/gpu/ozone_gpu_messages.h:104: IPC_MESSAGE_CONTROL0(OzoneGpuMsg_RelinquishDisplay) On 2014/11/19 18:50:13, gusfernandez wrote: > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > > Where is the code that uses this new message? > > This and OzoneHostMsg_ResourcesRelinquished below are used to communicate > between the browser and GPU processes by the internal Cast ozone_platform which > has vendor-specific code which we cannot open-source. I'm not a big fan of adding messages that have no (visible) users, nor an implementation. For the purpose of the chromium project, it has no value. I like spang@'s suggestion. A single cross-platform message that does generic cleanup as well as platform-specific cleanup. The platform specifics is hidden behind an ozone API.
On 2014/11/19 20:33:52, piman (Very slow to review) wrote: > https://codereview.chromium.org/712343003/diff/40001/content/browser/gpu/gpu_... > File content/browser/gpu/gpu_process_host_ui_shim.cc (right): > > https://codereview.chromium.org/712343003/diff/40001/content/browser/gpu/gpu_... > content/browser/gpu/gpu_process_host_ui_shim.cc:192: Send(new > GpuMsg_DeleteDefaultOffscreenSurface()); > On 2014/11/19 18:50:13, gusfernandez wrote: > > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > > > Would you not want to wait for an ack before calling the callback? > > > > This is done effectively by the next call. We only need to be synchronous with > > the second (RelinquishGpuResources) API. > > But that's not true on !OZONE > > https://codereview.chromium.org/712343003/diff/40001/ui/ozone/common/gpu/ozon... > File ui/ozone/common/gpu/ozone_gpu_messages.h (right): > > https://codereview.chromium.org/712343003/diff/40001/ui/ozone/common/gpu/ozon... > ui/ozone/common/gpu/ozone_gpu_messages.h:104: > IPC_MESSAGE_CONTROL0(OzoneGpuMsg_RelinquishDisplay) > On 2014/11/19 18:50:13, gusfernandez wrote: > > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: > > > Where is the code that uses this new message? > > > > This and OzoneHostMsg_ResourcesRelinquished below are used to communicate > > between the browser and GPU processes by the internal Cast ozone_platform > which > > has vendor-specific code which we cannot open-source. > > I'm not a big fan of adding messages that have no (visible) users, nor an > implementation. For the purpose of the chromium project, it has no value. > > I like spang@'s suggestion. A single cross-platform message that does generic > cleanup as well as platform-specific cleanup. The platform specifics is hidden > behind an ozone API. Per offline chat with Gus: Gus is saying when he receives the shutdown message in the GPU process, there are still EGL surfaces that have not been destroyed, preventing immediate shutdown inside the platform. So right now this implementation relies on the platform delaying the IPC response until those surfaces are destroyed. This is a little unexpected to me though, the EGL resources are destroyed in the GPU process so it should not be necessary to rely on the platform to synchronize this.
On Wed, Nov 19, 2014 at 12:39 PM, <spang@chromium.org> wrote: > On 2014/11/19 20:33:52, piman (Very slow to review) wrote: > > https://codereview.chromium.org/712343003/diff/40001/ > content/browser/gpu/gpu_process_host_ui_shim.cc > >> File content/browser/gpu/gpu_process_host_ui_shim.cc (right): >> > > > https://codereview.chromium.org/712343003/diff/40001/ > content/browser/gpu/gpu_process_host_ui_shim.cc#newcode192 > >> content/browser/gpu/gpu_process_host_ui_shim.cc:192: Send(new >> GpuMsg_DeleteDefaultOffscreenSurface()); >> On 2014/11/19 18:50:13, gusfernandez wrote: >> > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: >> > > Would you not want to wait for an ack before calling the callback? >> > >> > This is done effectively by the next call. We only need to be >> synchronous >> > with > >> > the second (RelinquishGpuResources) API. >> > > But that's not true on !OZONE >> > > > https://codereview.chromium.org/712343003/diff/40001/ui/ > ozone/common/gpu/ozone_gpu_messages.h > >> File ui/ozone/common/gpu/ozone_gpu_messages.h (right): >> > > > https://codereview.chromium.org/712343003/diff/40001/ui/ > ozone/common/gpu/ozone_gpu_messages.h#newcode104 > >> ui/ozone/common/gpu/ozone_gpu_messages.h:104: >> IPC_MESSAGE_CONTROL0(OzoneGpuMsg_RelinquishDisplay) >> On 2014/11/19 18:50:13, gusfernandez wrote: >> > On 2014/11/18 20:55:34, piman (Very slow to review) wrote: >> > > Where is the code that uses this new message? >> > >> > This and OzoneHostMsg_ResourcesRelinquished below are used to >> communicate >> > between the browser and GPU processes by the internal Cast >> ozone_platform >> which >> > has vendor-specific code which we cannot open-source. >> > > I'm not a big fan of adding messages that have no (visible) users, nor an >> implementation. For the purpose of the chromium project, it has no value. >> > > I like spang@'s suggestion. A single cross-platform message that does >> generic >> cleanup as well as platform-specific cleanup. The platform specifics is >> hidden >> behind an ozone API. >> > > Per offline chat with Gus: > > Gus is saying when he receives the shutdown message in the GPU process, > there > are still EGL surfaces that have not been destroyed, preventing immediate > shutdown inside the platform. So right now this implementation relies on > the > platform delaying the IPC response until those surfaces are destroyed. > > This is a little unexpected to me though, the EGL resources are destroyed > in the > GPU process so it should not be necessary to rely on the platform to > synchronize > this. > Sounds like we should investigate and understand that first. The 2 messages would typically be handled back-to-back. Something could happen in the middle (e.g. a posted task) which changes things, but that would be non-deterministic, and we don't want to rely on a race. > https://codereview.chromium.org/712343003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Here is an updated version which calls a new RelinquishResources Ozone_GpuPlatformSupport API after we delete the default_offscreen_resource in response to a message from the Browser process. In previous implementations, we were sometimes seeing the message from the Browser process coming before the last native EGL surface was destroyed. This may not be the case in newer versions of the Chromecast application manager, but we would rather maintain this since we are looking at new changes where the ordering may again be reversed.
On 2014/11/20 16:39:06, gusfernandez wrote: > Here is an updated version which calls a new RelinquishResources > Ozone_GpuPlatformSupport API after we delete the default_offscreen_resource in > response to a message from the Browser process. > > In previous implementations, we were sometimes seeing the message from the > Browser process coming before the last native EGL surface was destroyed. This > may not be the case in newer versions of the Chromecast application manager, but > we would rather maintain this since we are looking at new changes where the > ordering may again be reversed. API in ui/ozone/public lgtm
https://codereview.chromium.org/712343003/diff/80001/ui/ozone/public/gpu_plat... File ui/ozone/public/gpu_platform_support.h (right): https://codereview.chromium.org/712343003/diff/80001/ui/ozone/public/gpu_plat... ui/ozone/public/gpu_platform_support.h:26: virtual void RelinquishGpuResources(const base::Closure& callback); Er, one last nit: can you leave this as a pure interface and move the no-op implementation to StubGpuPlatformSupport?
lcwu@chromium.org changed reviewers: + lcwu@chromium.org - lcwu@google.com
https://codereview.chromium.org/712343003/diff/80001/content/common/gpu/gpu_c... File content/common/gpu/gpu_channel_manager.cc (right): https://codereview.chromium.org/712343003/diff/80001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel_manager.cc:339: &GpuChannelManager::OnResourcesReliuquished, base::Unretained(this))); Is Unretained safe? If it is, can you document why? https://codereview.chromium.org/712343003/diff/80001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel_manager.cc:341: OnResourcesReliuquished(); typo: Reliuquished -> Relinquished
Moved RelinquishGpuResources to StubGpuPlatformSupport as per spang's suggestion.
A couple of changes suggested by piman. https://codereview.chromium.org/712343003/diff/80001/content/common/gpu/gpu_c... File content/common/gpu/gpu_channel_manager.cc (right): https://codereview.chromium.org/712343003/diff/80001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel_manager.cc:339: &GpuChannelManager::OnResourcesReliuquished, base::Unretained(this))); On 2014/11/20 21:06:43, piman (Very slow to review) wrote: > Is Unretained safe? > If it is, can you document why? Used the safer weak_factory_.GetWeakPtr() instead. https://codereview.chromium.org/712343003/diff/80001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel_manager.cc:341: OnResourcesReliuquished(); On 2014/11/20 21:06:43, piman (Very slow to review) wrote: > typo: Reliuquished -> Relinquished Done. https://codereview.chromium.org/712343003/diff/80001/ui/ozone/public/gpu_plat... File ui/ozone/public/gpu_platform_support.h (right): https://codereview.chromium.org/712343003/diff/80001/ui/ozone/public/gpu_plat... ui/ozone/public/gpu_platform_support.h:26: virtual void RelinquishGpuResources(const base::Closure& callback); On 2014/11/20 18:08:22, spang wrote: > Er, one last nit: > > can you leave this as a pure interface and move the no-op implementation > to StubGpuPlatformSupport? I also had to implement a stub implementation for the DRI platform.
lgtm
The CQ bit was checked by gusfernandez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/712343003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
+dcheng for gpu_messages.h
lcwu@chromium.org changed reviewers: + dcheng@chromium.org
The CQ bit was checked by gusfernandez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/712343003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
New change to explicitly delete the display to avoid problems with win unit tests.
Is there a followup CL that will use the new methods on GpuProcessHostUIShim? They don't seem to be called. Also, there's a typo in the CL description (fefault should be default, I think) https://codereview.chromium.org/712343003/diff/180001/content/browser/gpu/gpu... File content/browser/gpu/gpu_process_host_ui_shim.cc (right): https://codereview.chromium.org/712343003/diff/180001/content/browser/gpu/gpu... content/browser/gpu/gpu_process_host_ui_shim.cc:185: const base::Closure& callback) { Nit: this indent should be 4 spaces. https://codereview.chromium.org/712343003/diff/180001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel_manager.cc (right): https://codereview.chromium.org/712343003/diff/180001/content/common/gpu/gpu_... content/common/gpu/gpu_channel_manager.cc:334: default_offscreen_surface_ = NULL; nullptr. https://codereview.chromium.org/712343003/diff/180001/content/common/gpu/gpu_... content/common/gpu/gpu_channel_manager.cc:337: ui::OzonePlatform::GetInstance()->GetGpuPlatformSupport()-> Out of curiosity, did you use clang-format or not? (Note: I'm not asking to, this just out of curiosity) https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface.h File ui/gl/gl_surface.h (right): https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface.h#newc... ui/gl/gl_surface.h:41: // Destroys the surface and Terminates its underlying display. This must be terminates. https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:65: // In the Cast environment, we need to destroy the displayType What is "displayType"? Is this reference to a Chrome type or some other C/C++ type? https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:250: if (!g_initialized) { This isn't a static method. How could we get here without initialization already being triggered? https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:258: if (!g_initialized) { Why is this safe to do if there are no live GLSurfaceEGL objects live? To me, the CL description seems to hint that the GPU resources would be owned by another app if there are no GLSurfaceEGL objects--so it is really safe to initialize here? https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:300: if (--g_num_surfaces == 0 && g_terminate_pending) { Why isn't this simply gated on --g_num_surfaces == 0 && g_initialized? What problem does g_terminate_pending solve? https://codereview.chromium.org/712343003/diff/180001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_gpu_platform_support.h (right): https://codereview.chromium.org/712343003/diff/180001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_gpu_platform_support.h:48: virtual void RelinquishGpuResources(const base::Closure& callback) override; No virtual. Also, this should probably be grouped with the other GpuPlatformSupport override.
Callers to new APIs are in Chromecast internal code. See links in comment b/18255574. https://codereview.chromium.org/712343003/diff/180001/content/browser/gpu/gpu... File content/browser/gpu/gpu_process_host_ui_shim.cc (right): https://codereview.chromium.org/712343003/diff/180001/content/browser/gpu/gpu... content/browser/gpu/gpu_process_host_ui_shim.cc:185: const base::Closure& callback) { On 2014/11/21 03:06:03, dcheng wrote: > Nit: this indent should be 4 spaces. Done. https://codereview.chromium.org/712343003/diff/180001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel_manager.cc (right): https://codereview.chromium.org/712343003/diff/180001/content/common/gpu/gpu_... content/common/gpu/gpu_channel_manager.cc:334: default_offscreen_surface_ = NULL; On 2014/11/21 03:06:04, dcheng wrote: > nullptr. Done. https://codereview.chromium.org/712343003/diff/180001/content/common/gpu/gpu_... content/common/gpu/gpu_channel_manager.cc:337: ui::OzonePlatform::GetInstance()->GetGpuPlatformSupport()-> On 2014/11/21 03:06:04, dcheng wrote: > Out of curiosity, did you use clang-format or not? > > (Note: I'm not asking to, this just out of curiosity) Clang-format does some bad things to this file such as dropping the indentation of the IPC_MESSAGE_HANDLER lines. https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface.h File ui/gl/gl_surface.h (right): https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface.h#newc... ui/gl/gl_surface.h:41: // Destroys the surface and Terminates its underlying display. This must be On 2014/11/21 03:06:04, dcheng wrote: > terminates. Done. https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:65: // In the Cast environment, we need to destroy the displayType On 2014/11/21 03:06:04, dcheng wrote: > What is "displayType"? Is this reference to a Chrome type or some other C/C++ > type? I clarified the comment and also s/g_native_display/g_native_display_type/ to be more consistent with EGL naming. https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:250: if (!g_initialized) { On 2014/11/21 03:06:04, dcheng wrote: > This isn't a static method. How could we get here without initialization already > being triggered? Agreed. This is old code and no longer needed now that we do reference counting in the constructor/destructor. I did leave a DCHECK there, however. https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:258: if (!g_initialized) { On 2014/11/21 03:06:04, dcheng wrote: > Why is this safe to do if there are no live GLSurfaceEGL objects live? To me, > the CL description seems to hint that the GPU resources would be owned by > another app if there are no GLSurfaceEGL objects--so it is really safe to > initialize here? Platform ensures that no EGL objects are used by chrome while the external app owns the GPU. However, there are various entry points which access the EGL display or NateiveDisplayType besides GLSurface, so this and similar code in GetNativeDisplay are still needed. Added //static comments here and below. https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:300: if (--g_num_surfaces == 0 && g_terminate_pending) { On 2014/11/21 03:06:04, dcheng wrote: > Why isn't this simply gated on --g_num_surfaces == 0 && g_initialized? What > problem does g_terminate_pending solve? This is to resolve a problem with Win GL unit test framework which are expecting the display to not already be terminated on exit. https://codereview.chromium.org/712343003/diff/180001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_gpu_platform_support.h (right): https://codereview.chromium.org/712343003/diff/180001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_gpu_platform_support.h:48: virtual void RelinquishGpuResources(const base::Closure& callback) override; On 2014/11/21 03:06:04, dcheng wrote: > No virtual. Also, this should probably be grouped with the other > GpuPlatformSupport override. Done.
Do I need some to get added to some list somewhere to see the internal bits? Right now, the linked patches on that bug don't load for me. https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:258: if (!g_initialized) { On 2014/11/21 19:13:56, gusfernandez wrote: > On 2014/11/21 03:06:04, dcheng wrote: > > Why is this safe to do if there are no live GLSurfaceEGL objects live? To me, > > the CL description seems to hint that the GPU resources would be owned by > > another app if there are no GLSurfaceEGL objects--so it is really safe to > > initialize here? > > Platform ensures that no EGL objects are used by chrome while the external app > owns the GPU. However, there are various entry points which access the EGL > display or NateiveDisplayType besides GLSurface, so this and similar code in > GetNativeDisplay are still needed. > > Added //static comments here and below. I know these are static, but I don't follow the reasoning. It seems like the whole point of this patch is to deinitialize the GPU while an external app is using it. Presumably that's because there's an issue with leaving the resources initialized while another app wants to use them. If that's the case, why is it safe to re-initialize the GPU resources while an external app might be using them? https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:300: if (--g_num_surfaces == 0 && g_terminate_pending) { On 2014/11/21 19:13:56, gusfernandez wrote: > On 2014/11/21 03:06:04, dcheng wrote: > > Why isn't this simply gated on --g_num_surfaces == 0 && g_initialized? What > > problem does g_terminate_pending solve? > > This is to resolve a problem with Win GL unit test framework which are expecting > the display to not already be terminated on exit. Shouldn't we fix the tests, rather than having to hack around it? Makes things hard to reason about.
On 2014/11/21 20:43:00, dcheng wrote: > Do I need some to get added to some list somewhere to see the internal bits? > Right now, the linked patches on that bug don't load for me. > > https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc > File ui/gl/gl_surface_egl.cc (right): > > https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc... > ui/gl/gl_surface_egl.cc:258: if (!g_initialized) { > On 2014/11/21 19:13:56, gusfernandez wrote: > > On 2014/11/21 03:06:04, dcheng wrote: > > > Why is this safe to do if there are no live GLSurfaceEGL objects live? To > me, > > > the CL description seems to hint that the GPU resources would be owned by > > > another app if there are no GLSurfaceEGL objects--so it is really safe to > > > initialize here? > > > > Platform ensures that no EGL objects are used by chrome while the external app > > owns the GPU. However, there are various entry points which access the EGL > > display or NateiveDisplayType besides GLSurface, so this and similar code in > > GetNativeDisplay are still needed. > > > > Added //static comments here and below. > > I know these are static, but I don't follow the reasoning. It seems like the > whole point of this patch is to deinitialize the GPU while an external app is > using it. Presumably that's because there's an issue with leaving the resources > initialized while another app wants to use them. If that's the case, why is it > safe to re-initialize the GPU resources while an external app might be using > them? > > https://codereview.chromium.org/712343003/diff/180001/ui/gl/gl_surface_egl.cc... > ui/gl/gl_surface_egl.cc:300: if (--g_num_surfaces == 0 && g_terminate_pending) { > On 2014/11/21 19:13:56, gusfernandez wrote: > > On 2014/11/21 03:06:04, dcheng wrote: > > > Why isn't this simply gated on --g_num_surfaces == 0 && g_initialized? What > > > problem does g_terminate_pending solve? > > > > This is to resolve a problem with Win GL unit test framework which are > expecting > > the display to not already be terminated on exit. > > Shouldn't we fix the tests, rather than having to hack around it? Makes things > hard to reason about. Yes please (didn't realize this code had changed).
LGTM, but per our discussion, please follow up file bugs to address the initialization issues (maybe with some sort of explicit re-initialize IPC so GLSurfaceEGL doesn't need to randomly re-initialize stuff--ideally the initialization path shouldn't be that different from when the GPU process originally started and called GLSurface::InitializeOneOff) and the deinitialization issues (so we don't need g_terminate_pending).
The CQ bit was checked by gusfernandez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/712343003/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/44a73aac041d87ae260a46f8c8e917af0e3a29ce Cr-Commit-Position: refs/heads/master@{#305331} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
