Implement DRM Native Pixmap using prime buffer

Implement DRM Native Pixmap using prime buffer

This CL implements a new client native pixmap through the prime buffer. In
particular it uses a new feature of prime to map the exported graphics fd,
which conveniently can be used in the GpuMemoryBuffer for zero-copy (and
one-copy) rasterization in Chrome.

This pretty much replaces VGEM infrastructure when that's used to share buffers
in Chrome processes, so support for VGEM in Chrome is being removed as well in
this CL.

This feature requires Kernel changes in i915 and drm subsystems, and also in
userspace mini-gbm library.

BUG=581151
TEST=memory coherent (IvyBridge) and non-coherent hardware (BayTrail-T).

Committed: https://crrev.com/eb129a0286bf817ce4e2d182ec1ce8222d50a316
Cr-Commit-Position: refs/heads/master@{#383513}

[vignatti (out of this project), 2016-01-14 22:01:20]

Description was changed from

==========
WIP: Native GpuMemoryBuffer implementation through prime buffer (NOT FOR REVIEW)

no VGEM, no vendor dependent ioctls. No drm fd passing to the unpriviledged
process either. Just dma-buf.

This requires Kernel changes in i915 and drm subsystems. Also, in mini-gbm.

BUG=
==========

to

==========
Implement Native Pixmap using prime buffer
    
This CL implements a new client native pixmap through the prime buffer. In
particular it uses a new feature of prime to map the exported graphics fd,
which conveniently can be used in the GpuMemoryBuffer for zero-copy (and
one-copy) rasterization in Chrome.
    
To some extent, this replaces VGEM infrastructure when that's used to share
buffers in Chrome processes. It's backed by the --ozone-use-dmabuf-mmap switch
and turned off by default.
    
This feature requires Kernel changes in i915 and drm subsystems, and also in
userspace mini-gbm library.
    
BUG=
TEST=memory coherent (IvyBridge) and non-coherent hardware (BayTrail-T).
==========

[vignatti (out of this project), 2016-01-14 22:01:20]

tiago.vignatti@intel.com changed reviewers:
+ dongseong.hwang@intel.com

[spang, 2016-03-21 23:24:19]

spang@chromium.org changed reviewers:
+ spang@chromium.org

[spang, 2016-03-21 23:24:20]

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc (right):

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc:65:
switches::kOzoneUseDmaBufMmap))
Check the command line in the constructor or Initialize().

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right):

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:29: : size_(size),
stride_(stride), data_(nullptr) {
You don't need to initialize data_ here and in the body.

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:30:
TRACE_EVENT0("nativepixmap", "ClientNativePixmapDmaBuf");
Please use the "drm" category.

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:38:
TRACE_EVENT0("nativepixmap", "~ClientNativePixmapDmaBuf");
Also here

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:41: DCHECK(!ret) <<
"fail to munmap a dmabuf.";
DPCHECK(!ret);

Don't really need a message for DCHECK.

[vignatti (out of this project), 2016-03-22 20:18:54]

Description was changed from

==========
Implement Native Pixmap using prime buffer
    
This CL implements a new client native pixmap through the prime buffer. In
particular it uses a new feature of prime to map the exported graphics fd,
which conveniently can be used in the GpuMemoryBuffer for zero-copy (and
one-copy) rasterization in Chrome.
    
To some extent, this replaces VGEM infrastructure when that's used to share
buffers in Chrome processes. It's backed by the --ozone-use-dmabuf-mmap switch
and turned off by default.
    
This feature requires Kernel changes in i915 and drm subsystems, and also in
userspace mini-gbm library.
    
BUG=
TEST=memory coherent (IvyBridge) and non-coherent hardware (BayTrail-T).
==========

to

==========
Implement DRM Native Pixmap using prime buffer

This CL implements a new client native pixmap through the prime buffer. In
particular it uses a new feature of prime to map the exported graphics fd,
which conveniently can be used in the GpuMemoryBuffer for zero-copy (and
one-copy) rasterization in Chrome.

This pretty much replaces VGEM infrastructure when that's used to share buffers
in Chrome processes, so support for VGEM in Chrome is being removed as well in
this CL.

This feature requires Kernel changes in i915 and drm subsystems, and also in
userspace mini-gbm library.

BUG=581151
TEST=memory coherent (IvyBridge) and non-coherent hardware (BayTrail-T).
==========

[vignatti (out of this project), 2016-03-22 20:18:54]

tiago.vignatti@intel.com changed reviewers:
+ dnicoara@chromium.org, marcheu@chromium.org, reveman@chromium.org

[vignatti (out of this project), 2016-03-22 20:24:30]

PTAL again.

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc (right):

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc:65:
switches::kOzoneUseDmaBufMmap))
On 2016/03/21 23:24:20, spang wrote:
> Check the command line in the constructor or Initialize().

