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

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

 // 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/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:
   std::vector<Configuration> GetSupportedConfigurations() const override {
     const Configuration kConfiguratioins[] = {
         {gfx::BufferFormat::BGRA_8888, gfx::BufferUsage::SCANOUT},
         {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(
       const gfx::NativePixmapHandle& handle,
       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);
 };
 
 }  // namespace
 
 ClientNativePixmapFactory* CreateClientNativePixmapFactoryGbm() {
   return new ClientNativePixmapFactoryGbm();
 }
 
 }  // namespace ui