ozone: Implement zero/one-copy texture for Ozone GBM.

ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc

Index: ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc
diff --git a/ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc b/ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc
new file mode 100644
index 0000000000000000000000000000000000000000..adef42adbd47fa51377adc48207be4965aca8a30
--- /dev/null
+++ b/ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.cc
@@ -0,0 +1,106 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h"
+
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <vgem_drm.h>

[vignatti (out of this project), 2015/08/06 20:56:26]

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.

[dshwang, 2015/08/07 08:54:53]

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.

+#include <xf86drm.h>
+
+#include "base/trace_event/trace_event.h"
+
+namespace ui {
+
+// static
+scoped_ptr<ClientNativePixmap> ClientNativePixmapVgem::Create(
+    const gfx::NativePixmapHandle& handle,
+    const base::FileDescriptor& vgem_handle,
+    const gfx::Size& size,
+    gfx::BufferFormat format,
+    gfx::BufferUsage usage) {
+  DCHECK_GE(vgem_handle.fd, 0);
+  DCHECK_GE(handle.fd.fd, 0);
+  DCHECK(usage != gfx::BufferUsage::SCANOUT);

[reveman, 2015/08/07 14:35:16]

Why do we care? What part of the code here doesn't work when usage is SCANOUT?

[dshwang, 2015/08/07 15:36:46]

drmPrimeFDToHandle can fail on the hardware, which doesn't support vgem
SCANOUT should work well on the hardware.

+  uint32_t vgem_bo_handle = 0;
+  int ret = drmPrimeFDToHandle(vgem_handle.fd, handle.fd.fd, &vgem_bo_handle);

[reveman, 2015/08/07 14:35:16]

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..

[dshwang, 2015/08/07 15:36:46]

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.

[reveman, 2015/08/07 17:59:44]

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.

[dshwang, 2015/08/11 17:44:55]

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?

+  if (ret) {
+    PLOG(ERROR) << "drmPrimeFDToHandle failed, handle:" << vgem_bo_handle;
+    return nullptr;
+  }
+  return make_scoped_ptr(new ClientNativePixmapVgem(
+      handle, vgem_handle, size, format, usage, vgem_bo_handle));
+}
+
+ClientNativePixmapVgem::ClientNativePixmapVgem(
+    const gfx::NativePixmapHandle& handle,
+    const base::FileDescriptor& vgem_handle,
+    const gfx::Size& size,
+    gfx::BufferFormat format,
+    gfx::BufferUsage usage,
+    uint32_t vgem_bo_handle)
+    : dma_buf_(handle.fd.fd),
+      vgem_fd_(vgem_handle),
+      size_(size),
+      format_(format),
+      usage_(usage),
+      vgem_bo_handle_(vgem_bo_handle),
+      stride_(handle.stride),
+      mmap_ptr_(nullptr) {
+  DCHECK(vgem_bo_handle_);
+}
+
+ClientNativePixmapVgem::~ClientNativePixmapVgem() {
+  if (mmap_ptr_) {
+    size_t size = stride_ * size_.height();
+    int ret = munmap(mmap_ptr_, size);
+    if (ret)
+      PLOG(ERROR) << "fail to munmap a vgem buffer. error:" << ret;
+    mmap_ptr_ = nullptr;
+  }
+
+  struct drm_gem_close close = {0};
+  close.handle = vgem_bo_handle_;
+  int ret = drmIoctl(vgem_fd_.fd, DRM_IOCTL_GEM_CLOSE, &close);
+  if (ret)
+    PLOG(ERROR) << "fail to free a vgem buffer. error:" << ret;
+  vgem_bo_handle_ = 0;
+}
+
+bool ClientNativePixmapVgem::Map(void** data) {
+  TRACE_EVENT0("gpu", "ClientNativePixmapVgem::Map");
+  if (mmap_ptr_) {
+    *data = mmap_ptr_;
+    return true;
+  }
+
+  struct drm_mode_map_dumb mmap_arg = {0};
+  mmap_arg.handle = vgem_bo_handle_;
+
+  int ret = drmIoctl(vgem_fd_.fd, DRM_IOCTL_VGEM_MODE_MAP_DUMB, &mmap_arg);
+  if (ret) {
+    PLOG(ERROR) << "fail to map a vgem buffer. error:" << ret;
+    return false;
+  }
+  DCHECK(mmap_arg.offset);
+
+  size_t size = stride_ * size_.height();
+  mmap_ptr_ = mmap(nullptr, size, (PROT_READ | PROT_WRITE), MAP_SHARED,

[reveman, 2015/08/07 14:35:16]

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.

[dshwang, 2015/08/07 15:36:46]

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.

+                   vgem_fd_.fd, mmap_arg.offset);
+  DCHECK(mmap_ptr_ != MAP_FAILED);

[reveman, 2015/08/07 14:35:16]

nit: DCHECK_NE

[dshwang, 2015/08/07 15:36:46]

Done.

+  *data = mmap_ptr_;
+  return true;
+}
+
+void ClientNativePixmapVgem::Unmap() {
+  TRACE_EVENT0("gpu", "ClientNativePixmapVgem::Unmap");

[reveman, 2015/08/07 14:35:16]

this trace doesn't seem very useful

[dshwang, 2015/08/07 15:36:46]

after roll back, it seems useful

+  DCHECK(mmap_ptr_);
+  // keep it consistently mapped, and unmap it in dtor.
+}
+
+void ClientNativePixmapVgem::GetStride(int* stride) const {
+  *stride = stride_;
+}
+
+}  // namespace ui