> -----Original Message----- > From: Chad Versace [mailto:[email protected]] > Sent: Saturday, April 29, 2017 12:19 AM > To: Emil Velikov <[email protected]> > Cc: Xu, Randy <[email protected]>; [email protected] > 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 <[email protected]> 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 <[email protected]> > Cc: [email protected] > > 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 <[email protected]> " > > > > 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. _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
