[Mesa-dev] [PATCH] egl_dri2: fix EXT_image_dma_buf_import fds

[Pohjolainen, Topi]

On Fri, Aug 08, 2014 at 05:28:59PM +0300, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> 
> The EGL_EXT_image_dma_buf_import specification was revised (according to
> its revision history) on Dec 5th, 2013, for EGL to not take ownership of
> the file descriptors.
> 
> Do not close the file descriptors passed in to eglCreateImageKHR with
> EGL_LINUX_DMA_BUF_EXT target.
> 
> It is assumed, that the drivers, which ultimately process the file
> descriptors, do not close or modify them in any way either. This avoids
> the need to dup(), as it seems we would only need to just close the
> dup'd file descriptors right after.
> 
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

I wrote the current logic based on the older version, and at least to me this
is the right thing to do. Thanks for fixing it as well as taking care of the
piglit test.

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

I would be happier though if someone else gave his/her approval as well.

> ---
> 
> Hi,
> 
> the corresponding Piglit fix has already been sent to the piglit mailing
> list. Both this and that need to be applied to not regress Mesa' piglit run
> by one test (ext_image_dma_buf_import-ownership_transfer).
> 
> This patch fixes my test case on heavily modified Weston.
> 
> Thanks,
> pq
> ---
>  src/egl/drivers/dri2/egl_dri2.c | 37 ++++++-------------------------------
>  1 file changed, 6 insertions(+), 31 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 5602ec3..cd85fd3 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1678,36 +1678,13 @@ dri2_check_dma_buf_format(const _EGLImageAttribs *attrs)
>  /**
>   * The spec says:
>   *
> - * "If eglCreateImageKHR is successful for a EGL_LINUX_DMA_BUF_EXT target,
> - *  the EGL takes ownership of the file descriptor and is responsible for
> - *  closing it, which it may do at any time while the EGLDisplay is
> - *  initialized."
> + * "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."
> + *
> + * Therefore we must never close or otherwise modify the file descriptors.
>   */
> -static void
> -dri2_take_dma_buf_ownership(const int *fds, unsigned num_fds)
> -{
> -   int already_closed[num_fds];
> -   unsigned num_closed = 0;
> -   unsigned i, j;
> -
> -   for (i = 0; i < num_fds; ++i) {
> -      /**
> -       * The same file descriptor can be referenced multiple times in case more
> -       * than one plane is found in the same buffer, just with a different
> -       * offset.
> -       */
> -      for (j = 0; j < num_closed; ++j) {
> -         if (already_closed[j] == fds[i])
> -            break;
> -      }
> -
> -      if (j == num_closed) {
> -         close(fds[i]);
> -         already_closed[num_closed++] = fds[i];
> -      }
> -   }
> -}
> -
>  static _EGLImage *
>  dri2_create_image_dma_buf(_EGLDisplay *disp, _EGLContext *ctx,
>  			  EGLClientBuffer buffer, const EGLint *attr_list)
> @@ -1770,8 +1747,6 @@ dri2_create_image_dma_buf(_EGLDisplay *disp, _EGLContext *ctx,
>        return EGL_NO_IMAGE_KHR;
>  
>     res = dri2_create_image_from_dri(disp, dri_image);
> -   if (res)
> -      dri2_take_dma_buf_ownership(fds, num_fds);
>  
>     return res;
>  }
> -- 
> 1.8.5.5
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev