> -----Original Message----- > From: Chad Versace [mailto:chadvers...@chromium.org] > Sent: Saturday, April 29, 2017 12:19 AM > To: Emil Velikov <emil.l.veli...@gmail.com> > Cc: Xu, Randy <randy...@intel.com>; mesa-dev@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...@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 <chadvers...@chromium.org> > Cc: mesa-sta...@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.veli...@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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev