Bug 76188 - EGL_EXT_image_dma_buf_import fd ownership is incorrect
Summary: EGL_EXT_image_dma_buf_import fd ownership is incorrect
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: EGL (show other bugs)
Version: git
Hardware: All Linux (All)
: medium normal
Assignee: mesa-dev
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-14 23:07 UTC by Chad Versace
Modified: 2014-09-06 11:24 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Chad Versace 2014-03-14 23:07:32 UTC
Version #6 of EGL_EXT_image_dma_buf_import changed the semantics of fd ownership. The EGL implementation no longer claims ownership of the fd.

#6 (David Garbett, December 05, 2013)
   - Application now retains ownership of dma_buf file descriptors.

Mesa and Piglit needs to update to the new behavior.
Comment 1 Gwenole Beauchesne 2014-03-19 06:45:21 UTC
(In reply to comment #0)
> Version #6 of EGL_EXT_image_dma_buf_import changed the semantics of fd
> ownership. The EGL implementation no longer claims ownership of the fd.
> 
> #6 (David Garbett, December 05, 2013)
>    - Application now retains ownership of dma_buf file descriptors.
> 
> Mesa and Piglit needs to update to the new behavior.

Is there a way to {compile,run}-time detech the behaviour? Or is it expected that applications migrate once and for all when a new Mesa release occurs? Thanks.

Or, should this be considered as a genuine bug in Mesa implementation only, and applications should readily have been supporting #6 behaviour? On the other hand, #5 was out-of-draft already, thus indicating production-status somehow.
Comment 2 Gwenole Beauchesne 2014-03-19 13:10:42 UTC
According to SVN log, the most approaching constant that could be checked would be EGL_EGLEXT_VERSION >= 20140114. i.e. r20692 for the EXT_image_dma_buf_import spec, and r24760 for the earliest version bump in eglext.h.

Anyway, our Mesa headers are not in sync, so that's probably not much useful. :)
Comment 3 Axel Davy 2014-03-22 03:59:09 UTC
As an additional comment,
I would like to add that glamor uses EGL_EXT_image_dma_buf_import with the new behaviour (actually I didn't know the spec was updated in December, but I knew the decision was taken to go this way).
Comment 4 Pekka Paalanen 2014-08-13 06:00:09 UTC
Hi,

I have patches sent for fixing it:
http://lists.freedesktop.org/archives/mesa-dev/2014-August/065137.html
http://lists.freedesktop.org/archives/piglit/2014-August/012029.html

At least the Mesa patch will get a re-post after I get some more comments.
Comment 5 Pekka Paalanen 2014-08-15 06:01:21 UTC
Patches are now in Piglit and Mesa:
http://cgit.freedesktop.org/piglit/commit/?id=f51d966f0610721a0947619950f43ddd4d29d42d
http://cgit.freedesktop.org/mesa/mesa/commit/?id=08264e5dad4df448e7718e782ad9077902089a07

There is still the question of what should be done with Mesa stable branches 10.0, 10.1, and 10.2. The patch applies on those.
Comment 6 Topi Pohjolainen 2014-08-19 08:56:01 UTC
I would apply the patch as piglit is already updated. Ian, Chad, any opinion?
Comment 7 Chad Versace 2014-08-20 21:10:14 UTC
I see little risk in cherry-picking the fix to stable branches. The fix is isolated and only *removes* code.

I do see risk in not cherry-picking the fix. If an app uses this extension with unfixed Mesa 10.2, then that app will leak file descriptors.
Comment 8 Matt Turner 2014-08-20 21:28:27 UTC
(In reply to comment #7)
> I see little risk in cherry-picking the fix to stable branches. The fix is
> isolated and only *removes* code.
> 
> I do see risk in not cherry-picking the fix. If an app uses this extension
> with unfixed Mesa 10.2, then that app will leak file descriptors.

Sounds good to me. I just wanted you or Ian to take a look at the patch before we shipped it in a release. :)
Comment 9 Pekka Paalanen 2014-08-21 06:11:51 UTC
(In reply to comment #7)
> I do see risk in not cherry-picking the fix. If an app uses this extension
> with unfixed Mesa 10.2, then that app will leak file descriptors.

Hmm, isn't it the vice versa though?

If an app is written to work on unfixed Mesa (the app is broken), it works on both unfixed and fixed Mesa, but leaks fds on fixed Mesa, because nothing will close the fds given to Mesa.

If an app is written to work on fixed Mesa (the app is correct), it will not work on unfixed Mesa, because unfixed Mesa will close the fds behind the app's back. (Assuming the app actually needs to store and use the fds again.)

So, concretely, do I have to send a copy of the patch to mesa-stable@ or something, or is it already taken care of?
Any tag lines to add to the patch?
Comment 10 Chad Versace 2014-08-30 00:48:35 UTC
(In reply to comment #9)
> (In reply to comment #7)
> > I do see risk in not cherry-picking the fix. If an app uses this extension
> > with unfixed Mesa 10.2, then that app will leak file descriptors.
> 
> Hmm, isn't it the vice versa though?

Pekka, you're right. I spoke backwards.
 
> If an app is written to work on unfixed Mesa (the app is broken), it works
> on both unfixed and fixed Mesa, but leaks fds on fixed Mesa, because nothing
> will close the fds given to Mesa.
> 
> If an app is written to work on fixed Mesa (the app is correct), it will not
> work on unfixed Mesa, because unfixed Mesa will close the fds behind the
> app's back. (Assuming the app actually needs to store and use the fds again.)

So regardless of our decision, to backport or not to backport, there will be bugs. I think the right decision in this case is to prefer supporting apps written to the correct behavior. So let's backport.

There is another reason to backport: There likely have never existed apps that relied on the old behavior. During the Khronos discussion where we decided to "fix" the extension, the consensus was that there were no known in-production apps that used the extension. Therefore we believed it was safe to "fix" the extension to follow more traditional Linux fd ownership rules. No existent apps => no real compatibility break. Anyway, we predicted that the yet-to-appear client would might assume traditional fd ownership rules even if we decided to not fix the extension.

> So, concretely, do I have to send a copy of the patch to mesa-stable@ or
> something, or is it already taken care of?
> Any tag lines to add to the patch?

Just send a copy to mesa-stable@. If you're unsure about the protocol, ask Emil (xexaxo on #dri-devel).

Everyone following along... does backporting sound sensible to you too? Am I alone in that opinion?
Comment 11 Pekka Paalanen 2014-09-01 11:18:45 UTC
Sent to Mesa-stable:
http://lists.freedesktop.org/archives/mesa-stable/2014-September/001683.html
Comment 12 Emil Velikov 2014-09-06 11:24:58 UTC
Hello gents,

The commit has been picked up for the latest 10.2.7 release. The 10.0 and 10.1 branches are effectively EOL, so I do not anticipate any further releases for them.


bug/show.html.tmpl processed on Mar 26, 2017 at 13:04:48.
(provided by the Example extension).