Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2774583002:
ui: Reduce logging in ui::ClientNativePixmapDmaBuf. (Closed)
|
Created:
3 years, 9 months ago by reveman Modified:
3 years, 8 months ago Reviewers:
dnicoara, Daniele Castagna, vignatti (out of this project), danakj, inactive_dshwang_plz_cc_intel, marcheu, spang CC:
chromium-reviews, kalyank, ozone-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionui: Reduce logging in ui::ClientNativePixmapDmaBuf.
Reduce PrimeSync error logging to when DCHECKS are on.
BUG=641392
TBR=danakj@chromium.org
Review-Url: https://codereview.chromium.org/2774583002
Cr-Commit-Position: refs/heads/master@{#459674}
Committed: https://chromium.googlesource.com/chromium/src/+/7467681237852ada4d444509d065a4a0de6e6131
Patch Set 1 #
Total comments: 2
Patch Set 2 : reduce logging to DCHECK_IS_ON #
Total comments: 3
Patch Set 3 : rebase #
Total comments: 1
Messages
Total messages: 42 (22 generated)
reveman@chromium.org changed reviewers: + dcastagna@chromium.org
https://codereview.chromium.org/2774583002/diff/1/ui/ozone/platform/drm/commo... File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2774583002/diff/1/ui/ozone/platform/drm/commo... ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:135: ++map_count_; nit: If we increment it here it's not really a map count (i.e: if someones calls map once then map_count is still 0). Can we think of a better name than map_count_? Another option could be to increment it in map and log only when map_count_ is 1. https://codereview.chromium.org/2774583002/diff/1/ui/ozone/platform/drm/commo... File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.h (right): https://codereview.chromium.org/2774583002/diff/1/ui/ozone/platform/drm/commo... ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.h:43: unsigned map_count_ = 0; We should use uint32_t or similar type, probably I would just use int32_t.
Description was changed from ========== ui: Reduce logging in ui::ClientNativePixmapDmaBuf. Only log PrimeSync errors first time we map a buffer. BUG= ========== to ========== ui: Reduce logging in ui::ClientNativePixmapDmaBuf. Reduce PrimeSync error logging to when DCHECKS are on. BUG= ==========
Made this simpler as discussed by reducing logging to when DCHECKs are on. Ptal.
And since no one but us is running Chrome on CrOS with DCHECKs on, this really means you're reducing the logging for everyone but us... LGTM. https://codereview.chromium.org/2774583002/diff/20001/ui/ozone/platform/drm/c... File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2774583002/diff/20001/ui/ozone/platform/drm/c... ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:61: #if DCHECK_IS_ON() It's ugly, but if it's the standard way of doing it I'm ok with it.
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...
reveman@chromium.org changed reviewers: + dnicoara@chromium.org
+dnicoara for owner review
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This lgtm, just one question wrt devices with release builds. https://codereview.chromium.org/2774583002/diff/20001/ui/ozone/platform/drm/c... File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2774583002/diff/20001/ui/ozone/platform/drm/c... ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:54: DPLOG_IF(ERROR, rv) << "Failed DMA_BUF_SYNC_START"; Without the PLOG, can we tell if these are failing on consumer devices? Would it make sense to have it output this error at least once in release builds? Perhaps a simple "static bool logged_once" here to simplify things?
https://codereview.chromium.org/2774583002/diff/20001/ui/ozone/platform/drm/c... File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2774583002/diff/20001/ui/ozone/platform/drm/c... ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:54: DPLOG_IF(ERROR, rv) << "Failed DMA_BUF_SYNC_START"; On 2017/03/24 at 15:03:59, dnicoara wrote: > Without the PLOG, can we tell if these are failing on consumer devices? > > Would it make sense to have it output this error at least once in release builds? Perhaps a simple "static bool logged_once" here to simplify things? I did that in previous patch but we thought this was better for now. We currently have drivers not implementing this but that's fine as they return uncached memory from mmap so I'd rather not have people worry about these messages in the log until we've updated the drivers to have proper no op implementations of this ioctl instead.
sgtm, still lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by reveman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dnicoara@chromium.org, dcastagna@chromium.org Link to the patchset: https://codereview.chromium.org/2774583002/#ps40001 (title: "rebase")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== ui: Reduce logging in ui::ClientNativePixmapDmaBuf. Reduce PrimeSync error logging to when DCHECKS are on. BUG= ========== to ========== ui: Reduce logging in ui::ClientNativePixmapDmaBuf. Reduce PrimeSync error logging to when DCHECKS are on. BUG= TBR=danakj@chromium.org ==========
reveman@chromium.org changed reviewers: + danakj@chromium.org
The CQ bit was checked by reveman@chromium.org
TBR danakj as the file was just moved from ozone to ui/gfx/linux/
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": 1490507189988130,
"parent_rev": "71d33c3169cf175d2d367834907b86c4023c0496", "commit_rev":
"7467681237852ada4d444509d065a4a0de6e6131"}
Message was sent while issue was closed.
Description was changed from ========== ui: Reduce logging in ui::ClientNativePixmapDmaBuf. Reduce PrimeSync error logging to when DCHECKS are on. BUG= TBR=danakj@chromium.org ========== to ========== ui: Reduce logging in ui::ClientNativePixmapDmaBuf. Reduce PrimeSync error logging to when DCHECKS are on. BUG= TBR=danakj@chromium.org Review-Url: https://codereview.chromium.org/2774583002 Cr-Commit-Position: refs/heads/master@{#459674} Committed: https://chromium.googlesource.com/chromium/src/+/7467681237852ada4d444509d065... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/7467681237852ada4d444509d065...
Message was sent while issue was closed.
marcheu@chromium.org changed reviewers: + marcheu@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2774583002/diff/40001/ui/gfx/linux/client_nat... File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2774583002/diff/40001/ui/gfx/linux/client_nat... ui/gfx/linux/client_native_pixmap_dmabuf.cc:53: drmIoctl(dmabuf_fd, LOCAL_DMA_BUF_IOCTL_SYNC, &sync_start); I don't think drmIoctl is meant to be used for non-drm ioctls. In other words, you should use ioctl() here and wrap your own error handler around it.
Message was sent while issue was closed.
reveman@chromium.org changed reviewers: + dongseong.hwang@chromium.org, spang@chromium.org, tiago.vignatti@intel.com
Message was sent while issue was closed.
On 2017/03/28 at 01:07:26, marcheu wrote: > https://codereview.chromium.org/2774583002/diff/40001/ui/gfx/linux/client_nat... > File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): > > https://codereview.chromium.org/2774583002/diff/40001/ui/gfx/linux/client_nat... > ui/gfx/linux/client_native_pixmap_dmabuf.cc:53: drmIoctl(dmabuf_fd, LOCAL_DMA_BUF_IOCTL_SYNC, &sync_start); > I don't think drmIoctl is meant to be used for non-drm ioctls. In other words, you should use ioctl() here and wrap your own error handler around it. +tiago.vignatti, +dongseong +spang who implemented and reviewed the change that added this.
Message was sent while issue was closed.
On 2017/03/28 06:09:20, reveman wrote: > On 2017/03/28 at 01:07:26, marcheu wrote: > > > https://codereview.chromium.org/2774583002/diff/40001/ui/gfx/linux/client_nat... > > File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): > > > > > https://codereview.chromium.org/2774583002/diff/40001/ui/gfx/linux/client_nat... > > ui/gfx/linux/client_native_pixmap_dmabuf.cc:53: drmIoctl(dmabuf_fd, > LOCAL_DMA_BUF_IOCTL_SYNC, &sync_start); > > I don't think drmIoctl is meant to be used for non-drm ioctls. In other words, > you should use ioctl() here and wrap your own error handler around it. > > +tiago.vignatti, > +dongseong > +spang > > who implemented and reviewed the change that added this. Yes, it's wrong. Thank you for catching up. I submitted new CL https://codereview.chromium.org/2805503003
Message was sent while issue was closed.
Description was changed from ========== ui: Reduce logging in ui::ClientNativePixmapDmaBuf. Reduce PrimeSync error logging to when DCHECKS are on. BUG= TBR=danakj@chromium.org Review-Url: https://codereview.chromium.org/2774583002 Cr-Commit-Position: refs/heads/master@{#459674} Committed: https://chromium.googlesource.com/chromium/src/+/7467681237852ada4d444509d065... ========== to ========== ui: Reduce logging in ui::ClientNativePixmapDmaBuf. Reduce PrimeSync error logging to when DCHECKS are on. BUG=641392 TBR=danakj@chromium.org Review-Url: https://codereview.chromium.org/2774583002 Cr-Commit-Position: refs/heads/master@{#459674} Committed: https://chromium.googlesource.com/chromium/src/+/7467681237852ada4d444509d065... ========== |
