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

[Xu, Randy]

> -----Original Message-----
> From: Chad Versace [mailto:chadversary at chromium.org]
> Sent: Saturday, April 29, 2017 12:19 AM
> To: Emil Velikov <emil.l.velikov at gmail.com>
> Cc: Xu, Randy <randy.xu at intel.com>; mesa-dev at lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH] i965: Solve Android native fence fd double
> close issue
> 
> 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.

It's not dEQP issue, but the instability. The same fd double close will cause GLES or Vulkan app crash on Android platform.
It may take 20 minutes to reproduce it.

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

Thanks, I prefer to merge this simple solution first. 

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