Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846203002:
exo: no-roundtrip dmabuf import (Closed)
Descriptionexo: no-roundtrip dmabuf import
support importing dmabufs without an explicit roundtrip
to get the imported buffer handle into client as per
zwp_linux_dmabuf_v1_interface version 2, and make
clients use this by default.
TEST=`wayland_rects_client --use-drm`
Review-Url: https://codereview.chromium.org/2846203002
Cr-Commit-Position: refs/heads/master@{#481219}
Committed: https://chromium.googlesource.com/chromium/src/+/857a5bbf18cda0df2a0133171cafb3b129ff9462
Patch Set 1 #
Total comments: 2
Patch Set 2 : use no roundtrip import by default, refactor #
Total comments: 5
Patch Set 3 : follow naming convention, fixup error checking #
Total comments: 3
Patch Set 4 : proper coding style #Patch Set 5 : rebase to master, author fixup #Messages
Total messages: 36 (18 generated)
Description was changed from ========== RFC: exo: add no-roundtrip dmabuf import support introduce no-roundtrip dmabuf import in exo, planned for zwp_linux_dmabuf_v1_interface version 2 [1], [1] https://patchwork.freedesktop.org/patch/138175/ ========== to ========== RFC: exo: add no-roundtrip dmabuf import support introduce no-roundtrip dmabuf import in exo, planned for zwp_linux_dmabuf_v1_interface version 2 [1], [1] https://patchwork.freedesktop.org/patch/138175/ TEST=`wayland_rects_client --use-drm --no-roundtrip-import` ==========
varadgautam@gmail.com changed reviewers: + reveman@chromium.org
Great! Assuming the upstream change has landed then we can go ahead and land this after the following comments have been addressed. https://codereview.chromium.org/2846203002/diff/1/components/exo/wayland/clie... File components/exo/wayland/clients/client_base.cc (right): https://codereview.chromium.org/2846203002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/client_base.cc:61: const char kNoRoundtripImport[] = "no-roundtrip-import"; No need for this flag. Please refactor the code so it works in an ideal way for this new type of buffer import. https://codereview.chromium.org/2846203002/diff/1/components/exo/wayland/serv... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2846203002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:649: void linux_buffer_params_create_impl(wl_client* client, I think the code will be easier to read if you didn't add this _impl function but instead some code duplication in immed and non-immed functions. if the code duplication is too great then maybe add a utility function or two.
On 2017/04/28 17:21:24, reveman wrote: > Great! Assuming the upstream change has landed then we can go ahead and land > this after the following comments have been addressed. > > https://codereview.chromium.org/2846203002/diff/1/components/exo/wayland/clie... > File components/exo/wayland/clients/client_base.cc (right): > > https://codereview.chromium.org/2846203002/diff/1/components/exo/wayland/clie... > components/exo/wayland/clients/client_base.cc:61: const char > kNoRoundtripImport[] = "no-roundtrip-import"; > No need for this flag. Please refactor the code so it works in an ideal way for > this new type of buffer import. > done in v2. > https://codereview.chromium.org/2846203002/diff/1/components/exo/wayland/serv... > File components/exo/wayland/server.cc (right): > > https://codereview.chromium.org/2846203002/diff/1/components/exo/wayland/serv... > components/exo/wayland/server.cc:649: void > linux_buffer_params_create_impl(wl_client* client, > I think the code will be easier to read if you didn't add this _impl function > but instead some code duplication in immed and non-immed functions. if the code > duplication is too great then maybe add a utility function or two. moved param validation to a new function in v2.
Description was changed from ========== RFC: exo: add no-roundtrip dmabuf import support introduce no-roundtrip dmabuf import in exo, planned for zwp_linux_dmabuf_v1_interface version 2 [1], [1] https://patchwork.freedesktop.org/patch/138175/ TEST=`wayland_rects_client --use-drm --no-roundtrip-import` ========== to ========== exo: no-roundtrip dmabuf import support importing dmabufs without an explicit roundtrip to get the imported buffer handle into client as per zwp_linux_dmabuf_v1_interface version 2, and make clients use this by default. TEST=`wayland_rects_client --use-drm` ==========
Looks great! Just two minor nits https://codereview.chromium.org/2846203002/diff/20001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2846203002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:626: std::map<uint32_t, Plane> planes; Fyi, this would be a good use case for the new base::flat_map but that's unrelated to this change no need to change anything. https://codereview.chromium.org/2846203002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:656: bool linux_buffer_params_validate(wl_resource* resource, nit: ValidateLinuxBufferParams as the current pattern is to only keep wayland interface functions lower case and everything else follows standard chromium code style https://codereview.chromium.org/2846203002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:680: if (linux_buffer_params->planes.size() != num_planes) { nit: should we iterate over all linux_buffer_params->planes and check that index is within bounds before we do this instead of the loop below at l.686. That way ERROR_PLANE_IDX actually means out of bounds and we know that "planes.size() != num_planes" means missing plane as code above prevents two plans with the same index.
On 2017/06/02 14:28:10, reveman wrote: > Looks great! Just two minor nits > > https://codereview.chromium.org/2846203002/diff/20001/components/exo/wayland/... > File components/exo/wayland/server.cc (right): > > https://codereview.chromium.org/2846203002/diff/20001/components/exo/wayland/... > components/exo/wayland/server.cc:626: std::map<uint32_t, Plane> planes; > Fyi, this would be a good use case for the new base::flat_map but that's > unrelated to this change no need to change anything. > > https://codereview.chromium.org/2846203002/diff/20001/components/exo/wayland/... > components/exo/wayland/server.cc:656: bool > linux_buffer_params_validate(wl_resource* resource, > nit: ValidateLinuxBufferParams as the current pattern is to only keep wayland > interface functions lower case and everything else follows standard chromium > code style > > https://codereview.chromium.org/2846203002/diff/20001/components/exo/wayland/... > components/exo/wayland/server.cc:680: if (linux_buffer_params->planes.size() != > num_planes) { > nit: should we iterate over all linux_buffer_params->planes and check that index > is within bounds before we do this instead of the loop below at l.686. That way > ERROR_PLANE_IDX actually means out of bounds and we know that "planes.size() != > num_planes" means missing plane as code above prevents two plans with the same > index. v3 addresses these. the wl-proto patch for immed import is available upstream (https://cgit.freedesktop.org/wayland/wayland-protocols/commit/?id=a840b3634ad...).
https://codereview.chromium.org/2846203002/diff/20001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2846203002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:656: bool linux_buffer_params_validate(wl_resource* resource, On 2017/06/02 14:28:10, reveman wrote: > nit: ValidateLinuxBufferParams as the current pattern is to only keep wayland > interface functions lower case and everything else follows standard chromium > code style Done. https://codereview.chromium.org/2846203002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:680: if (linux_buffer_params->planes.size() != num_planes) { On 2017/06/02 14:28:10, reveman wrote: > nit: should we iterate over all linux_buffer_params->planes and check that index > is within bounds before we do this instead of the loop below at l.686. That way > ERROR_PLANE_IDX actually means out of bounds and we know that "planes.size() != > num_planes" means missing plane as code above prevents two plans with the same > index. Done.
lgtm with small nits https://codereview.chromium.org/2846203002/diff/40001/components/exo/wayland/... File components/exo/wayland/clients/client_base.cc (right): https://codereview.chromium.org/2846203002/diff/40001/components/exo/wayland/... components/exo/wayland/clients/client_base.cc:303: // if the buffer handle doesn't exist, we would either be killed by the nit: "If" as comments should have proper capitalization https://codereview.chromium.org/2846203002/diff/40001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2846203002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:656: bool LinuxBufferParamsValidate(wl_resource* resource, nit: I prefer ValidateLinuxBufferParams as it feels a bit more chromium/c++-ish where we don't have to use prefix for namespaces https://codereview.chromium.org/2846203002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:801: // on import failure in case of a create_immed request, the protocol nit: "On" as comments should have proper capitalization
v4 uploaded with stylefixes from reveman.
please go ahead and commit this when ready
The CQ bit was checked by varadgautam@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2846203002/#ps60001 (title: "proper coding style")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
The author varadgautam@gmail.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by varadgautam@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2846203002/#ps80001 (title: "rebase to master, author fixup")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
The author varadgautam@gmail.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by varadgautam@gmail.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by reveman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm going ahead and landing this now that dependencies have landed as we have some follow up changes that depend on it
The CQ bit was checked by reveman@chromium.org
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": 80001, "attempt_start_ts": 1498061697534110,
"parent_rev": "e9294ad21604f461226df94577094c361f1cdc49", "commit_rev":
"857a5bbf18cda0df2a0133171cafb3b129ff9462"}
Message was sent while issue was closed.
Description was changed from
==========
exo: no-roundtrip dmabuf import
support importing dmabufs without an explicit roundtrip
to get the imported buffer handle into client as per
zwp_linux_dmabuf_v1_interface version 2, and make
clients use this by default.
TEST=`wayland_rects_client --use-drm`
==========
to
==========
exo: no-roundtrip dmabuf import
support importing dmabufs without an explicit roundtrip
to get the imported buffer handle into client as per
zwp_linux_dmabuf_v1_interface version 2, and make
clients use this by default.
TEST=`wayland_rects_client --use-drm`
Review-Url: https://codereview.chromium.org/2846203002
Cr-Commit-Position: refs/heads/master@{#481219}
Committed:
https://chromium.googlesource.com/chromium/src/+/857a5bbf18cda0df2a0133171caf...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/857a5bbf18cda0df2a0133171caf... |
