Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1955803002:
Added Validation to Vulkan CommandBuffer API's. (Closed)
DescriptionThis adds validation to Begin/End CommandBuffer API's and
avoids resetting record_type if vkResetCommanBuffer fails.
BUG=582558
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/d2b4bdaa4d479e18aae4a1242ae297f2ce4477f2
Cr-Commit-Position: refs/heads/master@{#394072}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Moved declaration to initialization #Messages
Total messages: 17 (8 generated)
Description was changed from ========== --amend Added Validation to Vulkan CommandBuffer API's. --amend This adds validation to Begin/End CommandBuffer API's and avoids resetting record_type if vkResetCommanBuffer fails. ========== to ========== --amend Added Validation to Vulkan CommandBuffer API's. --amend This adds validation to Begin/End CommandBuffer API's and avoids resetting record_type if vkResetCommanBuffer fails. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== --amend Added Validation to Vulkan CommandBuffer API's. --amend This adds validation to Begin/End CommandBuffer API's and avoids resetting record_type if vkResetCommanBuffer fails. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== This adds validation to Begin/End CommandBuffer API's and avoids resetting record_type if vkResetCommanBuffer fails. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
md.sami@samsung.com changed reviewers: + sohan.jyoti@samsung.com
On 2016/05/06 08:25:04, ssami wrote: > mailto:md.sami@samsung.com changed reviewers: > + mailto:sohan.jyoti@samsung.com Can you please add a relevant bug id for it. I think either 582558 or 582554 should be ok. That will help track the patch better. Thanks.
Description was changed from ========== This adds validation to Begin/End CommandBuffer API's and avoids resetting record_type if vkResetCommanBuffer fails. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== This adds validation to Begin/End CommandBuffer API's and avoids resetting record_type if vkResetCommanBuffer fails. BUG=582558 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
On 2016/05/06 08:56:50, sohanjg wrote: > On 2016/05/06 08:25:04, ssami wrote: > > mailto:md.sami@samsung.com changed reviewers: > > + mailto:sohan.jyoti@samsung.com > > Can you please add a relevant bug id for it. I think either 582558 or 582554 > should be ok. > That will help track the patch better. Thanks. Ok, I added Bug-Id 582558.
On 2016/05/06 09:08:32, ssami wrote: > On 2016/05/06 08:56:50, sohanjg wrote: > > On 2016/05/06 08:25:04, ssami wrote: > > > mailto:md.sami@samsung.com changed reviewers: > > > + mailto:sohan.jyoti@samsung.com > > > > Can you please add a relevant bug id for it. I think either 582558 or 582554 > > should be ok. > > That will help track the patch better. Thanks. > > Ok, I added Bug-Id 582558. looks good. piman@, dyen@, can you please take a look. Sami started working on Vulkan from our side, he has signed CLA, and plans to support Vulkan work for Android.
sohan.jyoti@samsung.com changed reviewers: + dyen@chromium.org, piman@chromium.org
Some nits. This is reasonable as a first step, but we will need at some point to propagate the error if it is VK_ERROR_OUT_OF_DEVICE_MEMORY, and probably treat it as a lost context. VK_ERROR_OUT_OF_HOST_MEMORY is something that we'll need to hook via the allocator, and just make sure we abort safely (like we do for general malloc failure). https://codereview.chromium.org/1955803002/diff/1/gpu/vulkan/vulkan_command_b... File gpu/vulkan/vulkan_command_buffer.cc (right): https://codereview.chromium.org/1955803002/diff/1/gpu/vulkan/vulkan_command_b... gpu/vulkan/vulkan_command_buffer.cc:147: VkResult result = VK_SUCCESS; nit: move declaration to initialization (l. 153) https://codereview.chromium.org/1955803002/diff/1/gpu/vulkan/vulkan_command_b... gpu/vulkan/vulkan_command_buffer.cc:153: result = vkResetCommandBuffer(command_buffer_, 0); nit: VkResult result = vkResetCommandBuffer(command_buffer_, 0); https://codereview.chromium.org/1955803002/diff/1/gpu/vulkan/vulkan_command_b... gpu/vulkan/vulkan_command_buffer.cc:164: result = vkEndCommandBuffer(handle_); nit: VkResult result = vkEndCommandBuffer(handle_); https://codereview.chromium.org/1955803002/diff/1/gpu/vulkan/vulkan_command_b... gpu/vulkan/vulkan_command_buffer.cc:178: result = vkBeginCommandBuffer(handle_, &begin_info); nit: VkResult result = vkBeginCommandBuffer(handle_, &begin_info); https://codereview.chromium.org/1955803002/diff/1/gpu/vulkan/vulkan_command_b... gpu/vulkan/vulkan_command_buffer.cc:193: result = vkBeginCommandBuffer(handle_, &begin_info); nit: VkResult result = vkBeginCommandBuffer(handle_, &begin_info);
Please take a look. Do you want to propagate the errors as part of this patch or can we do it later? Thanks https://codereview.chromium.org/1955803002/diff/1/gpu/vulkan/vulkan_command_b... File gpu/vulkan/vulkan_command_buffer.cc (right): https://codereview.chromium.org/1955803002/diff/1/gpu/vulkan/vulkan_command_b... gpu/vulkan/vulkan_command_buffer.cc:147: VkResult result = VK_SUCCESS; On 2016/05/12 23:20:43, piman OOO back 2016-5-16 wrote: > nit: move declaration to initialization (l. 153) Done. https://codereview.chromium.org/1955803002/diff/1/gpu/vulkan/vulkan_command_b... gpu/vulkan/vulkan_command_buffer.cc:153: result = vkResetCommandBuffer(command_buffer_, 0); On 2016/05/12 23:20:43, piman OOO back 2016-5-16 wrote: > nit: VkResult result = vkResetCommandBuffer(command_buffer_, 0); Done. https://codereview.chromium.org/1955803002/diff/1/gpu/vulkan/vulkan_command_b... gpu/vulkan/vulkan_command_buffer.cc:164: result = vkEndCommandBuffer(handle_); On 2016/05/12 23:20:43, piman OOO back 2016-5-16 wrote: > nit: VkResult result = vkEndCommandBuffer(handle_); Done. https://codereview.chromium.org/1955803002/diff/1/gpu/vulkan/vulkan_command_b... gpu/vulkan/vulkan_command_buffer.cc:178: result = vkBeginCommandBuffer(handle_, &begin_info); On 2016/05/12 23:20:43, piman OOO back 2016-5-16 wrote: > nit: VkResult result = vkBeginCommandBuffer(handle_, &begin_info); Done. https://codereview.chromium.org/1955803002/diff/1/gpu/vulkan/vulkan_command_b... gpu/vulkan/vulkan_command_buffer.cc:193: result = vkBeginCommandBuffer(handle_, &begin_info); On 2016/05/12 23:20:43, piman OOO back 2016-5-16 wrote: > nit: VkResult result = vkBeginCommandBuffer(handle_, &begin_info); Done.
lgtm
The CQ bit was checked by md.sami@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955803002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955803002/20001
Message was sent while issue was closed.
Description was changed from ========== This adds validation to Begin/End CommandBuffer API's and avoids resetting record_type if vkResetCommanBuffer fails. BUG=582558 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== This adds validation to Begin/End CommandBuffer API's and avoids resetting record_type if vkResetCommanBuffer fails. BUG=582558 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== This adds validation to Begin/End CommandBuffer API's and avoids resetting record_type if vkResetCommanBuffer fails. BUG=582558 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== This adds validation to Begin/End CommandBuffer API's and avoids resetting record_type if vkResetCommanBuffer fails. BUG=582558 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/d2b4bdaa4d479e18aae4a1242ae297f2ce4477f2 Cr-Commit-Position: refs/heads/master@{#394072} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d2b4bdaa4d479e18aae4a1242ae297f2ce4477f2 Cr-Commit-Position: refs/heads/master@{#394072} |
