Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2805503003:
ClientNativePixmapFactoryDmabuf uses ioctl, instead of drmIoctl. (Closed)
DescriptionClientNativePixmapFactoryDmabuf uses ioctl, instead of drmIoctl.
DMA_BUF_SYNC ioctl is not drmIoctl, because it uses dma-buf fd, instead of drm
device fd.
In addition, remove LOCAL_ prefix to fix build failure >= kernel 4.6
Actually, ChromeOS doesn't need this local DMA_BUF_SYNC definition as all
verion of kernel for cros has dma-buf.h header.
https://chromium-review.googlesource.com/c/459544/
However, there is not any way to distinguish real ChromeOS build and
current_os="chromeos" build, so remain the local definition to ChromeOS as
well.
BUG=584248
R=reveman@chromium.org
Review-Url: https://codereview.chromium.org/2805503003
Cr-Commit-Position: refs/heads/master@{#465425}
Committed: https://chromium.googlesource.com/chromium/src/+/27bab2297187099229a1e4304d8feb866c8da55a
Patch Set 1 #
Total comments: 3
Patch Set 2 : use HANDLE_EINTR #
Total comments: 6
Patch Set 3 : remove dcheck #Messages
Total messages: 40 (15 generated)
The CQ bit was checked by dongseong.hwang@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...
dongseong.hwang@intel.com changed reviewers: + danakj@chromium.org
reveman, could you review this follow-up CL of https://codereview.chromium.org/2774583002/ danakj, could you review as owner?
marcheu@chromium.org changed reviewers: + marcheu@chromium.org
https://codereview.chromium.org/2805503003/diff/1/ui/gfx/linux/client_native_... File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2805503003/diff/1/ui/gfx/linux/client_native_... ui/gfx/linux/client_native_pixmap_dmabuf.cc:51: ioctl(dmabuf_fd, DMA_BUF_IOCTL_SYNC, &sync_start); ioctl returns an error value, you have to check it and react accordingly
https://codereview.chromium.org/2805503003/diff/1/ui/gfx/linux/client_native_... File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2805503003/diff/1/ui/gfx/linux/client_native_... ui/gfx/linux/client_native_pixmap_dmabuf.cc:21: #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 6, 0) I want to change it to #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 6, 0) || defined(OS_CHROMEOS) However, it makes build fail for current_os="chromeos" in Ubuntu.
The CQ bit was checked by dongseong.hwang@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...
https://codereview.chromium.org/2805503003/diff/1/ui/gfx/linux/client_native_... File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2805503003/diff/1/ui/gfx/linux/client_native_... ui/gfx/linux/client_native_pixmap_dmabuf.cc:51: ioctl(dmabuf_fd, DMA_BUF_IOCTL_SYNC, &sync_start); On 2017/04/06 00:56:11, marcheu wrote: > ioctl returns an error value, you have to check it and react accordingly new patch set wraps it by HANDLE_EINTR like other ioctl code in chromium. if it return error from HANDLE_EINTR, I don't have idea how we should handle it. It means kernel has some bug about it, and user may see some artifacts as sync failed.
gurchetansingh@chromium.org changed reviewers: + gurchetansingh@chromium.org
https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... ui/gfx/linux/client_native_pixmap_dmabuf.cc:22: #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 6, 0) Can we just check #ifdef _DMA_BUF_UAPI_H_ (exported by linux/dma-buf.h) and not duplicate code? If the header is not there (it will be for ChromeOS since Dongseong added it), we shouldn't do local definitions.
spang@chromium.org changed reviewers: + spang@chromium.org
https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... ui/gfx/linux/client_native_pixmap_dmabuf.cc:22: #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 6, 0) On 2017/04/06 01:39:09, gurchetansingh wrote: > Can we just check #ifdef _DMA_BUF_UAPI_H_ (exported by linux/dma-buf.h) and not > duplicate code? If the header is not there (it will be for ChromeOS since > Dongseong added it), we shouldn't do local definitions. If the header is not there, you have two options: (1) Fail the build (2) Make a local definition since these APIs cannot change anyway There's an option, but it's the worst one: (3) Stub it out I don't think we should ever do (3). You'll probably fail on some buildbot if you do (1); what's why these definitions are here.
On 2017/04/06 02:17:46, spang wrote: > https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... > File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): > > https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... > ui/gfx/linux/client_native_pixmap_dmabuf.cc:22: #if LINUX_VERSION_CODE >= > KERNEL_VERSION(4, 6, 0) > On 2017/04/06 01:39:09, gurchetansingh wrote: > > Can we just check #ifdef _DMA_BUF_UAPI_H_ (exported by linux/dma-buf.h) and > not > > duplicate code? If the header is not there (it will be for ChromeOS since > > Dongseong added it), we shouldn't do local definitions. > > If the header is not there, you have two options: > > (1) Fail the build > (2) Make a local definition since these APIs cannot change anyway > > There's an option, but it's the worst one: > > (3) Stub it out > > I don't think we should ever do (3). > > You'll probably fail on some buildbot if you do (1); what's why these > definitions are here. True. How about we do if defined(OS_CHROMEOS) [we added the header everywhere in ChromeOS] or if the version is greater then the Linux version >= 4.6, then we try using the sync api? I'm wondering since I assume vanilla Linux did not do backports like we did and it will always return an error.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/06 03:18:34, gurchetansingh wrote: > On 2017/04/06 02:17:46, spang wrote: > > > https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... > > File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): > > > > > https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... > > ui/gfx/linux/client_native_pixmap_dmabuf.cc:22: #if LINUX_VERSION_CODE >= > > KERNEL_VERSION(4, 6, 0) > > On 2017/04/06 01:39:09, gurchetansingh wrote: > > > Can we just check #ifdef _DMA_BUF_UAPI_H_ (exported by linux/dma-buf.h) and > > not > > > duplicate code? If the header is not there (it will be for ChromeOS since > > > Dongseong added it), we shouldn't do local definitions. > > > > If the header is not there, you have two options: > > > > (1) Fail the build > > (2) Make a local definition since these APIs cannot change anyway > > > > There's an option, but it's the worst one: > > > > (3) Stub it out > > > > I don't think we should ever do (3). > > > > You'll probably fail on some buildbot if you do (1); what's why these > > definitions are here. > > True. How about we do if defined(OS_CHROMEOS) [we added the header everywhere > in ChromeOS] or if the version is greater then the Linux version >= 4.6, then we > try using the sync api? I'm wondering since I assume vanilla Linux did not do > backports like we did and it will always return an error. If you call the ioctl on an old kernel you should get ENOTTY. What happens at runtime is actually irrelevant though; we're talking about compiling the binary. The build time and runtime issues are actually orthogonal: - You can compile the same headers that you run with. This case is boring. - You can build with new headers, and run with old headers. Nothing blows up; you just get ENOTTY. - You can build with old headers, and run with new headers. Oops, you compiled out the synchronization. The third case is scary; I don't think it's OK to go down that route. It's better to just handle ENOTTY (or ENOSYS) instead. If we need to build on a system with old headers, and we can't easily upgrade them, then duplicating the ioctl definition is the best option. OS_CHROMEOS doesn't mean "cross compiling for a chromebook with a CrOS sysroot", unfortunately. Sometimes it means you're building the CrOS UI for Ubuntu, in the worst case on a buildbot running a LTS version with old kernel headers. There's no supported way to tell if you're actually cross compiling for a Chromebook at build time; the supported way to find that out is a runtime check called IsRunningOnChromeOS().
On 2017/04/06 05:11:35, spang wrote: > On 2017/04/06 03:18:34, gurchetansingh wrote: > > On 2017/04/06 02:17:46, spang wrote: > > > > > > https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... > > > File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): > > > > > > > > > https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... > > > ui/gfx/linux/client_native_pixmap_dmabuf.cc:22: #if LINUX_VERSION_CODE >= > > > KERNEL_VERSION(4, 6, 0) > > > On 2017/04/06 01:39:09, gurchetansingh wrote: > > > > Can we just check #ifdef _DMA_BUF_UAPI_H_ (exported by linux/dma-buf.h) > and > > > not > > > > duplicate code? If the header is not there (it will be for ChromeOS since > > > > Dongseong added it), we shouldn't do local definitions. > > > > > > If the header is not there, you have two options: > > > > > > (1) Fail the build > > > (2) Make a local definition since these APIs cannot change anyway > > > > > > There's an option, but it's the worst one: > > > > > > (3) Stub it out > > > > > > I don't think we should ever do (3). > > > > > > You'll probably fail on some buildbot if you do (1); what's why these > > > definitions are here. > > > > True. How about we do if defined(OS_CHROMEOS) [we added the header everywhere > > in ChromeOS] or if the version is greater then the Linux version >= 4.6, then > we > > try using the sync api? I'm wondering since I assume vanilla Linux did not do > > backports like we did and it will always return an error. > > If you call the ioctl on an old kernel you should get ENOTTY. > > What happens at runtime is actually irrelevant though; we're talking about > compiling the binary. The build time and runtime issues are actually orthogonal: > > - You can compile the same headers that you run with. This case is boring. > - You can build with new headers, and run with old headers. Nothing blows up; > you just get ENOTTY. > - You can build with old headers, and run with new headers. Oops, you compiled > out the synchronization. > > The third case is scary; I don't think it's OK to go down that route. It's > better to just handle ENOTTY (or ENOSYS) instead. > > If we need to build on a system with old headers, and we can't easily upgrade > them, then duplicating the ioctl definition is the best option. > > OS_CHROMEOS doesn't mean "cross compiling for a chromebook with a CrOS sysroot", > unfortunately. Sometimes it means you're building the CrOS UI for Ubuntu, in the > worst case on a buildbot running a LTS version with old kernel headers. There's > no supported way to tell if you're actually cross compiling for a Chromebook at > build time; the supported way to find that out is a runtime check called > IsRunningOnChromeOS(). Okay, thanks for the info, then doing the local definitions seems reasonable.
spang, marcheu, could you review again? I think I resolved your comments. If I miss, please let me know. https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... ui/gfx/linux/client_native_pixmap_dmabuf.cc:22: #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 6, 0) On 2017/04/06 01:39:09, gurchetansingh wrote: > Can we just check #ifdef _DMA_BUF_UAPI_H_ (exported by linux/dma-buf.h) and not > duplicate code? If the header is not there (it will be for ChromeOS since > Dongseong added it), we shouldn't do local definitions. In addition of spang's nice explanation, this code will be used for plane linux. however, this code should be compiled in old kernel also. that's the main reason why we have local definition. https://bugs.chromium.org/p/chromium/issues/detail?id=584248
https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... ui/gfx/linux/client_native_pixmap_dmabuf.cc:63: HANDLE_EINTR(ioctl(dmabuf_fd, DMA_BUF_IOCTL_SYNC, &sync_end)); HANDLE_EINTR can still return an error in non-debug builds, can you always handle this error? I don't know why drmIoctl wasn't checked before, but especially in a world where we have weird dmabuf crashes it's not a good idea.
https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... ui/gfx/linux/client_native_pixmap_dmabuf.cc:63: HANDLE_EINTR(ioctl(dmabuf_fd, DMA_BUF_IOCTL_SYNC, &sync_end)); On 2017/04/06 22:47:10, marcheu wrote: > HANDLE_EINTR can still return an error in non-debug builds, can you always > handle this error? I don't know why drmIoctl wasn't checked before, but > especially in a world where we have weird dmabuf crashes it's not a good idea. Not checking the return value was added b/c of crbug.com/705189. Since that bug has been resolved, we can check the return code again.
could you review again? thx! https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... ui/gfx/linux/client_native_pixmap_dmabuf.cc:63: HANDLE_EINTR(ioctl(dmabuf_fd, DMA_BUF_IOCTL_SYNC, &sync_end)); On 2017/04/06 22:52:02, gurchetansingh wrote: > On 2017/04/06 22:47:10, marcheu wrote: > > HANDLE_EINTR can still return an error in non-debug builds, can you always > > handle this error? I don't know why drmIoctl wasn't checked before, but > > especially in a world where we have weird dmabuf crashes it's not a good idea. > > Not checking the return value was added b/c of crbug.com/705189. Since that bug > has been resolved, we can check the return code again. In addition, haswell dump lots of err message: https://bugs.chromium.org/p/chromium/issues/detail?id=641392 It's why reveman landed https://codereview.chromium.org/2774583002/ When dma-buf sync ioctl is stable enough, I'll remove dcheck guard.
all, could you review again? danakj, could you review as owner?
On 2017/04/12 21:13:11, dshwang wrote: > all, could you review again? > > danakj, could you review as owner? I can stamp it once the others are happy.
spang@google.com changed reviewers: + spang@google.com
lgtm Can you make a CL to turn the logging back on? I agree with Stephane, we should not be ignoring errors from the kernel in release builds.
rs LGTM
On 2017/04/18 15:48:42, spang1 wrote: > lgtm > > Can you make a CL to turn the logging back on? > > I agree with Stephane, we should not be ignoring errors from the kernel in > release builds. Thank you for reviewing! reveman deliberately disabled the log in https://codereview.chromium.org/2774583002/ It's because kernel v3.8 (i.e. Haswell) and v3.10 produces stream of err messages, which make it hard to read other err messages. From Intel side, a kernel engineer is finding what it missing part between v3.10 and v3.14. Rockchip has similar verbose issue according to gurchetansingh. When we resolve it, I'll turn the logging back on. spang, what do you think?
On 2017/04/18 16:44:40, dshwang wrote: > On 2017/04/18 15:48:42, spang1 wrote: > > lgtm > > > > Can you make a CL to turn the logging back on? > > > > I agree with Stephane, we should not be ignoring errors from the kernel in > > release builds. > > Thank you for reviewing! > reveman deliberately disabled the log in > https://codereview.chromium.org/2774583002/ > It's because kernel v3.8 (i.e. Haswell) and v3.10 produces stream of err > messages, which make it hard to read other err messages. > From Intel side, a kernel engineer is finding what it missing part between v3.10 > and v3.14. > Rockchip has similar verbose issue according to gurchetansingh. > When we resolve it, I'll turn the logging back on. spang, what do you think? We can turn the feature off right? I thought we had already. This is not even ratelimiting the messages, just dropping all of them, including potentially new ones. This isn't the way to handle errors happening in production.
On 2017/04/18 16:44:40, dshwang wrote: > On 2017/04/18 15:48:42, spang1 wrote: > > lgtm > > > > Can you make a CL to turn the logging back on? > > > > I agree with Stephane, we should not be ignoring errors from the kernel in > > release builds. > > Thank you for reviewing! > reveman deliberately disabled the log in > https://codereview.chromium.org/2774583002/ > It's because kernel v3.8 (i.e. Haswell) and v3.10 produces stream of err > messages, which make it hard to read other err messages. > From Intel side, a kernel engineer is finding what it missing part between v3.10 > and v3.14. > Rockchip has similar verbose issue according to gurchetansingh. > When we resolve it, I'll turn the logging back on. spang, what do you think? The rockchip logging messages were legitimate, and have been fixed.
On 2017/04/18 16:55:05, spang1 wrote: > On 2017/04/18 16:44:40, dshwang wrote: > > On 2017/04/18 15:48:42, spang1 wrote: > > > lgtm > > > > > > Can you make a CL to turn the logging back on? > > > > > > I agree with Stephane, we should not be ignoring errors from the kernel in > > > release builds. > > > > Thank you for reviewing! > > reveman deliberately disabled the log in > > https://codereview.chromium.org/2774583002/ > > It's because kernel v3.8 (i.e. Haswell) and v3.10 produces stream of err > > messages, which make it hard to read other err messages. > > From Intel side, a kernel engineer is finding what it missing part between > v3.10 > > and v3.14. > > Rockchip has similar verbose issue according to gurchetansingh. > > When we resolve it, I'll turn the logging back on. spang, what do you think? > > We can turn the feature off right? I thought we had already. > > This is not even ratelimiting the messages, just dropping all of them, including > potentially new ones. This isn't the way to handle errors happening in > production. yes, native GMB was disabled. OK. reveman, I'll remove DCHECK guard on next patch set. is it ok for you?
On 2017/04/07 01:32:20, dshwang wrote: > could you review again? thx! > > https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... > File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): > > https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... > ui/gfx/linux/client_native_pixmap_dmabuf.cc:63: HANDLE_EINTR(ioctl(dmabuf_fd, > DMA_BUF_IOCTL_SYNC, &sync_end)); > On 2017/04/06 22:52:02, gurchetansingh wrote: > > On 2017/04/06 22:47:10, marcheu wrote: > > > HANDLE_EINTR can still return an error in non-debug builds, can you always > > > handle this error? I don't know why drmIoctl wasn't checked before, but > > > especially in a world where we have weird dmabuf crashes it's not a good > idea. > > > > Not checking the return value was added b/c of crbug.com/705189. Since that > bug > > has been resolved, we can check the return code again. > > In addition, haswell dump lots of err message: > https://bugs.chromium.org/p/chromium/issues/detail?id=641392 > It's why reveman landed https://codereview.chromium.org/2774583002/ > > When dma-buf sync ioctl is stable enough, I'll remove dcheck guard. You have to check the ioctl return values. If some ioctls are returning errors, ignoring them isn't the right way to address it. We already have issues with this code, let's not make this worse.
On 2017/04/18 22:40:47, marcheu wrote: > On 2017/04/07 01:32:20, dshwang wrote: > > could you review again? thx! > > > > > https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... > > File ui/gfx/linux/client_native_pixmap_dmabuf.cc (right): > > > > > https://codereview.chromium.org/2805503003/diff/20001/ui/gfx/linux/client_nat... > > ui/gfx/linux/client_native_pixmap_dmabuf.cc:63: HANDLE_EINTR(ioctl(dmabuf_fd, > > DMA_BUF_IOCTL_SYNC, &sync_end)); > > On 2017/04/06 22:52:02, gurchetansingh wrote: > > > On 2017/04/06 22:47:10, marcheu wrote: > > > > HANDLE_EINTR can still return an error in non-debug builds, can you always > > > > handle this error? I don't know why drmIoctl wasn't checked before, but > > > > especially in a world where we have weird dmabuf crashes it's not a good > > idea. > > > > > > Not checking the return value was added b/c of crbug.com/705189. Since that > > bug > > > has been resolved, we can check the return code again. > > > > In addition, haswell dump lots of err message: > > https://bugs.chromium.org/p/chromium/issues/detail?id=641392 > > It's why reveman landed https://codereview.chromium.org/2774583002/ > > > > When dma-buf sync ioctl is stable enough, I'll remove dcheck guard. > > You have to check the ioctl return values. If some ioctls are returning errors, > ignoring them isn't the right way to address it. We already have issues with > this code, let's not make this worse. I understand. Done. thx!
The CQ bit was checked by dongseong.hwang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from spang@google.com, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2805503003/#ps40001 (title: "remove dcheck")
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": 1492555702699500,
"parent_rev": "e5c53812891bbcafe6945f41066381644883954b", "commit_rev":
"27bab2297187099229a1e4304d8feb866c8da55a"}
Message was sent while issue was closed.
Description was changed from ========== ClientNativePixmapFactoryDmabuf uses ioctl, instead of drmIoctl. DMA_BUF_SYNC ioctl is not drmIoctl, because it uses dma-buf fd, instead of drm device fd. In addition, remove LOCAL_ prefix to fix build failure >= kernel 4.6 Actually, ChromeOS doesn't need this local DMA_BUF_SYNC definition as all verion of kernel for cros has dma-buf.h header. https://chromium-review.googlesource.com/c/459544/ However, there is not any way to distinguish real ChromeOS build and current_os="chromeos" build, so remain the local definition to ChromeOS as well. BUG=584248 R=reveman@chromium.org ========== to ========== ClientNativePixmapFactoryDmabuf uses ioctl, instead of drmIoctl. DMA_BUF_SYNC ioctl is not drmIoctl, because it uses dma-buf fd, instead of drm device fd. In addition, remove LOCAL_ prefix to fix build failure >= kernel 4.6 Actually, ChromeOS doesn't need this local DMA_BUF_SYNC definition as all verion of kernel for cros has dma-buf.h header. https://chromium-review.googlesource.com/c/459544/ However, there is not any way to distinguish real ChromeOS build and current_os="chromeos" build, so remain the local definition to ChromeOS as well. BUG=584248 R=reveman@chromium.org Review-Url: https://codereview.chromium.org/2805503003 Cr-Commit-Position: refs/heads/master@{#465425} Committed: https://chromium.googlesource.com/chromium/src/+/27bab2297187099229a1e4304d8f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/27bab2297187099229a1e4304d8f... |
