Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 7 months ago by dshwang Modified:
5 years, 4 months ago Reviewers:
dnicoara, reveman, ccameron, vignatti (out of this project), fjhenigman, kalyank_, zachr, no sievers, alexst (slow to review), marcheu, spang CC:
cc-bugs_chromium.org, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, jbauman+watch_chromium.org, jln+watch_chromium.org, kalyank, mcasas+watch_chromium.org, ozone-reviews_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org, rickyz+watch_chromium.org, sievers+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionozone: Implement zero/one-copy texture for Ozone GBM.
The design doc is https://docs.google.com/document/d/1qpLLo4mBkzHBh5cuBtBjJZAzXK2X9BgBJtpESh-mNn8
Implement GpuMemoryBuffer using GBM and VGEM. VGEM makes it possible for Render
Process to map/unmap GPU memory. GPU process creates GBM bo and sends the handle
to Renderer. Renderer imports VGEM bo via the handle and map/unmap the VGEM bo.
Define OZONE_USE_VGEM_MAP because VGEM map is not in linux upsteam.
BUILD=add "ozone_use_vgem_map=1" in GYP_DEFINES, or "ozone_use_vgem_map=true" in GN
TEST=Run chromeos using amd64-generic_freon image with --enable-native-gpu-memory-buffers, --disable-persistent-gpu-memory-buffer and --no-sandbox flag.
PERF_TEST=Run on Acer 720P > tools/perf/run_benchmark --browser=cros-chrome --remote=<machine_url> smoothness.tough_texture_upload_cases --page-repeat=3 --extra-browser-args="--enable-native-gpu-memory-buffers"
smoothness.tough_texture_upload_cases:frame_time_discrepancy 144.02 to 126.75
smoothness.tough_texture_upload_cases:percentage_smooth 55.83 to 56.56
BUG=475633
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/e2e2c1dc3ef88009d11d6d644a9c60fb9293e7b5
Cr-Commit-Position: refs/heads/master@{#344755}
Patch Set 1 #
Total comments: 20
Patch Set 2 : not-leak USE_OZONE_GBM, GetSupportedGpuMemoryBufferConfigurations from ozone, return handle #
Total comments: 13
Patch Set 3 : depend on crrev.com/1128113011 #
Total comments: 3
Patch Set 4 : rebase to new crrev.com/1128113011 #
Total comments: 4
Patch Set 5 : rebase to ToT #
Total comments: 13
Patch Set 6 : improve by spang's comment #Patch Set 7 : rebase to latest crrev.com/1248713002 #Patch Set 8 : open VGEM device in renderer in ad-hoc manner #
Total comments: 4
Patch Set 9 : rebase on crrev.com/1263323004 #
Total comments: 26
Patch Set 10 : resolve reveman's concerns #
Total comments: 29
Patch Set 11 : address msg#47 #Patch Set 12 : make content_unittests (GpuMemoryBuffer*) pass #
Total comments: 20
Patch Set 13 : resolve reveman's comments #Patch Set 14 : check that the configuration is returned by GetSupportedConfigurations() #
Total comments: 4
Patch Set 15 : update from reveman's comments #
Total comments: 32
Patch Set 16 : resolve nits #
Total comments: 19
Patch Set 17 : restore switch #
Total comments: 2
Patch Set 18 : resolve reveman's nits #
Total comments: 3
Patch Set 19 : add NOTREACHED() #
Total comments: 2
Patch Set 20 : address sievers's comment #
Total comments: 4
Patch Set 21 : remove the return type from Pixmap::Map() #
Total comments: 4
Patch Set 22 : remove redundant return nullptr #Patch Set 23 : rebase to ToT #
Created: 5 years, 4 months ago
Messages
Total messages: 93 (21 generated)
reveman, could you review this CL? zachr, could you review how this CL use vgem? especially, content/common/gpu/client/gpu_memory_buffer_impl_ozone_gbm.cc and ui/ozone/platform/drm/gpu/gbm_buffer.cc It's physically blocked by https://codereview.chromium.org/1124063003/ Intel DRM implementation is in https://codereview.chromium.org/1071273002/ https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_memory_buffer_impl_ozone_gbm.cc:93: } zachr, I could not release the handle because both drmIoctl(DRM_IOCTL_MODE_DESTROY_DUMB) and drmIoctl(DRM_IOCTL_GEM_CLOSE) cause renderer crash. I guess kernel has bug. could you advice?
dongseong.hwang@intel.com changed reviewers: + dnicoara@chromium.org
spang@chromium.org changed reviewers: + spang@chromium.org
https://codereview.chromium.org/1134993003/diff/1/build/config/ui.gni File build/config/ui.gni (right): https://codereview.chromium.org/1134993003/diff/1/build/config/ui.gni#newcode40 build/config/ui.gni:40: ozone_platform_gbm = false It's not acceptable to put this here. We don't want the enabled platforms to leak outside of the ui/ozone component. Please use the ui/ozone/public interface in USE_OZONE builds (and change it if necessary).
https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_memory_buffer_impl.cc (right): https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_memory_buffer_impl.cc:39: DCHECK(!mapped_); this is fine but unrelated to this change. please change it in a separate patch https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_gbm.h (right): https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_memory_buffer_impl_ozone_gbm.h:15: class GpuMemoryBufferImplOzoneGbm We shouldn't have multiple Ozone abstractions for GMBs. As long as OzoneNativeBuffer exists, all ozone GMB support should be hidden behind that interface. https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right): https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:17: #if defined(USE_OZONE_GBM) There shouldn't be a USE_OZONE_GBM ifdef here. I think you should remove the ifdef here and instead filter out MAP formats in GetSupportedGpuMemoryBufferConfigurations. https://codereview.chromium.org/1134993003/diff/1/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/1134993003/diff/1/ui/gfx/gpu_memory_buffer.h#... ui/gfx/gpu_memory_buffer.h:36: base::SharedMemoryHandle device_handle; Please use |handle| instead. https://codereview.chromium.org/1134993003/diff/1/ui/gl/gl_image_linux_dma_bu... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/1134993003/diff/1/ui/gl/gl_image_linux_dma_bu... ui/gl/gl_image_linux_dma_buffer.cc:119: static_cast<EGLClientBuffer>(nullptr), attrs); nit: I'd avoid this style change and leave this file unchanged unless it's necessary as part of this patch. https://codereview.chromium.org/1134993003/diff/1/ui/ozone/gpu/gpu_memory_buf... File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right): https://codereview.chromium.org/1134993003/diff/1/ui/ozone/gpu/gpu_memory_buf... ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:75: DCHECK(pixmap_->GetBufferUsage() == NativePixmap::SCANOUT); nit: use DCHECK_EQ https://codereview.chromium.org/1134993003/diff/1/ui/ozone/gpu/gpu_memory_buf... File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.h (right): https://codereview.chromium.org/1134993003/diff/1/ui/ozone/gpu/gpu_memory_buf... ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.h:41: gfx::GpuMemoryBufferHandle* new_handle); Please pass the handle as return value to be consistent with SharedMemory impl.
thx for reviewing. I want to listen to your opinion about merging GpuMemoryBuffer with GpuMemoryBufferImpl. https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_gbm.h (right): https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_memory_buffer_impl_ozone_gbm.h:15: class GpuMemoryBufferImplOzoneGbm On 2015/05/11 16:40:56, reveman wrote: > We shouldn't have multiple Ozone abstractions for GMBs. As long as > OzoneNativeBuffer exists, all ozone GMB support should be hidden behind that > interface. it's directly related to spang's concern also. GpuMemoryBufferImpl class is defined in content/common/gpu/client, so I could not move this implementation to ui/ozone/platform due to cyclic dependency. To hide this in ui/ozone/, GpuMemoryBufferImpl should move to another module which ui/ozone can depend on. I think it's better that GpuMemoryBuffer in ui/gfx should be merged with GpuMemoryBufferImpl, rather than moving GpuMemoryBufferImpl to ui/gfx. I'll do it in another CL. WDYT?
On 2015/05/11 at 17:11:53, dongseong.hwang wrote: > thx for reviewing. I want to listen to your opinion about merging GpuMemoryBuffer with GpuMemoryBufferImpl. > > https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... > File content/common/gpu/client/gpu_memory_buffer_impl_ozone_gbm.h (right): > > https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... > content/common/gpu/client/gpu_memory_buffer_impl_ozone_gbm.h:15: class GpuMemoryBufferImplOzoneGbm > On 2015/05/11 16:40:56, reveman wrote: > > We shouldn't have multiple Ozone abstractions for GMBs. As long as > > OzoneNativeBuffer exists, all ozone GMB support should be hidden behind that > > interface. > > it's directly related to spang's concern also. > GpuMemoryBufferImpl class is defined in content/common/gpu/client, so I could not move this implementation to ui/ozone/platform due to cyclic dependency. > To hide this in ui/ozone/, GpuMemoryBufferImpl should move to another module which ui/ozone can depend on. > I think it's better that GpuMemoryBuffer in ui/gfx should be merged with GpuMemoryBufferImpl, rather than moving GpuMemoryBufferImpl to ui/gfx. I'll do it in another CL. > WDYT? You shouldn't have to create a new GpuMemoryBufferImpl type for this. You already have GpuMemoryBufferImplOzoneNativeBuffer for this purpose. Basic interface that knows nothing about multi process usage lives ui/gfx. Multi-process implementations live in content/. I don't think we should change that.
thanks for reviewing. I address all concerns. https://codereview.chromium.org/1134993003/diff/1/build/config/ui.gni File build/config/ui.gni (right): https://codereview.chromium.org/1134993003/diff/1/build/config/ui.gni#newcode40 build/config/ui.gni:40: ozone_platform_gbm = false On 2015/05/11 15:44:58, spang wrote: > It's not acceptable to put this here. We don't want the enabled platforms to > leak outside of the ui/ozone component. > > Please use the ui/ozone/public interface in USE_OZONE builds (and change it if > necessary). Done. https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_memory_buffer_impl.cc (right): https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_memory_buffer_impl.cc:39: DCHECK(!mapped_); On 2015/05/11 16:40:56, reveman wrote: > this is fine but unrelated to this change. please change it in a separate patch got it https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_gbm.h (right): https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_memory_buffer_impl_ozone_gbm.h:15: class GpuMemoryBufferImplOzoneGbm On 2015/05/11 16:40:56, reveman wrote: > We shouldn't have multiple Ozone abstractions for GMBs. As long as > OzoneNativeBuffer exists, all ozone GMB support should be hidden behind that > interface. I understand. I merge this into OzoneNativeBuffer. https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right): https://codereview.chromium.org/1134993003/diff/1/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:17: #if defined(USE_OZONE_GBM) On 2015/05/11 16:40:56, reveman wrote: > There shouldn't be a USE_OZONE_GBM ifdef here. I think you should remove the > ifdef here and instead filter out MAP formats in > GetSupportedGpuMemoryBufferConfigurations. Done. https://codereview.chromium.org/1134993003/diff/1/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/1134993003/diff/1/ui/gfx/gpu_memory_buffer.h#... ui/gfx/gpu_memory_buffer.h:36: base::SharedMemoryHandle device_handle; On 2015/05/11 16:40:56, reveman wrote: > Please use |handle| instead. |handle| is used for prime fd, and |device_handle| is used for vgem fd. https://codereview.chromium.org/1134993003/diff/1/ui/gl/gl_image_linux_dma_bu... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/1134993003/diff/1/ui/gl/gl_image_linux_dma_bu... ui/gl/gl_image_linux_dma_buffer.cc:119: static_cast<EGLClientBuffer>(nullptr), attrs); On 2015/05/11 16:40:56, reveman wrote: > nit: I'd avoid this style change and leave this file unchanged unless it's > necessary as part of this patch. Done. https://codereview.chromium.org/1134993003/diff/1/ui/ozone/gpu/gpu_memory_buf... File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right): https://codereview.chromium.org/1134993003/diff/1/ui/ozone/gpu/gpu_memory_buf... ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:75: DCHECK(pixmap_->GetBufferUsage() == NativePixmap::SCANOUT); On 2015/05/11 16:40:57, reveman wrote: > nit: use DCHECK_EQ Done. https://codereview.chromium.org/1134993003/diff/1/ui/ozone/gpu/gpu_memory_buf... File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.h (right): https://codereview.chromium.org/1134993003/diff/1/ui/ozone/gpu/gpu_memory_buf... ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.h:41: gfx::GpuMemoryBufferHandle* new_handle); On 2015/05/11 16:40:57, reveman wrote: > Please pass the handle as return value to be consistent with SharedMemory impl. Done. In addition, I remove all pointer passing code. all function this CL adds return gfx::GpuMemoryBufferHandle. https://codereview.chromium.org/1134993003/diff/20001/content/common/gpu/clie... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc (right): https://codereview.chromium.org/1134993003/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:30: // TODO(dshwang): Both causes CRASH. How to release a handle? zachr, could you advice how to release vgem bo handle?
https://codereview.chromium.org/1134993003/diff/1/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/1134993003/diff/1/ui/gfx/gpu_memory_buffer.h#... ui/gfx/gpu_memory_buffer.h:36: base::SharedMemoryHandle device_handle; On 2015/05/14 12:25:47, dshwang wrote: > On 2015/05/11 16:40:56, reveman wrote: > > Please use |handle| instead. > > |handle| is used for prime fd, and |device_handle| is used for vgem fd. Is the device_handle different for each buffer? If not, then I don't think it belongs here. https://codereview.chromium.org/1134993003/diff/20001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1134993003/diff/20001/cc/resources/resource_p... cc/resources/resource_provider.cc:1976: // shared memory backed buffers. crbug.com/436314 GL_COMMANDS_COMPLETED_CHROMIUM has to be used with native buffers. This TODO needs to be removed before you can land this patch. https://codereview.chromium.org/1134993003/diff/20001/content/common/gpu/clie... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h (right): https://codereview.chromium.org/1134993003/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:43: void* mmap_ptr_; Please move all this into ui/ozone/ and keep one opaque object here instead. GpuMemoryBufferImplOzoneNativeBuffer is supposed to be a thin wrapper around an ozone implementation and not contain any real logic. Maybe use a NativePixmap here on the client side too or create some new type of object. SurfaceFactoryOzone::CreateNativePixmapFromHandle(base::FileDescriptor fd) maybe.. https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer.cc:14: #include "ui/gfx/gpu_memory_buffer.h" This code should not know about GpuMemoryBuffers. https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer.h (right): https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer.h:18: struct GpuMemoryBufferHandle; This code should not know about GpuMemoryBuffers. https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/public/native_... File ui/ozone/public/native_pixmap.h (right): https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/public/native_... ui/ozone/public/native_pixmap.h:12: struct GpuMemoryBufferHandle; This code should not know about GpuMemoryBuffers. https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/public/surface... File ui/ozone/public/surface_factory_ozone.h (right): https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/public/surface... ui/ozone/public/surface_factory_ozone.h:14: #include "ui/gfx/gpu_memory_buffer.h" I don't think you should include this here for circular dependency reasons. Can we use base::FileDescriptor below instead of gfx::GpuMemoryBufferHandle?
thx for quick reviewing. I'll address soon. https://codereview.chromium.org/1134993003/diff/1/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/1134993003/diff/1/ui/gfx/gpu_memory_buffer.h#... ui/gfx/gpu_memory_buffer.h:36: base::SharedMemoryHandle device_handle; On 2015/05/14 13:22:03, reveman wrote: > On 2015/05/14 12:25:47, dshwang wrote: > > On 2015/05/11 16:40:56, reveman wrote: > > > Please use |handle| instead. > > > > |handle| is used for prime fd, and |device_handle| is used for vgem fd. > > Is the device_handle different for each buffer? If not, then I don't think it > belongs here. |device_handle| is kind of singleton. I'll make (Child|Browser)GpuMemoryBufferManager keep vgem fd once to remove it in GpuMemoryBufferHandle. https://codereview.chromium.org/1134993003/diff/20001/content/common/gpu/clie... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h (right): https://codereview.chromium.org/1134993003/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:43: void* mmap_ptr_; On 2015/05/14 13:22:03, reveman wrote: > Please move all this into ui/ozone/ and keep one opaque object here instead. > GpuMemoryBufferImplOzoneNativeBuffer is supposed to be a thin wrapper around an > ozone implementation and not contain any real logic. > > Maybe use a NativePixmap here on the client side too or create some new type of > object. > > SurfaceFactoryOzone::CreateNativePixmapFromHandle(base::FileDescriptor fd) > maybe.. now I understand what you mean. It's very reasonable. I'll do it soon. https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/public/surface... File ui/ozone/public/surface_factory_ozone.h (right): https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/public/surface... ui/ozone/public/surface_factory_ozone.h:14: #include "ui/gfx/gpu_memory_buffer.h" On 2015/05/14 13:22:03, reveman wrote: > I don't think you should include this here for circular dependency reasons. > > Can we use base::FileDescriptor below instead of gfx::GpuMemoryBufferHandle? as I'll remove |device_handle| in GpuMemoryBufferHandle, export method will return just base::FileDescriptor as you mentioned.
https://codereview.chromium.org/1134993003/diff/20001/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/1134993003/diff/20001/build/linux/system.gyp#... build/linux/system.gyp:535: ['use_ozone==1', { vgem only makes sense now under gbm, so you want to wrap this around use_platform_gbm variable instead. Similarly, you will need to apply this same logic for the gn and gyp code you're changing next in this CL. https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/gpu/gpu_memory... File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right): https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/gpu/gpu_memory... ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:156: SurfaceFactoryOzone::GetInstance()->ExportGpuMemoryBufferHandle( Am I wrong or the concept here is the opposite? GPU process is *importing* a device handle through vgem, which was opened in the Renderer process? ImportGpuMemoryBufferHandle therefore makes more sense.
https://codereview.chromium.org/1134993003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/vgem_pixmap.cc:97: DCHECK_EQ(usage_, SurfaceFactoryOzone::MAP); comparison of two values with different enumeration types. s/SurfaceFactoryOzone::MAP/MAP does the needed trick. https://codereview.chromium.org/1134993003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/vgem_pixmap.cc:121: DCHECK_EQ(usage_, SurfaceFactoryOzone::MAP); ditto.
https://codereview.chromium.org/1134993003/diff/20001/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/1134993003/diff/20001/build/linux/system.gyp#... build/linux/system.gyp:535: ['use_ozone==1', { On 2015/05/14 19:20:04, vignatti wrote: > vgem only makes sense now under gbm, so you want to wrap this around > use_platform_gbm variable instead. Similarly, you will need to apply this same > logic for the gn and gyp code you're changing next in this CL. indeed, but spang don't want to leak use_platform_gbm outside ui/ozone. target drmheader is used in sandbox gyp. we don't want to let sandbox know ozone platform detail. spang, do you have good idea? https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/gpu/gpu_memory... File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right): https://codereview.chromium.org/1134993003/diff/20001/ui/ozone/gpu/gpu_memory... ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:156: SurfaceFactoryOzone::GetInstance()->ExportGpuMemoryBufferHandle( On 2015/05/14 19:20:04, vignatti wrote: > Am I wrong or the concept here is the opposite? GPU process is *importing* a > device handle through vgem, which was opened in the Renderer process? > ImportGpuMemoryBufferHandle therefore makes more sense. GPU process create gbm bo and *exporting* the dma_buf, and then Renderer is *importing* the dam_buf as vgem bo. https://codereview.chromium.org/1134993003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/vgem_pixmap.cc:97: DCHECK_EQ(usage_, SurfaceFactoryOzone::MAP); On 2015/06/18 17:00:52, vignatti wrote: > comparison of two values with different enumeration types. > s/SurfaceFactoryOzone::MAP/MAP does the needed trick. Oops, mistake. interestingly, compiler didn't complain. there is some macro dark magic. Done.
Patchset #4 (id:60001) has been deleted
Does this depend on another patch? As I don't see how this could work otherwise. https://codereview.chromium.org/1134993003/diff/80001/cc/resources/resource_p... File cc/resources/resource_provider.cc (left): https://codereview.chromium.org/1134993003/diff/80001/cc/resources/resource_p... cc/resources/resource_provider.cc:960: #if defined(OS_CHROMEOS) we should just remove this. if there are still performance issues with using fences then we need to solve them. https://codereview.chromium.org/1134993003/diff/80001/content/common/gpu/clie... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h (right): https://codereview.chromium.org/1134993003/diff/80001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:14: class GpuMemoryBufferImplOzoneNativeBuffer : public GpuMemoryBufferImpl { There's now both OzoneNativeBuffer and NativePixmap and they both refer to the same thing, afaict. Can we just use one name instead? GpuMemoryBufferFactoryOzoneNativeBuffer, GpuMemoryBufferImplOzoneNativeBuffer, OzoneNativeBufferManager, etc.
On 2015/06/25 14:07:51, reveman wrote: > Does this depend on another patch? As I don't see how this could work otherwise. This CL requires https://codereview.chromium.org/1128113011/ Thanks for reviewing. When the prerequisite CL is landed, I'll request review for this CL again. https://codereview.chromium.org/1134993003/diff/80001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1134993003/diff/80001/cc/resources/resource_p... cc/resources/resource_provider.cc:1868: else This quirk is for ARM ChromeOS perf issue. After this CL, only SHARED_MEMORY_BUFFER on ChromeOS uses this quirk. If native memory buffer is enabled, GL_COMMANDS_COMPLETED_CHROMIUM is used for correctness. If you think this quirk is not needed anymore, I'll remove it in another (prerequisite) CL. https://codereview.chromium.org/1134993003/diff/80001/content/common/gpu/clie... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h (right): https://codereview.chromium.org/1134993003/diff/80001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:14: class GpuMemoryBufferImplOzoneNativeBuffer : public GpuMemoryBufferImpl { On 2015/06/25 14:07:51, reveman wrote: > There's now both OzoneNativeBuffer and NativePixmap and they both refer to the > same thing, afaict. Can we just use one name instead? > GpuMemoryBufferFactoryOzoneNativeBuffer, GpuMemoryBufferImplOzoneNativeBuffer, > OzoneNativeBufferManager, etc. I think GpuMemoryBufferFactoryOzoneNativeBuffer and GpuMemoryBufferImplOzoneNativeBuffer should be renamed to GpuMemoryBufferFactoryOzone and GpuMemoryBufferImplOzone. Those are just ozone implementation of GpuMemoryBufferFactory and GpuMemoryBufferImpl. OzoneNativeBuffer term is redundant. After that, we have only NativePixmap term, which is ozone term for native buffer like SurfaceTexture and IOSurface. WDYT? If you are ok, I'll submit prerequisite renaming patch in another CL
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/vgem_pixmap.cc:22: return width * 4; This isn't working right and is causing rendering issues. The buffer might get created with larger than minimum stride.
https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/vgem_pixmap.cc:22: return width * 4; On 2015/06/29 19:32:18, spang wrote: > This isn't working right and is causing rendering issues. The buffer might get > created with larger than minimum stride. > I've worked around the issue by plumbing the stride through the handle. reveman@, do you have thoughts on what the proper way to communicate the stride to the renderer is? Unlike shmem we can't assume the minimum is used.
On 2015/06/29 at 21:28:19, spang wrote: > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > ui/ozone/platform/drm/gpu/vgem_pixmap.cc:22: return width * 4; > On 2015/06/29 19:32:18, spang wrote: > > This isn't working right and is causing rendering issues. The buffer might get > > created with larger than minimum stride. > > > > I've worked around the issue by plumbing the stride through the handle. > > reveman@, do you have thoughts on what the proper way to communicate the stride to the renderer is? Unlike shmem we can't assume the minimum is used. can we perform the same width->stride computation in the renderer instead of having to communicate it across process boundaries? what is the stride in this case? aligned to 64 bytes?
On 2015/06/30 04:09:09, reveman wrote: > On 2015/06/29 at 21:28:19, spang wrote: > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > ui/ozone/platform/drm/gpu/vgem_pixmap.cc:22: return width * 4; > > On 2015/06/29 19:32:18, spang wrote: > > > This isn't working right and is causing rendering issues. The buffer might > get > > > created with larger than minimum stride. > > > > > > > I've worked around the issue by plumbing the stride through the handle. > > > > reveman@, do you have thoughts on what the proper way to communicate the > stride to the renderer is? Unlike shmem we can't assume the minimum is used. > > can we perform the same width->stride computation in the renderer instead of > having to communicate it across process boundaries? what is the stride in this > case? aligned to 64 bytes? spang, thx for raising this issue. I agree. width*4 is too naive. gbm might allocate tiling buffer or something. we should ask stride to gbm like `gbm_bo_get_stride(buffer_->bo())`. However, VGEM doesn't have API to ask stride. There are two alternative solutions: 1. plumb the stride through the handle. 2. add new IOCTL to ask stride to VGEM. #1 is way easier.
spang@chromium.org changed reviewers: + marcheu@chromium.org
On 2015/06/30 11:01:40, dshwang wrote: > On 2015/06/30 04:09:09, reveman wrote: > > On 2015/06/29 at 21:28:19, spang wrote: > > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > > File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): > > > > > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > > ui/ozone/platform/drm/gpu/vgem_pixmap.cc:22: return width * 4; > > > On 2015/06/29 19:32:18, spang wrote: > > > > This isn't working right and is causing rendering issues. The buffer might > > get > > > > created with larger than minimum stride. > > > > > > > > > > I've worked around the issue by plumbing the stride through the handle. > > > > > > reveman@, do you have thoughts on what the proper way to communicate the > > stride to the renderer is? Unlike shmem we can't assume the minimum is used. > > > > can we perform the same width->stride computation in the renderer instead of > > having to communicate it across process boundaries? what is the stride in this > > case? aligned to 64 bytes? Doubtful, it would require vendor driver code in the render because that's where the calculation is. The purpose of VGEM is partly to avoid having to load a vendor driver .so in the renderer. > > spang, thx for raising this issue. I agree. width*4 is too naive. gbm might > allocate tiling buffer or something. we should ask stride to gbm like > `gbm_bo_get_stride(buffer_->bo())`. > However, VGEM doesn't have API to ask stride. There are two alternative > solutions: > 1. plumb the stride through the handle. > 2. add new IOCTL to ask stride to VGEM. > > #1 is way easier.
On 2015/06/30 17:40:13, spang wrote: > On 2015/06/30 11:01:40, dshwang wrote: > > On 2015/06/30 04:09:09, reveman wrote: > > > On 2015/06/29 at 21:28:19, spang wrote: > > > > > > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > > > File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > > > ui/ozone/platform/drm/gpu/vgem_pixmap.cc:22: return width * 4; > > > > On 2015/06/29 19:32:18, spang wrote: > > > > > This isn't working right and is causing rendering issues. The buffer > might > > > get > > > > > created with larger than minimum stride. > > > > > > > > > > > > > I've worked around the issue by plumbing the stride through the handle. > > > > > > > > reveman@, do you have thoughts on what the proper way to communicate the > > > stride to the renderer is? Unlike shmem we can't assume the minimum is used. > > > > > > can we perform the same width->stride computation in the renderer instead of > > > having to communicate it across process boundaries? what is the stride in > this > > > case? aligned to 64 bytes? > > Doubtful, it would require vendor driver code in the render because that's where > the calculation is. The purpose of VGEM is partly to avoid having to load a > vendor driver .so in the renderer. > > > > > spang, thx for raising this issue. I agree. width*4 is too naive. gbm might > > allocate tiling buffer or something. we should ask stride to gbm like > > `gbm_bo_get_stride(buffer_->bo())`. > > However, VGEM doesn't have API to ask stride. There are two alternative > > solutions: > > 1. plumb the stride through the handle. > > 2. add new IOCTL to ask stride to VGEM. > > > > #1 is way easier. Stephane says there's no metadata attached to the kernel object (it's just memory). So any metadata will have to be communicated to the renderer. I'm not sure stuffing it in the handle is appropriate though. Should we do something behind the scenes like IO surface does with it's mach transaction?
On 2015/06/30 at 17:55:43, spang wrote: > On 2015/06/30 17:40:13, spang wrote: > > On 2015/06/30 11:01:40, dshwang wrote: > > > On 2015/06/30 04:09:09, reveman wrote: > > > > On 2015/06/29 at 21:28:19, spang wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > > > > File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > > > > ui/ozone/platform/drm/gpu/vgem_pixmap.cc:22: return width * 4; > > > > > On 2015/06/29 19:32:18, spang wrote: > > > > > > This isn't working right and is causing rendering issues. The buffer > > might > > > > get > > > > > > created with larger than minimum stride. > > > > > > > > > > > > > > > > I've worked around the issue by plumbing the stride through the handle. > > > > > > > > > > reveman@, do you have thoughts on what the proper way to communicate the > > > > stride to the renderer is? Unlike shmem we can't assume the minimum is used. > > > > > > > > can we perform the same width->stride computation in the renderer instead of > > > > having to communicate it across process boundaries? what is the stride in > > this > > > > case? aligned to 64 bytes? > > > > Doubtful, it would require vendor driver code in the render because that's where > > the calculation is. The purpose of VGEM is partly to avoid having to load a > > vendor driver .so in the renderer. > > > > > > > > spang, thx for raising this issue. I agree. width*4 is too naive. gbm might > > > allocate tiling buffer or something. we should ask stride to gbm like > > > `gbm_bo_get_stride(buffer_->bo())`. > > > However, VGEM doesn't have API to ask stride. There are two alternative > > > solutions: > > > 1. plumb the stride through the handle. > > > 2. add new IOCTL to ask stride to VGEM. > > > > > > #1 is way easier. > > Stephane says there's no metadata attached to the kernel object (it's just memory). > > So any metadata will have to be communicated to the renderer. I'm not sure stuffing it in the handle is appropriate though. Should we do something behind the scenes like IO surface does with it's mach transaction? Yes, the handle is not appropriate. What does the gbm_bo_get_stride implementation look like? is completely different depending on the driver or something basic as pad to 64 bytes? If the latter, then maybe we can just copy that logic to the renderer for now.
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
Patchset #5 (id:180001) has been deleted
On 2015/07/01 22:30:19, reveman wrote: > On 2015/06/30 at 17:55:43, spang wrote: > > On 2015/06/30 17:40:13, spang wrote: > > > On 2015/06/30 11:01:40, dshwang wrote: > > > > On 2015/06/30 04:09:09, reveman wrote: > > > > > On 2015/06/29 at 21:28:19, spang wrote: > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > > > > > File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > > > > > ui/ozone/platform/drm/gpu/vgem_pixmap.cc:22: return width * 4; > > > > > > On 2015/06/29 19:32:18, spang wrote: > > > > > > > This isn't working right and is causing rendering issues. The buffer > > > might > > > > > get > > > > > > > created with larger than minimum stride. > > > > > > > > > > > > > > > > > > > I've worked around the issue by plumbing the stride through the > handle. > > > > > > > > > > > > reveman@, do you have thoughts on what the proper way to communicate > the > > > > > stride to the renderer is? Unlike shmem we can't assume the minimum is > used. > > > > > > > > > > can we perform the same width->stride computation in the renderer > instead of > > > > > having to communicate it across process boundaries? what is the stride > in > > > this > > > > > case? aligned to 64 bytes? > > > > > > Doubtful, it would require vendor driver code in the render because that's > where > > > the calculation is. The purpose of VGEM is partly to avoid having to load a > > > vendor driver .so in the renderer. > > > > > > > > > > > spang, thx for raising this issue. I agree. width*4 is too naive. gbm > might > > > > allocate tiling buffer or something. we should ask stride to gbm like > > > > `gbm_bo_get_stride(buffer_->bo())`. > > > > However, VGEM doesn't have API to ask stride. There are two alternative > > > > solutions: > > > > 1. plumb the stride through the handle. > > > > 2. add new IOCTL to ask stride to VGEM. > > > > > > > > #1 is way easier. > > > > Stephane says there's no metadata attached to the kernel object (it's just > memory). > > > > So any metadata will have to be communicated to the renderer. I'm not sure > stuffing it in the handle is appropriate though. Should we do something behind > the scenes like IO surface does with it's mach transaction? > > Yes, the handle is not appropriate. What does the gbm_bo_get_stride > implementation look like? is completely different depending on the driver or > something basic as pad to 64 bytes? If the latter, then maybe we can just copy > that logic to the renderer for now. Unfortunately the stride initialization is completely different depending on the driver. Looks like Intel adds some alignment to the size based on tiling mode. It's probably not maintainable to duplicate these constraints in the renderer. So I think we'll need a mechanism to communicate metadata. So I think we do need some way to plumb buffer metadata.
On 2015/07/01 22:30:19, reveman wrote: > On 2015/06/30 at 17:55:43, spang wrote: > > On 2015/06/30 17:40:13, spang wrote: > > > On 2015/06/30 11:01:40, dshwang wrote: > > > > On 2015/06/30 04:09:09, reveman wrote: > > > > > On 2015/06/29 at 21:28:19, spang wrote: > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > > > > > File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > > > > > ui/ozone/platform/drm/gpu/vgem_pixmap.cc:22: return width * 4; > > > > > > On 2015/06/29 19:32:18, spang wrote: > > > > > > > This isn't working right and is causing rendering issues. The buffer > > > might > > > > > get > > > > > > > created with larger than minimum stride. > > > > > > > > > > > > > > > > > > > I've worked around the issue by plumbing the stride through the > handle. > > > > > > > > > > > > reveman@, do you have thoughts on what the proper way to communicate > the > > > > > stride to the renderer is? Unlike shmem we can't assume the minimum is > used. > > > > > > > > > > can we perform the same width->stride computation in the renderer > instead of > > > > > having to communicate it across process boundaries? what is the stride > in > > > this > > > > > case? aligned to 64 bytes? > > > > > > Doubtful, it would require vendor driver code in the render because that's > where > > > the calculation is. The purpose of VGEM is partly to avoid having to load a > > > vendor driver .so in the renderer. > > > > > > > > > > > spang, thx for raising this issue. I agree. width*4 is too naive. gbm > might > > > > allocate tiling buffer or something. we should ask stride to gbm like > > > > `gbm_bo_get_stride(buffer_->bo())`. > > > > However, VGEM doesn't have API to ask stride. There are two alternative > > > > solutions: > > > > 1. plumb the stride through the handle. > > > > 2. add new IOCTL to ask stride to VGEM. > > > > > > > > #1 is way easier. > > > > Stephane says there's no metadata attached to the kernel object (it's just > memory). > > > > So any metadata will have to be communicated to the renderer. I'm not sure > stuffing it in the handle is appropriate though. Should we do something behind > the scenes like IO surface does with it's mach transaction? > > Yes, the handle is not appropriate. What does the gbm_bo_get_stride > implementation look like? is completely different depending on the driver or > something basic as pad to 64 bytes? If the latter, then maybe we can just copy > that logic to the renderer for now. Unfortunately the stride initialization is completely different depending on the driver. Looks like Intel adds some alignment to the size based on tiling mode. It's probably not maintainable to duplicate these constraints in the renderer. So I think we'll need a mechanism to communicate metadata.
On 2015/07/13 17:29:26, spang wrote: > On 2015/07/01 22:30:19, reveman wrote: > > On 2015/06/30 at 17:55:43, spang wrote: > > > On 2015/06/30 17:40:13, spang wrote: > > > > On 2015/06/30 11:01:40, dshwang wrote: > > > > > On 2015/06/30 04:09:09, reveman wrote: > > > > > > On 2015/06/29 at 21:28:19, spang wrote: > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > > > > > > File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1134993003/diff/140001/ui/ozone/platform/drm/... > > > > > > > ui/ozone/platform/drm/gpu/vgem_pixmap.cc:22: return width * 4; > > > > > > > On 2015/06/29 19:32:18, spang wrote: > > > > > > > > This isn't working right and is causing rendering issues. The > buffer > > > > might > > > > > > get > > > > > > > > created with larger than minimum stride. > > > > > > > > > > > > > > > > > > > > > > I've worked around the issue by plumbing the stride through the > > handle. > > > > > > > > > > > > > > reveman@, do you have thoughts on what the proper way to communicate > > the > > > > > > stride to the renderer is? Unlike shmem we can't assume the minimum is > > used. > > > > > > > > > > > > can we perform the same width->stride computation in the renderer > > instead of > > > > > > having to communicate it across process boundaries? what is the stride > > in > > > > this > > > > > > case? aligned to 64 bytes? > > > > > > > > Doubtful, it would require vendor driver code in the render because that's > > where > > > > the calculation is. The purpose of VGEM is partly to avoid having to load > a > > > > vendor driver .so in the renderer. > > > > > > > > > > > > > > spang, thx for raising this issue. I agree. width*4 is too naive. gbm > > might > > > > > allocate tiling buffer or something. we should ask stride to gbm like > > > > > `gbm_bo_get_stride(buffer_->bo())`. > > > > > However, VGEM doesn't have API to ask stride. There are two alternative > > > > > solutions: > > > > > 1. plumb the stride through the handle. > > > > > 2. add new IOCTL to ask stride to VGEM. > > > > > > > > > > #1 is way easier. > > > > > > Stephane says there's no metadata attached to the kernel object (it's just > > memory). > > > > > > So any metadata will have to be communicated to the renderer. I'm not sure > > stuffing it in the handle is appropriate though. Should we do something behind > > the scenes like IO surface does with it's mach transaction? > > > > Yes, the handle is not appropriate. What does the gbm_bo_get_stride > > implementation look like? is completely different depending on the driver or > > something basic as pad to 64 bytes? If the latter, then maybe we can just copy > > that logic to the renderer for now. > > Unfortunately the stride initialization is completely different depending on the > driver. Looks like Intel adds some alignment to the size based on tiling mode. > It's probably not maintainable to duplicate these constraints in the renderer. > > So I think we'll need a mechanism to communicate metadata. Indeed. The stride depends on |size| of GpuMemoryBuffer, so stride should be communicated per each GpuMemoryBuffer. I think GpuMemoryBufferHandle is the best place.
https://codereview.chromium.org/1134993003/diff/200001/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/1134993003/diff/200001/build/linux/system.gyp... build/linux/system.gyp:527: ['use_ozone==1', { Please move to ui/ozone/platform/drm/drm.gypi https://codereview.chromium.org/1134993003/diff/200001/sandbox/linux/BUILD.gn File sandbox/linux/BUILD.gn (right): https://codereview.chromium.org/1134993003/diff/200001/sandbox/linux/BUILD.gn... sandbox/linux/BUILD.gn:232: pkg_config("libdrm") { This isn't needed. <drm.h> is provided by libc. Please just use <drm.h> and don't add unnecessary dependencies on libdrm to the sandbox. https://codereview.chromium.org/1134993003/diff/200001/sandbox/linux/seccomp-... File sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc (left): https://codereview.chromium.org/1134993003/diff/200001/sandbox/linux/seccomp-... sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc:152: ResultExpr RestrictIoctl() { I think you should split sandbox changes into a separate CL (for now, you can say test with --no-sandbox in the description). But anyway, I think as far as whitelisting the 3 ioctls, what you want is: ResultExpr RestrictIoctl() { auto reference_type = TCGETS; const Arg<decltype(reference_type)> request(1); return Switch(request) .CASES(((decltype(reference_type))TCGETS, FIONREAD), Allow()) #if defined(OS_CHROMEOS) .CASES(((decltype(reference_type))DRM_IOCTL_GEM_CLOSE, DRM_IOCTL_VGEM_MODE_MAP_DUMB, DRM_IOCTL_PRIME_FD_TO_HANDLE), Allow()) #endif .Default(CrashSIGSYSIoctl()); } https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/gpu/gpu_memor... File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right): https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/gpu/gpu_memor... ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:206: OzonePlatform::GetInstance()->GetSurfaceFactoryOzone()->ExportDmaBuf( What's the reason to have a SurfaceFactoryOzone::ExportDmaBuf() that just calls dup(pixmap->GetDmaBufFd())? Seems like this is just int dmabuf_fd = pixmap->GetDmaBufFd(); if (dmabuf_fd < 0) // fail dmabuf_fd = dup(dmabuf_fd); if (dmabuf_fd < 0) // also fail handle.handle = base::FileDescriptor(dmabuf_fd, true /* auto_close */); https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/vgem_pixmap.cc:70: int ret = drmIoctl(vgem_fd_.fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy); I think you want DRM_IOCTL_GEM_CLOSE here. https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/vgem_pixmap.cc:124: int ret = drmIoctl(vgem_fd_.fd, DRM_IOCTL_MODE_MAP_DUMB, &mmap_arg); I think you want DRM_IOCTL_VGEM_MODE_MAP_DUMB here. You'll probably need to make this entire feature conditional similar to how we do with USE_MESA_PLATFORM_NULL, because DRM_IOCTL_VGEM_MODE_MAP_DUMB is not upstream.
https://codereview.chromium.org/1134993003/diff/200001/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/1134993003/diff/200001/build/linux/system.gyp... build/linux/system.gyp:527: ['use_ozone==1', { On 2015/07/24 22:08:25, spang wrote: > Please move to ui/ozone/platform/drm/drm.gypi Actually, it seems is a duplicate platform/drm so should just remove it.
https://codereview.chromium.org/1134993003/diff/200001/sandbox/linux/BUILD.gn File sandbox/linux/BUILD.gn (right): https://codereview.chromium.org/1134993003/diff/200001/sandbox/linux/BUILD.gn... sandbox/linux/BUILD.gn:232: pkg_config("libdrm") { On 2015/07/24 22:08:25, spang wrote: > This isn't needed. > > <drm.h> is provided by libc. Please just use <drm.h> and don't add unnecessary > dependencies on libdrm to the sandbox. Er, it's actually <drm/drm.h>.
Thank you for reviewing. this CL is blocked by crrev.com/1128113011 I add new gyp define; ozone_use_vgem_map, because vgem map is not upstream. https://codereview.chromium.org/1134993003/diff/200001/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/1134993003/diff/200001/build/linux/system.gyp... build/linux/system.gyp:527: ['use_ozone==1', { On 2015/07/24 22:15:10, spang wrote: > On 2015/07/24 22:08:25, spang wrote: > > Please move to ui/ozone/platform/drm/drm.gypi > > Actually, it seems is a duplicate platform/drm so should just remove it. Done. removed it. https://codereview.chromium.org/1134993003/diff/200001/sandbox/linux/BUILD.gn File sandbox/linux/BUILD.gn (right): https://codereview.chromium.org/1134993003/diff/200001/sandbox/linux/BUILD.gn... sandbox/linux/BUILD.gn:232: pkg_config("libdrm") { On 2015/07/24 22:42:21, spang wrote: > On 2015/07/24 22:08:25, spang wrote: > > This isn't needed. > > > > <drm.h> is provided by libc. Please just use <drm.h> and don't add unnecessary > > dependencies on libdrm to the sandbox. > > Er, it's actually <drm/drm.h>. actually we need vgem_drm.h, so I add <libdrm/vgem_drm.h> https://codereview.chromium.org/1134993003/diff/200001/sandbox/linux/seccomp-... File sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc (left): https://codereview.chromium.org/1134993003/diff/200001/sandbox/linux/seccomp-... sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc:152: ResultExpr RestrictIoctl() { On 2015/07/24 22:08:25, spang wrote: > I think you should split sandbox changes into a separate CL (for now, you can > say test with --no-sandbox in the description). correct. I separate this part into https://codereview.chromium.org/1253363004 > But anyway, I think as far as whitelisting the 3 ioctls, what you want is: > > ResultExpr RestrictIoctl() { > auto reference_type = TCGETS; > const Arg<decltype(reference_type)> request(1); > return Switch(request) > .CASES(((decltype(reference_type))TCGETS, FIONREAD), Allow()) > #if defined(OS_CHROMEOS) > .CASES(((decltype(reference_type))DRM_IOCTL_GEM_CLOSE, > DRM_IOCTL_VGEM_MODE_MAP_DUMB, DRM_IOCTL_PRIME_FD_TO_HANDLE), > Allow()) > #endif > .Default(CrashSIGSYSIoctl()); > } > following code is needed because TCGETS is 32bit int but DRM_IOCTL_GEM_CLOSE is 64bit int. cast like "(decltype(reference_type))DRM_IOCTL_GEM_CLOSE" trims upper 32bit and causes unexpected behavior. #if defined(OZONE_USE_VGEM_MAP) auto reference_type = DRM_IOCTL_GEM_CLOSE; #else auto reference_type = TCGETS; #endif https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/gpu/gpu_memor... File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right): https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/gpu/gpu_memor... ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:206: OzonePlatform::GetInstance()->GetSurfaceFactoryOzone()->ExportDmaBuf( On 2015/07/24 22:08:25, spang wrote: > What's the reason to have a SurfaceFactoryOzone::ExportDmaBuf() that just calls > dup(pixmap->GetDmaBufFd())? Seems like this is just > > int dmabuf_fd = pixmap->GetDmaBufFd(); > if (dmabuf_fd < 0) > // fail > > dmabuf_fd = dup(dmabuf_fd); > if (dmabuf_fd < 0) > // also fail > > handle.handle = base::FileDescriptor(dmabuf_fd, true /* auto_close */); good point! Done. https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/vgem_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/vgem_pixmap.cc:124: int ret = drmIoctl(vgem_fd_.fd, DRM_IOCTL_MODE_MAP_DUMB, &mmap_arg); On 2015/07/24 22:08:25, spang wrote: > I think you want DRM_IOCTL_VGEM_MODE_MAP_DUMB here. Done. fyi, inside vgem implementation, DRM_IOCTL_VGEM_MODE_MAP_DUMB equals to DRM_IOCTL_MODE_MAP_DUMB. Now, include vgem_drm.h instead of drm.h > You'll probably need to make this entire feature conditional similar to how we > do with USE_MESA_PLATFORM_NULL, because DRM_IOCTL_VGEM_MODE_MAP_DUMB is not > upstream. Good point. I added OZONE_USE_VGEM_MAP. GYP_DEFINES=$GYP_DEFINES" ozone_use_vgem_map=1" enables this macro. Without ozone_use_vgem_map define, gbm doesn't support MAP
Patchset #7 (id:240001) has been deleted
Hi, I work this CL earlier than http://crrev.com/1248713002/ as spang said. Could you review this main CL of this project? :) https://codereview.chromium.org/1134993003/diff/280001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1134993003/diff/280001/cc/resources/resource_... cc/resources/resource_provider.cc:1867: gfx::SHARED_MEMORY_BUFFER) This quirk was introduced for ARM chrome os. until now, chrome os uses only gfx::SHARED_MEMORY_BUFFER, so this quirk is possible. This CL makes chromeos use gfx::OZONE_NATIVE_PIXMAP and gfx::SHARED_MEMORY_BUFFER. So this quirk is applied to only gfx::SHARED_MEMORY_BUFFER. reveman, If https://codereview.chromium.org/1221433002/ is landed, I'll remove this code change. https://codereview.chromium.org/1134993003/diff/280001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/280001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:20: // TODO(dshwang): remove ad-hoc file open. crrev.com/1248713002 as spang requested in https://codereview.chromium.org/1248713002/#msg5, I work this CL prior to the VGEM fd transfer CL. I'll remove this ad-hoc vgem open in the CL later.
Can you rebase this on https://codereview.chromium.org/1263323004/ ? https://codereview.chromium.org/1134993003/diff/280001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1134993003/diff/280001/cc/resources/resource_... cc/resources/resource_provider.cc:1867: gfx::SHARED_MEMORY_BUFFER) On 2015/08/05 at 12:33:16, dshwang wrote: > This quirk was introduced for ARM chrome os. until now, chrome os uses only gfx::SHARED_MEMORY_BUFFER, so this quirk is possible. > This CL makes chromeos use gfx::OZONE_NATIVE_PIXMAP and gfx::SHARED_MEMORY_BUFFER. So this quirk is applied to only gfx::SHARED_MEMORY_BUFFER. > > reveman, If https://codereview.chromium.org/1221433002/ is landed, I'll remove this code change. I'd rather not make this nastier than it already is. Let's try to fix the polling interval on the service side so this problem goes away. All platforms will benefit from that.
Patchset #9 (id:300001) has been deleted
On 2015/08/05 14:11:49, reveman wrote: > Can you rebase this on https://codereview.chromium.org/1263323004/ ? Yes, I rebase it on the CL. Could you review again? https://codereview.chromium.org/1134993003/diff/280001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1134993003/diff/280001/cc/resources/resource_... cc/resources/resource_provider.cc:1867: gfx::SHARED_MEMORY_BUFFER) On 2015/08/05 14:11:49, reveman wrote: > On 2015/08/05 at 12:33:16, dshwang wrote: > > This quirk was introduced for ARM chrome os. until now, chrome os uses only > gfx::SHARED_MEMORY_BUFFER, so this quirk is possible. > > This CL makes chromeos use gfx::OZONE_NATIVE_PIXMAP and > gfx::SHARED_MEMORY_BUFFER. So this quirk is applied to only > gfx::SHARED_MEMORY_BUFFER. > > > > reveman, If https://codereview.chromium.org/1221433002/ is landed, I'll remove > this code change. > > I'd rather not make this nastier than it already is. Let's try to fix the > polling interval on the service side so this problem goes away. All platforms > will benefit from that. Ok, I remove this change. We will resolve this issue in https://codereview.chromium.org/1221433002/
https://codereview.chromium.org/1134993003/diff/320001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/320001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:5: #include "content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.h" why do we need any of these changes to this file? I think it was much cleaner before. https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/BUILD.gn (right): https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/BUILD.gn:205: defines += [ "OZONE_USE_VGEM_MAP" ] why is this define needed? https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:42: if (vgem_fd_.get() >= 0) { why would this fail? https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:54: DCHECK(usage == gfx::BufferUsage::MAP); why do we care what the usage is? https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:59: if (!pixmap->Initialize()) can you move the work done in Initialize out here and use the ctor at the time we know this can't fail. https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:13: #if defined(OZONE_USE_VGEM_MAP) why would you be building this file if VGEM is not enabled? https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:31: DCHECK(vgem_fd_.fd >= 0); DCHECK_GT https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:32: DCHECK(dma_buf_.get() >= 0); DCHECK_GT https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:36: DCHECK(vgem_fd_.fd); why another dcheck here that is different from ctor? https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:46: if (vgem_bo_handle_) { note that this would be cleaner if you removed Initialize and didn't have to deal with the possibility that it can fail. see my ImportFromHandle comment. https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:48: memset(&close, 0, sizeof(close)); = { 0 } above instead? https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:75: mmap_ptr_ = mmap(nullptr, size, (PROT_READ | PROT_WRITE), MAP_SHARED, can we keep it consistently mapped instead of doing that each time Map is called? https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h (right): https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:26: // ClientNativePixmap: Overridden from Cli...
Thank you for nice review. I resolved all comments. Could you review again? https://codereview.chromium.org/1134993003/diff/320001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/320001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:5: #include "content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.h" On 2015/08/06 12:08:35, reveman wrote: > why do we need any of these changes to this file? I think it was much cleaner > before. Done. https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/BUILD.gn (right): https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/BUILD.gn:205: defines += [ "OZONE_USE_VGEM_MAP" ] On 2015/08/06 12:08:35, reveman wrote: > why is this define needed? upstream linux doesn't have vgem_drm.h, so only cros linux can use vgem_drm.h with the define. https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:42: if (vgem_fd_.get() >= 0) { On 2015/08/06 12:08:35, reveman wrote: > why would this fail? upstream linux doens't have vgem device. In addition, ctor opens "/dev/dri/renderD129" by itself. without --no-sandbox, it's failed. In addition, OZONE_USE_VGEM_MAP checks VGEM map is supported, not VGEM is enabled. In cros linux 3.14, VGEM is enabled on all platform. However, VGEM map is supported in only Intel platform. https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:54: DCHECK(usage == gfx::BufferUsage::MAP); On 2015/08/06 12:08:35, reveman wrote: > why do we care what the usage is? Either GpuMemoryBufferImplOzoneNativePixmap or ClientNativePixmapFactory or ClientNativePixmapVgem controls logic along with usage. I decided GpuMemoryBufferImplOzoneNativePixmap to control, but you seem to want ClientNativePixmapVgem to handle usage. In scanout case, it's overkill to creat vgem bo, because it's never used. It's why I didn't create pixmap explicitly in GpuMemoryBufferImplOzoneNativePixmap. In addition, upstream linux must be able to use scanout buffer without vgem device. Let me make ClientNativePixmapFactory control it. https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:59: if (!pixmap->Initialize()) On 2015/08/06 12:08:35, reveman wrote: > can you move the work done in Initialize out here and use the ctor at the time > we know this can't fail. do you mean that this function call directly "drmPrimeFDToHandle(vgem_fd_.fd, dma_buf_.get(), &vgem_bo_handle_)"? It's vgem detail. ClientNativePixmapFactoryGbm should not know it. I replace Initialize() with Create() https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:13: #if defined(OZONE_USE_VGEM_MAP) On 2015/08/06 12:08:35, reveman wrote: > why would you be building this file if VGEM is not enabled? Done. This file is compiled if only OZONE_USE_VGEM_MAP is defined. https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:31: DCHECK(vgem_fd_.fd >= 0); On 2015/08/06 12:08:35, reveman wrote: > DCHECK_GT Done. https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:32: DCHECK(dma_buf_.get() >= 0); On 2015/08/06 12:08:35, reveman wrote: > DCHECK_GT Done. https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:36: DCHECK(vgem_fd_.fd); On 2015/08/06 12:08:35, reveman wrote: > why another dcheck here that is different from ctor? Done. removed Initialize() https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:46: if (vgem_bo_handle_) { On 2015/08/06 12:08:35, reveman wrote: > note that this would be cleaner if you removed Initialize and didn't have to > deal with the possibility that it can fail. see my ImportFromHandle comment. Got it. Resolve it via Create(). Done. https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:48: memset(&close, 0, sizeof(close)); On 2015/08/06 12:08:35, reveman wrote: > = { 0 } above instead? good idea. I copied the code from linux :) https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:75: mmap_ptr_ = mmap(nullptr, size, (PROT_READ | PROT_WRITE), MAP_SHARED, On 2015/08/06 12:08:35, reveman wrote: > can we keep it consistently mapped instead of doing that each time Map is > called? Good idea. It's possible at least in Intel Core architecture, because cpu cache and gpu cache is coherent via last level cache snooping. Keep it consistently mapped now. If we will find ARM or Intel ATOM has different requirement, this code will be changed. Currently vgem map is enabled in only Intel Core architecture; https://chromium-review.googlesource.com/#/c/288890/ https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h (right): https://codereview.chromium.org/1134993003/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:26: // ClientNativePixmap: On 2015/08/06 12:08:35, reveman wrote: > Overridden from Cli... Done.
zachr@, wdyt? https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:9: #include <vgem_drm.h> IIUC this header file is meant to be used within the kernel only. I believe what needs to be done actually is to split up those userspace things (e.g DRM_IOCTL_VGEM_MODE_MAP_DUMB) from this file into another one that should be place under include/uapi.
https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:9: #include <vgem_drm.h> On 2015/08/06 20:56:26, vignatti wrote: > IIUC this header file is meant to be used within the kernel only. I believe what > needs to be done actually is to split up those userspace things (e.g > DRM_IOCTL_VGEM_MODE_MAP_DUMB) from this file into another one that should be > place under include/uapi. > it is in /usr/include/libdrm/vgem_drm.h. In my opinion, it's fine as /usr/include/libdrm/i915_drm.h or nouveau_drm.h is fine.
https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:57: if (usage == gfx::BufferUsage::SCANOUT) would it hurt to still create the client pixmap? https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gbm.gypi (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gbm.gypi:14: 'ozone_use_vgem_map%': 0, just to be sure I understand. we have hardware that use gbm but doesn't support vgem yet, correct? in that case, can you add a TODO about removing this flag when all gbm hardware support vgem? https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:25: DCHECK(usage != gfx::BufferUsage::SCANOUT); Why do we care? What part of the code here doesn't work when usage is SCANOUT? https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:27: int ret = drmPrimeFDToHandle(vgem_handle.fd, handle.fd.fd, &vgem_bo_handle); Would it not be more appropriate to keep this code with the owner of vgem_handle? It seems to me like the owner of the vgem handle is the code that should know how to create a vgem_bo_handle from it and not this code here.. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:89: mmap_ptr_ = mmap(nullptr, size, (PROT_READ | PROT_WRITE), MAP_SHARED, Why not mmap in the ctor instead? I think that would make the code significantly cleaner. It's also preferred to pay as much of the cost as possible in the ctor as GMB client code assumes that's expensive. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:91: DCHECK(mmap_ptr_ != MAP_FAILED); nit: DCHECK_NE https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:97: TRACE_EVENT0("gpu", "ClientNativePixmapVgem::Unmap"); this trace doesn't seem very useful https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:40: base::ScopedFD dma_buf_; what is this used for? https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:43: gfx::BufferFormat format_; where is this used? https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:44: gfx::BufferUsage usage_; what do we need this for? https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:47: void* mmap_ptr_; s/mmap_ptr_/data_/ ?
https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gbm.gypi (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gbm.gypi:14: 'ozone_use_vgem_map%': 0, On 2015/08/07 14:35:16, reveman wrote: > just to be sure I understand. we have hardware that use gbm but doesn't support > vgem yet, correct? in that case, can you add a TODO about removing this flag > when all gbm hardware support vgem? correct, but I don't think it's about hardware only or its driver supporting dma-buf userspace mmap etc. It's more about CrOS vs other Linux and their graphics subsystem allocator of choice. For example, we were shipping out a Web view based on Ozone-GBM running on Yocto, which doesn't have VGEM at all. So atm I think it's more about choosing a graphics module, testing it, consolidate the implementation, decide which is the one and so on.
I addressed all comments. I roll back consistent map, because it doesn't work well in IvyBridge. See below comment. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:57: if (usage == gfx::BufferUsage::SCANOUT) On 2015/08/07 14:35:16, reveman wrote: > would it hurt to still create the client pixmap? Upstream linux cannot create vgem pixmap because vgem is not upstream. SCANOUT should work with upstream linux. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gbm.gypi (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gbm.gypi:14: 'ozone_use_vgem_map%': 0, On 2015/08/07 14:35:16, reveman wrote: > just to be sure I understand. we have hardware that use gbm but doesn't support > vgem yet, correct? in that case, can you add a TODO about removing this flag > when all gbm hardware support vgem? correct. Done. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:25: DCHECK(usage != gfx::BufferUsage::SCANOUT); On 2015/08/07 14:35:16, reveman wrote: > Why do we care? What part of the code here doesn't work when usage is SCANOUT? drmPrimeFDToHandle can fail on the hardware, which doesn't support vgem SCANOUT should work well on the hardware. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:27: int ret = drmPrimeFDToHandle(vgem_handle.fd, handle.fd.fd, &vgem_bo_handle); On 2015/08/07 14:35:16, reveman wrote: > Would it not be more appropriate to keep this code with the owner of > vgem_handle? It seems to me like the owner of the vgem handle is the code > that should know how to create a vgem_bo_handle from it and not this code here.. ClientNativePixmapFactoryGbm has role to manage vgem device fd. ClientNativePixmapVgem has role to manage vgem bo (a.k.a buffer) fd. drmPrimeFDToHandle is drm bo api and vgem bo is one of drm bo. So I think ClientNativePixmapVgem should use this API. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:89: mmap_ptr_ = mmap(nullptr, size, (PROT_READ | PROT_WRITE), MAP_SHARED, On 2015/08/07 14:35:16, reveman wrote: > Why not mmap in the ctor instead? I think that would make the code significantly > cleaner. It's also preferred to pay as much of the cost as possible in the ctor > as GMB client code assumes that's expensive. I roll back consistent map. It worked well with Haswell, but there are many glitch with IvyBridge. Different hardware seems to have different cache coherent mechanism. We need full understand and lots of tests for "consistent map". We will re-visit it later. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:91: DCHECK(mmap_ptr_ != MAP_FAILED); On 2015/08/07 14:35:16, reveman wrote: > nit: DCHECK_NE Done. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:97: TRACE_EVENT0("gpu", "ClientNativePixmapVgem::Unmap"); On 2015/08/07 14:35:16, reveman wrote: > this trace doesn't seem very useful after roll back, it seems useful https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:40: base::ScopedFD dma_buf_; On 2015/08/07 14:35:16, reveman wrote: > what is this used for? Client should keep dmabuf fd and close it after vgem bo handle is closed. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:43: gfx::BufferFormat format_; On 2015/08/07 14:35:16, reveman wrote: > where is this used? Removed. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:44: gfx::BufferUsage usage_; On 2015/08/07 14:35:16, reveman wrote: > what do we need this for? Removed. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:47: void* mmap_ptr_; On 2015/08/07 14:35:16, reveman wrote: > s/mmap_ptr_/data_/ ? Done.
https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:27: int ret = drmPrimeFDToHandle(vgem_handle.fd, handle.fd.fd, &vgem_bo_handle); On 2015/08/07 at 15:36:46, dshwang wrote: > On 2015/08/07 14:35:16, reveman wrote: > > Would it not be more appropriate to keep this code with the owner of > > vgem_handle? It seems to me like the owner of the vgem handle is the code > > that should know how to create a vgem_bo_handle from it and not this code here.. > > ClientNativePixmapFactoryGbm has role to manage vgem device fd. > ClientNativePixmapVgem has role to manage vgem bo (a.k.a buffer) fd. > drmPrimeFDToHandle is drm bo api and vgem bo is one of drm bo. > So I think ClientNativePixmapVgem should use this API. ClientNativePixmapFactoryGbm is clearly aware of vgem too and you already have factory functionality there so I'm still failing to see why we'd need another level of factory code here but I don't feel that strongly about it. ui/ozone reviewer can decide how they prefer this to be done. https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:40: base::ScopedFD dma_buf_; On 2015/08/07 at 15:36:46, dshwang wrote: > On 2015/08/07 14:35:16, reveman wrote: > > what is this used for? > > Client should keep dmabuf fd and close it after vgem bo handle is closed. drmPrimeFDToHandle doesn't dup the fd? That's a bit weird. Maybe wrap the bo handle in a class that makes sure that's case https://codereview.chromium.org/1134993003/diff/380001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/380001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:39: DCHECK(!mapped_); Looks like you already have this dcheck in the pixmap implementation. Why are you also adding it here? One dcheck is enough. https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/BUILD.gn (right): https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/BUILD.gn:13: ozone_use_vgem_map = false nit: s/hardwares/hardware/ can you create a bug? ozone_use_vgem enough? however, I'm still a bit confused about this. is the idea that we define this when we know if hardware supports vgem? can we know that at compile time? if not, why not include vgem_drm.h in third_party and remove this compile time setting? both compile time and run time checking seems unnecessary.. https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:57: if (usage == gfx::BufferUsage::SCANOUT) { what happens if we simply ignore this check here? https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:64: DCHECK(format == gfx::BufferFormat::BGRA_8888); DCHECK_EQ and move outside the ifdef as we might as well check this always https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:65: DCHECK(usage == gfx::BufferUsage::MAP); can we instead check that the configuration is returned by GetSupportedConfigurations()? so that we don't have to update the code in multiple places when adding support for additional formats? https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h (right): https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:40: void* data_; can some of these members be const?
Thank you for review. I resolved some comments, and replied opposite opinion to others. Could you review again? https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:27: int ret = drmPrimeFDToHandle(vgem_handle.fd, handle.fd.fd, &vgem_bo_handle); On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > On 2015/08/07 at 15:36:46, dshwang wrote: > > On 2015/08/07 14:35:16, reveman wrote: > > > Would it not be more appropriate to keep this code with the owner of > > > vgem_handle? It seems to me like the owner of the vgem handle is the code > > > that should know how to create a vgem_bo_handle from it and not this code > here.. > > > > ClientNativePixmapFactoryGbm has role to manage vgem device fd. > > ClientNativePixmapVgem has role to manage vgem bo (a.k.a buffer) fd. > > drmPrimeFDToHandle is drm bo api and vgem bo is one of drm bo. > > So I think ClientNativePixmapVgem should use this API. > > ClientNativePixmapFactoryGbm is clearly aware of vgem too and you already have > factory functionality there so I'm still failing to see why we'd need another > level of factory code here but I don't feel that strongly about it. ui/ozone > reviewer can decide how they prefer this to be done. I still think drmPrimeFDToHandle() is Pixmap's job, not PixmapFactory's job. In addition, I intend to make only ClientNativePixmapVgem know drm/vgem header. spang, what do you think? https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h (right): https://codereview.chromium.org/1134993003/diff/340001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:40: base::ScopedFD dma_buf_; On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > On 2015/08/07 at 15:36:46, dshwang wrote: > > On 2015/08/07 14:35:16, reveman wrote: > > > what is this used for? > > > > Client should keep dmabuf fd and close it after vgem bo handle is closed. > > drmPrimeFDToHandle doesn't dup the fd? That's a bit weird. Maybe wrap the bo > handle in a class that makes sure that's case you are right. Don't need to keep dmabuf fd. removed this member. https://codereview.chromium.org/1134993003/diff/380001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/380001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:39: DCHECK(!mapped_); On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > Looks like you already have this dcheck in the pixmap implementation. Why are > you also adding it here? One dcheck is enough. All GpuMemoryBufferImplXXX::Map/Unmap has same dcheck, so it's a bit weird that only this doesn't have the dcheck. On the other hands, I'm reluctant to remove dcheck in ClientNativePixmapVgem::Map/Unmap because the dcheck helps to understand code. https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/BUILD.gn (right): https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/BUILD.gn:13: ozone_use_vgem_map = false On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > nit: s/hardwares/hardware/ > > can you create a bug? > > ozone_use_vgem enough? > > however, I'm still a bit confused about this. is the idea that we define this > when we know if hardware supports vgem? can we know that at compile time? if > not, why not include vgem_drm.h in third_party and remove this compile time > setting? both compile time and run time checking seems unnecessary.. good concern. I remove run time checking. It's decided in compile time like |use_mesa_platform_null| Even if a hardware support vgem, it's possible for the hardware to not support vgem map. e.g. ARM So I name it as ozone_use_vgem_map to avoid confusion. for example, currently ARM chromeos maintainer should not enable it. I think only certain IA chromebook will enable this compile flag. Haswell and Skylake are the first target. I create crbug.com/519587 to remove this flag in the future. https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:57: if (usage == gfx::BufferUsage::SCANOUT) { On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > what happens if we simply ignore this check here? ImportFromHandle() doesn't create pixmap, so GpuMemoryBufferImplOzoneNativePixmap::Map() will cause crash. https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:64: DCHECK(format == gfx::BufferFormat::BGRA_8888); On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > DCHECK_EQ and move outside the ifdef as we might as well check this always I tried DCHECK_EQ but compile fail because DCHECK_XX doesn't support enum class. In detail, c++11 std library is needed to support enum class, but chromium coding guide prohibits c++11 std library. GetSupportedConfigurations() returns BGRA_8888 in the ifdef, so current code is right. https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:65: DCHECK(usage == gfx::BufferUsage::MAP); On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > can we instead check that the configuration is returned by > GetSupportedConfigurations()? so that we don't have to update the code in > multiple places when adding support for additional formats? Look at line 47 in GetSupportedConfigurations(). GetSupportedConfigurations() is producer of this configuration, and this code is consumer of this configuration. So this sanity check makes sense, and all code is in sorta one place, this cc file. https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h (right): https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:40: void* data_; On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > can some of these members be const? good point. all except for |data_|. Done.
https://codereview.chromium.org/1134993003/diff/380001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/380001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:39: DCHECK(!mapped_); On 2015/08/11 at 17:44:56, dshwang wrote: > On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > > Looks like you already have this dcheck in the pixmap implementation. Why are > > you also adding it here? One dcheck is enough. > > All GpuMemoryBufferImplXXX::Map/Unmap has same dcheck, so it's a bit weird that only this doesn't have the dcheck. > On the other hands, I'm reluctant to remove dcheck in ClientNativePixmapVgem::Map/Unmap because the dcheck helps to understand code. Other implementations have DCHECKs as we're not in control over the API they are calling into and can make sure they crash in a reliable way when used inappropriately. We're in control over the ClientNativePixmap API and able to make sure that inappropriate use is handled correctly. I don't think DCHECKs here are needed here for that reason. https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:57: if (usage == gfx::BufferUsage::SCANOUT) { On 2015/08/11 at 17:44:56, dshwang wrote: > On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > > what happens if we simply ignore this check here? > > ImportFromHandle() doesn't create pixmap, so GpuMemoryBufferImplOzoneNativePixmap::Map() will cause crash. Why doesn't ImportFromHandle() create a pixmap? and even if it does, why is ::Map() crashing a problem if that's not an allowed usage? https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:65: DCHECK(usage == gfx::BufferUsage::MAP); On 2015/08/11 at 17:44:56, dshwang wrote: > On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > > can we instead check that the configuration is returned by > > GetSupportedConfigurations()? so that we don't have to update the code in > > multiple places when adding support for additional formats? > > Look at line 47 in GetSupportedConfigurations(). GetSupportedConfigurations() is producer of this configuration, and this code is consumer of this configuration. So this sanity check makes sense, and all code is in sorta one place, this cc file. All the code being in the same file is why I'm questioning this logic. As it is now, if I'm adding support for a new format I have to change the code in multiple places in this file and it makes it possible for the code to become inconsistent. We can avoid this by replacing these DCHECKs with one DCHECK that verify that GetSupportedConfigurations() contains the format. We would have the same protection against incorrect usage but without the risk of the code becoming inconsistent and without having to modify the code in multiple places in this file when adding support for a new format.
https://codereview.chromium.org/1134993003/diff/380001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/380001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:39: DCHECK(!mapped_); On 2015/08/11 18:53:52, reveman (OOO Aug 10 - Aug 11) wrote: > On 2015/08/11 at 17:44:56, dshwang wrote: > > On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > > > Looks like you already have this dcheck in the pixmap implementation. Why > are > > > you also adding it here? One dcheck is enough. > > > > All GpuMemoryBufferImplXXX::Map/Unmap has same dcheck, so it's a bit weird > that only this doesn't have the dcheck. > > On the other hands, I'm reluctant to remove dcheck in > ClientNativePixmapVgem::Map/Unmap because the dcheck helps to understand code. > > Other implementations have DCHECKs as we're not in control over the API they are > calling into and can make sure they crash in a reliable way when used > inappropriately. We're in control over the ClientNativePixmap API and able to > make sure that inappropriate use is handled correctly. I don't think DCHECKs > here are needed here for that reason. Done. Thank you for patient explanation. https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:57: if (usage == gfx::BufferUsage::SCANOUT) { On 2015/08/11 18:53:53, reveman (OOO Aug 10 - Aug 11) wrote: > On 2015/08/11 at 17:44:56, dshwang wrote: > > On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > > > what happens if we simply ignore this check here? > > > > ImportFromHandle() doesn't create pixmap, so > GpuMemoryBufferImplOzoneNativePixmap::Map() will cause crash. > > Why doesn't ImportFromHandle() create a pixmap? and even if it does, why is > ::Map() crashing a problem if that's not an allowed usage? I didn't fully understand your question "what happens if we simply ignore this check here?" As I understood previously, if removing "if (usage == gfx::BufferUsage::SCANOUT)", what happen? What's your question? BTW, ImportFromHandle() should not create a pixmap when SCANOUT, because SCANOUT buffer must work on the hardware not supporting VGEM. https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:65: DCHECK(usage == gfx::BufferUsage::MAP); On 2015/08/11 18:53:53, reveman (OOO Aug 10 - Aug 11) wrote: > On 2015/08/11 at 17:44:56, dshwang wrote: > > On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > > > can we instead check that the configuration is returned by > > > GetSupportedConfigurations()? so that we don't have to update the code in > > > multiple places when adding support for additional formats? > > > > Look at line 47 in GetSupportedConfigurations(). GetSupportedConfigurations() > is producer of this configuration, and this code is consumer of this > configuration. So this sanity check makes sense, and all code is in sorta one > place, this cc file. > > All the code being in the same file is why I'm questioning this logic. As it is > now, if I'm adding support for a new format I have to change the code in > multiple places in this file and it makes it possible for the code to become > inconsistent. > > We can avoid this by replacing these DCHECKs with one DCHECK that verify that > GetSupportedConfigurations() contains the format. We would have the same > protection against incorrect usage but without the risk of the code becoming > inconsistent and without having to modify the code in multiple places in this > file when adding support for a new format. Done. now I understand your request correctly. I check that the configuration is returned by GetSupportedConfigurations().
https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:57: if (usage == gfx::BufferUsage::SCANOUT) { On 2015/08/11 at 19:36:06, dshwang wrote: > On 2015/08/11 18:53:53, reveman (OOO Aug 10 - Aug 11) wrote: > > On 2015/08/11 at 17:44:56, dshwang wrote: > > > On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > > > > what happens if we simply ignore this check here? > > > > > > ImportFromHandle() doesn't create pixmap, so > > GpuMemoryBufferImplOzoneNativePixmap::Map() will cause crash. > > > > Why doesn't ImportFromHandle() create a pixmap? and even if it does, why is > > ::Map() crashing a problem if that's not an allowed usage? > > I didn't fully understand your question "what happens if we simply ignore this check here?" > As I understood previously, if removing "if (usage == gfx::BufferUsage::SCANOUT)", what happen? > What's your question? > > BTW, ImportFromHandle() should not create a pixmap when SCANOUT, because SCANOUT buffer must work on the hardware not supporting VGEM. I'm just trying to understand the motivation for the branch. That is, why not try to always create the pixmap and avoid branching based on usage here? if vgem is supported you'd get a pixmap that can be mapped independent of usage and if vgem is not supported you'll get either a pixmap that can't be mapped or no pixmap. If a branch based on usage is needed here then we should make it a switch statement so we don't forget to update it when adding a new usage mode. https://codereview.chromium.org/1134993003/diff/420001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/420001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:40: if (result) can we DCHECK(result) instead? https://codereview.chromium.org/1134993003/diff/420001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/420001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:55: base::ScopedFD close_fd(handle.fd.fd); it's a bit weird to reach into the fd and check auto_close from here. why do we need this and base::ScopedFD? is the caller not responsible for this?
https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/380001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:57: if (usage == gfx::BufferUsage::SCANOUT) { On 2015/08/11 20:15:16, reveman (OOO Aug 10 - Aug 11) wrote: > On 2015/08/11 at 19:36:06, dshwang wrote: > > On 2015/08/11 18:53:53, reveman (OOO Aug 10 - Aug 11) wrote: > > > On 2015/08/11 at 17:44:56, dshwang wrote: > > > > On 2015/08/07 17:59:44, reveman (OOO Aug 10 - Aug 11) wrote: > > > > > what happens if we simply ignore this check here? > > > > > > > > ImportFromHandle() doesn't create pixmap, so > > > GpuMemoryBufferImplOzoneNativePixmap::Map() will cause crash. > > > > > > Why doesn't ImportFromHandle() create a pixmap? and even if it does, why is > > > ::Map() crashing a problem if that's not an allowed usage? > > > > I didn't fully understand your question "what happens if we simply ignore this > check here?" > > As I understood previously, if removing "if (usage == > gfx::BufferUsage::SCANOUT)", what happen? > > What's your question? > > > > BTW, ImportFromHandle() should not create a pixmap when SCANOUT, because > SCANOUT buffer must work on the hardware not supporting VGEM. > > I'm just trying to understand the motivation for the branch. That is, why not > try to always create the pixmap and avoid branching based on usage here? if vgem > is supported you'd get a pixmap that can be mapped independent of usage and if > vgem is not supported you'll get either a pixmap that can't be mapped or no > pixmap. If a branch based on usage is needed here then we should make it a > switch statement so we don't forget to update it when adding a new usage mode. crrev.com/1263323004 returns ClientNativePixmapGbm now. I update that ClientNativePixmapVgem is returned only if usage is MAP. Now it's switch statement. https://codereview.chromium.org/1134993003/diff/420001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/420001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:40: if (result) On 2015/08/11 20:15:16, reveman (OOO Aug 10 - Aug 11) wrote: > can we DCHECK(result) instead? Yes, done. https://codereview.chromium.org/1134993003/diff/420001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/420001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:55: base::ScopedFD close_fd(handle.fd.fd); On 2015/08/11 20:15:16, reveman (OOO Aug 10 - Aug 11) wrote: > it's a bit weird to reach into the fd and check auto_close from here. why do we > need this and base::ScopedFD? is the caller not responsible for this? It's being handled in https://codereview.chromium.org/1263323004/
I have only nits - lgtm https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/BUILD.gn (right): https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/BUILD.gn:14: ozone_use_vgem_map = false remove ozone_ prefix https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/BUILD.gn:206: defines += [ "OZONE_USE_VGEM_MAP" ] remove OZONE_ prefix https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:72: break; This is pretty strange control flow - can you move the code below right here? A function would work too. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:90: bool IsConfigurationSupported(gfx::BufferFormat format, I'd suggest removing this to keep the signal/noise ratio high. It doesn't seem like a very valuable assertion. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:52: PLOG(ERROR) << "fail to free a vgem buffer. error:" << ret; PLOG() handles the error message for you, please don't print out the return value. It will always be -1. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:63: PLOG(ERROR) << "fail to map a vgem buffer. error:" << ret; and here https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:84: PLOG(ERROR) << "fail to munmap a vgem buffer. error:" << ret; and here https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h (right): https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:18: const gfx::NativePixmapHandle& handle, Could you unpack "handle" at the same place ownership of the contained fd is taken? scoped_ptr<ClientNativePixmap> ImportFromDmabuf( int dmabuf_fd, int stride, const gfx::Size& size); https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:19: const base::FileDescriptor& vgem_handle, I'd prefer int vgem_fd over const base::FileDescriptor& vgem_handle We should use base::ScopedFD if ownership is transferred and "int" otherwise. base::FileDescriptor is for IPC; the "auto_close" thing is not necessary here. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:35: const base::FileDescriptor vgem_fd_; Same here. Wrapping the fd inside base::FileDescriptor isn't useful and we just end up with "fd.fd" everywhere.
https://codereview.chromium.org/1134993003/diff/440001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/440001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:44: return result; nit: return true; https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:55: DCHECK_GE(vgem_fd_.get(), 0); nit: you already have this DCHECK in the ctor. I think you can remove it and the comment https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:64: gfx::BufferFormat format, Format is not used for anything but a DCHECK. Can we remove it or make sure it's passed to where it might be used? https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:70: switch (usage) { To keep things simple, I'd ignore the usage here and simply create a ClientNativePixmapVgem in a vgem supported build and ClientNativePixmapGbm otherwise but the current code is fine with me. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:73: case gfx::BufferUsage::PERSISTENT_MAP: nit: PERSISTENT_MAP is not supported. NOTREACHED here? https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:77: } nit: please NOTREACHED for when usage is an invalid enum value. fixing the control flow as spang suggested should take care of this. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:90: bool IsConfigurationSupported(gfx::BufferFormat format, On 2015/08/12 at 21:40:03, spang wrote: > I'd suggest removing this to keep the signal/noise ratio high. It doesn't seem like a very valuable assertion. I agree, especially if we ignore the |usage| and |format| in ImportFromHandle. Removing this and related DCHECKs sgtm.
I addressed all comments. reveman, could you review again? https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/BUILD.gn (right): https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/BUILD.gn:14: ozone_use_vgem_map = false On 2015/08/12 21:40:03, spang wrote: > remove ozone_ prefix Done. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/BUILD.gn:206: defines += [ "OZONE_USE_VGEM_MAP" ] On 2015/08/12 21:40:03, spang wrote: > remove OZONE_ prefix Done. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:55: DCHECK_GE(vgem_fd_.get(), 0); On 2015/08/12 22:32:25, reveman wrote: > nit: you already have this DCHECK in the ctor. I think you can remove it and the > comment Done. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:64: gfx::BufferFormat format, On 2015/08/12 22:32:25, reveman wrote: > Format is not used for anything but a DCHECK. Can we remove it or make sure it's > passed to where it might be used? remove |format| in this API https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:70: switch (usage) { On 2015/08/12 22:32:25, reveman wrote: > To keep things simple, I'd ignore the usage here and simply create a > ClientNativePixmapVgem in a vgem supported build and ClientNativePixmapGbm > otherwise but the current code is fine with me. Done. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:72: break; On 2015/08/12 21:40:03, spang wrote: > This is pretty strange control flow - can you move the code below right here? A > function would work too. Done. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:73: case gfx::BufferUsage::PERSISTENT_MAP: On 2015/08/12 22:32:25, reveman wrote: > nit: PERSISTENT_MAP is not supported. NOTREACHED here? As changing control flow, PERSISTENT_MAP branch disappears https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:77: } On 2015/08/12 22:32:25, reveman wrote: > nit: please NOTREACHED for when usage is an invalid enum value. fixing the > control flow as spang suggested should take care of this. Done. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:90: bool IsConfigurationSupported(gfx::BufferFormat format, On 2015/08/12 22:32:25, reveman wrote: > On 2015/08/12 at 21:40:03, spang wrote: > > I'd suggest removing this to keep the signal/noise ratio high. It doesn't seem > like a very valuable assertion. > > I agree, especially if we ignore the |usage| and |format| in ImportFromHandle. > Removing this and related DCHECKs sgtm. Done. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:52: PLOG(ERROR) << "fail to free a vgem buffer. error:" << ret; On 2015/08/12 21:40:03, spang wrote: > PLOG() handles the error message for you, please don't print out the return > value. It will always be -1. Done. Change it to DCHECK https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:63: PLOG(ERROR) << "fail to map a vgem buffer. error:" << ret; On 2015/08/12 21:40:03, spang wrote: > and here Done. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:84: PLOG(ERROR) << "fail to munmap a vgem buffer. error:" << ret; On 2015/08/12 21:40:03, spang wrote: > and here Done. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h (right): https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:18: const gfx::NativePixmapHandle& handle, On 2015/08/12 21:40:03, spang wrote: > Could you unpack "handle" at the same place ownership of the contained fd is > taken? > > scoped_ptr<ClientNativePixmap> ImportFromDmabuf( > int dmabuf_fd, int stride, const gfx::Size& size); > Done. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:19: const base::FileDescriptor& vgem_handle, On 2015/08/12 21:40:03, spang wrote: > I'd prefer > > int vgem_fd > > over > > const base::FileDescriptor& vgem_handle > > We should use base::ScopedFD if ownership is transferred and "int" otherwise. > base::FileDescriptor is for IPC; the "auto_close" thing is not necessary here. Done. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h:35: const base::FileDescriptor vgem_fd_; On 2015/08/12 21:40:03, spang wrote: > Same here. Wrapping the fd inside base::FileDescriptor isn't useful and we just > end up with "fd.fd" everywhere. Done.
https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:63: DCHECK(handle.fd.auto_close); This DCHECK is still inappropriate imo. This function should take ownership of the fd even if auto_close is false. https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:67: if (usage == gfx::BufferUsage::MAP) can we remove this if statement? if we need it then use a switch statement so we're forced to update this code when adding a new usage mode. https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:69: vgem_fd_.get(), scoped_fd.get(), size, handle.stride); nit: {} as multiple lines https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:26: if (ret) { can we DCHECK(!ret) instead? https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:61: if (ret) { What is the reason for this failing? I guess running out of virtual address space, in which case we should call TerminateBecauseOutOfMemory.
https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:63: DCHECK(handle.fd.auto_close); On 2015/08/13 14:03:58, reveman wrote: > This DCHECK is still inappropriate imo. This function should take ownership of > the fd even if auto_close is false. Ok, Done. https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:67: if (usage == gfx::BufferUsage::MAP) On 2015/08/13 14:03:58, reveman wrote: > can we remove this if statement? if we need it then use a switch statement so > we're forced to update this code when adding a new usage mode. I restored the switch because ClientNativePixmapGbm is better for scanout buffer.
https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:67: if (usage == gfx::BufferUsage::MAP) On 2015/08/13 at 14:59:02, dshwang wrote: > On 2015/08/13 14:03:58, reveman wrote: > > can we remove this if statement? if we need it then use a switch statement so > > we're forced to update this code when adding a new usage mode. > > I restored the switch because ClientNativePixmapGbm is better for scanout buffer. Just curious, why is ClientNativePixmapGbm better for scanout? https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:26: if (ret) { On 2015/08/13 at 14:03:58, reveman wrote: > can we DCHECK(!ret) instead? ping? https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:27: PLOG(ERROR) << "drmPrimeFDToHandle failed, handle:" << vgem_bo_handle; btw, logging the vgem_bo_handle after failure seems weird. why would it not be 0 in that case? https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:61: if (ret) { On 2015/08/13 at 14:03:58, reveman wrote: > What is the reason for this failing? I guess running out of virtual address space, in which case we should call TerminateBecauseOutOfMemory. ping? https://codereview.chromium.org/1134993003/diff/480001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/480001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:66: switch (usage) { nit: NOTREACHED and return nullptr if |usage| is an invalid enum value. I'd move "new ClientNativePixmapGbm" and the ifdef into the switch statement so we can NOTREACHED() return nullptr; below it.
https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:67: if (usage == gfx::BufferUsage::MAP) On 2015/08/13 15:15:58, reveman wrote: > On 2015/08/13 at 14:59:02, dshwang wrote: > > On 2015/08/13 14:03:58, reveman wrote: > > > can we remove this if statement? if we need it then use a switch statement > so > > > we're forced to update this code when adding a new usage mode. > > > > I restored the switch because ClientNativePixmapGbm is better for scanout > buffer. > > Just curious, why is ClientNativePixmapGbm better for scanout? ClientNativePixmapVgem imports handle via drmPrimeFDToHandle for nothing. It consumes little more battery and memory for nothing. https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:26: if (ret) { On 2015/08/13 15:15:58, reveman wrote: > On 2015/08/13 at 14:03:58, reveman wrote: > > can we DCHECK(!ret) instead? > > ping? ah, I missed. Done. BTW,Android and IOS return nullptr in the same situation. So now ozone behaves slightly differently. https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:27: PLOG(ERROR) << "drmPrimeFDToHandle failed, handle:" << vgem_bo_handle; On 2015/08/13 15:15:58, reveman wrote: > btw, logging the vgem_bo_handle after failure seems weird. why would it not be 0 > in that case? always 0. done. https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:61: if (ret) { On 2015/08/13 14:03:58, reveman wrote: > What is the reason for this failing? I guess running out of virtual address > space, in which case we should call TerminateBecauseOutOfMemory. yes, mostly oom. I followed Android code. Now I added TerminateBecauseOutOfMemory https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... https://codereview.chromium.org/1134993003/diff/480001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/480001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:66: switch (usage) { On 2015/08/13 15:15:58, reveman wrote: > nit: NOTREACHED and return nullptr if |usage| is an invalid enum value. > > I'd move "new ClientNativePixmapGbm" and the ifdef into the switch statement so > we can NOTREACHED() return nullptr; below it. Done.
lgtm with nits https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:67: if (usage == gfx::BufferUsage::MAP) On 2015/08/13 at 15:45:28, dshwang wrote: > On 2015/08/13 15:15:58, reveman wrote: > > On 2015/08/13 at 14:59:02, dshwang wrote: > > > On 2015/08/13 14:03:58, reveman wrote: > > > > can we remove this if statement? if we need it then use a switch statement > > so > > > > we're forced to update this code when adding a new usage mode. > > > > > > I restored the switch because ClientNativePixmapGbm is better for scanout > > buffer. > > > > Just curious, why is ClientNativePixmapGbm better for scanout? > > ClientNativePixmapVgem imports handle via drmPrimeFDToHandle for nothing. It consumes little more battery and memory for nothing. Why does it consume more battery? Could you explain the additional memory usage as we'll need to understand that for proper memory tracing? Note: future GMB usage on Mac requires both MAP and SCANOUT for the same buffer. Would be nice if we could just use SCANOUT for this instead of adding a new SCANOUT_AND_MAP usage mode. https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:26: if (ret) { On 2015/08/13 at 15:45:28, dshwang wrote: > On 2015/08/13 15:15:58, reveman wrote: > > On 2015/08/13 at 14:03:58, reveman wrote: > > > can we DCHECK(!ret) instead? > > > > ping? > > ah, I missed. Done. > > BTW,Android and IOS return nullptr in the same situation. So now ozone behaves slightly differently. > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... Yes, that's not ideal and we should change those implementations to fail hard too. Not sure OOM is the only cause for those functions to fail though. https://codereview.chromium.org/1134993003/diff/500001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/500001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:70: #endif nit: NOTREACHED(); return nullptr; or add a comment making it clear that we're intentionally falling through to the next case. https://codereview.chromium.org/1134993003/diff/500001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/500001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:63: return false; nit: no need to return false here. TerminateBecauseOutOfMemory will never return.
dongseong.hwang@intel.com changed reviewers: + ccameron@chromium.org
Thank you for reviewing! ccameron, could you review content/common? https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:67: if (usage == gfx::BufferUsage::MAP) On 2015/08/13 17:16:43, reveman wrote: > On 2015/08/13 at 15:45:28, dshwang wrote: > > On 2015/08/13 15:15:58, reveman wrote: > > > On 2015/08/13 at 14:59:02, dshwang wrote: > > > > On 2015/08/13 14:03:58, reveman wrote: > > > > > can we remove this if statement? if we need it then use a switch > statement > > > so > > > > > we're forced to update this code when adding a new usage mode. > > > > > > > > I restored the switch because ClientNativePixmapGbm is better for scanout > > > buffer. > > > > > > Just curious, why is ClientNativePixmapGbm better for scanout? > > > > ClientNativePixmapVgem imports handle via drmPrimeFDToHandle for nothing. It > consumes little more battery and memory for nothing. > > Why does it consume more battery? Could you explain the additional memory usage > as we'll need to understand that for proper memory tracing? > > Note: future GMB usage on Mac requires both MAP and SCANOUT for the same buffer. > Would be nice if we could just use SCANOUT for this instead of adding a new > SCANOUT_AND_MAP usage mode. I made you confused by ambiguous reply. What I mean that ClientNativePixmapVgem call drmPrimeFDToHandle, which calls ioctl system call. This uses CPU, so little battery is consumed. Also it makes drm bo handle, which consumed little memory as much as data structure. Ozone is fine with merging MAP and SCANOUT. At the time, SCANOUT will just use ClientNativePixmapVgem. https://codereview.chromium.org/1134993003/diff/500001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/500001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:70: #endif On 2015/08/13 17:16:43, reveman wrote: > nit: NOTREACHED(); return nullptr; or add a comment making it clear that we're > intentionally falling through to the next case. Done by code. I think code is better than comment.
https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:67: if (usage == gfx::BufferUsage::MAP) On 2015/08/13 18:13:25, dshwang wrote: > On 2015/08/13 17:16:43, reveman wrote: > > On 2015/08/13 at 15:45:28, dshwang wrote: > > > On 2015/08/13 15:15:58, reveman wrote: > > > > On 2015/08/13 at 14:59:02, dshwang wrote: > > > > > On 2015/08/13 14:03:58, reveman wrote: > > > > > > can we remove this if statement? if we need it then use a switch > > statement > > > > so > > > > > > we're forced to update this code when adding a new usage mode. > > > > > > > > > > I restored the switch because ClientNativePixmapGbm is better for > scanout > > > > buffer. > > > > > > > > Just curious, why is ClientNativePixmapGbm better for scanout? > > > > > > ClientNativePixmapVgem imports handle via drmPrimeFDToHandle for nothing. It > > consumes little more battery and memory for nothing. > > > > Why does it consume more battery? Could you explain the additional memory > usage > > as we'll need to understand that for proper memory tracing? > > > > Note: future GMB usage on Mac requires both MAP and SCANOUT for the same > buffer. > > Would be nice if we could just use SCANOUT for this instead of adding a new > > SCANOUT_AND_MAP usage mode. > > I made you confused by ambiguous reply. What I mean that ClientNativePixmapVgem > call drmPrimeFDToHandle, which calls ioctl system call. This uses CPU, so little > battery is consumed. > Also it makes drm bo handle, which consumed little memory as much as data > structure. > > Ozone is fine with merging MAP and SCANOUT. At the time, SCANOUT will just use > ClientNativePixmapVgem. After rethinking, I'm not sure SCANOUT and MAP is mergable for all platform. In general, SCANOUT uses x-tiling or no-tiling so that crt can copy line by line. MAP uses tiling or no-tiling. MacOS propably uses no-tiling for both case. However, in other platform, SCANOUT can use x-tiling, and MAP can use tiling. Some platforms restrict tiling buffer from overlaying. So I think BufferUsage needs to distinguish MAP and SCANOUT in the future also.
On 2015/08/13 at 18:22:09, dongseong.hwang wrote: > https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... > File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): > > https://codereview.chromium.org/1134993003/diff/460001/ui/ozone/platform/drm/... > ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:67: if (usage == gfx::BufferUsage::MAP) > On 2015/08/13 18:13:25, dshwang wrote: > > On 2015/08/13 17:16:43, reveman wrote: > > > On 2015/08/13 at 15:45:28, dshwang wrote: > > > > On 2015/08/13 15:15:58, reveman wrote: > > > > > On 2015/08/13 at 14:59:02, dshwang wrote: > > > > > > On 2015/08/13 14:03:58, reveman wrote: > > > > > > > can we remove this if statement? if we need it then use a switch > > > statement > > > > > so > > > > > > > we're forced to update this code when adding a new usage mode. > > > > > > > > > > > > I restored the switch because ClientNativePixmapGbm is better for > > scanout > > > > > buffer. > > > > > > > > > > Just curious, why is ClientNativePixmapGbm better for scanout? > > > > > > > > ClientNativePixmapVgem imports handle via drmPrimeFDToHandle for nothing. It > > > consumes little more battery and memory for nothing. > > > > > > Why does it consume more battery? Could you explain the additional memory > > usage > > > as we'll need to understand that for proper memory tracing? > > > > > > Note: future GMB usage on Mac requires both MAP and SCANOUT for the same > > buffer. > > > Would be nice if we could just use SCANOUT for this instead of adding a new > > > SCANOUT_AND_MAP usage mode. > > > > I made you confused by ambiguous reply. What I mean that ClientNativePixmapVgem > > call drmPrimeFDToHandle, which calls ioctl system call. This uses CPU, so little > > battery is consumed. > > Also it makes drm bo handle, which consumed little memory as much as data > > structure. > > > > Ozone is fine with merging MAP and SCANOUT. At the time, SCANOUT will just use > > ClientNativePixmapVgem. > > After rethinking, I'm not sure SCANOUT and MAP is mergable for all platform. In general, SCANOUT uses x-tiling or no-tiling so that crt can copy line by line. MAP uses tiling or no-tiling. MacOS propably uses no-tiling for both case. > However, in other platform, SCANOUT can use x-tiling, and MAP can use tiling. Some platforms restrict tiling buffer from overlaying. > So I think BufferUsage needs to distinguish MAP and SCANOUT in the future also. Ok, so we'll need a SCANOUT_AND_MAP usage mode then. Does the current vgem mechanism for mapping buffers on linux support that? ie. allow us to map a buffer using vgem that we explicitly ask to not be tiled.
On 2015/08/13 18:55:42, reveman wrote: > Ok, so we'll need a SCANOUT_AND_MAP usage mode then. Does the current vgem > mechanism for mapping buffers on linux support that? ie. allow us to map a > buffer using vgem that we explicitly ask to not be tiled. short answer is yes. It will depend on how the driver allocates and how the dma_buf mmap implementation gives it back to the client. In Ozone there are hints we can pass to gbm library telling the preferred tiling type and we guarantee in i915 that it maps back correctly. That said, vgem is simply the transport mechanism here and has not much to do with this (or maybe vgem is just a *wrapper* around the transport).
dongseong.hwang@intel.com changed reviewers: - ccameron@chromium.org
dongseong.hwang@intel.com changed reviewers: + ccameron@chromium.org, sievers@chromium.org
ccameron, sievers: could you review content/common? On 2015/08/13 22:13:04, vignatti wrote: > On 2015/08/13 18:55:42, reveman wrote: > > Ok, so we'll need a SCANOUT_AND_MAP usage mode then. Does the current vgem > > mechanism for mapping buffers on linux support that? ie. allow us to map a > > buffer using vgem that we explicitly ask to not be tiled. > > short answer is yes. > > It will depend on how the driver allocates and how the dma_buf mmap > implementation gives it back to the client. In Ozone there are hints we can pass > to gbm library telling the preferred tiling type and we guarantee in i915 that > it maps back correctly. That said, vgem is simply the transport mechanism here > and has not much to do with this (or maybe vgem is just a *wrapper* around the > transport). Tiago and I are improving kernel side. So far, we think no-tiling is the solution for chromium on IA.
ok, thanks! latest patch lgtm
On 2015/08/14 06:27:45, dshwang wrote: > ccameron, sievers: could you review content/common? ping
https://codereview.chromium.org/1134993003/diff/520001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/520001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:41: DCHECK(result); nit: It should be either DCHECK() or supported at runtime (since you are returning the result here), not both. maybe just log a warning here? Also, should it then be: mapped_ = pixmap_->Map() and only call Unmap() below if |mapped_|?
sievers, could you review again? This CL is blocked by https://codereview.chromium.org/1263323004/, so I didn't kick trybot yet. https://codereview.chromium.org/1134993003/diff/520001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/520001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:41: DCHECK(result); On 2015/08/17 17:33:18, sievers wrote: > nit: It should be either DCHECK() or supported at runtime (since you are > returning the result here), not both. > maybe just log a warning here? Currently this code in the transition from runtime to DCHECK(). This interface is shared with MacOS and Android, so it'll not return bool after MacOS and Android do DCHECK later. The dcheck was introduced as reveman requested in https://codereview.chromium.org/1134993003/#msg55 > Also, should it then be: mapped_ = pixmap_->Map() and only call Unmap() below if > |mapped_|? Yes, mapped_ = pixmap_->Map() is better. I removed redundant |result| local variable
lgtm https://codereview.chromium.org/1134993003/diff/540001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/540001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:46: pixmap_->Unmap(); so do you still want to call pixmap_->Unmap() here if !mapped_?
content/common/gpu lgtm
Thank you for reviewing! https://codereview.chromium.org/1134993003/diff/540001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/540001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:46: pixmap_->Unmap(); On 2015/08/18 19:05:49, sievers wrote: > so do you still want to call pixmap_->Unmap() here if !mapped_? it's checked inside pixmap_->Unmap(), so I didn't check here because reveman wants to check once.
https://codereview.chromium.org/1134993003/diff/540001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/540001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:46: pixmap_->Unmap(); On 2015/08/19 at 07:02:59, dshwang wrote: > On 2015/08/18 19:05:49, sievers wrote: > > so do you still want to call pixmap_->Unmap() here if !mapped_? > > it's checked inside pixmap_->Unmap(), so I didn't check here because reveman wants to check once. You should probably remove the return type from Pixmap::Map() as that function should never fail.
https://codereview.chromium.org/1134993003/diff/540001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/540001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:46: pixmap_->Unmap(); On 2015/08/19 09:09:50, reveman wrote: > On 2015/08/19 at 07:02:59, dshwang wrote: > > On 2015/08/18 19:05:49, sievers wrote: > > > so do you still want to call pixmap_->Unmap() here if !mapped_? > > > > it's checked inside pixmap_->Unmap(), so I didn't check here because reveman > wants to check once. > > You should probably remove the return type from Pixmap::Map() as that function > should never fail. Done.
https://codereview.chromium.org/1134993003/diff/560001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/560001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:40: mapped_ = !!(*data); nit: mapped_ = true; and remove the DCHECK below as pixmap_->Map() is not allowed to return null. https://codereview.chromium.org/1134993003/diff/560001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/560001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:63: return nullptr; nit: you don't need this return statement as base::Terminate.. will never return
https://codereview.chromium.org/1134993003/diff/560001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1134993003/diff/560001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:40: mapped_ = !!(*data); On 2015/08/19 13:18:11, reveman wrote: > nit: mapped_ = true; and remove the DCHECK below as pixmap_->Map() is not > allowed to return null. Done. https://codereview.chromium.org/1134993003/diff/560001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc (right): https://codereview.chromium.org/1134993003/diff/560001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc:63: return nullptr; On 2015/08/19 13:18:11, reveman wrote: > nit: you don't need this return statement as base::Terminate.. will never return Done.
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@chromium.org, reveman@chromium.org, ccameron@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1134993003/#ps600001 (title: "rebase to ToT to land")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1134993003/600001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1134993003/600001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #23 (id:600001) has been deleted
The CQ bit was checked by dongseong.hwang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, ccameron@chromium.org, sievers@chromium.org, spang@chromium.org Link to the patchset: https://codereview.chromium.org/1134993003/#ps620001 (title: "rebase to ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1134993003/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1134993003/620001
Message was sent while issue was closed.
Committed patchset #23 (id:620001)
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/e2e2c1dc3ef88009d11d6d644a9c60fb9293e7b5 Cr-Commit-Position: refs/heads/master@{#344755} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
