[Mesa-dev] [PATCH] i965: Make brw_clear_cache NULL out stale program pointers.

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> The L3 partitioning code tries to look at all programs - both render
> programs (VS/TCS/TES/GS/FS) and compute (CS).
>
> After calling brw_clear_cache, all prog_data pointers are invalid and
> point to freed data.  The intention was that flagging the dirty bits for
> all programs would cause the next draw call to re-run the atoms for each
> program stage, uploading new programs and installing new, valid pointers.
>
> However, this doesn't quite work in our new multi-pipeline world.  When
> drawing or dispatching a compute workload, we only consider the programs
> for the appropriate pipeline: drawing sets up VS/TCS/TES/GS/FS, but not
> CS, and vice versa.  This leaves pointers dangling a bit longer than
> intended.
>
> The L3 configuration code tries to inspect the prog_data for all shader
> stages, so that we avoid having to reconfigure it when swapping back and
> forth between render and compute workloads.  So we can't have dangling
> pointers.
>
> The fix is simple: have brw_clear_cache NULL out stale prog_data
> pointers, making it safe to inspect.  The next L3 configuration pass
> will see either the render shaders or compute shader as missing for
> one go around, but will pick them up when both pipelines have run.
>
> In other words, we'll simply reconfigure L3 twice, which is safe,
> if a tiny bit wasteful - but then again, we just threw every compiled
> shader we had on the floor and started recompiling the from scratch,
> which is massively more wasteful, so it's not much of a concern.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93790
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> Cc: Francisco Jerez <currojerez at riseup.net>
> Cc: Jordan Justen <jljusten at gmail.com>
> ---
>  src/mesa/drivers/dri/i965/brw_state_cache.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_state_cache.c b/src/mesa/drivers/dri/i965/brw_state_cache.c
> index ce178aa..c6aa134 100644
> --- a/src/mesa/drivers/dri/i965/brw_state_cache.c
> +++ b/src/mesa/drivers/dri/i965/brw_state_cache.c
> @@ -393,6 +393,21 @@ brw_clear_cache(struct brw_context *brw, struct brw_cache *cache)
>     brw->state.pipelines[BRW_RENDER_PIPELINE].brw = ~0ull;
>     brw->state.pipelines[BRW_COMPUTE_PIPELINE].mesa = ~0;
>     brw->state.pipelines[BRW_COMPUTE_PIPELINE].brw = ~0ull;
> +
> +   /* Also, NULL out any stale program pointers. */
> +   brw->vs.prog_data = NULL;
> +   brw->vs.base.prog_data = NULL;
> +   brw->tcs.prog_data = NULL;
> +   brw->tcs.base.prog_data = NULL;
> +   brw->tes.prog_data = NULL;
> +   brw->tes.base.prog_data = NULL;
> +   brw->gs.prog_data = NULL;
> +   brw->gs.base.prog_data = NULL;
> +   brw->wm.prog_data = NULL;
> +   brw->wm.base.prog_data = NULL;
> +   brw->cs.prog_data = NULL;
> +   brw->cs.base.prog_data = NULL;
> +

Thanks, this seems far more convincing,

Reviewed-by: Francisco Jerez <currojerez at riseup.net>

>     intel_batchbuffer_flush(brw);
>  }
>  
> -- 
> 2.7.0
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160211/2f91f01d/attachment.sig>