I was actually thinking about removing the cmd line option altogether with the
vgem support now.

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right):

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:29: : size_(size),
stride_(stride), data_(nullptr) {
On 2016/03/21 23:24:20, spang wrote:
> You don't need to initialize data_ here and in the body.

Done.

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:30:
TRACE_EVENT0("nativepixmap", "ClientNativePixmapDmaBuf");
On 2016/03/21 23:24:20, spang wrote:
> Please use the "drm" category.

Done.

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:38:
TRACE_EVENT0("nativepixmap", "~ClientNativePixmapDmaBuf");
On 2016/03/21 23:24:20, spang wrote:
> Also here

Done.

https://codereview.chromium.org/1262043002/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:41: DCHECK(!ret) <<
"fail to munmap a dmabuf.";
On 2016/03/21 23:24:20, spang wrote:
> DPCHECK(!ret);
> 
> Don't really need a message for DCHECK.

Done.

[spang, 2016-03-23 15:40:48]

lgtm

[reveman, 2016-03-23 15:49:04]

lgtm

have the kernel changes and the minigbm changes required for this landed
already?

[dshwang, 2016-03-23 15:52:51]

Great! lgtm with nits

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc (right):

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc:50: return format ==
gfx::BufferFormat::BGRA_8888;
After this CL, Ozone GBM support BGRA_8888, no matter wheter kernel support
dma_buf mmap or not. Only ChromeOS kernel and v4.6+ kernel support it though.

We may need another definition. e.g. USE_DMA_BUF_MMAP
and then return true only if the definition is defined.

spang, what do you think?

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.h (right):

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.h:10: #include
"base/file_descriptor_posix.h"
needed?

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.h:13: #include
"ui/gfx/native_pixmap_handle_ozone.h"
needed?

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_display_host_manager.cc (left):

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_display_host_manager.cc:12: #include
"base/files/file_enumerator.h"
It's not needed.

[spang, 2016-03-23 16:04:01]

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc (right):

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc:50: return format ==
gfx::BufferFormat::BGRA_8888;
On 2016/03/23 15:52:51, dshwang wrote:
> After this CL, Ozone GBM support BGRA_8888, no matter wheter kernel support
> dma_buf mmap or not. Only ChromeOS kernel and v4.6+ kernel support it though.
> 
> We may need another definition. e.g. USE_DMA_BUF_MMAP
> and then return true only if the definition is defined.
> 
> spang, what do you think?

Aren't we only turning on one-copy via flags on vetted boards anyway?

I guess it's not ideal if manually flipping the flag can causes breakage.

If this is merged upstream, there's no reason to have an ifdef around it. We
should compile it in always and detect it at runtime.

[dshwang, 2016-03-23 16:31:11]

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc (right):

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc:50: return format ==
gfx::BufferFormat::BGRA_8888;
On 2016/03/23 16:04:01, spang wrote:
> Aren't we only turning on one-copy via flags on vetted boards anyway?
> 
> I guess it's not ideal if manually flipping the flag can causes breakage.
> 
> If this is merged upstream, there's no reason to have an ifdef around it. We
> should compile it in always and detect it at runtime.

Ok, it makes sense.
We add --enable-native-gpu-memory-buffers to specific device supporting it. 
(currently only Broadwell)

As we have runtime flag --enable-native-gpu-memory-buffers, we don't need
additional ifdef. So this CL is lgtm.

Note: ozone-gbm with supported kernel will crash with
--enable-native-gpu-memory-buffers flag.

[dshwang, 2016-03-23 16:32:00]

On 2016/03/23 16:31:11, dshwang wrote:
> Note: ozone-gbm with supported kernel will crash with
> --enable-native-gpu-memory-buffers flag.

errata. s/supported/unsupported

[spang, 2016-03-23 16:35:04]

On 2016/03/23 16:32:00, dshwang wrote:
> On 2016/03/23 16:31:11, dshwang wrote:
> > Note: ozone-gbm with supported kernel will crash with
> > --enable-native-gpu-memory-buffers flag.
> 
> errata. s/supported/unsupported

There's no way to detect it at runtime? If not how will anyone else (e.g. other
linuxes that don't atomically update kernel & userspace together) use this
kernel API?

[vignatti (out of this project), 2016-03-23 19:13:25]

On 2016/03/23 15:49:04, reveman wrote:
> lgtm
> 
> have the kernel changes and the minigbm changes required for this landed
> already?

yup. By now all the kernel changes are in for all CrOS kernel versions and also
in the Linus upstream tree. We still need to land the minigbm's though:
https://chromium-review.googlesource.com/#/c/323990/

[vignatti (out of this project), 2016-03-23 19:46:08]

PTAL.

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.h (right):

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.h:10: #include
"base/file_descriptor_posix.h"
On 2016/03/23 15:52:51, dshwang wrote:
> needed?

Done.

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.h:13: #include
"ui/gfx/native_pixmap_handle_ozone.h"
On 2016/03/23 15:52:51, dshwang wrote:
> needed?

Done.

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_display_host_manager.cc (left):

https://codereview.chromium.org/1262043002/diff/60001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_display_host_manager.cc:12: #include
"base/files/file_enumerator.h"
On 2016/03/23 15:52:51, dshwang wrote:
> It's not needed.

Done.

[vignatti (out of this project), 2016-03-23 21:17:15]

tiago.vignatti@intel.com changed reviewers:
+ piman@chromium.org

[vignatti (out of this project), 2016-03-23 21:17:41]

PTAL piman@ for content/

[piman, 2016-03-23 21:23:31]

piman@chromium.org changed reviewers:
+ mdempsky@chromium.org

[piman, 2016-03-23 21:23:32]

LGTM, but +mdempsky for content/common/sandbox_linux/

[piman, 2016-03-23 21:26:01]

https://codereview.chromium.org/1262043002/diff/80001/content/common/sandbox_...
File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right):

