[Dr-Dc] Direct Rendering Display Compositor gpu thread. [chromium/src : main]
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, Sunny Sachanandani.
vikas soni would like Vasiliy Telezhnikov and Sunny Sachanandani to review this change.
[Dr-Dc] Direct Rendering Display Compositor gpu thread.
1. Add support for a new gpu thread to be used by display compositor.
This new gpu thread is known as dr-dc aka direct rendering display
compositor. A new shared context will be current on this thread.
2. Add finch feature flags to enable it only on android P devices via
finch rollout. It will be currently disabled for all other platforms
and is currently disabled by default on android P also.
3. Viz changes to use this new thread when enabled.
Change-Id: I10f72e331f9273b35e90372c291de1c41a906e28
---
M android_webview/browser/gfx/deferred_gpu_command_service.cc
M android_webview/browser/gfx/deferred_gpu_command_service.h
M android_webview/lib/aw_main_delegate.cc
M cc/resources/ui_resource_bitmap.cc
M components/viz/service/BUILD.gn
A components/viz/service/display_embedder/display_compositor_gpu_thread.cc
A components/viz/service/display_embedder/display_compositor_gpu_thread.h
M components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc
M components/viz/service/gl/gpu_service_impl.cc
M components/viz/service/gl/gpu_service_impl.h
M components/viz/service/main/viz_main_impl.cc
M components/viz/service/main/viz_main_impl.h
M components/viz/test/test_gpu_service_holder.cc
M components/viz/test/test_gpu_service_holder.h
M content/gpu/gpu_main.cc
M gpu/command_buffer/service/gr_shader_cache.h
M gpu/command_buffer/service/memory_program_cache.cc
M gpu/config/gpu_finch_features.cc
M gpu/config/gpu_finch_features.h
M gpu/ipc/command_buffer_task_executor.cc
M gpu/ipc/command_buffer_task_executor.h
M gpu/ipc/gpu_in_process_thread_service.cc
M gpu/ipc/gpu_in_process_thread_service.h
M gpu/ipc/in_process_command_buffer.cc
M gpu/ipc/in_process_gpu_thread_holder.cc
M gpu/ipc/in_process_gpu_thread_holder.h
M gpu/ipc/service/gpu_channel_manager.cc
M gpu/ipc/service/gpu_channel_manager.h
28 files changed, 449 insertions(+), 61 deletions(-)
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, Sunny Sachanandani.
1 comment:
Patchset:
Hi Vasiliy/Sunny,
This CL contains most of the primary changes for dr-dc. can you please take a look.
dr-dc will be disabled by default before landing. i have enabled it right now for testing purposes.
Currently only video tests are failing with dr-dc. I am working on another patch which will make media stack thread safe and will fix the issues.
thanks
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Vasiliy Telezhnikov (Gerrit)
Attention is currently required from: Sunny Sachanandani, vikas soni.
13 comments:
Patchset:
I'll be looking more, but here are few comments
File components/viz/service/display_embedder/display_compositor_gpu_thread.cc:
Patch Set #26, Line 97:
gpu_channel_manager_->GetWeakPtr()Normally WeakPtrs are not thread safe, so we need to get them and check them on the same thread. You probably could move this to Initialize() and pass callback to InitializeOnThread().
Patch Set #26, Line 128:
mailbox_manager_ = std::make_unique<gpu::gles2::MailboxManagerImpl>();How do we live with to mailbox managers? i.e display compositor can ConsumeTexture(), but who will ProduceTexture()?
Patch Set #26, Line 130:
initialized_This is accessed on two threads.
Patch Set #26, Line 160:
gpu_channel_manager_This class outlives GpuChannelManager so we need to do something about it. e.g you could create callback binded to weakptr in Initialize()
File components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc:
Patch Set #26, Line 112:
gpu_task_executor_->ScheduleOutOfOrderTask(Could we just have DrDC task_runner() here? All ScheduleOutOfOrderTask/ScheduleDelayedWork are somewhat command buffer specific, like out of which order is it here? How much it will be delayed?
Patch Set #26, Line 130:
gpu_task_executor_->ScheduleGrContextCleanup();I looked a bit and it seems life time of GrCacheController and SharedContextState is 1:1. Do I miss something or we can move it inside SharedContextState so we don't need to plumb this all way through?
File components/viz/service/gl/gpu_service_impl.cc:
Patch Set #26, Line 842:
DCHECK(main_runner_->BelongsToCurrentThread());Why all DCHECKs are gone? Can this be now called on different thread? Do we need locks?
File components/viz/service/main/viz_main_impl.cc:
Patch Set #26, Line 212:
LOG(ERROR) << "vikas: dr dc thread initialized";nit: remove
Patch Set #26, Line 249:
dr_dc_thread->task_runner()->PostTask(Do we need to hop to other thread? I guess that depends on how we live with two mailbox managers. If we don't, than the only thing that is needed below is task_runner? (And format which can be const)
Patch Set #26, Line 329:
scoped_refptr<gpu::SharedContextState> VizMainImpl::GetSharedContextState() {This (and GetShareGroup) are GLRenderer specific, you could DCHECK that we're not using DrDC, so it will be easier to delete later (we won't need to guess if this is needed for DrDC specifically)
File gpu/config/gpu_finch_features.cc:
Patch Set #26, Line 190:
const base::Feature kEnableDrDc{"EnableDrDc", base::FEATURE_ENABLED_BY_DEFAULT};Just leaving a comment so we won't forget to change this to disabled before landing.
Patch Set #26, Line 285:
return false;nit: Is this reachable?
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, vikas soni.
11 comments:
Patchset:
I think one thing we want to ensure is a good boundary point between GPU main, DrDC, and shared state (sync points, mailboxes, shared images, etc.) I think a natural solution is GpuServiceImpl owns all the shared objects (plus GpuChannelManager and DrDcThread), GpuChannelManager owns all of GPU main resources, and DrDcThread owns the DrDc stuff.
File cc/resources/ui_resource_bitmap.cc:
// TODO(vikassoni): Forcing everything to N32 while android backing cannot
// support some other formats.
SkBitmap copy;This seems a bit heavy-handed - can we at least restrict this to OS_ANDROID?
File components/viz/service/display_embedder/display_compositor_gpu_thread.h:
Patch Set #26, Line 26:
DrDcThreadnit: can we call this DisplayCompositorGpuThread to match the filename? or just CompositorGpuThread (also change the filename) if you prefer a shorter name?
Patch Set #26, Line 32:
static DrDcThread* GetInstance();For the prototype we used a singleton because it was the simplest approach, but perhaps we should consider making something else own an instance of this - GpuMain or GpuServiceImpl perhaps?
File components/viz/service/display_embedder/display_compositor_gpu_thread.cc:
Patch Set #26, Line 77:
resourcesnit: share GL resources
Patch Set #26, Line 97:
gpu_channel_manager_->GetWeakPtr()
Normally WeakPtrs are not thread safe, so we need to get them and check them on the same thread. […]
IIUC you can retrieve weak ptrs on another thread, but they bind to the thread they are dereferenced on:
// To ensure correct use, the first time a WeakPtr issued by a WeakPtrFactory
// is dereferenced, the factory and its WeakPtrs become bound to the calling
// sequence or current SequencedWorkerPool token, and cannot be dereferenced or
// invalidated on any other task runner. Bound WeakPtrs can still be handed
// off to other task runners, e.g. to use to post tasks back to object on the
// bound sequence.https://source.chromium.org/chromium/chromium/src/+/master:base/memory/weak_ptr.h;l=54
Here the weak ptr is used to post a task to the GPU main thread task runner, and will be dereferenced on that thread so it's safe.
Patch Set #26, Line 128:
mailbox_manager_ = std::make_unique<gpu::gles2::MailboxManagerImpl>();
How do we live with to mailbox managers? i. […]
I think the MailboxManager only exists to prevent some DCHECKs from failing - it's not supposed to be used since DrDc requires all resources to be in shared images since they need to be shared across (logical) devices.
Patch Set #26, Line 130:
initialized_This is accessed on two threads.
This should be set after the event.Wait() in Initialize() above.
Patch Set #26, Line 160:
gpu_channel_manager_
This class outlives GpuChannelManager so we need to do something about it. e. […]
I think this is destroyed before GpuChannelManager - Destroy() is called before GpuServiceImpl is torn down and in any case this method should not be called after all command buffers are destroyed.
I think we should switch this from a singleton to be owned by GpuServiceImpl so that the order of destruction between this and GpuChannelManager is clear.
File components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc:
Patch Set #26, Line 112:
gpu_task_executor_->ScheduleOutOfOrderTask(
Could we just have DrDC task_runner() here? All ScheduleOutOfOrderTask/ScheduleDelayedWork are somew […]
"OutOfOrder" is a holdover name from previous scheduling APIs - it means that the task is not in the sequence of tasks associated with the task executor but can execute at any time - the implementation just posts a task to the task runner instead of enquing it in the sequence.
Exposing the task_runner also sgtm, but there's a risk of someone (in the future) posting a task directly that's supposed to go through ScheduleTask instead.
File components/viz/service/main/viz_main_impl.cc:
Patch Set #26, Line 249:
dr_dc_thread->task_runner()->PostTask(Do we need to hop to other thread? I guess that depends on how we live with two mailbox managers. […]
We need to hop on to the other thread because the task executor needs to be created there - the scheduler sequence gets bound to the thread calling CreateSequence. There's probably another way to achieve the same result - maybe pass in task runner explicitly to CreateSequence and have that be the key of the sequence_map_ in the scheduler (instead of thread id being the key).
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Vasiliy Telezhnikov (Gerrit)
Attention is currently required from: Sunny Sachanandani, vikas soni.
4 comments:
File components/viz/service/display_embedder/display_compositor_gpu_thread.cc:
Patch Set #26, Line 128:
mailbox_manager_ = std::make_unique<gpu::gles2::MailboxManagerImpl>();I think the MailboxManager only exists to prevent some DCHECKs from failing - it's not supposed to b […]
Should we use MailboxManagerDummy instead of impl then?
Patch Set #26, Line 160:
gpu_channel_manager_
I think this is destroyed before GpuChannelManager - Destroy() is called before GpuServiceImpl is to […]
Destroy() should reset |gpu_channel_manager_| then, but I agree if we can make this not singleton it will be safer and easier to understand.
File components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc:
Patch Set #26, Line 112:
gpu_task_executor_->ScheduleOutOfOrderTask(
"OutOfOrder" is a holdover name from previous scheduling APIs - it means that the task is not in the […]
We used task_runner() before and generally it would be good to be able delete CommandBufferTaskExecutor with the InProcessCommandBuffer. For this reason we have two ctors in DisplayCompositorMemoryAndTaskController. One with CommandBufferTaskExecutor for GLRenderer and one with SkiaOutputSurfaceDependency for SkiaRenderer.
Now it's just embedded more.
File components/viz/service/main/viz_main_impl.cc:
Patch Set #26, Line 249:
dr_dc_thread->task_runner()->PostTask(
We need to hop on to the other thread because the task executor needs to be created there - the sche […]
I think I might be missing something. CreateSequence for display compositor is normally called on Viz thread [1] and I don't see where we might call it in CreateFrameSinkManagerInternal.
And to make sure we bind sequence to right thread we do another thread hop here:
https://source.chromium.org/chromium/chromium/src/+/main:gpu/ipc/gpu_in_process_thread_service.cc;drc=4fa4c09a8511a374f79475d6c78f447481e0c4de;l=59Generally, about CreateSequence: Jumping to the thread to create object that will be used only viz thread is a bit weird. We normally create and use object on the same thread, which is not the case here. That change also makes it weird for webview which doesn't have CommandBufferTaskExecutor at all and this doesn't work anymore [2]. So if we can CreateSequence with task runner it feels cleaner way to me.
Out of curiosity, why just don't create two Gpu Schedulers? It feels like single scheduler doesn't really share anything across threads. Maybe I miss something about priorities?
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, Sunny Sachanandani.
5 comments:
Patchset:
I think one thing we want to ensure is a good boundary point between GPU main, DrDC, and shared stat […]
sounds good to me. i will refactor to remove singleton and move drdc ownership to GpuServiceImpl.
thanks for the reviews!
File components/viz/service/display_embedder/display_compositor_gpu_thread.cc:
Patch Set #26, Line 97:
gpu_channel_manager_->GetWeakPtr()
Normally WeakPtrs are not thread safe, so we need to get them and check them on the same thread. […]
agree.. thanks.
Patch Set #26, Line 130:
initialized_This is accessed on two threads.
yes. but the current call order guarantees that it will never happen concurrently.
i have added lock now but still wondering if we should skip locking when there is no concurrent access(and add comments to say so) OR continue to keep it under lock irrespective of concurrent access.
File components/viz/service/main/viz_main_impl.cc:
Patch Set #26, Line 249:
dr_dc_thread->task_runner()->PostTask(
I think I might be missing something. […]
ack on CreateFrameSinkManagerInternal thing. it seems like we dont need to hop for it since we do not call CreateSequence() in it/ via it.
createsequence with task_runner seems good/cleaner to me as you mentioned and will remove the thread hops and any confusions.
btw i dont get the webview thing you mentioned for [2]. where is CommandBufferTaskExecutor in that case. we are just using gpu::SchedulerSequence() to create sequence.also i think 2 gpu schedulers will be nicer. that will help revert back to old non-thread safe scheduler which uses task runner of the thread it was created on and will simplyfy things. i think the priorities are only among sequences of the same thread and not among sequences cross thread. so seems safe to create 2 scheduler.
will wait on what sunny has to say though.
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, Sunny Sachanandani.
2 comments:
File components/viz/service/display_embedder/display_compositor_gpu_thread.cc:
Patch Set #26, Line 97:
gpu_channel_manager_->GetWeakPtr()
agree.. thanks.
nevermind. it was from my old comments in progress
Patch Set #26, Line 130:
initialized_
yes. but the current call order guarantees that it will never happen concurrently. […]
nevermind. it was from my old comments in progress.
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Vasiliy Telezhnikov (Gerrit)
Attention is currently required from: Sunny Sachanandani, vikas soni.
1 comment:
File components/viz/service/main/viz_main_impl.cc:
Patch Set #26, Line 249:
dr_dc_thread->task_runner()->PostTask(
ack on CreateFrameSinkManagerInternal thing. […]
WebView thing: There is no CommandBufferTaskExecutor there, so we just create SchedulerSequence and after Sequence became thread-bound that code is broken because it runs tasks on Viz thread instead of gpu main. To fix it, I'll have to PostTask to gpu thread and wait.
That is not in production yet and it's not even finished, so it's not big deal it got broken, I just mentioned it because current way how it's implemented (thread hop in GpuInProcessThreadService) doesn't work for webview.
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, vikas soni.
1 comment:
File components/viz/service/main/viz_main_impl.cc:
Patch Set #26, Line 249:
dr_dc_thread->task_runner()->PostTask(
WebView thing: There is no CommandBufferTaskExecutor there, so we just create SchedulerSequence and […]
I think we would need priority propagation from display compositor sequence to raster sequence. If we used separate schedulers, these sequences would belong to separate schedulers and there can be no priority propagation.
CreateSequence with task runner solves both the webview issue and having a single scheduler.
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, Sunny Sachanandani.
1 comment:
File components/viz/service/main/viz_main_impl.cc:
Patch Set #26, Line 249:
dr_dc_thread->task_runner()->PostTask(
I think we would need priority propagation from display compositor sequence to raster sequence. […]
ah yes sorry. makes sense.
i will land a CL with CreateSequence using a task_runner before this CL.
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, Sunny Sachanandani.
14 comments:
Patchset:
updated CL to address all comments.
PTAL.
File cc/resources/ui_resource_bitmap.cc:
// TODO(vikassoni): Forcing everything to N32 while android backing cannot
// support some other formats.
SkBitmap copy;This seems a bit heavy-handed - can we at least restrict this to OS_ANDROID?
Done
File components/viz/service/display_embedder/display_compositor_gpu_thread.h:
Patch Set #26, Line 26:
DrDcThread
nit: can we call this DisplayCompositorGpuThread to match the filename? or just CompositorGpuThread […]
Done
Patch Set #26, Line 32:
static DrDcThread* GetInstance();
For the prototype we used a singleton because it was the simplest approach, but perhaps we should co […]
done. GpuServiceImpl now owns drdcthread.
File components/viz/service/display_embedder/display_compositor_gpu_thread.cc:
Patch Set #26, Line 77:
resourcesnit: share GL resources
Done
Patch Set #26, Line 128:
mailbox_manager_ = std::make_unique<gpu::gles2::MailboxManagerImpl>();
Should we use MailboxManagerDummy instead of impl then?
Done
Patch Set #26, Line 160:
gpu_channel_manager_
Destroy() should reset |gpu_channel_manager_| then, but I agree if we can make this not singleton it […]
Done
File components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc:
Patch Set #26, Line 112:
gpu_task_executor_->ScheduleOutOfOrderTask(
We used task_runner() before and generally it would be good to be able delete CommandBufferTaskExecu […]
using task_runner post task instead of ScheduleOutOfOrderTask/ScheduleDelayedWork since we want to be able to delete CommandBufferTaskExecutor in future.
Patch Set #26, Line 130:
gpu_task_executor_->ScheduleGrContextCleanup();
I looked a bit and it seems life time of GrCacheController and SharedContextState is 1:1. […]
GrCacheController is like a helper class which has logic for clearing the GrContext cache. seems like that logic can be moved into SharedContextState.
i will try to put a new CL to that to see how it goes or if there are any issues with doing that.
will rebase this CL based on that.
File components/viz/service/gl/gpu_service_impl.cc:
Patch Set #26, Line 842:
DCHECK(main_runner_->BelongsToCurrentThread());Why all DCHECKs are gone? Can this be now called on different thread? Do we need locks?
added back. thanks
File components/viz/service/main/viz_main_impl.cc:
Patch Set #26, Line 212:
LOG(ERROR) << "vikas: dr dc thread initialized";nit: remove
Done
Patch Set #26, Line 249:
dr_dc_thread->task_runner()->PostTask(
ah yes sorry. makes sense. […]
Done
Patch Set #26, Line 329:
scoped_refptr<gpu::SharedContextState> VizMainImpl::GetSharedContextState() {
This (and GetShareGroup) are GLRenderer specific, you could DCHECK that we're not using DrDC, so it […]
This method is called from https://source.chromium.org/chromium/chromium/src/+/main:gpu/ipc/gpu_in_process_thread_service.cc;drc=820892f6ee7d4405cebda668fbddfdaa18bbaaf9;l=96
which in turn is called from SkiaOutputSurfaceImplOnGpu::Create()It needs to return dr-dc shared context.
File gpu/config/gpu_finch_features.cc:
Patch Set #26, Line 285:
return false;nit: Is this reachable?
sorry it was old code. removed now.
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Vasiliy Telezhnikov (Gerrit)
Attention is currently required from: Sunny Sachanandani, vikas soni.
5 comments:
File components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc:
Patch Set #26, Line 112:
gpu_task_executor_->ScheduleOutOfOrderTask(
using task_runner post task instead of ScheduleOutOfOrderTask/ScheduleDelayedWork since we want to b […]
Thanks!
Patch Set #26, Line 130:
gpu_task_executor_->ScheduleGrContextCleanup();
GrCacheController is like a helper class which has logic for clearing the GrContext cache. […]
My idea was we could create GrCacheController inside SharedContextState, not move the logic itself to not make SharedContextState more complicated, but either could work.
File components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc:
Patch Set #27, Line 37:
return gpu_task_executor_->CreateSequence();nit: Could we just create sequence with task runner here? And not use gpu_task_executor_ anymore.
File components/viz/service/main/viz_main_impl.cc:
Patch Set #26, Line 329:
scoped_refptr<gpu::SharedContextState> VizMainImpl::GetSharedContextState() {This method is called from https://source.chromium. […]
Maybe I miss something, but from code search I see it's called from: https://source.chromium.org/chromium/chromium/src/+/main:gpu/ipc/display_compositor_memory_and_task_controller_on_gpu.cc;drc=d17745f350d4956a07cb1113ee19e9cbc4be699f;l=56
Which is used only with in process command buffer.
File components/viz/service/main/viz_main_impl.cc:
Patch Set #27, Line 286:
task_executor_ = std::make_unique<gpu::GpuInProcessThreadService>(Just a thought, maybe not the scope of this CL:
Provided we don't want to use task_executor for SkiaRenderer (it currently used for CreateSequence which we don't need any more and can create sequence with task runner and for ScheduleGrContextCleanup), should we just leave everything as is in this file?Or even don't create it here with SkiaRenderer, although that a bit more complicated.
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, Sunny Sachanandani.
Patch set 28:Commit-Queue +1
4 comments:
File components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc:
Patch Set #26, Line 130:
gpu_task_executor_->ScheduleGrContextCleanup();My idea was we could create GrCacheController inside SharedContextState, not move the logic itself t […]
oh ya ok that sounds great. working on that in a new CL.
File components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc:
Patch Set #27, Line 37:
return gpu_task_executor_->CreateSequence();nit: Could we just create sequence with task runner here? And not use gpu_task_executor_ anymore.
Done. thanks. probably i should have also updated it in my last CL where we added task runner with create sequence.
File components/viz/service/main/viz_main_impl.cc:
Patch Set #26, Line 329:
scoped_refptr<gpu::SharedContextState> VizMainImpl::GetSharedContextState() {
Maybe I miss something, but from code search I see it's called from: https://source.chromium. […]
sorry just added stacktrace to check and i agree with you.
File components/viz/service/main/viz_main_impl.cc:
Patch Set #27, Line 286:
task_executor_ = std::make_unique<gpu::GpuInProcessThreadService>(
Just a thought, maybe not the scope of this CL: […]
that makes sense since we now no longer rely on task_executor.
updated code now.
Looking into not creating task_executor in a new CL.
thanks!
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, vikas soni.
Patch set 28:Code-Review +1
4 comments:
Patchset:
lgtm % nits
File components/viz/service/display_embedder/compositor_gpu_thread.cc:
Patch Set #28, Line 148:
initialized_ = true;nit: move to Initialize() after the event wait
File components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc:
base::BindOnce(
[](scoped_refptr<base::SingleThreadTaskRunner> task_runner,
gl::GLSurface* surface) {
task_runner->PostTask(
FROM_HERE,
base::BindOnce(&gl::GLSurface::Release, base::Unretained(surface)));
},
std::move(task_runner), base::Unretained(surface));nit: while you're at it, can you replace this with base::BindPostTask?
auto task_runner =
gpu_service_impl_->compositor_gpu_thread()
? gpu_service_impl_->compositor_gpu_thread()->task_runner()
: gpu_service_impl_->main_runner();nit: maybe we should factor this into a gpu_task_runner() method since it's repeated in many places?
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, Sunny Sachanandani.
Patch set 29:Commit-Queue +1
6 comments:
File components/viz/service/display_embedder/compositor_gpu_thread.cc:
Patch Set #28, Line 148:
initialized_ = true;nit: move to Initialize() after the event wait
moving it to Initialize() after the event wait will always set it to true.
currently we do not set it to true always since we could return in middle due to some failures like in line 130,137.
File components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc:
Patch Set #26, Line 130:
gpu_task_executor_->ScheduleGrContextCleanup();
oh ya ok that sounds great. working on that in a new CL.
this is done now.
File components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc:
base::BindOnce(
[](scoped_refptr<base::SingleThreadTaskRunner> task_runner,
gl::GLSurface* surface) {
task_runner->PostTask(
FROM_HERE,
base::BindOnce(&gl::GLSurface::Release, base::Unretained(surface)));
},
std::move(task_runner), base::Unretained(surface));nit: while you're at it, can you replace this with base::BindPostTask?
sounds nicer.
auto task_runner =
gpu_service_impl_->compositor_gpu_thread()
? gpu_service_impl_->compositor_gpu_thread()->task_runner()
: gpu_service_impl_->main_runner();nit: maybe we should factor this into a gpu_task_runner() method since it's repeated in many places?
Done
File components/viz/service/main/viz_main_impl.cc:
Patch Set #26, Line 329:
scoped_refptr<gpu::SharedContextState> VizMainImpl::GetSharedContextState() {
sorry just added stacktrace to check and i agree with you.
Done
File gpu/config/gpu_finch_features.cc:
Patch Set #26, Line 190:
const base::Feature kEnableDrDc{"EnableDrDc", base::FEATURE_ENABLED_BY_DEFAULT};
Just leaving a comment so we won't forget to change this to disabled before landing.
Done
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Vasiliy Telezhnikov (Gerrit)
Attention is currently required from: Sunny Sachanandani, vikas soni.
11 comments:
File cc/resources/ui_resource_bitmap.cc:
Patch Set #30, Line 77:
#if defined(OS_ANDROID)Is is needed only for DrDC? Could we check feature here?
File components/viz/service/display_embedder/compositor_gpu_thread.h:
Patch Set #30, Line 34:
gpu::MailboxManager* mailbox_manager() const {nit: Is this used?
Patch Set #30, Line 66:
CompositorGpuThread(const CompositorGpuThread&) = delete;deleted member function should be public (https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html)
Please fix.
Patch Set #30, Line 67:
CompositorGpuThread& operator=(const CompositorGpuThread&) = delete;deleted member function should be public (https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html)
Please fix.
File components/viz/service/display_embedder/compositor_gpu_thread.cc:
Patch Set #28, Line 148:
initialized_ = true;nit: move to Initialize() after the event wait
Now that this isn't singleton and Initialize() is private we don't really need this to be in class? We can make InitializeOnThread return value or have bool& success arg.
nit: mistype
File components/viz/service/display_embedder/compositor_gpu_thread.cc:
Patch Set #30, Line 104:
auto context_lost_callback = base::BindOnce(nit: We could use BindPostTask here too.
Patch Set #30, Line 172:
gpu_channel_managerAs this is weakptr, do we need to check that it's not null before call?
File components/viz/service/main/viz_main_impl.cc:
Patch Set #27, Line 286:
task_executor_ = std::make_unique<gpu::GpuInProcessThreadService>(
that makes sense since we now no longer rely on task_executor. […]
Thanks!
File content/gpu/gpu_main.cc:
Patch Set #30, Line 309:
if (base::FeatureList::IsEnabled(features::kGpuUseDisplayThreadPriority) &&Just curious, do we expect that gpu main now doesn't need to be in high priority? i.e it still does most of the heavy load for drawing.
File gpu/ipc/service/gpu_channel_manager.cc:
Patch Set #30, Line 324:
auto weak_this = weak_factory_.GetWeakPtr();I don't think this does anything. According to docs, DCHECK(weak_this) would bind factory to the thread, but when it's destroyed (i.e after we will leave the function) it will be unbound:
What is the intent here? Why do we want to bind it here, to get better DCHECKs?
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, vikas soni.
Patch set 30:Code-Review +1
1 comment:
File components/viz/service/display_embedder/compositor_gpu_thread.cc:
Patch Set #30, Line 172:
gpu_channel_manager
As this is weakptr, do we need to check that it's not null before call?
Nice catch! Even though this looks like a method call it's not, and the behavior of Bind is different for regular functions[1] vs methods[2] when the first argument is a weak ptr - the latter checks the weak ptr before invoking the method. Here a manual null check is required.
[1] https://source.chromium.org/chromium/chromium/src/+/master:base/bind_internal.h;l=410
[2] https://source.chromium.org/chromium/chromium/src/+/master:base/bind_internal.h;l=498
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, vikas soni.
1 comment:
File gpu/ipc/service/gpu_channel_manager.cc:
Patch Set #30, Line 324:
auto weak_this = weak_factory_.GetWeakPtr();
I don't think this does anything. […]
We should really put this in a member var. There are two requirements here:
1) We want to bind the weak ptr to the main thread at all times, and guard against accidental first dereference on another thread (there's a thread checker in the weak ptr implementation somewhere I believe).
2) We want to be able to get the weak ptr from another thread (dr dc thread) even though it's not bound there.
I think the assumption here (which I probably made during the prototype - or maybe this was just a temporary debugging thing - don't remember) was that the weak ptr factory would keep around the weak reference indefinitely, but that's not the case.
Making the weak ptr a member var should solve these issues.
See https://chromium-review.googlesource.com/c/chromium/src/+/1352858 and crbug.com/908669 for a bug where a weak ptr was accidentally dereferenced on another thread.
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Vasiliy Telezhnikov (Gerrit)
Attention is currently required from: Sunny Sachanandani, vikas soni.
1 comment:
File gpu/ipc/service/gpu_channel_manager.cc:
Patch Set #30, Line 324:
auto weak_this = weak_factory_.GetWeakPtr();
We should really put this in a member var. There are two requirements here: […]
Just to make sure I don't miss anything:
1) This only changes where DCHECK will be triggered, right? Without it we might get error when dereferencing on main thread, while we'd really want to see where it happend on other thread?
2) This two lines don't affect it, right? You still can get GetWeakPtr anywhere, you just need to invalidate/dereference on the gpu main thread?
Maybe we should just not expose GetWeakPtr() at all to make it less subtle?
For context lost we could create that callback in GpuChannelManager and make sure it's bound on the right sequence.For memory tracking we could do the same or have thread-safe OnMemoryAllocatedChange() as we already rely that GpuChannelManager outlives CompositorGpuThread. By thread-safe here I mean it could post task to main if needed, not that we will lock.
What do you think?
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, vikas soni.
1 comment:
File gpu/ipc/service/gpu_channel_manager.cc:
Patch Set #30, Line 324:
auto weak_this = weak_factory_.GetWeakPtr();
1) This only changes where DCHECK will be triggered, right? Without it we might get error when dereferencing on main thread, while we'd really want to see where it happend on other thread?
In general we would have a potential data race on two threads calling GetWeakPtr(), but here that's not possible since the GPU main thread waits for DrDC thread to initialize (where GetWeakPtr is called).
2) This two lines don't affect it, right? You still can get GetWeakPtr anywhere, you just need to invalidate/dereference on the gpu main thread?
I think the core issue is that calling GetWeakPtr() itself is not thread safe even though passing the weak ptr around is (as long as you dereference on one thread).
Maybe we should just not expose GetWeakPtr() at all to make it less subtle?
For context lost we could create that callback in GpuChannelManager and make sure it's bound on the right sequence.For memory tracking we could do the same or have thread-safe OnMemoryAllocatedChange() as we already rely that GpuChannelManager outlives CompositorGpuThread. By thread-safe here I mean it could post task to main if needed, not that we will lock.
+1 to both of these - let's not expose GetWeakPtr()
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov.
Patch set 31:Commit-Queue +1
11 comments:
Patchset:
thanks for great inputs!
File cc/resources/ui_resource_bitmap.cc:
Patch Set #30, Line 77:
#if defined(OS_ANDROID)Is is needed only for DrDC? Could we check feature here?
Done
File components/viz/service/display_embedder/compositor_gpu_thread.h:
Patch Set #30, Line 34:
gpu::MailboxManager* mailbox_manager() const {nit: Is this used?
removed now.
Patch Set #30, Line 66:
CompositorGpuThread(const CompositorGpuThread&) = delete;
> deleted member function should be public (https://clang.llvm. […]
Done
Patch Set #30, Line 67:
CompositorGpuThread& operator=(const CompositorGpuThread&) = delete;
> deleted member function should be public (https://clang.llvm. […]
Done
File components/viz/service/display_embedder/compositor_gpu_thread.cc:
Patch Set #28, Line 148:
initialized_ = true;
Now that this isn't singleton and Initialize() is private we don't really need this to be in class? […]
makes sense. thanks
nit: mistype
Done
File components/viz/service/display_embedder/compositor_gpu_thread.cc:
Patch Set #30, Line 104:
auto context_lost_callback = base::BindOnce(
nit: We could use BindPostTask here too.
Done
Patch Set #30, Line 172:
gpu_channel_manager
Nice catch! Even though this looks like a method call it's not, and the behavior of Bind is differe […]
thanks for info.
File content/gpu/gpu_main.cc:
Patch Set #30, Line 309:
if (base::FeatureList::IsEnabled(features::kGpuUseDisplayThreadPriority) &&
Just curious, do we expect that gpu main now doesn't need to be in high priority? i. […]
so base::ThreadPriority::DISPLAY is supposed to be set for the thread which is used for display. now since gpu main is not used for display in drdc, i believe we need to not set it.
if base::ThreadPriority::DISPLAY essentially just means higher priority then probably we should set it for both threads for now and then maybe do some benchmarking later to see which is better.
i will revert this for now.
File gpu/ipc/service/gpu_channel_manager.cc:
Patch Set #30, Line 324:
auto weak_this = weak_factory_.GetWeakPtr();
> 1) This only changes where DCHECK will be triggered, right? Without it we might get error when der […]
Done
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Vasiliy Telezhnikov (Gerrit)
Attention is currently required from: vikas soni.
Patch set 32:Code-Review +1
4 comments:
Patchset:
lgtm % comments, thanks for working on it!
File components/viz/service/display_embedder/compositor_gpu_thread.h:
Patch Set #32, Line 10:
#include "base/no_destructor.h"nit: not used if I don't miss anything
File components/viz/service/display_embedder/compositor_gpu_thread.cc:
Patch Set #32, Line 15:
#include "gpu/command_buffer/service/mailbox_manager_dummy.h"nit: not used
File gpu/command_buffer/service/memory_program_cache.cc:
Patch Set #32, Line 331:
if (activity_flags_) {Sorry, missed this in previous review. I'm a bit confused, I can understand if we don't want to create ScopedSetFlag for some reason, but is not calling glProgramBinary expected?
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, vikas soni.
Patch set 32:Code-Review +1
1 comment:
File gpu/command_buffer/service/memory_program_cache.cc:
Patch Set #32, Line 331:
if (activity_flags_) {Sorry, missed this in previous review. […]
I suspect this is a left over from the time we implemented the prototype for GLRenderer - we had to create the program cache on the DrDC thread without activity_flags_.
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, vikas soni.
vikas soni uploaded patch set #33 to this change.
[Dr-Dc] Direct Rendering Display Compositor gpu thread.
1. Add support for a new gpu thread to be used by display compositor.
This new gpu thread is known as dr-dc aka direct rendering display
compositor. A new shared context will be current on this thread.
2. Add finch feature flags to enable it only on android P devices via
finch rollout. It will be currently disabled for all other platforms
and is currently disabled by default on android P also.
3. Viz changes to use this new thread when enabled.
Bug: 1186275
Change-Id: I10f72e331f9273b35e90372c291de1c41a906e28
---
M android_webview/lib/aw_main_delegate.cc
M cc/resources/ui_resource_bitmap.cc
M cc/test/layer_tree_pixel_test.cc
M cc/test/pixel_test.cc
M components/viz/service/BUILD.gn
M components/viz/service/DEPS
M components/viz/service/display/renderer_perftest.cc
A components/viz/service/display_embedder/compositor_gpu_thread.cc
A components/viz/service/display_embedder/compositor_gpu_thread.h
M components/viz/service/display_embedder/output_surface_provider_impl.cc
M components/viz/service/display_embedder/skia_output_device_buffer_queue_unittest.cc
M components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc
M components/viz/service/display_embedder/skia_output_surface_dependency_impl.h
M components/viz/service/display_embedder/skia_output_surface_impl_unittest.cc
M components/viz/service/gl/gpu_service_impl.cc
M components/viz/service/gl/gpu_service_impl.h
M components/viz/service/main/viz_main_impl.cc
M gpu/command_buffer/service/gr_shader_cache.h
M gpu/command_buffer/service/memory_program_cache.cc
M gpu/config/gpu_finch_features.cc
M gpu/config/gpu_finch_features.h
M gpu/ipc/in_process_command_buffer.cc
M gpu/ipc/service/gpu_channel_manager.cc
M gpu/ipc/service/gpu_channel_manager.h
M ui/compositor/test/in_process_context_factory.cc
M ui/gl/gl_surface_egl.cc
26 files changed, 379 insertions(+), 52 deletions(-)
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov.
Patch set 35:Commit-Queue +2
3 comments:
File components/viz/service/display_embedder/compositor_gpu_thread.h:
Patch Set #32, Line 10:
#include "base/no_destructor.h"nit: not used if I don't miss anything
Done
File components/viz/service/display_embedder/compositor_gpu_thread.cc:
Patch Set #32, Line 15:
#include "gpu/command_buffer/service/mailbox_manager_dummy.h"nit: not used
Done
File gpu/command_buffer/service/memory_program_cache.cc:
Patch Set #32, Line 331:
if (activity_flags_) {
I suspect this is a left over from the time we implemented the prototype for GLRenderer - we had to […]
reverted the change since its not needed for skia renderer as we do not create any program cache in compositor gpu thread.
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov.
CL chromium-review.googlesource.com/2901026 must be approved before triggering CQ
Code Owners
vikas soni (Gerrit)
Attention is currently required from: Bo, Vasiliy Telezhnikov, kylechar.
vikas soni would like Bo and kylechar to review this change.
M gpu/config/gpu_finch_features.cc
M gpu/config/gpu_finch_features.h
M gpu/ipc/in_process_command_buffer.cc
M gpu/ipc/service/gpu_channel_manager.cc
M gpu/ipc/service/gpu_channel_manager.h
M ui/compositor/test/in_process_context_factory.cc
M ui/gl/gl_surface_egl.cc
25 files changed, 376 insertions(+), 51 deletions(-)
vikas soni (Gerrit)
Attention is currently required from: Bo, Vasiliy Telezhnikov, kylechar.
Patch set 35:Commit-Queue +1
1 comment:
Patchset:
adding bo for aw_main* and kyle for in_process_context_factory.cc.
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
Bo (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, vikas soni, kylechar.
Patch set 35:Code-Review +1
kylechar (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, vikas soni.
Patch set 35:Code-Review +1
To view, visit change 2901026. To unsubscribe, or for help writing mail filters, visit settings.
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov, vikas soni.
Patch set 35:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov.
Failed builds:
luci.chromium.try/dawn-mac-x64-deps-rel JOB_FAILED https://cr-buildbucket.appspot.com/build/8844438525628302880
Step('test_pre_run (retry shards with patch).missing failed shards') (retcode: 1)
luci.chromium.try/dawn-mac-x64-deps-rel JOB_FAILED https://cr-buildbucket.appspot.com/build/8844436824110626976
Step('test_pre_run (retry shards with patch).missing failed shards') (retcode: 1)
vikas soni (Gerrit)
Attention is currently required from: Vasiliy Telezhnikov.
Patch set 35:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[Dr-Dc] Direct Rendering Display Compositor gpu thread.
1. Add support for a new gpu thread to be used by display compositor.
This new gpu thread is known as dr-dc aka direct rendering display
compositor. A new shared context will be current on this thread.
2. Add finch feature flags to enable it only on android P devices via
finch rollout. It will be currently disabled for all other platforms
and is currently disabled by default on android P also.
3. Viz changes to use this new thread when enabled.
Bug: 1186275
Change-Id: I10f72e331f9273b35e90372c291de1c41a906e28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2901026
Reviewed-by: Bo <bo...@chromium.org>
Reviewed-by: kylechar <kyle...@chromium.org>
Reviewed-by: Vasiliy Telezhnikov <vas...@chromium.org>
Reviewed-by: Sunny Sachanandani <sun...@chromium.org>
Commit-Queue: vikas soni <vika...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#892199}