[Mesa-dev] [PATCH] i965: Solve Android native fence fd double close issue

[Chad Versace]

On Thu 27 Apr 2017, Emil Velikov wrote:
> On 27 April 2017 at 12:14, Xu, Randy <randy.xu at intel.com> wrote:
> > Hi, Chad
> >
> > Please review this patch, we need it to solve some instability issues

Randy and Tapani, could you provide a few dEQP test names that this
patch fixes? I'd like to mention at least one EGL and one Vulkan test in
the commit message.

> The patch is correct, although the commit message can be improved upon.
> Read through the following example and consider the alternative
> solution mentioned within.

Yes, this patch is correct. It makes brw_dri_create_fence_fd() behave
like all the other drivers' create_fence_fd funcs, which call dup().
Since this is an easy one-liner that can backport to stable, let's take
it.

However, I believe the fully correct solution is Emil's plan B:
__DRI2fenceExtensionRec::create_fence_fd should transfer fd ownership to
the driver, and therefore no dup is needed. But that's a slightly more
invasive change that's not as easily backported to stable.

Reviewed-by: Chad Versace <chadversary at chromium.org>
Cc: mesa-stable at lists.freedesktop.org

Emil, how about one of us appends your extended commit message to
Randy's, and then pushes?

> Then either polish and resend, or send patch that implements plan B.
> If you opt for B you want to drop the dup/close from the existing
> users - freedreno and etnaviv.
> 
> "
> The semantics of __DRI2fenceExtensionRec::create_fence_fd are unclear
> if the DRI driver takes ownership of the fd or not.
> Since the i965 driver supports both "in" and "out" fd it assumes "yes,
> driver takes ownership", which results in a double close.
> First time in our destroy_fence() callback and then in the loader.
> 
> Other DRI modules rely on the loader issuing close().
> 
> Thus we have two solutions:
>  - dup() the file descriptor
>  - close() only if we have an out fence.
> 
> This patch implements the former, simpler solution.
> 
> Fixes: 6403e376511 ("i965/sync: Implement fences based on Linux sync_file")
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
> "
> 
> In either case you want to augment create_fence_fd and destroy_fence
> (in dri_interface.h) to explicitly define the behaviour.
> Please keep that a separate patch part of this series.