Reviewed-by: Jose Fonseca <jfons...@vmware.com>
Series looks excellent Brian. Thanks for nailing this issue. I never stop to be
amazed on how state derivation bugs go unnoticed for so long, just because most
apps ends up touch a lot of state at the same time. One could try to write
piglit tests that draw something, change one bit of state, re-draw, but it
would take a lot of tests to cover everything.
Slight different issue -- you originally were looking into draw module and user
buffer pointers -- is draw module actually properly flushing whenever user
buffers are used? Or is that still outstanding?
Jose
----- Original Message -----
> This patch does two things:
>
> 1. Constant buffer state changes were broken (but happened to work by
> dumb luck). The problem is we weren't calling draw_do_flush() in
> draw_set_mapped_constant_buffer() when we changed that state. All
> the
> other draw_set_foo() functions were calling draw_do_flush()
> already.
>
> 2. Use a simpler state validation step when we're changing
> light-weight
> parameter state such as constant buffers, viewport dims or clip
> planes.
> There's no need to revalidate the whole pipeline when changing
> state
> like that. The new validation method is called bind_parameters()
> and is called instead of the prepare() method. A new
> DRAW_FLUSH_PARAMETER_CHANGE flag is used to signal these
> light-weight
> state changes. This results in a modest but measurable increase
> in
> FPS for many Mesa demos.
> ---
> src/gallium/auxiliary/draw/draw_context.c | 6 ++-
> src/gallium/auxiliary/draw/draw_pipe.c | 2 +-
> src/gallium/auxiliary/draw/draw_private.h | 7 ++-
> src/gallium/auxiliary/draw/draw_pt.c | 14 ++++++-
> src/gallium/auxiliary/draw/draw_pt.h | 7 +++
> .../auxiliary/draw/draw_pt_fetch_shade_pipeline.c | 10 +++++
> .../draw/draw_pt_fetch_shade_pipeline_llvm.c | 43
> ++++++++++++-------
> src/gallium/auxiliary/draw/draw_pt_vsplit.c | 2 +-
> 8 files changed, 68 insertions(+), 23 deletions(-)
>
> diff --git a/src/gallium/auxiliary/draw/draw_context.c
> b/src/gallium/auxiliary/draw/draw_context.c
> index c231aba..bc2f0e1 100644
> --- a/src/gallium/auxiliary/draw/draw_context.c
> +++ b/src/gallium/auxiliary/draw/draw_context.c
> @@ -289,7 +289,7 @@ void draw_set_rasterize_stage( struct
> draw_context *draw,
> void draw_set_clip_state( struct draw_context *draw,
> const struct pipe_clip_state *clip )
> {
> - draw_do_flush( draw, DRAW_FLUSH_STATE_CHANGE );
> + draw_do_flush(draw, DRAW_FLUSH_PARAMETER_CHANGE);
>
> memcpy(&draw->plane[6], clip->ucp, sizeof(clip->ucp));
> }
> @@ -301,7 +301,7 @@ void draw_set_clip_state( struct draw_context
> *draw,
> void draw_set_viewport_state( struct draw_context *draw,
> const struct pipe_viewport_state
> *viewport )
> {
> - draw_do_flush( draw, DRAW_FLUSH_STATE_CHANGE );
> + draw_do_flush(draw, DRAW_FLUSH_PARAMETER_CHANGE);
> draw->viewport = *viewport; /* struct copy */
> draw->identity_viewport = (viewport->scale[0] == 1.0f &&
> viewport->scale[1] == 1.0f &&
> @@ -368,6 +368,8 @@ draw_set_mapped_constant_buffer(struct
> draw_context *draw,
> shader_type == PIPE_SHADER_GEOMETRY);
> debug_assert(slot < PIPE_MAX_CONSTANT_BUFFERS);
>
> + draw_do_flush(draw, DRAW_FLUSH_PARAMETER_CHANGE);
> +
> switch (shader_type) {
> case PIPE_SHADER_VERTEX:
> draw->pt.user.vs_constants[slot] = buffer;
> diff --git a/src/gallium/auxiliary/draw/draw_pipe.c
> b/src/gallium/auxiliary/draw/draw_pipe.c
> index ac449b7..f1ee6cb 100644
> --- a/src/gallium/auxiliary/draw/draw_pipe.c
> +++ b/src/gallium/auxiliary/draw/draw_pipe.c
> @@ -347,6 +347,6 @@ void draw_pipeline_flush( struct draw_context
> *draw,
> unsigned flags )
> {
> draw->pipeline.first->flush( draw->pipeline.first, flags );
> - if (!(flags & DRAW_FLUSH_BACKEND))
> + if (flags & DRAW_FLUSH_STATE_CHANGE)
> draw->pipeline.first = draw->pipeline.validate;
> }
> diff --git a/src/gallium/auxiliary/draw/draw_private.h
> b/src/gallium/auxiliary/draw/draw_private.h
> index e52b3fd..2223fcb 100644
> --- a/src/gallium/auxiliary/draw/draw_private.h
> +++ b/src/gallium/auxiliary/draw/draw_private.h
> @@ -144,6 +144,8 @@ struct draw_context
> unsigned opt; /**< bitmask of PT_x flags */
> unsigned eltSize; /* saved eltSize for flushing */
>
> + boolean rebind_parameters;
> +
> struct {
> struct draw_pt_middle_end *fetch_emit;
> struct draw_pt_middle_end *fetch_shade_emit;
> @@ -434,8 +436,9 @@ void draw_pipeline_flush( struct draw_context
> *draw,
> * Flushing
> */
>
> -#define DRAW_FLUSH_STATE_CHANGE 0x8
> -#define DRAW_FLUSH_BACKEND 0x10
> +#define DRAW_FLUSH_PARAMETER_CHANGE 0x1 /**< Constants, viewport,
> etc */
> +#define DRAW_FLUSH_STATE_CHANGE 0x2 /**< Other/heavy state
> changes */
> +#define DRAW_FLUSH_BACKEND 0x4 /**< Flush the output
> buffer */
>
>
> void draw_do_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 7113b9e..ddaedcd 100644
> --- a/src/gallium/auxiliary/draw/draw_pt.c
> +++ b/src/gallium/auxiliary/draw/draw_pt.c
> @@ -139,6 +139,12 @@ draw_pt_arrays(struct draw_context *draw,
> draw->pt.opt = opt;
> }
>
> + if (draw->pt.rebind_parameters) {
> + /* update constants, viewport dims, clip planes, etc */
> + middle->bind_parameters(middle);
> + draw->pt.rebind_parameters = FALSE;
> + }
> +
> frontend->run( frontend, start, count );
>
> return TRUE;
> @@ -146,13 +152,19 @@ draw_pt_arrays(struct draw_context *draw,
>
> void draw_pt_flush( struct draw_context *draw, unsigned flags )
> {
> + assert(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))
> + if (flags & DRAW_FLUSH_STATE_CHANGE)
> draw->pt.frontend = NULL;
> }
> +
> + if (flags & DRAW_FLUSH_PARAMETER_CHANGE) {
> + draw->pt.rebind_parameters = TRUE;
> + }
> }
>
>
> diff --git a/src/gallium/auxiliary/draw/draw_pt.h
> b/src/gallium/auxiliary/draw/draw_pt.h
> index 2c2efdc..7d07363 100644
> --- a/src/gallium/auxiliary/draw/draw_pt.h
> +++ b/src/gallium/auxiliary/draw/draw_pt.h
> @@ -93,6 +93,13 @@ struct draw_pt_middle_end {
> unsigned opt,
> unsigned *max_vertices );
>
> + /**
> + * Bind/update parameter state such as constants, viewport dims
> + * and clip planes. Basically, stuff which isn't "baked" into
> the
> + * shader or pipeline state.
> + */
> + void (*bind_parameters)(struct draw_pt_middle_end *);
> +
> void (*run)( struct draw_pt_middle_end *,
> const unsigned *fetch_elts,
> unsigned fetch_count,
> diff --git
> a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline.c
> b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline.c
> index a6f5484..d1b76b1 100644
> --- a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline.c
> +++ b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline.c
> @@ -137,6 +137,15 @@ static void fetch_pipeline_prepare( struct
> draw_pt_middle_end *middle,
> }
>
>
> +static void
> +fetch_pipeline_bind_parameters(struct draw_pt_middle_end *middle)
> +{
> + /* No-op since the vertex shader executor and drawing pipeline
> + * just grab the constants, viewport, etc. from the draw context
> state.
> + */
> +}
> +
> +
> static void fetch( struct pt_fetch *fetch,
> const struct draw_fetch_info *fetch_info,
> char *output)
> @@ -421,6 +430,7 @@ struct draw_pt_middle_end
> *draw_pt_fetch_pipeline_or_emit( struct draw_context *
> goto fail;
>
> fpme->base.prepare = fetch_pipeline_prepare;
> + fpme->base.bind_parameters = fetch_pipeline_bind_parameters;
> fpme->base.run = fetch_pipeline_run;
> fpme->base.run_linear = fetch_pipeline_linear_run;
> fpme->base.run_linear_elts = fetch_pipeline_linear_run_elts;
> diff --git
> a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
> b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
> index 2230a7e..bfad54b 100644
> --- a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
> +++ b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
> @@ -180,24 +180,34 @@ llvm_middle_end_prepare( struct
> draw_pt_middle_end *middle,
>
> fpme->current_variant = variant;
> }
> +}
>
> - /* Bind the VS and GS input constants, clip planes and viewport
> */
> - {
> - unsigned i;
>
> - for (i = 0; i <
> Elements(fpme->llvm->jit_context.vs_constants); ++i) {
> - fpme->llvm->jit_context.vs_constants[i] =
> - draw->pt.user.vs_constants[i];
> - }
> - for (i = 0; i <
> Elements(fpme->llvm->jit_context.gs_constants); ++i) {
> - fpme->llvm->jit_context.gs_constants[i] =
> - draw->pt.user.gs_constants[i];
> - }
> - fpme->llvm->jit_context.planes =
> - (float (*) [DRAW_TOTAL_CLIP_PLANES][4])
> draw->pt.user.planes[0];
> - fpme->llvm->jit_context.viewport =
> - (float *) draw->viewport.scale;
> - }
> +/**
> + * Bind/update constant buffer pointers, clip planes and viewport
> dims.
> + * These are "light weight" parameters which aren't baked into the
> + * generated code. Updating these items is much cheaper than
> revalidating
> + * and rebuilding the generated pipeline code.
> + */
> +static void
> +llvm_middle_end_bind_parameters(struct draw_pt_middle_end *middle)
> +{
> + struct llvm_middle_end *fpme = (struct llvm_middle_end *)middle;
> + struct draw_context *draw = fpme->draw;
> + unsigned i;
> +
> + for (i = 0; i < Elements(fpme->llvm->jit_context.vs_constants);
> ++i) {
> + fpme->llvm->jit_context.vs_constants[i] =
> draw->pt.user.vs_constants[i];
> + }
> +
> + for (i = 0; i < Elements(fpme->llvm->jit_context.gs_constants);
> ++i) {
> + fpme->llvm->jit_context.gs_constants[i] =
> draw->pt.user.gs_constants[i];
> + }
> +
> + fpme->llvm->jit_context.planes =
> + (float (*)[DRAW_TOTAL_CLIP_PLANES][4])
> draw->pt.user.planes[0];
> +
> + fpme->llvm->jit_context.viewport = (float *)
> draw->viewport.scale;
> }
>
>
> @@ -448,6 +458,7 @@ draw_pt_fetch_pipeline_or_emit_llvm(struct
> draw_context *draw)
> goto fail;
>
> fpme->base.prepare = llvm_middle_end_prepare;
> + fpme->base.bind_parameters = llvm_middle_end_bind_parameters;
> fpme->base.run = llvm_middle_end_run;
> fpme->base.run_linear = llvm_middle_end_linear_run;
> fpme->base.run_linear_elts = llvm_middle_end_linear_run_elts;
> diff --git a/src/gallium/auxiliary/draw/draw_pt_vsplit.c
> b/src/gallium/auxiliary/draw/draw_pt_vsplit.c
> index 0fed057..c2d2a8f 100644
> --- a/src/gallium/auxiliary/draw/draw_pt_vsplit.c
> +++ b/src/gallium/auxiliary/draw/draw_pt_vsplit.c
> @@ -182,7 +182,7 @@ static void vsplit_flush(struct draw_pt_front_end
> *frontend, unsigned flags)
> {
> struct vsplit_frontend *vsplit = (struct vsplit_frontend *)
> frontend;
>
> - if (!(flags & DRAW_FLUSH_BACKEND)) {
> + if (flags & DRAW_FLUSH_STATE_CHANGE) {
> vsplit->middle->finish(vsplit->middle);
> vsplit->middle = NULL;
> }
> --
> 1.7.3.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
