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

[Pekka Paalanen]

On Wed, 13 Aug 2014 19:46:40 +0300
"Pohjolainen, Topi" <topi.pohjolainen at intel.com> wrote:

> 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.

Thank you, I have added your R-b, and will wait some more. I think I
want the piglit patch landed first before I try to push this, anyway.

Thanks for the piglit review too, I sent a new version with your R-b
and the comment fix.


- pq

> 
> > ---
> > 
> > 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