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

ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc

Index: ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc
diff --git a/ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc b/ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc
index 1dbb94d7908b85242ff3ed265b1be00733eed640..7a943413b2605139256e5efe9630c33d7b2b0080 100644
--- a/ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc
+++ b/ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc
@@ -4,16 +4,33 @@
 
 #include "ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.h"
 
+#include <fcntl.h>
+
 #include "base/file_descriptor_posix.h"
 #include "ui/ozone/public/client_native_pixmap_factory.h"
 
+#if defined(OZONE_USE_VGEM_MAP)
+#include "ui/ozone/platform/drm/gpu/client_native_pixmap_vgem.h"
+#endif
+
 namespace ui {
 
 namespace {
 
 class ClientNativePixmapFactoryGbm : public ClientNativePixmapFactory {
  public:
-  ClientNativePixmapFactoryGbm() {}
+  ClientNativePixmapFactoryGbm() {
+#if defined(OZONE_USE_VGEM_MAP)
+    // TODO(dshwang): remove ad-hoc file open. crrev.com/1248713002
+    static const char kVgemPath[] = "/dev/dri/renderD129";
+    int vgem_fd = open(kVgemPath, O_RDWR | O_CLOEXEC);
+    if (vgem_fd < 0) {
+      PLOG(ERROR) << "Failed to open: " << kVgemPath;
+      return;
+    }
+    vgem_fd_.reset(vgem_fd);
+#endif
+  }
   ~ClientNativePixmapFactoryGbm() override {}
 
   // ClientNativePixmapFactory:
@@ -23,6 +40,13 @@ class ClientNativePixmapFactoryGbm : public ClientNativePixmapFactory {
         {gfx::BufferFormat::RGBX_8888, gfx::BufferUsage::SCANOUT}};
     std::vector<Configuration> configurations(
         kConfiguratioins, kConfiguratioins + arraysize(kConfiguratioins));
+#if defined(OZONE_USE_VGEM_MAP)
+    // Map requires VGEM supports.
+    if (vgem_fd_.get() >= 0) {
+      configurations.push_back(
+          {gfx::BufferFormat::BGRA_8888, gfx::BufferUsage::MAP});
+    }
+#endif
     return configurations;
   }
   scoped_ptr<ClientNativePixmap> ImportFromHandle(
@@ -30,11 +54,26 @@ class ClientNativePixmapFactoryGbm : public ClientNativePixmapFactory {
       const gfx::Size& size,
       gfx::BufferFormat format,
       gfx::BufferUsage usage) override {
-    NOTIMPLEMENTED();
+    if (usage == gfx::BufferUsage::SCANOUT) {

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

what happens if we simply ignore this check here?

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

ImportFromHandle() doesn't create pixmap, so
GpuMemoryBufferImplOzoneNativePixmap::Map() will cause crash.

[reveman, 2015/08/11 18:53:53]

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?

[dshwang, 2015/08/11 19:36:06]

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.

[reveman, 2015/08/11 20:15:16]

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.

[dshwang, 2015/08/12 08:57:35]

crrev.com/1263323004 returns ClientNativePixmapGbm now. I update that
ClientNativePixmapVgem is returned only if usage is MAP. Now it's switch
statement. 

+      DCHECK(handle.fd.auto_close);
+      base::ScopedFD close_fd(handle.fd.fd);
+      return nullptr;
+    }
+#if defined(OZONE_USE_VGEM_MAP)
+    DCHECK_GE(vgem_fd_.get(), 0);
+    DCHECK(format == gfx::BufferFormat::BGRA_8888);

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

DCHECK_EQ and move outside the ifdef as we might as well check this always

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

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.

+    DCHECK(usage == gfx::BufferUsage::MAP);

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

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?

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

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.

[reveman, 2015/08/11 18:53:53]

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.

[dshwang, 2015/08/11 19:36:06]

Done. now I understand your request correctly. I check that the configuration is
returned by GetSupportedConfigurations().

+    return ClientNativePixmapVgem::Create(
+        handle, base::FileDescriptor(vgem_fd_.get(), false), size);
+#else
+    NOTREACHED();
     return nullptr;
+#endif
   }
 
  private:
+  base::ScopedFD vgem_fd_;
+
   DISALLOW_COPY_AND_ASSIGN(ClientNativePixmapFactoryGbm);
 };