https://codereview.chromium.org/1262043002/diff/80001/content/common/sandbox_...
content/common/sandbox_linux/bpf_renderer_policy_linux.cc:37:
DRM_IOCTL_PRIME_FD_TO_HANDLE),
Actually, do we still need these in the renderer? It sounds like we now directly
mmap the fd.

[marcheu1, 2016-03-23 21:36:51]

On 2016/03/23 21:26:01, piman wrote:
>
https://codereview.chromium.org/1262043002/diff/80001/content/common/sandbox_...
> File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right):
> 
>
https://codereview.chromium.org/1262043002/diff/80001/content/common/sandbox_...
> content/common/sandbox_linux/bpf_renderer_policy_linux.cc:37:
> DRM_IOCTL_PRIME_FD_TO_HANDLE),
> Actually, do we still need these in the renderer? It sounds like we now
directly
> mmap the fd.

Yeah this shouldn't be needed

[vignatti (out of this project), 2016-03-23 21:51:30]

On 2016/03/23 21:36:51, marcheu1 wrote:
> On 2016/03/23 21:26:01, piman wrote:
> >
>
https://codereview.chromium.org/1262043002/diff/80001/content/common/sandbox_...
> > File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right):
> > 
> >
>
https://codereview.chromium.org/1262043002/diff/80001/content/common/sandbox_...
> > content/common/sandbox_linux/bpf_renderer_policy_linux.cc:37:
> > DRM_IOCTL_PRIME_FD_TO_HANDLE),
> > Actually, do we still need these in the renderer? It sounds like we now
> directly
> > mmap the fd.
> 
> Yeah this shouldn't be needed

ah, true that! Chrome IO thread now maps it so I'll remove. Thank you.

[piman, 2016-03-23 22:51:20]

https://codereview.chromium.org/1262043002/diff/100001/content/common/sandbox...
File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right):

https://codereview.chromium.org/1262043002/diff/100001/content/common/sandbox...
content/common/sandbox_linux/bpf_renderer_policy_linux.cc:36:
(static_cast<unsigned long>(DRM_IOCTL_GEM_CLOSE)),
Same with this, right? The only other use besides vgem is 
DrmDevice::CloseBufferHandle, which I think is only called from the GPU process
(DrmConsoleBuffer)

[vignatti (out of this project), 2016-03-24 15:50:53]

On 2016/03/23 22:51:20, piman wrote:
>
https://codereview.chromium.org/1262043002/diff/100001/content/common/sandbox...
> File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right):
> 
>
https://codereview.chromium.org/1262043002/diff/100001/content/common/sandbox...
> content/common/sandbox_linux/bpf_renderer_policy_linux.cc:36:
> (static_cast<unsigned long>(DRM_IOCTL_GEM_CLOSE)),
> Same with this, right? The only other use besides vgem is 
> DrmDevice::CloseBufferHandle, which I think is only called from the GPU
process
> (DrmConsoleBuffer)

yup! PTAL.

[vignatti (out of this project), 2016-03-24 17:48:15]

tiago.vignatti@intel.com changed reviewers:
+ mkwst@chromium.org

[vignatti (out of this project), 2016-03-24 17:48:41]

mkwst@ PTAL on content/common/child_process_messages.h

[piman, 2016-03-24 18:44:57]

lgtm

[mdempsky, 2016-03-24 19:18:14]

lgtm with nit for content/common/sandbox_linux/OWNERS

https://codereview.chromium.org/1262043002/diff/120001/content/common/sandbox...
File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right):

