[Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out fence

Fri Sep 8 16:54:55 UTC 2017

On Fri, Sep 08, 2017 at 08:32:05AM -0700, Marathe, Yogesh wrote:
> > -----Original Message-----
> > From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
> > Sent: Friday, September 8, 2017 8:28 PM
> > To: Marathe, Yogesh <yogesh.marathe at intel.com>
> > Cc: Tomasz Figa <tfiga at chromium.org>; Antognolli, Rafael
> > <rafael.antognolli at intel.com>; Janes, Mark A <mark.a.janes at intel.com>;
> > mesa-dev at lists.freedesktop.org; Gao, Shuo <shuo.gao at intel.com>; Liu,
> > Zhiquan <zhiquan.liu at intel.com>; daniels at collabora.com;
> > nicolai.haehnle at amd.com; eric at engestrom.ch; Wu, Zhongmin
> > <zhongmin.wu at intel.com>; kenneth at whitecape.org; Kondapally, Kalyan
> > <kalyan.kondapally at intel.com>; fernetmenta at online.de;
> > tarceri at itsqueeze.com; varad.gautam at collabora.com
> > Subject: Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out
> > fence
> > 
> > On 8 September 2017 at 14:47, Marathe, Yogesh <yogesh.marathe at intel.com>
> > wrote:
> > > Hello Folks,
> > >
> > > Sorry for late reply, I took quite some time to CTS up, comments below.
> > >
> > >> -----Original Message-----
> > >> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On
> > >> Behalf Of Marathe, Yogesh
> > >> Sent: Friday, September 1, 2017 10:16 AM
> > >> To: Tomasz Figa <tfiga at chromium.org>
> > >> Cc: Gao, Shuo <shuo.gao at intel.com>; Liu, Zhiquan
> > >> <zhiquan.liu at intel.com>; daniels at collabora.com;
> > >> nicolai.haehnle at amd.com; Antognolli, Rafael
> > >> <rafael.antognolli at intel.com>; eric at engestrom.ch; Emil Velikov
> > >> <emil.l.velikov at gmail.com>; Wu, Zhongmin <zhongmin.wu at intel.com>;
> > >> kenneth at whitecape.org; Kondapally, Kalyan
> > >> <kalyan.kondapally at intel.com>; fernetmenta at online.de;
> > >> mesa-dev at lists.freedesktop.org; tarceri at itsqueeze.com;
> > >> varad.gautam at collabora.com
> > >> Subject: Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per
> > >> surface out fence
> > >>
> > >> Tomasz,
> > >>
> > >> > -----Original Message-----
> > >> > From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On
> > >> > Behalf Of Tomasz Figa
> > >> > Sent: Friday, September 1, 2017 9:53 AM
> > >> > To: Marathe, Yogesh <yogesh.marathe at intel.com> On Thu, Aug 31, 2017
> > >> > at 2:18 AM, Marathe, Yogesh <yogesh.marathe at intel.com> wrote:
> > >> > >> -----Original Message-----
> > >> > >> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org]
> > >> > >> On Behalf Of Emil Velikov
> > >> > >> Sent: Wednesday, August 30, 2017 9:44 PM
> > >> > >> To: Marathe, Yogesh <yogesh.marathe at intel.com>
> > >> > >> Cc: Gao, Shuo <shuo.gao at intel.com>; Liu, Zhiquan
> > >> > >> <zhiquan.liu at intel.com>; daniels at collabora.com;
> > >> > >> nicolai.haehnle at amd.com; Antognolli, Rafael
> > >> > >> <rafael.antognolli at intel.com>; eric at engestrom.ch; Wu, Zhongmin
> > >> > >> <zhongmin.wu at intel.com>; tfiga at chromium.org;
> > >> kenneth at whitecape.org;
> > >> > >> Kondapally, Kalyan <kalyan.kondapally at intel.com>;
> > >> > >> fernetmenta at online.de; mesa-dev at lists.freedesktop.org;
> > >> > >> tarceri at itsqueeze.com; varad.gautam at collabora.com
> > >> > >> Subject: Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per
> > >> > >> surface out fence
> > >> > >>
> > >> > >> On 30 August 2017 at 15:39, Marathe, Yogesh
> > >> > >> <yogesh.marathe at intel.com>
> > >> > >> wrote:
> > >> > >>
> > >> > >> >
> > >> > >> > Thank you, Tomasz and all involved for the help and guidance.
> > >> > >> >
> > >> > >> Our excitement was short lived - see commit
> > >> > >> 8c9df0daf20206fafb7df77b1edcbc41b8e91372.
> > >> > >>
> > >> > >> Seems the patch was not run through the Intel CI, though I'm
> > >> > >> should not have assumed that you're aware of if.
> > >> > >> Please get in touch with Mark Janes (Cc'd here, janesma on IRC).
> > >> > >> He can set you up and/or run a branch for you.
> > >> > >>
> > >> > >
> > >> > > No problem. I will contact Mark.
> > >> > >
> > >> > > Primarily looks like platform / kernel version issue.
> > >> > > intel_get_boolean() for I915_PARAM_HAS_EXEC_FENCE is false, but I
> > >> > > see following in i915_drv.c:915_getparam in kernel, no clue why
> > >> > > that would come false in UMD.
> > >> > >
> > >> > > ...
> > >> > >         case I915_PARAM_HAS_EXEC_FENCE:
> > >> > >                 /* For the time being all of these are always true;
> > >> > >                  * if some supported hardware does not have one of these
> > >> > >                  * features this value needs to be provided from
> > >> > >                  * INTEL_INFO(), a feature macro, or similar.
> > >> > >                  */
> > >> > >                 value = 1;
> > >> > >                 break;
> > >> > > ...
> > >> >
> > >> > Which kernel are you looking at? Remember that not everyone uses
> > >> > current upstream master. There is a number of upstream stable
> > >> > releases and downstream forks. Grepping for
> > >> > I915_PARAM_HAS_EXEC_FENCE on http://elixir.free-electrons.com, shows
> > that it was only added in Linux 4.12.
> > >> >
> > >>
> > >> I'm on 4.9.x but I see my kernel  tree has following patch, so this
> > >> looks like it is done for android (cherry picked / backport). That’s why it
> > worked for me always!
> > >>
> > >> commit f0683754f03fa308a2657cb1dadbf235c9607188
> > >> Author: Chris Wilson <chris at chris-wilson.co.uk>
> > >> Date:   Fri Jan 27 09:40:08 2017 +0000
> > >>
> > >>     drm/i915: Support explicit fencing for execbuf
> > >>
> > >> Nonetheless, as you mentioned, I've synced up with Mark and we've
> > >> created a separate branch where CTS / intel mesa CI can run. Let me try to fix
> > this.
> > >>
> > >> Caveat: To have flatland running on android  there was another issue
> > >> in kernel which needed a fix. Details -
> > >> https://bugs.freedesktop.org/show_bug.cgi?id=101656
> > >
> > > I was able to run CTS (https://github.com/KhronosGroup/VK-GL-CTS) on
> > > this patch for x11_egl. I see exact same results before and after patch on
> > Ubuntu 16.04 setup.
> > > Mark had also mentioned  this works fine on 4.12 onwards (essentially with
> > drm/i915:
> > > Support explicit fencing for execbuf patch in kernel).
> > >
> > > Regarding the primary reason why this was reverted
> > >
> > > mesa/drivers/dri/i965/brw_sync.c:491: brw_dri_create_fence_fd:
> > > Assertion `brw->screen->has_exec_fence' failed.
> > >
> > > This assertion evident on older kernels. Although I'm bit surprised
> > > here after looking at the code. Older kernels where this explicit
> > > fencing is not supported must've returned 'false' for has_exec_fence in
> > intelInitScreen2().
> > >
> > >    screen->has_exec_fence =
> > >      intel_get_boolean(screen, I915_PARAM_HAS_EXEC_FENCE);
> > >
> > > It appears that’s coming 'true' and due to that we set
> > > enable_out_fence in dri2_init_surface() (based on get_capabilities()) which
> > causes create_fence_fd call on non-supported kernels.
> > > Isn't this strange? Can someone please comment?
> > >
> > In all fairness there was a few wtf moments as Mark mentioned the issue. As on
> > a quick look "it cannot happen" :-\
> > 
> > One way is to add some printfs "debugging" across the board and check with
> > Mark if he can run (only?) the affected test on the CI.
> > 
> 
> Number of tests failing on CI due to this are huge, any 'one' can be picked up. I do have
> my CI branch setup now but I don’t think I can use it for debugging (not advised).  I'll sync
> up with Mark again. Just wanted a confirmation, I'm not missing something obvious. Thanks.

Hi Yogesh,

I replied to you already when you messaged in private, the error is not
related to the kernel returning true for that, it's related to a memory
corruption caused by wrong use of the dri2_surf_init inside
platform_x11_dri3.c. Quoting myself:

"More specifically, it looks like this test fails every time:

glcts -n dEQP-EGL.functional.query_context.get_current_context.rgb888_window

I see several valgrind warnings inside platform_x11_dri3.c. I believe you are
probably accessing the dri2_surf before it was allocated, or after it was
freed..."

When I tested this back then, the "out_fence_enable" (or whatever was called)
in dri2 was false, but after a couple runs it would become a bogus number,
which also points to memory corruption.

I suggest ignoring the kernel and focusing on valgrind debugging.

Rafael

> > Personally I would be quite generous with my printfs - from the ioctl call via the
> > has_exec_fence handling to the __DRI_FENCE_CAP_NATIVE_FD check.
> > 
> > -Emil