Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
215143002:
ui: Add Linux dma-buf implementation of GLImage. (Closed)
|
Created:
6 years, 9 months ago by reveman Modified:
6 years, 2 months ago CC:
chromium-reviews, fjhenigman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionui: Add Linux dma-buf implementation of GLImage.
This adds an implementation of GLImage base on
EXT_image_dma_buf_import. This can be used to implement
a GpuMemoryBuffer type that use a Linux dma_buf file
descriptor as export.
BUG=356871
Committed: https://crrev.com/194331b394178f526b37c51ba0f663e33f46431a
Cr-Commit-Position: refs/heads/master@{#299650}
Patch Set 1 #
Total comments: 2
Patch Set 2 : remove dup #Patch Set 3 : add comment about eglCreateImageKHR taking a reference to the dma_buf #Patch Set 4 : support a more useful set of stride values #
Total comments: 3
Patch Set 5 : rebae #Patch Set 6 : rebase #Patch Set 7 : rebase #
Total comments: 8
Patch Set 8 : add comment to header #
Messages
Total messages: 25 (2 generated)
https://codereview.chromium.org/215143002/diff/1/ui/gl/gl_image_linux_dma_buf... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/215143002/diff/1/ui/gl/gl_image_linux_dma_buf... ui/gl/gl_image_linux_dma_buffer.cc:92: close(fd_); The spec [1] and a mesa-dev thread [2] and the current mesa implementation suggest to me that we don't need to keep the file descriptor open after Initialize(). A piglit test seems to disagree but it may be out of date. If so, we can do away with the dup() and close(). I wouldn't bet my life on it but I would give it a try. [1] https://www.khronos.org/registry/egl/extensions/EXT/EGL_EXT_image_dma_buf_imp... : "If eglCreateImageKHR is successful for a EGL_LINUX_DMA_BUF_EXT target, the EGL will take a reference to the dma_buf(s) which it will release at any time while the EGLDisplay is initialized. It is the responsibility of the application to close the dma_buf file descriptors." [2] http://marc.info/?t=138153116700005
https://codereview.chromium.org/215143002/diff/1/ui/gl/gl_image_linux_dma_buf... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/215143002/diff/1/ui/gl/gl_image_linux_dma_buf... ui/gl/gl_image_linux_dma_buffer.cc:92: close(fd_); On 2014/03/27 23:35:49, fjhenigman wrote: > The spec [1] and a mesa-dev thread [2] and the current mesa implementation > suggest to me that we don't need to keep the file descriptor open after > Initialize(). A piglit test seems to disagree but it may be out of date. > If so, we can do away with the dup() and close(). I wouldn't bet my life on it > but I would give it a try. > > [1] > https://www.khronos.org/registry/egl/extensions/EXT/EGL_EXT_image_dma_buf_imp... > : "If eglCreateImageKHR is successful for a EGL_LINUX_DMA_BUF_EXT target, the > EGL will take a reference to the dma_buf(s) which it will release at any time > while the EGLDisplay is initialized. It is the responsibility of the > application to close the dma_buf file descriptors." > > [2] http://marc.info/?t=138153116700005 Ok, cool. Removed dup from latest patch.
lgtm with a small nit. Thank you for adding this, this will be useful for us on ozone as well. https://codereview.chromium.org/215143002/diff/60001/ui/gl/gl_image_linux_dma... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/215143002/diff/60001/ui/gl/gl_image_linux_dma... ui/gl/gl_image_linux_dma_buffer.cc:51: // Aligns 'value' by rounding it up to the next multiple of 'alignment' period at the end of line.
lgtm, but it looks like mesa is out of date with EXT_image_dma_buf_import. Version 5 of the spec says to close the dma-buf fd if eglCreateImageKHR succeeds, but not if it fails, and this is what mesa does. Version 6 of the spec says not to close in either case. I'll see about getting this fixed in mesa. Until then, you might have problems with a double close. If so you can either ignore errors from close, patch mesa, or code to the version 5 spec. We can quickly patch chromeos mesa, no need to wait for upstream, once my analysis is confirmed. https://codereview.chromium.org/215143002/diff/60001/ui/gl/gl_image_linux_dma... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/215143002/diff/60001/ui/gl/gl_image_linux_dma... ui/gl/gl_image_linux_dma_buffer.cc:76: int stride = AlignValue(size_.width() * BytesPerPixel(internalformat_), 64); would it make sense to pass in the stride as a parameter?
Rebased onto CHROMIUM_image refactor PTAL. https://codereview.chromium.org/215143002/diff/60001/ui/gl/gl_image_linux_dma... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/215143002/diff/60001/ui/gl/gl_image_linux_dma... ui/gl/gl_image_linux_dma_buffer.cc:76: int stride = AlignValue(size_.width() * BytesPerPixel(internalformat_), 64); On 2014/06/27 18:19:31, fjhenigman wrote: > would it make sense to pass in the stride as a parameter? Done in latest patch.
https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... ui/gl/gl_image_linux_dma_buffer.cc:23: case gfx::GpuMemoryBuffer::RGBX_8888: I noticed you left plumbing for internal format here and in your big refactor. Is there a particular reason you chose to keep it once the code leases the command buffer? I may have missed it in your big patch, but it seemed like gfx::GpuMemoryBuffer::Format alone was sufficient once GL calls are processed.
https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... ui/gl/gl_image_linux_dma_buffer.cc:23: case gfx::GpuMemoryBuffer::RGBX_8888: On 2014/10/10 14:46:35, alexst wrote: > I noticed you left plumbing for internal format here and in your big refactor. > Is there a particular reason you chose to keep it once the code leases the > command buffer? > > I may have missed it in your big patch, but it seemed like > gfx::GpuMemoryBuffer::Format alone was sufficient once GL calls are processed. I think it's better to explicitly express the internalformat of buffers when introducing them to the GL rather than having that magically happen. This way the client knows what to expect when sampling from a texture with a bound image. It also allows us to potentially use a RGBA_8888 buffer with internalformat GL_RGB and ignore the alpha channel.
Still lgtm. To follow up my comment #5, mesa was indeed doing the wrong thing. This is fixed upstream so eglCreateImage will never close dmabuf fds passed to it, but has not yet trickled down to chrome os. I can backport if needed.
On 2014/10/10 15:06:14, reveman wrote: > https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... > File ui/gl/gl_image_linux_dma_buffer.cc (right): > > https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... > ui/gl/gl_image_linux_dma_buffer.cc:23: case gfx::GpuMemoryBuffer::RGBX_8888: > On 2014/10/10 14:46:35, alexst wrote: > > I noticed you left plumbing for internal format here and in your big refactor. > > Is there a particular reason you chose to keep it once the code leases the > > command buffer? > > > > I may have missed it in your big patch, but it seemed like > > gfx::GpuMemoryBuffer::Format alone was sufficient once GL calls are processed. > > I think it's better to explicitly express the internalformat of buffers when > introducing them to the GL rather than having that magically happen. This way > the client knows what to expect when sampling from a texture with a bound image. > > It also allows us to potentially use a RGBA_8888 buffer with internalformat > GL_RGB and ignore the alpha channel. Okay, lgtm.
Dave, can we cq this?
reveman@chromium.org changed reviewers: + piman@chromium.org, sievers@chromium.org
just have piman or sievers rubber stamp it first
lgtm https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl.gyp File ui/gl/gl.gyp (right): https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl.gyp#newcode203 ui/gl/gl.gyp:203: 'gl_image_linux_dma_buffer.h', If you name it gl_image_dma_buffer_linux.cc/h you don't have to do the conditional logic here and in GN
https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... ui/gl/gl_image_linux_dma_buffer.cc:75: bool GLImageLinuxDMABuffer::Initialize(const base::FileDescriptor& handle, Should this take ownership of the file descriptor, and close it (on destruction?) If not, what keeps ownership?
https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl.gyp File ui/gl/gl.gyp (right): https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl.gyp#newcode203 ui/gl/gl.gyp:203: 'gl_image_linux_dma_buffer.h', On 2014/10/14 21:57:54, sievers wrote: > If you name it gl_image_dma_buffer_linux.cc/h you don't have to do the > conditional logic here and in GN I chose LinuxDMABuffer to match EGL_LINUX_DMA_BUF_EXT and I prefer that as it makes it less likely to be confused with the "DMA buffer" implementation used on linux. For example, we might want to limit this to ozone or chromeos and renaming it to gl_image_dma_buffer_ozone.cc/h would be incorrect in that case.. https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... ui/gl/gl_image_linux_dma_buffer.cc:75: bool GLImageLinuxDMABuffer::Initialize(const base::FileDescriptor& handle, On 2014/10/14 23:52:26, piman (Very slow to review) wrote: > Should this take ownership of the file descriptor, and close it (on > destruction?) > > If not, what keeps ownership? Ownership is not passed to this function. It is the responsibility of the caller to close the file descriptor on success and failure. eglCreateImageKHR with EGL_LINUX_DMA_BUF_EXT as target will dup the fd on success if I understand it correctly. The comment below was supposed to make this clear but maybe it can be improved? Some discussion related to this earlier and I think there used to be a bug in Mesa that prevented this from working correctly but it should be fixed now, I think. fjhenigman, is this correct?
On 2014/10/15 00:48:07, reveman wrote: > https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl.gyp > File ui/gl/gl.gyp (right): > > https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl.gyp#newcode203 > ui/gl/gl.gyp:203: 'gl_image_linux_dma_buffer.h', > On 2014/10/14 21:57:54, sievers wrote: > > If you name it gl_image_dma_buffer_linux.cc/h you don't have to do the > > conditional logic here and in GN > > I chose LinuxDMABuffer to match EGL_LINUX_DMA_BUF_EXT and I prefer that as it > makes it less likely to be confused with the "DMA buffer" implementation used on > linux. For example, we might want to limit this to ozone or chromeos and > renaming it to gl_image_dma_buffer_ozone.cc/h would be incorrect in that case.. > > https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... > File ui/gl/gl_image_linux_dma_buffer.cc (right): > > https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... > ui/gl/gl_image_linux_dma_buffer.cc:75: bool > GLImageLinuxDMABuffer::Initialize(const base::FileDescriptor& handle, > On 2014/10/14 23:52:26, piman (Very slow to review) wrote: > > Should this take ownership of the file descriptor, and close it (on > > destruction?) > > > > If not, what keeps ownership? > > Ownership is not passed to this function. It is the responsibility of the caller > to close the file descriptor on success and failure. eglCreateImageKHR with > EGL_LINUX_DMA_BUF_EXT as target will dup the fd on success if I understand it > correctly. The comment below was supposed to make this clear but maybe it can be > improved? > > Some discussion related to this earlier and I think there used to be a bug in > Mesa that prevented this from working correctly but it should be fixed now, I > think. fjhenigman, is this correct? Stephane and Zachary merged some Mesa patches to get it going. There is a working prototype using this path, and I'll follow up on Frank's comment to make sure eglImageCreate isn't closing the fd.
On 2014/10/15 00:48:07, reveman wrote: > https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl.gyp > File ui/gl/gl.gyp (right): > > https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl.gyp#newcode203 > ui/gl/gl.gyp:203: 'gl_image_linux_dma_buffer.h', > On 2014/10/14 21:57:54, sievers wrote: > > If you name it gl_image_dma_buffer_linux.cc/h you don't have to do the > > conditional logic here and in GN > > I chose LinuxDMABuffer to match EGL_LINUX_DMA_BUF_EXT and I prefer that as it > makes it less likely to be confused with the "DMA buffer" implementation used on > linux. For example, we might want to limit this to ozone or chromeos and > renaming it to gl_image_dma_buffer_ozone.cc/h would be incorrect in that case.. > > https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... > File ui/gl/gl_image_linux_dma_buffer.cc (right): > > https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... > ui/gl/gl_image_linux_dma_buffer.cc:75: bool > GLImageLinuxDMABuffer::Initialize(const base::FileDescriptor& handle, > On 2014/10/14 23:52:26, piman (Very slow to review) wrote: > > Should this take ownership of the file descriptor, and close it (on > > destruction?) > > > > If not, what keeps ownership? > > Ownership is not passed to this function. It is the responsibility of the caller > to close the file descriptor on success and failure. eglCreateImageKHR with > EGL_LINUX_DMA_BUF_EXT as target will dup the fd on success if I understand it > correctly. The comment below was supposed to make this clear but maybe it can be > improved? > > Some discussion related to this earlier and I think there used to be a bug in > Mesa that prevented this from working correctly but it should be fixed now, I > think. fjhenigman, is this correct? Stephane and Zachary merged some Mesa patches to get it going. There is a working prototype using this path, and I'll follow up on Frank's comment to make sure eglImageCreate isn't closing the fd.
lgtm https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... ui/gl/gl_image_linux_dma_buffer.cc:75: bool GLImageLinuxDMABuffer::Initialize(const base::FileDescriptor& handle, On 2014/10/15 00:48:07, reveman wrote: > On 2014/10/14 23:52:26, piman (Very slow to review) wrote: > > Should this take ownership of the file descriptor, and close it (on > > destruction?) > > > > If not, what keeps ownership? > > Ownership is not passed to this function. It is the responsibility of the caller > to close the file descriptor on success and failure. eglCreateImageKHR with > EGL_LINUX_DMA_BUF_EXT as target will dup the fd on success if I understand it > correctly. The comment below was supposed to make this clear but maybe it can be > improved? I see... it was not evident at first. Maybe "the EGL will take a reference to the dma_buf and the file descriptor can be closed." ? Maybe move that comment to the header file? > > Some discussion related to this earlier and I think there used to be a bug in > Mesa that prevented this from working correctly but it should be fixed now, I > think. fjhenigman, is this correct?
lgtm lgtm https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... ui/gl/gl_image_linux_dma_buffer.cc:75: bool GLImageLinuxDMABuffer::Initialize(const base::FileDescriptor& handle, On 2014/10/15 00:48:07, reveman wrote: > On 2014/10/14 23:52:26, piman (Very slow to review) wrote: > > Should this take ownership of the file descriptor, and close it (on > > destruction?) > > > > If not, what keeps ownership? > > Ownership is not passed to this function. It is the responsibility of the caller > to close the file descriptor on success and failure. eglCreateImageKHR with > EGL_LINUX_DMA_BUF_EXT as target will dup the fd on success if I understand it > correctly. The comment below was supposed to make this clear but maybe it can be > improved? I see... it was not evident at first. Maybe "the EGL will take a reference to the dma_buf and the file descriptor can be closed." ? Maybe move that comment to the header file? > > Some discussion related to this earlier and I think there used to be a bug in > Mesa that prevented this from working correctly but it should be fixed now, I > think. fjhenigman, is this correct?
https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... File ui/gl/gl_image_linux_dma_buffer.cc (right): https://codereview.chromium.org/215143002/diff/120001/ui/gl/gl_image_linux_dm... ui/gl/gl_image_linux_dma_buffer.cc:75: bool GLImageLinuxDMABuffer::Initialize(const base::FileDescriptor& handle, On 2014/10/15 03:19:52, piman (Very slow to review) wrote: > On 2014/10/15 00:48:07, reveman wrote: > > On 2014/10/14 23:52:26, piman (Very slow to review) wrote: > > > Should this take ownership of the file descriptor, and close it (on > > > destruction?) > > > > > > If not, what keeps ownership? > > > > Ownership is not passed to this function. It is the responsibility of the > caller > > to close the file descriptor on success and failure. eglCreateImageKHR with > > EGL_LINUX_DMA_BUF_EXT as target will dup the fd on success if I understand it > > correctly. The comment below was supposed to make this clear but maybe it can > be > > improved? > > I see... it was not evident at first. Maybe "the EGL will take a reference to > the dma_buf and the file descriptor can be closed." ? > Maybe move that comment to the header file? > > > > > Some discussion related to this earlier and I think there used to be a bug in > > Mesa that prevented this from working correctly but it should be fixed now, I > > think. fjhenigman, is this correct? > Added a comment to the header. Kept the comment here too as it's more implementation specific and explains why we don't have to explicitly dup the fd here.
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/215143002/270001
Message was sent while issue was closed.
Committed patchset #8 (id:270001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/194331b394178f526b37c51ba0f663e33f46431a Cr-Commit-Position: refs/heads/master@{#299650} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