https://codereview.chromium.org/1262043002/diff/120001/content/common/sandbox...
content/common/sandbox_linux/bpf_renderer_policy_linux.cc:8: #include
<libdrm/drm.h>
Why is this needed?  I think it can be removed too?

[vignatti (out of this project), 2016-03-24 19:54:56]

https://codereview.chromium.org/1262043002/diff/120001/content/common/sandbox...
File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right):

https://codereview.chromium.org/1262043002/diff/120001/content/common/sandbox...
content/common/sandbox_linux/bpf_renderer_policy_linux.cc:8: #include
<libdrm/drm.h>
On 2016/03/24 19:18:14, mdempsky wrote:
> Why is this needed?  I think it can be removed too?

Done.

[vignatti (out of this project), 2016-03-28 12:57:32]

tiago.vignatti@intel.com changed reviewers:
+ tsepez@chromium.org

[vignatti (out of this project), 2016-03-28 12:58:04]

tsepez@ PTAL on content/common/child_process_messages.h

[Tom Sepez, 2016-03-28 16:04:54]

RS LGTM on deleting code.

[vignatti (out of this project), 2016-03-28 16:50:54]

The CQ bit was checked by tiago.vignatti@intel.com

[vignatti (out of this project), 2016-03-28 16:50:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
dongseong.hwang@intel.com, reveman@chromium.org, spang@chromium.org,
piman@chromium.org, mdempsky@chromium.org
Link to the patchset: https://codereview.chromium.org/1262043002/#ps140001
(title: "remove drm header")

[commit-bot: I haz the power, 2016-03-28 16:51:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1262043002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1262043002/140001

[commit-bot: I haz the power, 2016-03-28 17:50:50]

Description was changed from

==========
Implement DRM Native Pixmap using prime buffer

This CL implements a new client native pixmap through the prime buffer. In
particular it uses a new feature of prime to map the exported graphics fd,
which conveniently can be used in the GpuMemoryBuffer for zero-copy (and
one-copy) rasterization in Chrome.

This pretty much replaces VGEM infrastructure when that's used to share buffers
in Chrome processes, so support for VGEM in Chrome is being removed as well in
this CL.

This feature requires Kernel changes in i915 and drm subsystems, and also in
userspace mini-gbm library.

BUG=581151
TEST=memory coherent (IvyBridge) and non-coherent hardware (BayTrail-T).
==========

to

==========
Implement DRM Native Pixmap using prime buffer

This CL implements a new client native pixmap through the prime buffer. In
particular it uses a new feature of prime to map the exported graphics fd,
which conveniently can be used in the GpuMemoryBuffer for zero-copy (and
one-copy) rasterization in Chrome.

This pretty much replaces VGEM infrastructure when that's used to share buffers
in Chrome processes, so support for VGEM in Chrome is being removed as well in
this CL.

This feature requires Kernel changes in i915 and drm subsystems, and also in
userspace mini-gbm library.

BUG=581151
TEST=memory coherent (IvyBridge) and non-coherent hardware (BayTrail-T).
==========

[commit-bot: I haz the power, 2016-03-28 17:50:52]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-28 17:52:03]

Description was changed from

==========
Implement DRM Native Pixmap using prime buffer

This CL implements a new client native pixmap through the prime buffer. In
particular it uses a new feature of prime to map the exported graphics fd,
which conveniently can be used in the GpuMemoryBuffer for zero-copy (and
one-copy) rasterization in Chrome.

This pretty much replaces VGEM infrastructure when that's used to share buffers
in Chrome processes, so support for VGEM in Chrome is being removed as well in
this CL.

This feature requires Kernel changes in i915 and drm subsystems, and also in
userspace mini-gbm library.

BUG=581151
TEST=memory coherent (IvyBridge) and non-coherent hardware (BayTrail-T).
==========

to

==========
Implement DRM Native Pixmap using prime buffer

This CL implements a new client native pixmap through the prime buffer. In
particular it uses a new feature of prime to map the exported graphics fd,
which conveniently can be used in the GpuMemoryBuffer for zero-copy (and
one-copy) rasterization in Chrome.

This pretty much replaces VGEM infrastructure when that's used to share buffers
in Chrome processes, so support for VGEM in Chrome is being removed as well in
this CL.

This feature requires Kernel changes in i915 and drm subsystems, and also in
userspace mini-gbm library.

BUG=581151
TEST=memory coherent (IvyBridge) and non-coherent hardware (BayTrail-T).

Committed: https://crrev.com/eb129a0286bf817ce4e2d182ec1ce8222d50a316
Cr-Commit-Position: refs/heads/master@{#383513}
==========

[commit-bot: I haz the power, 2016-03-28 17:52:05]

Patchset 8 (id:??) landed as
https://crrev.com/eb129a0286bf817ce4e2d182ec1ce8222d50a316
Cr-Commit-Position: refs/heads/master@{#383513}