[Mesa-dev] Mesa (master): draw: Only run prepare when state, prim and opt changes
Jose Fonseca
jfonseca at vmware.com
Fri Jan 27 01:39:44 PST 2012
Ah. Thanks for investigating this Stephane.
Garbagge collection happens infrequently, and it really needs to destroy everything.
We should just make sure there are no dangling pointers when it does.
I'll take a closer look.
Jose
----- Original Message -----
> Ok so I'm thinking of adding a refcount to the variant to know if
> they
> are bound, and not garbage collect them in that case. Let me know
> what
> you think.
>
> Stéphane
>
>
> 2012/1/26 Stéphane Marchesin <stephane.marchesin at gmail.com>:
> > So actually it's a case of a use-after free. The variant is freed
> > with
> > draw_llvm_destroy_variant and then reused through
> > llvm_pipeline_generic after free_gallium_state (and llvm) reused
> > the
> > memory for something else. What prevents a variant bound to an fpme
> > from being freed by the garbage collection?
> >
> > Stéphane
> >
> >
> > 2012/1/26 Stéphane Marchesin <stephane.marchesin at gmail.com>:
> >> I just took a look at it in gdb. Basically the jit_func pointer is
> >> corrupted by the free_gallivm_state function (in lp_bld_init.c).
> >> There
> >> is a comment to that effect already. It seems like the bug was
> >> always
> >> there but hidden because we regenerated state more than we had to.
> >> I'll keep digging...
> >>
> >> Stéphane
> >>
> >>
> >> 2012/1/26 Stéphane Marchesin <stephane.marchesin at gmail.com>:
> >>> Hmm, I'll take a look later today.
> >>>
> >>> Stéphane
> >>>
> >>> 2012/1/26 Jose Fonseca <jfonseca at vmware.com>:
> >>>> Stephane,
> >>>>
> >>>> This commit caused a segmentation fault on glean texSwizzle test
> >>>> + llvmpipe:
> >>>>
> >>>> $ gdb --args glean --run results --overwrite --quick --tests
> >>>> texSwizzle
> >>>> (gdb) r
> >>>> Starting program: glean --run results --overwrite --quick
> >>>> --tests texSwizzle
> >>>> [Thread debugging using libthread_db enabled]
> >>>>
> >>>> Program received signal SIGSEGV, Segmentation fault.
> >>>> 0xfffffffffffffffc in ?? ()
> >>>> (gdb) bt
> >>>> #0 0xfffffffffffffffc in ?? ()
> >>>> #1 0x00007ffff6a26438 in llvm_pipeline_generic
> >>>> (middle=0x76e4a0, fetch_info=0x7fffffffd730,
> >>>> prim_info=0x7fffffffd700)
> >>>> at
> >>>> src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c:240
> >>>> #2 0x00007ffff6a266fe in llvm_middle_end_linear_run
> >>>> (middle=0x76e4a0, start=0, count=4, prim_flags=0)
> >>>> at
> >>>> src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c:358
> >>>> #3 0x00007ffff697bf23 in vsplit_segment_simple_linear
> >>>> (vsplit=0x76b670, flags=0, istart=0, icount=4) at
> >>>> src/gallium/auxiliary/draw/draw_pt_vsplit_tmp.h:237
> >>>> #4 0x00007ffff697c228 in vsplit_run_linear (frontend=0x76b670,
> >>>> start=0, count=4) at
> >>>> src/gallium/auxiliary/draw/draw_split_tmp.h:61
> >>>> #5 0x00007ffff697224e in draw_pt_arrays (draw=0x762510, prim=6,
> >>>> start=0, count=4) at src/gallium/auxiliary/draw/draw_pt.c:142
> >>>> #6 0x00007ffff6972eb1 in draw_vbo (draw=0x762510,
> >>>> info=0x7fffffffd910) at
> >>>> src/gallium/auxiliary/draw/draw_pt.c:534
> >>>> #7 0x00007ffff6689f67 in llvmpipe_draw_vbo (pipe=0x72fa10,
> >>>> info=0x7fffffffd910) at
> >>>> src/gallium/drivers/llvmpipe/lp_draw_arrays.c:85
> >>>> #8 0x00007ffff68037f4 in st_draw_vbo (ctx=0x7c4b30,
> >>>> arrays=0x831c88, prims=0x7fffffffd9e0, nr_prims=1, ib=0x0,
> >>>> index_bounds_valid=1 '\001', min_index=0,
> >>>> max_index=3, tfb_vertcount=0x0) at
> >>>> src/mesa/state_tracker/st_draw.c:1113
> >>>> #9 0x00007ffff689d811 in vbo_draw_arrays (ctx=0x7c4b30, mode=6,
> >>>> start=0, count=4, numInstances=1) at
> >>>> src/mesa/vbo/vbo_exec_array.c:635
> >>>> #10 0x00007ffff689d950 in vbo_exec_DrawArrays (mode=6, start=0,
> >>>> count=4) at src/mesa/vbo/vbo_exec_array.c:667
> >>>> #11 0x0000000000458205 in GLEAN::TexSwizzleTest::TestSwizzles
> >>>> (this=0x6f10e0) at
> >>>> /var/lib/hudson/jobs/glean-ubuntu64/workspace/src/glean/ttexswizzle.cpp:293
> >>>> #12 0x0000000000458558 in GLEAN::TexSwizzleTest::runOne
> >>>> (this=0x6f10e0, r=..., w=<optimized out>)
> >>>> at
> >>>> /var/lib/hudson/jobs/glean-ubuntu64/workspace/src/glean/ttexswizzle.cpp:387
> >>>> #13 0x0000000000458ec7 in
> >>>> GLEAN::BaseTest<GLEAN::TexSwizzleResult>::run (this=0x6f10e0,
> >>>> environment=<optimized out>)
> >>>> at
> >>>> /var/lib/hudson/jobs/glean-ubuntu64/workspace/src/glean/tbase.h:317
> >>>> #14 0x00000000004610b8 in main (argc=7, argv=0x7fffffffdec8) at
> >>>> /var/lib/hudson/jobs/glean-ubuntu64/workspace/src/glean/main.cpp:140
> >>>> (gdb)
> >>>>
> >>>> Can you look into it?
> >>>>
> >>>> Jose
> >>>>
> >>>>
> >>>> ----- Original Message -----
> >>>>> Module: Mesa
> >>>>> Branch: master
> >>>>> Commit: b6d3a435a0e0e53a9e8cc4c4249dc7c2f897a83d
> >>>>> URL:
> >>>>> http://cgit.freedesktop.org/mesa/mesa/commit/?id=b6d3a435a0e0e53a9e8cc4c4249dc7c2f897a83d
> >>>>>
> >>>>> Author: Jakob Bornecrantz <wallbraker at gmail.com>
> >>>>> Date: Mon Jan 24 02:11:59 2011 +0100
> >>>>>
> >>>>> draw: Only run prepare when state, prim and opt changes
> >>>>>
> >>>>> In bad applications like ipers which does a lot of draw calls
> >>>>> with
> >>>>> no state changes this helps to greatly reduce time spent in
> >>>>> prepare.
> >>>>> In ipers around 7% of CPU was spent in various prepare
> >>>>> functions,
> >>>>> after this commit no prepare function show on the profile.
> >>>>>
> >>>>> This commit also has the added benefit of now grouping all
> >>>>> pipelined
> >>>>> drawing into a single draw call if the driver uses vbuf_render.
> >>>>>
> >>>>> Reviewed-by: Stéphane Marchesin <marcheu at chromium.org>
> >>>>> Tested-by: Stéphane Marchesin <marcheu at chromium.org>
> >>>>>
> >>>>> ---
> >>>>>
> >>>>> src/gallium/auxiliary/draw/draw_context.c | 6 +++
> >>>>> src/gallium/auxiliary/draw/draw_private.h | 8 ++++
> >>>>> src/gallium/auxiliary/draw/draw_pt.c | 49
> >>>>> ++++++++++++++++++++++++---
> >>>>> src/gallium/auxiliary/draw/draw_pt.h | 2 +-
> >>>>> src/gallium/auxiliary/draw/draw_pt_vsplit.c | 11 ++++--
> >>>>> 5 files changed, 66 insertions(+), 10 deletions(-)
> >>>>>
> >>>>> diff --git a/src/gallium/auxiliary/draw/draw_context.c
> >>>>> b/src/gallium/auxiliary/draw/draw_context.c
> >>>>> index 4ce4445..3c0b1aa 100644
> >>>>> --- a/src/gallium/auxiliary/draw/draw_context.c
> >>>>> +++ b/src/gallium/auxiliary/draw/draw_context.c
> >>>>> @@ -355,6 +355,10 @@ draw_set_vertex_elements(struct
> >>>>> draw_context
> >>>>> *draw,
> >>>>> {
> >>>>> assert(count <= PIPE_MAX_ATTRIBS);
> >>>>>
> >>>>> + /* We could improve this by only flushing the frontend and
> >>>>> the
> >>>>> fetch part
> >>>>> + * of the middle. This would avoid recalculating the emit
> >>>>> keys.*/
> >>>>> + draw_do_flush( draw, DRAW_FLUSH_STATE_CHANGE );
> >>>>> +
> >>>>> memcpy(draw->pt.vertex_element, elements, count *
> >>>>> sizeof(elements[0]));
> >>>>> draw->pt.nr_vertex_elements = count;
> >>>>> }
> >>>>> @@ -654,6 +658,8 @@ void draw_do_flush( struct draw_context
> >>>>> *draw,
> >>>>> unsigned flags )
> >>>>>
> >>>>> draw_pipeline_flush( draw, flags );
> >>>>>
> >>>>> + draw_pt_flush( draw, flags );
> >>>>> +
> >>>>> draw->flushing = FALSE;
> >>>>> }
> >>>>> }
> >>>>> diff --git a/src/gallium/auxiliary/draw/draw_private.h
> >>>>> b/src/gallium/auxiliary/draw/draw_private.h
> >>>>> index 1a0286d..c3eca97 100644
> >>>>> --- a/src/gallium/auxiliary/draw/draw_private.h
> >>>>> +++ b/src/gallium/auxiliary/draw/draw_private.h
> >>>>> @@ -63,6 +63,7 @@ struct draw_stage;
> >>>>> struct vbuf_render;
> >>>>> struct tgsi_exec_machine;
> >>>>> struct tgsi_sampler;
> >>>>> +struct draw_pt_front_end;
> >>>>>
> >>>>>
> >>>>> /**
> >>>>> @@ -137,6 +138,12 @@ struct draw_context
> >>>>> /* Support prototype passthrough path:
> >>>>> */
> >>>>> struct {
> >>>>> + /* Current active frontend */
> >>>>> + struct draw_pt_front_end *frontend;
> >>>>> + unsigned prim;
> >>>>> + unsigned opt;
> >>>>> + unsigned eltSize; /* saved eltSize for flushing */
> >>>>> +
> >>>>> struct {
> >>>>> struct draw_pt_middle_end *fetch_emit;
> >>>>> struct draw_pt_middle_end *fetch_shade_emit;
> >>>>> @@ -391,6 +398,7 @@ void
> >>>>> draw_remove_extra_vertex_attribs(struct
> >>>>> draw_context *draw);
> >>>>> boolean draw_pt_init( struct draw_context *draw );
> >>>>> void draw_pt_destroy( struct draw_context *draw );
> >>>>> void draw_pt_reset_vertex_ids( struct draw_context *draw );
> >>>>> +void draw_pt_flush( struct draw_context *draw, unsigned flags
> >>>>> );
> >>>>>
> >>>>>
> >>>>> /*******************************************************************************
> >>>>> diff --git a/src/gallium/auxiliary/draw/draw_pt.c
> >>>>> b/src/gallium/auxiliary/draw/draw_pt.c
> >>>>> index 9a017fd..025d539 100644
> >>>>> --- a/src/gallium/auxiliary/draw/draw_pt.c
> >>>>> +++ b/src/gallium/auxiliary/draw/draw_pt.c
> >>>>> @@ -52,7 +52,7 @@ DEBUG_GET_ONCE_BOOL_OPTION(draw_no_fse,
> >>>>> "DRAW_NO_FSE", FALSE)
> >>>>> * - backend -- the vbuf_render provided by the driver.
> >>>>> */
> >>>>> static boolean
> >>>>> -draw_pt_arrays(struct draw_context *draw,
> >>>>> +draw_pt_arrays(struct draw_context *draw,
> >>>>> unsigned prim,
> >>>>> unsigned start,
> >>>>> unsigned count)
> >>>>> @@ -106,17 +106,56 @@ draw_pt_arrays(struct draw_context *draw,
> >>>>> middle = draw->pt.middle.general;
> >>>>> }
> >>>>>
> >>>>> - frontend = draw->pt.front.vsplit;
> >>>>> + frontend = draw->pt.frontend;
> >>>>> +
> >>>>> + if (frontend ) {
> >>>>> + if (draw->pt.prim != prim || draw->pt.opt != opt) {
> >>>>> + /* In certain conditions switching primitives
> >>>>> requires us
> >>>>> to flush
> >>>>> + * and validate the different stages. One example is
> >>>>> when
> >>>>> smooth
> >>>>> + * lines are active but first drawn with triangles
> >>>>> and then
> >>>>> with
> >>>>> + * lines.
> >>>>> + */
> >>>>> + draw_do_flush( draw, DRAW_FLUSH_STATE_CHANGE );
> >>>>> + frontend = NULL;
> >>>>> + } else if (draw->pt.eltSize != draw->pt.user.eltSize) {
> >>>>> + /* Flush draw state if eltSize changed.
> >>>>> + * This could be improved so only the frontend is
> >>>>> flushed
> >>>>> since it
> >>>>> + * converts all indices to ushorts and the fetch part
> >>>>> of
> >>>>> the middle
> >>>>> + * always perpares both linear and indexed.
> >>>>> + */
> >>>>> + frontend->flush( frontend, DRAW_FLUSH_STATE_CHANGE );
> >>>>> + frontend = NULL;
> >>>>> + }
> >>>>> + }
> >>>>>
> >>>>> - frontend->prepare( frontend, prim, middle, opt );
> >>>>> + if (!frontend) {
> >>>>> + frontend = draw->pt.front.vsplit;
> >>>>>
> >>>>> - frontend->run(frontend, start, count);
> >>>>> + frontend->prepare( frontend, prim, middle, opt );
> >>>>>
> >>>>> - frontend->finish( frontend );
> >>>>> + draw->pt.frontend = frontend;
> >>>>> + draw->pt.eltSize = draw->pt.user.eltSize;
> >>>>> + draw->pt.prim = prim;
> >>>>> + draw->pt.opt = opt;
> >>>>> + }
> >>>>> +
> >>>>> + frontend->run( frontend, start, count );
> >>>>>
> >>>>> return TRUE;
> >>>>> }
> >>>>>
> >>>>> +void draw_pt_flush( struct draw_context *draw, unsigned flags
> >>>>> )
> >>>>> +{
> >>>>> + if (draw->pt.frontend) {
> >>>>> + draw->pt.frontend->flush( draw->pt.frontend, flags );
> >>>>> +
> >>>>> + /* don't prepare if we only are flushing the backend */
> >>>>> + if (!(flags & DRAW_FLUSH_BACKEND))
> >>>>> + draw->pt.frontend = NULL;
> >>>>> + }
> >>>>> +}
> >>>>> +
> >>>>> +
> >>>>>
> >>>>> boolean draw_pt_init( struct draw_context *draw )
> >>>>> {
> >>>>> diff --git a/src/gallium/auxiliary/draw/draw_pt.h
> >>>>> b/src/gallium/auxiliary/draw/draw_pt.h
> >>>>> index 9a45845..2c2efdc 100644
> >>>>> --- a/src/gallium/auxiliary/draw/draw_pt.h
> >>>>> +++ b/src/gallium/auxiliary/draw/draw_pt.h
> >>>>> @@ -73,7 +73,7 @@ struct draw_pt_front_end {
> >>>>> unsigned start,
> >>>>> unsigned count );
> >>>>>
> >>>>> - void (*finish)( struct draw_pt_front_end * );
> >>>>> + void (*flush)( struct draw_pt_front_end *, unsigned flags
> >>>>> );
> >>>>> void (*destroy)( struct draw_pt_front_end * );
> >>>>> };
> >>>>>
> >>>>> diff --git a/src/gallium/auxiliary/draw/draw_pt_vsplit.c
> >>>>> b/src/gallium/auxiliary/draw/draw_pt_vsplit.c
> >>>>> index c19dcd9..0fed057 100644
> >>>>> --- a/src/gallium/auxiliary/draw/draw_pt_vsplit.c
> >>>>> +++ b/src/gallium/auxiliary/draw/draw_pt_vsplit.c
> >>>>> @@ -178,11 +178,14 @@ static void vsplit_prepare(struct
> >>>>> draw_pt_front_end *frontend,
> >>>>> }
> >>>>>
> >>>>>
> >>>>> -static void vsplit_finish(struct draw_pt_front_end *frontend)
> >>>>> +static void vsplit_flush(struct draw_pt_front_end *frontend,
> >>>>> unsigned flags)
> >>>>> {
> >>>>> struct vsplit_frontend *vsplit = (struct vsplit_frontend *)
> >>>>> frontend;
> >>>>> - vsplit->middle->finish(vsplit->middle);
> >>>>> - vsplit->middle = NULL;
> >>>>> +
> >>>>> + if (!(flags & DRAW_FLUSH_BACKEND)) {
> >>>>> + vsplit->middle->finish(vsplit->middle);
> >>>>> + vsplit->middle = NULL;
> >>>>> + }
> >>>>> }
> >>>>>
> >>>>>
> >>>>> @@ -202,7 +205,7 @@ struct draw_pt_front_end
> >>>>> *draw_pt_vsplit(struct
> >>>>> draw_context *draw)
> >>>>>
> >>>>> vsplit->base.prepare = vsplit_prepare;
> >>>>> vsplit->base.run = NULL;
> >>>>> - vsplit->base.finish = vsplit_finish;
> >>>>> + vsplit->base.flush = vsplit_flush;
> >>>>> vsplit->base.destroy = vsplit_destroy;
> >>>>> vsplit->draw = draw;
> >>>>>
> >>>>>
> >>>>> _______________________________________________
> >>>>> mesa-commit mailing list
> >>>>> mesa-commit at lists.freedesktop.org
> >>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-commit
> >>>>>
>
More information about the mesa-dev
mailing list