[Mesa-dev] [PATCH v5 0/3] asynchronous pbo transfer with glthread

[gregory hainaut]

On Thu, 20 Apr 2017 11:57:08 +0200
Marek Olšák <maraeo at gmail.com> wrote:

> On Thu, Apr 20, 2017 at 10:28 AM, gregory hainaut
> <gregory.hainaut at gmail.com> wrote:
> > On Thu, 20 Apr 2017 12:29:11 +0900
> > Michel Dänzer <michel at daenzer.net> wrote:
> >
> >> On 20/04/17 01:54 AM, Gregory Hainaut wrote:
> >> > Hello,
> >> >
> >> > Please find the latest version that include a small fix for hash deletion. I
> >> > think the series is good now. Please review/ack it.
> >>
> >> I'm afraid I have to NACK it. As discussed in the v4 cover letter
> >> thread, Mesa's glthread cannot make any libX11 API calls.
> >>
> >>
> >
> > Hello Michel,
> >
> > Just to be sure we are on the same line, let's me do a summary.
> >
> > PCSX2 does the following bad pattern
> > 1/ no call to XInitThread
> > 2/ XGetGeometry to get the size of the surface
> > 3/ glDrawArray into the default framebuffer 0 (the window, I'm not sure how to call it)
> >    => which seem to call DRI2GetBuffersWithFormat under the hood. I guess to get the
> >       associated buffer of the window.
> >
> >
> > So far it was (kind of) working fine because PCSX2 does tons of PBO transfer. So glthread
> > was mostly synchronous.
> >
> > This series removes the (useless) PBO transfer synchronization. So now glthread is really
> > asynchronous and the above bad pattern crash as expected.
> >
> > I didn't add any libX11 API call on the patches. And I don't think we can remvove the DRI stuff.
> >
> > Hum, I don't know what will be the impact on the perf but potentially we can force a synchronization
> > when there is a draw to framebuffer 0.
> 
> Can you send us the backtrace when DRI2GetBuffersWithFormat is called
> by glDrawArrays?
> 
> Marek

Hello Marek, Michel,

Here the full backtrace.

#0  DRI2GetBuffersWithFormat (dpy=0xc6307e48, drawable=104857784, width=0xdef348c0, height=0xdef348c4, attachments=0xdcc15c88, count=1, outCount=0xdcc15c74) at dri2.c:464
#1  0xf05bac45 in dri2GetBuffersWithFormat (driDrawable=0xdef348a8, width=0xdef348c0, height=0xdef348c4, attachments=0xdcc15c88, count=1, out_count=0xdcc15c74, loaderPrivate=0xdefc6ef0) at dri2_glx.c:894
#2  0xe3ec3111 in dri2_drawable_get_buffers (count=<synthetic pointer>, atts=0xdef252f8, drawable=0xdefc7b08) at dri2.c:285
#3  dri2_allocate_textures (ctx=0xdef15928, drawable=0xdefc7b08, statts=0xdef252f8, statts_count=2) at dri2.c:480
#4  0xe3ebcbb0 in dri_st_framebuffer_validate (stctx=0xdef4ab10, stfbi=0xdefc7b08, statts=0xdef252f8, count=2, out=0xdcc15d78) at dri_drawable.c:83
#5  0xe3d37afc in st_framebuffer_validate (stfb=stfb at entry=0xdef24f58, st=st at entry=0xdef4ab10) at state_tracker/st_manager.c:189
    Note "print stfb->Base->Name" give me 0
#6  0xe3d38649 in st_manager_validate_framebuffers (st=0xdef4ab10) at state_tracker/st_manager.c:869
#7  0xe3cd0580 in st_validate_state (st=0xdef4ab10, pipeline=ST_PIPELINE_RENDER) at state_tracker/st_atom.c:174
#8  0xe3cf935d in st_draw_vbo (ctx=0xdef7f8f8, prims=0xdcc15f40, nr_prims=1, ib=0x0, index_bounds_valid=1 '\001', min_index=39911, max_index=39914, tfb_vertcount=0x0, stream=0, indirect=0x0) at state_tracker/st_draw.c:191
#9  0xe3caf35e in vbo_draw_arrays (baseInstance=0, numInstances=1, count=4, start=39911, mode=5, ctx=0xdef7f8f8) at vbo/vbo_exec_array.c:427
#10 vbo_exec_DrawArrays (mode=5, start=39911, count=4) at vbo/vbo_exec_array.c:575
#11 0xe3be4f07 in _mesa_unmarshal_DrawArrays (ctx=0xdef7f8f8, cmd=0xc41574b0) at main/marshal_generated.c:26644
#12 _mesa_unmarshal_dispatch_cmd (ctx=0xdef7f8f8, cmd=0xc41574b0) at main/marshal_generated.c:42457
#13 0xe3ba8af0 in glthread_unmarshal_batch (release_batch=true, batch=0xc4157410, ctx=0xdef7f8f8) at main/glthread.c:64
#14 glthread_worker (data=0xdef7f8f8) at main/glthread.c:110


> If this DRI2GetBuffersWithFormat call results in libX11 API calls on the
> glthread, that's a bug which needs to be fixed, either by moving the
> DRI2GetBuffersWithFormat call to the main thread or (if possible) by
> changing DRI2GetBuffersWithFormat to use XCB directly.

Ok. On one hand, moving DRI2GetBuffersWithFormat to main thread won't be easy. I think we need to 
keep track of the current bound framebuffer on the app thread. So we can force a sync
on gl operation that will access the framebuffer 0.

On the other hand, GLX/XLIB/XCB interactions are  quite foggy for me. It seem there already some xcb codes
in various place typically EGL and the xcb_dri2 extension. So maybe there is a reason that code wasn't ported here.
In particular, I saw this option in configure
  --enable-glx[=dri|xlib|gallium-xlib] enable the GLX library and choose an implementation
              [default=auto]


> It sounds like the glthread bug above exists independently from this
> patch series, which might just make it more likely to result in a crash.
> I think it would still be better to fix the bug before landing this series.
Yes it increases the probability to trigger the bug.


Cheers,
Gregory