ozone: Implement support for CreateGpuMemoryBufferFromHandle for OzoneNativePixmap backed GMBs.

ozone: Implement support for CreateGpuMemoryBufferFromHandle for OzoneNativePixmap backed GMBs.

BUG=538325
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[reveman, 2015-10-08 19:15:13]

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

[piman, 2015-10-08 23:24:37]

https://codereview.chromium.org/1381133002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1381133002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:79:
LOG(ERROR) << "Failed to create pixmap from handle";
nit: DLOG?

https://codereview.chromium.org/1381133002/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right):

https://codereview.chromium.org/1381133002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:154: if (!buffer_) {
I assume eventually we'll want to support this for imported handles too, right?
Do we have an idea what we need to do to make it possible?

https://codereview.chromium.org/1381133002/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/1381133002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:141: int dmabuf_fd =
HANDLE_EINTR(dup(handle.fd.fd));
Why do we need to dup here, and if so, where is the close for the handle that is
passed as a parameter? I would assume
GMBFactory::CreateGpuMemoryBufferFromHandle would take ownership of the handle,
and if not, it needs to close it. The IPC system will not.

[reveman, 2015-10-09 12:47:02]

ptal

https://codereview.chromium.org/1381133002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1381133002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:79:
LOG(ERROR) << "Failed to create pixmap from handle";
On 2015/10/08 at 23:24:36, piman (slow to review) wrote:
> nit: DLOG?

Done. Here and above for consistency.

https://codereview.chromium.org/1381133002/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right):

https://codereview.chromium.org/1381133002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:154: if (!buffer_) {
On 2015/10/08 at 23:24:36, piman (slow to review) wrote:
> I assume eventually we'll want to support this for imported handles too,
right? Do we have an idea what we need to do to make it possible?

I haven't looked at this much in detail but it seems like we need to implement
gbm_bo_import(GBM_BO_IMPORT_FD) in minigbm for this to work properly. Created a
bug and linked to it here in a TODO.

https://codereview.chromium.org/1381133002/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/1381133002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:141: int dmabuf_fd =
HANDLE_EINTR(dup(handle.fd.fd));
On 2015/10/08 at 23:24:36, piman (slow to review) wrote:
> Why do we need to dup here, and if so, where is the close for the handle that
is passed as a parameter? I would assume
GMBFactory::CreateGpuMemoryBufferFromHandle would take ownership of the handle,
and if not, it needs to close it. The IPC system will not.

Good call. We shouldn't dup it here. This just happened to work as we were not
dup-ing the fd returned to the browser so these two errors were cancelling each
other out. Fixed this in latest patch by not calling dup() here and instead
using ExportHandle() in
GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBufferFromHandle. I also
added a comment to the SurfaceFactoryOzone interface to make it clear that this
function takes ownership of the handle.

[piman, 2015-10-09 17:12:45]

Make sense. LGTM!

[spang, 2015-10-09 17:29:20]

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

[spang, 2015-10-09 17:29:21]

https://codereview.chromium.org/1381133002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_buffer.h (right):

https://codereview.chromium.org/1381133002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.h:45: void Initialize(int dma_buf, int
dma_buf_pitch);
I think we should use ScopedFD instead of int here since ownership is
transferred.

[reveman, 2015-10-09 18:45:32]

PTAL

https://codereview.chromium.org/1381133002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_buffer.h (right):

https://codereview.chromium.org/1381133002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.h:45: void Initialize(int dma_buf, int
dma_buf_pitch);
On 2015/10/09 at 17:29:21, spang wrote:
> I think we should use ScopedFD instead of int here since ownership is
transferred.

Done. I also included the GbmPixmap::dma_buf_ change to ScopedFD as it's awkward
to mix.

[reveman, 2015-10-09 18:45:41]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[spang, 2015-10-09 18:47:35]

lgtm

[commit-bot: I haz the power, 2015-10-09 18:47:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1381133002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1381133002/80001

[reveman, 2015-10-09 18:52:20]

The CQ bit was unchecked by reveman@chromium.org

[reveman, 2015-10-09 18:52:24]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-10-09 18:52:24]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1381133002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2015-10-09 18:53:22]

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

[commit-bot: I haz the power, 2015-10-09 19:13:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-09 19:15:05]

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