Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488253005:
exo: Use use-drm flag to pick a driver. (Closed)
|
Created:
4 years, 1 month ago by Daniele Castagna Modified:
4 years, 1 month ago Reviewers:
reveman CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Use use-drm flag to pick a driver.
The test wayland_motion_event used had an hardcoded render
node path.
This render node path turned out to use vgem on kevin.
This CL adds logic to allow to specify a string for the flag
use-drm.
The first drm device with a driver name containing the substring
will be picked.
BUG=661010
Committed: https://crrev.com/106d9c981130ca2de709c14f368655b56479eb71
Cr-Commit-Position: refs/heads/master@{#431615}
Patch Set 1 #Patch Set 2 : Address incoming nits and avoid opening drm device when not used. #
Total comments: 10
Patch Set 3 : Address reveman's comments. #Messages
Total messages: 20 (13 generated)
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
dcastagna@chromium.org changed reviewers: + 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dcastagna@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...
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits https://codereview.chromium.org/2488253005/diff/20001/components/exo/wayland/... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2488253005/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:397: std::ostringstream dri_render_node( nit: s/std::ostringstream/std::string/ https://codereview.chromium.org/2488253005/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:405: LOG(WARNING) << "Can't get version for device: '" << dri_render_node nit: make this an error and return 1 if possible https://codereview.chromium.org/2488253005/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:410: if (!use_drm_->length() || nit: can we make this a bit easier to read using a continue statement? Something like: if (use_drm_->length()) { drmVersionPtr drm_version = drmGetVersion(drm_fd.get()); if (drm_version_name.find(*use_drm_) == std::string::npos) continue; } drm_fd_ = std::move(drm_fd); break; that way we're also not calling drmGetVersion unless needed.. https://codereview.chromium.org/2488253005/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:420: LOG(ERROR) << "Can't find drm device"; nit: completely optional; reduce this to 2 lines with: LOG_IF(ERROR, use_drm_) << "Can't find drm device: '" << *use_drm_ << "'"; LOG_IF(ERROR, !use_drm_) << "Can't find drm device"; https://codereview.chromium.org/2488253005/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:744: if (command_line->HasSwitch(switches::kUseDrm)) nit: missing { } for multi-line if-statement
https://codereview.chromium.org/2488253005/diff/20001/components/exo/wayland/... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2488253005/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:397: std::ostringstream dri_render_node( On 2016/11/11 at 01:46:17, reveman wrote: > nit: s/std::ostringstream/std::string/ Done. https://codereview.chromium.org/2488253005/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:405: LOG(WARNING) << "Can't get version for device: '" << dri_render_node On 2016/11/11 at 01:46:17, reveman wrote: > nit: make this an error and return 1 if possible Done. https://codereview.chromium.org/2488253005/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:410: if (!use_drm_->length() || On 2016/11/11 at 01:46:17, reveman wrote: > nit: can we make this a bit easier to read using a continue statement? Something like: > > if (use_drm_->length()) { > drmVersionPtr drm_version = drmGetVersion(drm_fd.get()); > if (drm_version_name.find(*use_drm_) == std::string::npos) > continue; > } > drm_fd_ = std::move(drm_fd); > break; > > that way we're also not calling drmGetVersion unless needed.. Done as discussed IRL. https://codereview.chromium.org/2488253005/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:420: LOG(ERROR) << "Can't find drm device"; On 2016/11/11 at 01:46:17, reveman wrote: > nit: completely optional; reduce this to 2 lines with: > LOG_IF(ERROR, use_drm_) << "Can't find drm device: '" << *use_drm_ << "'"; > LOG_IF(ERROR, !use_drm_) << "Can't find drm device"; Done. https://codereview.chromium.org/2488253005/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:744: if (command_line->HasSwitch(switches::kUseDrm)) On 2016/11/11 at 01:46:17, reveman wrote: > nit: missing { } for multi-line if-statement Done.
Description was changed from ========== exo: Use use-drm flag to pick a driver. The test wayland_motion_event used had an hardcoded render node path. This render node path turned out to use vgem on kevin. This CL adds logic to allow to specify a string for the flag use-drm. The first drm device with a driver name containing the substring will be picked. BUG= ========== to ========== exo: Use use-drm flag to pick a driver. The test wayland_motion_event used had an hardcoded render node path. This render node path turned out to use vgem on kevin. This CL adds logic to allow to specify a string for the flag use-drm. The first drm device with a driver name containing the substring will be picked. BUG=661010 ==========
The CQ bit was checked by dcastagna@chromium.org
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/2488253005/#ps40001 (title: "Address reveman's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== exo: Use use-drm flag to pick a driver. The test wayland_motion_event used had an hardcoded render node path. This render node path turned out to use vgem on kevin. This CL adds logic to allow to specify a string for the flag use-drm. The first drm device with a driver name containing the substring will be picked. BUG=661010 ========== to ========== exo: Use use-drm flag to pick a driver. The test wayland_motion_event used had an hardcoded render node path. This render node path turned out to use vgem on kevin. This CL adds logic to allow to specify a string for the flag use-drm. The first drm device with a driver name containing the substring will be picked. BUG=661010 Committed: https://crrev.com/106d9c981130ca2de709c14f368655b56479eb71 Cr-Commit-Position: refs/heads/master@{#431615} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/106d9c981130ca2de709c14f368655b56479eb71 Cr-Commit-Position: refs/heads/master@{#431615} |
