[Mesa-dev] [PATCH 52/61] radeonsi/gfx9: 2nd shader of merged shaders should hold a reference of the 1st

[Nicolai Hähnle]

On 24.04.2017 10:45, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
>
> ---
>  src/gallium/drivers/radeonsi/si_shader.h        |  1 +
>  src/gallium/drivers/radeonsi/si_state_shaders.c | 35 ++++++++++++++++++-------
>  2 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/src/gallium/drivers/radeonsi/si_shader.h b/src/gallium/drivers/radeonsi/si_shader.h
> index 514bb3c..5b665b5 100644
> --- a/src/gallium/drivers/radeonsi/si_shader.h
> +++ b/src/gallium/drivers/radeonsi/si_shader.h
> @@ -498,20 +498,21 @@ struct si_shader_info {
>  	char			face_vgpr_index;
>  	bool			uses_instanceid;
>  	ubyte			nr_pos_exports;
>  	ubyte			nr_param_exports;
>  };
>
>  struct si_shader {
>  	struct si_compiler_ctx_state	compiler_ctx_state;
>
>  	struct si_shader_selector	*selector;
> +	struct si_shader_selector	*previous_stage_sel; /* for refcounting */
>  	struct si_shader		*next_variant;
>
>  	struct si_shader_part		*prolog;
>  	struct si_shader		*previous_stage; /* for GFX9 */
>  	struct si_shader_part		*prolog2;
>  	struct si_shader_part		*epilog;
>
>  	struct si_pm4_state		*pm4;
>  	struct r600_resource		*bo;
>  	struct r600_resource		*scratch_bo;
> diff --git a/src/gallium/drivers/radeonsi/si_state_shaders.c b/src/gallium/drivers/radeonsi/si_state_shaders.c
> index c5fa01d..94fc0e4 100644
> --- a/src/gallium/drivers/radeonsi/si_state_shaders.c
> +++ b/src/gallium/drivers/radeonsi/si_state_shaders.c
> @@ -1517,20 +1517,21 @@ static void si_shader_selector_reference(struct si_context *sctx,
>  }
>
>  /* Select the hw shader variant depending on the current state. */
>  static int si_shader_select_with_key(struct si_screen *sscreen,
>  				     struct si_shader_ctx_state *state,
>  				     struct si_compiler_ctx_state *compiler_state,
>  				     struct si_shader_key *key,
>  				     int thread_index)
>  {
>  	struct si_shader_selector *sel = state->cso;
> +	struct si_shader_selector *previous_stage_sel = NULL;
>  	struct si_shader *current = state->current;
>  	struct si_shader *iter, *shader = NULL;
>
>  	if (unlikely(sscreen->b.debug_flags & DBG_NO_OPT_VARIANT)) {
>  		memset(&key->opt, 0, sizeof(key->opt));
>  	}
>
>  again:
>  	/* Check if we don't need to change anything.
>  	 * This path is also used for most shaders that don't need multiple
> @@ -1584,64 +1585,77 @@ again:
>  	/* Build a new shader. */
>  	shader = CALLOC_STRUCT(si_shader);
>  	if (!shader) {
>  		mtx_unlock(&sel->mutex);
>  		return -ENOMEM;
>  	}
>  	shader->selector = sel;
>  	shader->key = *key;
>  	shader->compiler_ctx_state = *compiler_state;
>
> +	/* If this is a merged shader, get the first shader's selector. */
> +	if (sscreen->b.chip_class >= GFX9) {
> +		if (sel->type == PIPE_SHADER_TESS_CTRL)
> +			previous_stage_sel  = key->part.tcs.ls;
> +		else if (sel->type == PIPE_SHADER_GEOMETRY)
> +			previous_stage_sel  = key->part.gs.es;

Extra space around the '='.

Cheers,
Nicolai


> +	}
> +
>  	/* Compile the main shader part if it doesn't exist. This can happen
>  	 * if the initial guess was wrong. */
>  	bool is_pure_monolithic =
>  		sscreen->use_monolithic_shaders ||
>  		memcmp(&key->mono, &zeroed.mono, sizeof(key->mono)) != 0;
>
>  	if (!is_pure_monolithic) {
>  		bool ok;
>
>  		/* Make sure the main shader part is present. This is needed
>  		 * for shaders that can be compiled as VS, LS, or ES, and only
>  		 * one of them is compiled at creation.
>  		 *
>  		 * For merged shaders, check that the starting shader's main
>  		 * part is present.
>  		 */
> -		if (sscreen->b.chip_class >= GFX9 &&
> -		    (sel->type == PIPE_SHADER_TESS_CTRL ||
> -		     sel->type == PIPE_SHADER_GEOMETRY)) {
> -			struct si_shader_selector *shader1 = NULL;
> +		if (previous_stage_sel) {
>  			struct si_shader_key shader1_key = zeroed;
>
> -			if (sel->type == PIPE_SHADER_TESS_CTRL) {
> -				shader1 = key->part.tcs.ls;
> +			if (sel->type == PIPE_SHADER_TESS_CTRL)
>  				shader1_key.as_ls = 1;
> -			} else if (sel->type == PIPE_SHADER_GEOMETRY) {
> -				shader1 = key->part.gs.es;
> +			else if (sel->type == PIPE_SHADER_GEOMETRY)
>  				shader1_key.as_es = 1;
> -			} else
> +			else
>  				assert(0);
>
> -			ok = si_check_missing_main_part(sscreen, shader1,
> +			ok = si_check_missing_main_part(sscreen,
> +							previous_stage_sel,
>  							compiler_state, &shader1_key);
>  		} else {
>  			ok = si_check_missing_main_part(sscreen, sel,
>  							compiler_state, key);
>  		}
>  		if (!ok) {
>  			FREE(shader);
>  			mtx_unlock(&sel->mutex);
>  			return -ENOMEM; /* skip the draw call */
>  		}
>  	}
>
> +	/* Keep the reference to the 1st shader of merged shaders, so that
> +	 * Gallium can't destroy it before we destroy the 2nd shader.
> +	 *
> +	 * Set sctx = NULL, because it's unused if we're not releasing
> +	 * the shader, and we don't have any sctx here.
> +	 */
> +	si_shader_selector_reference(NULL, &shader->previous_stage_sel,
> +				     previous_stage_sel);
> +
>  	/* Monolithic-only shaders don't make a distinction between optimized
>  	 * and unoptimized. */
>  	shader->is_monolithic =
>  		is_pure_monolithic ||
>  		memcmp(&key->opt, &zeroed.opt, sizeof(key->opt)) != 0;
>
>  	shader->is_optimized =
>  		!is_pure_monolithic &&
>  		memcmp(&key->opt, &zeroed.opt, sizeof(key->opt)) != 0;
>  	if (shader->is_optimized)
> @@ -2237,20 +2251,21 @@ static void si_delete_shader(struct si_context *sctx, struct si_shader *shader)
>  				si_pm4_delete_state(sctx, vs, shader->pm4);
>  			else
>  				si_pm4_delete_state(sctx, gs, shader->pm4);
>  			break;
>  		case PIPE_SHADER_FRAGMENT:
>  			si_pm4_delete_state(sctx, ps, shader->pm4);
>  			break;
>  		}
>  	}
>
> +	si_shader_selector_reference(sctx, &shader->previous_stage_sel, NULL);
>  	si_shader_destroy(shader);
>  	free(shader);
>  }
>
>  static void si_destroy_shader_selector(struct si_context *sctx,
>  				       struct si_shader_selector *sel)
>  {
>  	struct si_shader *p = sel->first_variant, *c;
>  	struct si_shader_ctx_state *current_shader[SI_NUM_SHADERS] = {
>  		[PIPE_SHADER_VERTEX] = &sctx->vs_shader,
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.