[Mesa-dev] [PATCH 3/4] i965: Make L3 partitioning code only use the current pipeline's shaders.

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> When uploading state for the compute pipeline, we don't want to
> look at VS/TCS/TES/GS/FS programs, as they might be stale, and
> aren't relevant anyway.  Likewise, the render pipeline shouldn't
> look at CS.

The intended behaviour of this function is to look at the whole pipeline
state.  The reason is that reprogramming the L3 is expensive and
selecting an L3 configuration that works for the whole pipeline avoids
an unnecessary L3 reprogramming on pipeline select if the bound shader
programs didn't change (or changed to a different program with the
similar L3 requirements).

Regardless of its performance implications, it doesn't seem correct to
do this unless you can guarantee that the pipeline state is evaluated
again after pipeline select, which I don't think is the case right now.

Anyway it seems like this is hiding a bug elsewhere.  It smells rather
fishy that the brw_context structure can suddenly lose its consistency
anytime.  brw_state_cache_check_size() frees the stage prog data
structures pointed to by the context and then leaves dangling pointers
pointing at the deallocated storage -- that seems like the real bug to
me.  You need to set the prog data pointers to NULL when they become
invalid.  If you do that get_pipeline_state_l3_weights() won't look at
them until the program switches back to the pipeline they belong to, at
which point the invalidated prog data structures will be regenerated and
BRW_NEW_XS_PROG_DATA will be signaled causing the L3 configuration to be
reevaluated.

>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93790
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>
> ---
>  src/mesa/drivers/dri/i965/gen7_l3_state.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/gen7_l3_state.c b/src/mesa/drivers/dri/i965/gen7_l3_state.c
> index c4babc2..b2e9306 100644
> --- a/src/mesa/drivers/dri/i965/gen7_l3_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_l3_state.c
> @@ -307,7 +307,12 @@ get_pipeline_state_l3_weights(const struct brw_context *brw)
>     };
>     bool needs_dc = false, needs_slm = false;
>  
> -   for (unsigned i = 0; i < ARRAY_SIZE(stage_states); i++) {
> +   unsigned first_stage = MESA_SHADER_VERTEX;
> +   unsigned last_stage = MESA_SHADER_FRAGMENT;
> +   if (brw->last_pipeline == BRW_COMPUTE_PIPELINE)
> +      first_stage = last_stage = MESA_SHADER_COMPUTE;
> +
> +   for (unsigned i = first_stage; i <= last_stage; i++) {
>        const struct gl_shader_program *prog =
>           brw->ctx._Shader->CurrentProgram[stage_states[i]->stage];
>        const struct brw_stage_prog_data *prog_data = stage_states[i]->prog_data;
> -- 
> 2.7.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- 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/20160210/d91b36d4/attachment.sig>