Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2878573002:
gpu: guarantee to match pending GBM buffer request to actual GBM handle (Closed)
Descriptiongpu: guarantee to match pending GBM buffer request to actual GBM handle
This CL makes |gpu_proecess_host| select the right callback by GMB id, not
brute-force FIFO. It becomes more stable code for the future change.
e.g. reordering IPC for priotization
BUG=475633, 629521
Review-Url: https://codereview.chromium.org/2878573002
Cr-Commit-Position: refs/heads/master@{#471116}
Committed: https://chromium.googlesource.com/chromium/src/+/e17baa4e12ee81c41668d7ef07e08a1a8e096dc6
Patch Set 1 #
Total comments: 4
Patch Set 2 : resolved nits #Patch Set 3 : errata: GBM -> GpuMemoryBuffer #
Total comments: 2
Messages
Total messages: 36 (20 generated)
The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run
Description was changed from ========== gpu: Fix dma-buf mmap failure It's possible not to match pending GBM buffer request to actual GBM handle transfered from GPU process. It's because |gpu_proecess_host| has wrong expectation of the request order. It expects the handle arrive to client process by FIFO order, but somehow it's not always true. This CL makes |gpu_proecess_host| select the right callback by GMB id, not brute-force FIFO. BUG=629521 ========== to ========== gpu: Fix dma-buf mmap failure It's possible not to match pending GBM buffer request to actual GBM handle transfered from GPU process. It's because |gpu_proecess_host| has wrong expectation of the request order. It expects the handle arrive to client process by FIFO order, but somehow it's not always true. This CL makes |gpu_proecess_host| select the right callback by GMB id, not brute-force FIFO. BUG=629521 ==========
dongseong.hwang@intel.com changed reviewers: + dcastagna@chromium.org, reveman@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Daniele, Reveman, could you review? I think it fix flaky mmap failure, according to Daniele's crash report. https://bugs.chromium.org/p/chromium/issues/detail?id=629521#c149 Thank you Daniele for sharing all information.
On 2017/05/10 at 20:48:30, dongseong.hwang wrote: > Daniele, Reveman, could you review? > I think it fix flaky mmap failure, according to Daniele's crash report. https://bugs.chromium.org/p/chromium/issues/detail?id=629521#c149 > Thank you Daniele for sharing all information. Unfortunately, I don't think this is fixing the issue. As explained in https://bugs.chromium.org/p/chromium/issues/detail?id=629521#c149, responses from the GPU process seem to always match the correct pending requests. In the logs we print out both the size contained in the request and the buffer size contained in the GPU memory buffer handle received from the GPU process. These two values are always correctly matching. The non matching value is the size of the underlying bo.
On 2017/05/10 20:54:55, Daniele Castagna wrote: > On 2017/05/10 at 20:48:30, dongseong.hwang wrote: > > Daniele, Reveman, could you review? > > I think it fix flaky mmap failure, according to Daniele's crash report. > https://bugs.chromium.org/p/chromium/issues/detail?id=629521#c149 > > Thank you Daniele for sharing all information. > > Unfortunately, I don't think this is fixing the issue. > As explained in > https://bugs.chromium.org/p/chromium/issues/detail?id=629521#c149, responses > from the GPU process seem to always match the correct pending requests. In the > logs we print out both the size contained in the request and the buffer size > contained in the GPU memory buffer handle received from the GPU process. These > two values are always correctly matching. The non matching value is the size of > the underlying bo. Thanks for review. Do you mention about buffer size from ClientNativePixmapDmaBuf constructor? https://cs.chromium.org/chromium/src/ui/gfx/linux/client_native_pixmap_dmabuf... It's actually from request. https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b... From ClientNativePixmapDmaBuf constructor perspective, only NativePixmapHandle handle is actually from GPU process. I guess NativePixmapHandle has the size as small as the size of the underlying bo. You added LOG(ERROR), not ScopedCrashKey about it. Could you check the LOG(ERROR) from crash report?
On 2017/05/10 at 21:08:17, dongseong.hwang wrote: > On 2017/05/10 20:54:55, Daniele Castagna wrote: > > On 2017/05/10 at 20:48:30, dongseong.hwang wrote: > > > Daniele, Reveman, could you review? > > > I think it fix flaky mmap failure, according to Daniele's crash report. > > https://bugs.chromium.org/p/chromium/issues/detail?id=629521#c149 > > > Thank you Daniele for sharing all information. > > > > Unfortunately, I don't think this is fixing the issue. > > As explained in > > https://bugs.chromium.org/p/chromium/issues/detail?id=629521#c149, responses > > from the GPU process seem to always match the correct pending requests. In the > > logs we print out both the size contained in the request and the buffer size > > contained in the GPU memory buffer handle received from the GPU process. These > > two values are always correctly matching. The non matching value is the size of > > the underlying bo. > > Thanks for review. Do you mention about buffer size from ClientNativePixmapDmaBuf constructor? > https://cs.chromium.org/chromium/src/ui/gfx/linux/client_native_pixmap_dmabuf... > > It's actually from request. > https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b... > > From ClientNativePixmapDmaBuf constructor perspective, only NativePixmapHandle handle is actually from GPU process. I guess NativePixmapHandle has the size as small as the size of the underlying bo. You added LOG(ERROR), not ScopedCrashKey about it. Could you check the LOG(ERROR) from crash report? We print out buffer size from ClientNativePixmapDmaBuf and the size parameter we pass to mmap, that is from NativePixmapHandle. Those two sizes match. NativePixmapHandle doesn't match the bo size, otherwise mmap wouldn't be failing....
On 2017/05/10 21:12:16, Daniele Castagna wrote: > On 2017/05/10 at 21:08:17, dongseong.hwang wrote: > > On 2017/05/10 20:54:55, Daniele Castagna wrote: > > > On 2017/05/10 at 20:48:30, dongseong.hwang wrote: > > > > Daniele, Reveman, could you review? > > > > I think it fix flaky mmap failure, according to Daniele's crash report. > > > https://bugs.chromium.org/p/chromium/issues/detail?id=629521#c149 > > > > Thank you Daniele for sharing all information. > > > > > > Unfortunately, I don't think this is fixing the issue. > > > As explained in > > > https://bugs.chromium.org/p/chromium/issues/detail?id=629521#c149, responses > > > from the GPU process seem to always match the correct pending requests. In > the > > > logs we print out both the size contained in the request and the buffer size > > > contained in the GPU memory buffer handle received from the GPU process. > These > > > two values are always correctly matching. The non matching value is the size > of > > > the underlying bo. > > > > Thanks for review. Do you mention about buffer size from > ClientNativePixmapDmaBuf constructor? > > > https://cs.chromium.org/chromium/src/ui/gfx/linux/client_native_pixmap_dmabuf... > > > > It's actually from request. > > > https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b... > > > > From ClientNativePixmapDmaBuf constructor perspective, only NativePixmapHandle > handle is actually from GPU process. I guess NativePixmapHandle has the size as > small as the size of the underlying bo. You added LOG(ERROR), not ScopedCrashKey > about it. Could you check the LOG(ERROR) from crash report? > > We print out buffer size from ClientNativePixmapDmaBuf and the size parameter we > pass to mmap, that is from NativePixmapHandle. Those two sizes match. > NativePixmapHandle doesn't match the bo size, otherwise mmap wouldn't be > failing.... Ah, buffer size which is from request matches to the size parameter, which from handle. I see. only the underlying bo has different size. Thank you for explaining. I've carefully read minigbm and gpu/drm/i915 code but cannot understand how it happens.. If minigbm cannot allocate the size, it must return nullptr, rather than returning smaller bo.
Description was changed from ========== gpu: Fix dma-buf mmap failure It's possible not to match pending GBM buffer request to actual GBM handle transfered from GPU process. It's because |gpu_proecess_host| has wrong expectation of the request order. It expects the handle arrive to client process by FIFO order, but somehow it's not always true. This CL makes |gpu_proecess_host| select the right callback by GMB id, not brute-force FIFO. BUG=629521 ========== to ========== gpu: guarantee to match pending GBM buffer request to actual GBM handle This CL makes |gpu_proecess_host| select the right callback by GMB id, not brute-force FIFO. It becomes more stable code for the future change. e.g. reordering IPC for priotization BUG=629521 ==========
dongseong.hwang@intel.com changed reviewers: + piman@chromium.org - dcastagna@chromium.org, dongseong.hwang@chromium.org, reveman@chromium.org
piman@, could you review? It's by-product of effort fixing BUG=629521 I suspected request and handle dismatch causes the BUG=629521, but it's not true. BTW, do you think it's good to land anyway? In my opinion, it's more solid code.
Description was changed from ========== gpu: guarantee to match pending GBM buffer request to actual GBM handle This CL makes |gpu_proecess_host| select the right callback by GMB id, not brute-force FIFO. It becomes more stable code for the future change. e.g. reordering IPC for priotization BUG=629521 ========== to ========== gpu: guarantee to match pending GBM buffer request to actual GBM handle This CL makes |gpu_proecess_host| select the right callback by GMB id, not brute-force FIFO. It becomes more stable code for the future change. e.g. reordering IPC for priotization BUG=475633 ==========
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_TIMED_OUT, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2878573002/diff/1/content/browser/gpu/gpu_pro... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2878573002/diff/1/content/browser/gpu/gpu_pro... content/browser/gpu/gpu_process_host.cc:798: create_gpu_memory_buffer_requests_.end()); Actually, because we shouldn't trust the GPU process, we should avoid those DCHECKs and instead ignore the message if the id isn't in the map. Note that the condition checked on l.796 is redundant (if the map is empty, this will fail anyway). https://codereview.chromium.org/2878573002/diff/1/content/browser/gpu/gpu_pro... File content/browser/gpu/gpu_process_host.h (right): https://codereview.chromium.org/2878573002/diff/1/content/browser/gpu/gpu_pro... content/browser/gpu/gpu_process_host.h:234: std::map<gfx::GpuMemoryBufferId, CreateGpuMemoryBufferCallback> nit: use base::flat_map since this likely has only few entries
The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run
thx for reviewing. could you take a look again? https://codereview.chromium.org/2878573002/diff/1/content/browser/gpu/gpu_pro... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2878573002/diff/1/content/browser/gpu/gpu_pro... content/browser/gpu/gpu_process_host.cc:798: create_gpu_memory_buffer_requests_.end()); On 2017/05/11 18:04:43, piman wrote: > Actually, because we shouldn't trust the GPU process, we should avoid those > DCHECKs and instead ignore the message if the id isn't in the map. > > Note that the condition checked on l.796 is redundant (if the map is empty, this > will fail anyway). Done. https://codereview.chromium.org/2878573002/diff/1/content/browser/gpu/gpu_pro... File content/browser/gpu/gpu_process_host.h (right): https://codereview.chromium.org/2878573002/diff/1/content/browser/gpu/gpu_pro... content/browser/gpu/gpu_process_host.h:234: std::map<gfx::GpuMemoryBufferId, CreateGpuMemoryBufferCallback> On 2017/05/11 18:04:43, piman wrote: > nit: use base::flat_map since this likely has only few entries Done.
Description was changed from ========== gpu: guarantee to match pending GBM buffer request to actual GBM handle This CL makes |gpu_proecess_host| select the right callback by GMB id, not brute-force FIFO. It becomes more stable code for the future change. e.g. reordering IPC for priotization BUG=475633 ========== to ========== gpu: guarantee to match pending GBM buffer request to actual GBM handle This CL makes |gpu_proecess_host| select the right callback by GMB id, not brute-force FIFO. It becomes more stable code for the future change. e.g. reordering IPC for priotization BUG=475633, 629521 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by dongseong.hwang@intel.com
The CQ bit was checked by dongseong.hwang@intel.com
The CQ bit was unchecked by dongseong.hwang@intel.com
The CQ bit was checked by dongseong.hwang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/2878573002/#ps40001 (title: "errata: GBM -> GpuMemoryBuffer")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494535671536300,
"parent_rev": "bfb116be5c0899dc76e114b04d0954248a1380d2", "commit_rev":
"e17baa4e12ee81c41668d7ef07e08a1a8e096dc6"}
Message was sent while issue was closed.
Description was changed from ========== gpu: guarantee to match pending GBM buffer request to actual GBM handle This CL makes |gpu_proecess_host| select the right callback by GMB id, not brute-force FIFO. It becomes more stable code for the future change. e.g. reordering IPC for priotization BUG=475633, 629521 ========== to ========== gpu: guarantee to match pending GBM buffer request to actual GBM handle This CL makes |gpu_proecess_host| select the right callback by GMB id, not brute-force FIFO. It becomes more stable code for the future change. e.g. reordering IPC for priotization BUG=475633, 629521 Review-Url: https://codereview.chromium.org/2878573002 Cr-Commit-Position: refs/heads/master@{#471116} Committed: https://chromium.googlesource.com/chromium/src/+/e17baa4e12ee81c41668d7ef07e0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e17baa4e12ee81c41668d7ef07e0...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2879723003/ by jochen@chromium.org. The reason for reverting is: DCHECK triggers on tests in GpuProcessHost::CreateGpuMemoryBuffer.
Message was sent while issue was closed.
On 2017/05/11 at 01:24:58, dongseong.hwang wrote: > dongseong.hwang@intel.com changed reviewers: > + piman@chromium.org > - dcastagna@chromium.org, dongseong.hwang@chromium.org, reveman@chromium.org I don't think it's appropriate to remove reviewers after they've provided valuable feedback that questions the usefulness of a patch. Please resolve concerns with existing reviewers before asking for owner review and landing patches in the future.
Message was sent while issue was closed.
On 2017/05/12 16:37:02, reveman wrote: > On 2017/05/11 at 01:24:58, dongseong.hwang wrote: > > mailto:dongseong.hwang@intel.com changed reviewers: > > + mailto:piman@chromium.org > > - mailto:dcastagna@chromium.org, mailto:dongseong.hwang@chromium.org, mailto:reveman@chromium.org > > I don't think it's appropriate to remove reviewers after they've provided > valuable feedback that questions the usefulness of a patch. Please resolve > concerns with existing reviewers before asking for owner review and landing > patches in the future. I see. Sorry for your concern. I added you and daniele because I thought this CL fix mmap failure, but it is not true. So I thought it's GPU IPC CL and maybe noise to you so remove CCs. I'll not remove cc at next time.
Message was sent while issue was closed.
dcastagna@chromium.org changed reviewers: + dcastagna@chromium.org, reveman@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2878573002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2878573002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_process_host.cc:739: DCHECK(create_gpu_memory_buffer_requests_.find(id) == DS, do you know if this is the DCHECK that was going off?
Message was sent while issue was closed.
https://codereview.chromium.org/2878573002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2878573002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_process_host.cc:739: DCHECK(create_gpu_memory_buffer_requests_.find(id) == On 2017/05/12 19:19:15, Daniele Castagna wrote: > DS, do you know if this is the DCHECK that was going off? I'm surprising the DCHECK fails. I tested just CHECK with same condition on samus and it works well. I think we should figure out why it's failed. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
