[Mesa-dev,2/2] st/mesa: fix GLSL uniform updates for glBitmap & glDrawPixels

Submitted by Marek Olšák on Dec. 6, 2015, 11:34 p.m.

Details

Message ID 1449444858-13308-2-git-send-email-maraeo@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Marek Olšák Dec. 6, 2015, 11:34 p.m.
From: Marek Olšák <marek.olsak@amd.com>

Spotted by luck. The GLSL uniform storage is only associated once
in LinkShader and can't be reallocated afterwards, because that would
break the association.
---
 src/mesa/state_tracker/st_cb_bitmap.c      |  6 +++++-
 src/mesa/state_tracker/st_cb_drawpixels.c  |  6 ------
 src/mesa/state_tracker/st_glsl_to_tgsi.cpp |  6 ++++++
 src/mesa/state_tracker/st_program.c        | 17 ++++-------------
 src/mesa/state_tracker/st_program.h        |  1 -
 5 files changed, 15 insertions(+), 21 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/state_tracker/st_cb_bitmap.c b/src/mesa/state_tracker/st_cb_bitmap.c
index cbc6845..a4a48a6 100644
--- a/src/mesa/state_tracker/st_cb_bitmap.c
+++ b/src/mesa/state_tracker/st_cb_bitmap.c
@@ -287,7 +287,8 @@  draw_bitmap_quad(struct gl_context *ctx, GLint x, GLint y, GLfloat z,
       GLfloat colorSave[4];
       COPY_4V(colorSave, ctx->Current.Attrib[VERT_ATTRIB_COLOR0]);
       COPY_4V(ctx->Current.Attrib[VERT_ATTRIB_COLOR0], color);
-      st_upload_constants(st, fpv->parameters, PIPE_SHADER_FRAGMENT);
+      st_upload_constants(st, st->fp->Base.Base.Parameters,
+                          PIPE_SHADER_FRAGMENT);
       COPY_4V(ctx->Current.Attrib[VERT_ATTRIB_COLOR0], colorSave);
    }
 
@@ -404,6 +405,9 @@  draw_bitmap_quad(struct gl_context *ctx, GLint x, GLint y, GLfloat z,
    cso_restore_stream_outputs(cso);
 
    pipe_resource_reference(&vbuf, NULL);
+
+   /* We uploaded modified constants, need to invalidate them. */
+   st->dirty.mesa |= _NEW_PROGRAM_CONSTANTS;
 }
 
 
diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c b/src/mesa/state_tracker/st_cb_drawpixels.c
index 262ad80..e295f54 100644
--- a/src/mesa/state_tracker/st_cb_drawpixels.c
+++ b/src/mesa/state_tracker/st_cb_drawpixels.c
@@ -1109,9 +1109,6 @@  st_DrawPixels(struct gl_context *ctx, GLint x, GLint y,
                                      st->pixel_xfer.pixelmap_sampler_view);
          num_sampler_view++;
       }
-
-      /* update fragment program constants */
-      st_upload_constants(st, fpv->parameters, PIPE_SHADER_FRAGMENT);
    }
 
    /* Put glDrawPixels image into a texture */
@@ -1462,9 +1459,6 @@  st_CopyPixels(struct gl_context *ctx, GLint srcx, GLint srcy,
                                      st->pixel_xfer.pixelmap_sampler_view);
          num_sampler_view++;
       }
-
-      /* update fragment program constants */
-      st_upload_constants(st, fpv->parameters, PIPE_SHADER_FRAGMENT);
    }
    else {
       assert(type == GL_DEPTH);
diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
index 40c7725..a32c4cf 100644
--- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
@@ -5640,6 +5640,12 @@  get_mesa_program(struct gl_context *ctx,
 
    _mesa_reference_program(ctx, &shader->Program, prog);
 
+   /* Avoid reallocation of the program parameter list, because the uniform
+    * storage is only associated with the original parameter list.
+    * This should be enough for Bitmap and DrawPixels constants.
+    */
+   _mesa_reserve_parameter_storage(prog->Parameters, 8);
+
    /* This has to be done last.  Any operation the can cause
     * prog->ParameterValues to get reallocated (e.g., anything that adds a
     * program constant) has to happen before creating this linkage.
diff --git a/src/mesa/state_tracker/st_program.c b/src/mesa/state_tracker/st_program.c
index 75ccaf2..39c54c2 100644
--- a/src/mesa/state_tracker/st_program.c
+++ b/src/mesa/state_tracker/st_program.c
@@ -112,8 +112,6 @@  delete_fp_variant(struct st_context *st, struct st_fp_variant *fpv)
 {
    if (fpv->driver_shader) 
       cso_delete_fragment_shader(st->cso_context, fpv->driver_shader);
-   if (fpv->parameters)
-      _mesa_free_parameter_list(fpv->parameters);
    free(fpv);
 }
 
@@ -914,8 +912,6 @@  st_create_fp_variant(struct st_context *st,
          if (tgsi.tokens != stfp->tgsi.tokens)
             tgsi_free_tokens(tgsi.tokens);
          tgsi.tokens = tokens;
-         variant->parameters =
-            _mesa_clone_parameter_list(stfp->Base.Base.Parameters);
       } else
          fprintf(stderr, "mesa: cannot create a shader for glBitmap\n");
    }
@@ -924,6 +920,7 @@  st_create_fp_variant(struct st_context *st,
    if (key->drawpixels) {
       const struct tgsi_token *tokens;
       unsigned scale_const = 0, bias_const = 0, texcoord_const = 0;
+      struct gl_program_parameter_list *params = stfp->Base.Base.Parameters;
 
       /* Find the first unused slot. */
       variant->drawpix_sampler = ffs(~stfp->Base.Base.SamplersUsed) - 1;
@@ -935,27 +932,21 @@  st_create_fp_variant(struct st_context *st,
          variant->pixelmap_sampler = ffs(~samplers_used) - 1;
       }
 
-      variant->parameters =
-         _mesa_clone_parameter_list(stfp->Base.Base.Parameters);
-
       if (key->scaleAndBias) {
          static const gl_state_index scale_state[STATE_LENGTH] =
             { STATE_INTERNAL, STATE_PT_SCALE };
          static const gl_state_index bias_state[STATE_LENGTH] =
             { STATE_INTERNAL, STATE_PT_BIAS };
 
-         scale_const = _mesa_add_state_reference(variant->parameters,
-                                                 scale_state);
-         bias_const = _mesa_add_state_reference(variant->parameters,
-                                                bias_state);
+         scale_const = _mesa_add_state_reference(params, scale_state);
+         bias_const = _mesa_add_state_reference(params, bias_state);
       }
 
       {
          static const gl_state_index state[STATE_LENGTH] =
             { STATE_INTERNAL, STATE_CURRENT_ATTRIB, VERT_ATTRIB_TEX0 };
 
-         texcoord_const = _mesa_add_state_reference(variant->parameters,
-                                                    state);
+         texcoord_const = _mesa_add_state_reference(params, state);
       }
 
       tokens = st_get_drawpix_shader(tgsi.tokens,
diff --git a/src/mesa/state_tracker/st_program.h b/src/mesa/state_tracker/st_program.h
index d9b53ac..a8571f0 100644
--- a/src/mesa/state_tracker/st_program.h
+++ b/src/mesa/state_tracker/st_program.h
@@ -80,7 +80,6 @@  struct st_fp_variant
    void *driver_shader;
 
    /** For glBitmap variants */
-   struct gl_program_parameter_list *parameters;
    uint bitmap_sampler;
 
    /** For glDrawPixels variants */

Comments

On 12/06/2015 04:34 PM, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@amd.com>
>
> Spotted by luck. The GLSL uniform storage is only associated once
> in LinkShader and can't be reallocated afterwards, because that would
> break the association.
> ---
>   src/mesa/state_tracker/st_cb_bitmap.c      |  6 +++++-
>   src/mesa/state_tracker/st_cb_drawpixels.c  |  6 ------
>   src/mesa/state_tracker/st_glsl_to_tgsi.cpp |  6 ++++++
>   src/mesa/state_tracker/st_program.c        | 17 ++++-------------
>   src/mesa/state_tracker/st_program.h        |  1 -
>   5 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_cb_bitmap.c b/src/mesa/state_tracker/st_cb_bitmap.c
> index cbc6845..a4a48a6 100644
> --- a/src/mesa/state_tracker/st_cb_bitmap.c
> +++ b/src/mesa/state_tracker/st_cb_bitmap.c
> @@ -287,7 +287,8 @@ draw_bitmap_quad(struct gl_context *ctx, GLint x, GLint y, GLfloat z,
>         GLfloat colorSave[4];
>         COPY_4V(colorSave, ctx->Current.Attrib[VERT_ATTRIB_COLOR0]);
>         COPY_4V(ctx->Current.Attrib[VERT_ATTRIB_COLOR0], color);
> -      st_upload_constants(st, fpv->parameters, PIPE_SHADER_FRAGMENT);
> +      st_upload_constants(st, st->fp->Base.Base.Parameters,
> +                          PIPE_SHADER_FRAGMENT);
>         COPY_4V(ctx->Current.Attrib[VERT_ATTRIB_COLOR0], colorSave);
>      }
>
> @@ -404,6 +405,9 @@ draw_bitmap_quad(struct gl_context *ctx, GLint x, GLint y, GLfloat z,
>      cso_restore_stream_outputs(cso);
>
>      pipe_resource_reference(&vbuf, NULL);
> +
> +   /* We uploaded modified constants, need to invalidate them. */
> +   st->dirty.mesa |= _NEW_PROGRAM_CONSTANTS;
>   }
>
>
> diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c b/src/mesa/state_tracker/st_cb_drawpixels.c
> index 262ad80..e295f54 100644
> --- a/src/mesa/state_tracker/st_cb_drawpixels.c
> +++ b/src/mesa/state_tracker/st_cb_drawpixels.c
> @@ -1109,9 +1109,6 @@ st_DrawPixels(struct gl_context *ctx, GLint x, GLint y,
>                                        st->pixel_xfer.pixelmap_sampler_view);
>            num_sampler_view++;
>         }
> -
> -      /* update fragment program constants */
> -      st_upload_constants(st, fpv->parameters, PIPE_SHADER_FRAGMENT);
>      }
>
>      /* Put glDrawPixels image into a texture */
> @@ -1462,9 +1459,6 @@ st_CopyPixels(struct gl_context *ctx, GLint srcx, GLint srcy,
>                                        st->pixel_xfer.pixelmap_sampler_view);
>            num_sampler_view++;
>         }
> -
> -      /* update fragment program constants */
> -      st_upload_constants(st, fpv->parameters, PIPE_SHADER_FRAGMENT);
>      }
>      else {
>         assert(type == GL_DEPTH);
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index 40c7725..a32c4cf 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -5640,6 +5640,12 @@ get_mesa_program(struct gl_context *ctx,
>
>      _mesa_reference_program(ctx, &shader->Program, prog);
>
> +   /* Avoid reallocation of the program parameter list, because the uniform
> +    * storage is only associated with the original parameter list.
> +    * This should be enough for Bitmap and DrawPixels constants.
> +    */
> +   _mesa_reserve_parameter_storage(prog->Parameters, 8);
> +
>      /* This has to be done last.  Any operation the can cause
>       * prog->ParameterValues to get reallocated (e.g., anything that adds a
>       * program constant) has to happen before creating this linkage.
> diff --git a/src/mesa/state_tracker/st_program.c b/src/mesa/state_tracker/st_program.c
> index 75ccaf2..39c54c2 100644
> --- a/src/mesa/state_tracker/st_program.c
> +++ b/src/mesa/state_tracker/st_program.c
> @@ -112,8 +112,6 @@ delete_fp_variant(struct st_context *st, struct st_fp_variant *fpv)
>   {
>      if (fpv->driver_shader)
>         cso_delete_fragment_shader(st->cso_context, fpv->driver_shader);
> -   if (fpv->parameters)
> -      _mesa_free_parameter_list(fpv->parameters);
>      free(fpv);
>   }
>
> @@ -914,8 +912,6 @@ st_create_fp_variant(struct st_context *st,
>            if (tgsi.tokens != stfp->tgsi.tokens)
>               tgsi_free_tokens(tgsi.tokens);
>            tgsi.tokens = tokens;
> -         variant->parameters =
> -            _mesa_clone_parameter_list(stfp->Base.Base.Parameters);
>         } else
>            fprintf(stderr, "mesa: cannot create a shader for glBitmap\n");
>      }
> @@ -924,6 +920,7 @@ st_create_fp_variant(struct st_context *st,
>      if (key->drawpixels) {
>         const struct tgsi_token *tokens;
>         unsigned scale_const = 0, bias_const = 0, texcoord_const = 0;
> +      struct gl_program_parameter_list *params = stfp->Base.Base.Parameters;
>
>         /* Find the first unused slot. */
>         variant->drawpix_sampler = ffs(~stfp->Base.Base.SamplersUsed) - 1;
> @@ -935,27 +932,21 @@ st_create_fp_variant(struct st_context *st,
>            variant->pixelmap_sampler = ffs(~samplers_used) - 1;
>         }
>
> -      variant->parameters =
> -         _mesa_clone_parameter_list(stfp->Base.Base.Parameters);
> -
>         if (key->scaleAndBias) {
>            static const gl_state_index scale_state[STATE_LENGTH] =
>               { STATE_INTERNAL, STATE_PT_SCALE };
>            static const gl_state_index bias_state[STATE_LENGTH] =
>               { STATE_INTERNAL, STATE_PT_BIAS };
>
> -         scale_const = _mesa_add_state_reference(variant->parameters,
> -                                                 scale_state);
> -         bias_const = _mesa_add_state_reference(variant->parameters,
> -                                                bias_state);
> +         scale_const = _mesa_add_state_reference(params, scale_state);
> +         bias_const = _mesa_add_state_reference(params, bias_state);
>         }
>
>         {
>            static const gl_state_index state[STATE_LENGTH] =
>               { STATE_INTERNAL, STATE_CURRENT_ATTRIB, VERT_ATTRIB_TEX0 };
>
> -         texcoord_const = _mesa_add_state_reference(variant->parameters,
> -                                                    state);
> +         texcoord_const = _mesa_add_state_reference(params, state);
>         }
>
>         tokens = st_get_drawpix_shader(tgsi.tokens,
> diff --git a/src/mesa/state_tracker/st_program.h b/src/mesa/state_tracker/st_program.h
> index d9b53ac..a8571f0 100644
> --- a/src/mesa/state_tracker/st_program.h
> +++ b/src/mesa/state_tracker/st_program.h
> @@ -80,7 +80,6 @@ struct st_fp_variant
>      void *driver_shader;
>
>      /** For glBitmap variants */
> -   struct gl_program_parameter_list *parameters;
>      uint bitmap_sampler;
>
>      /** For glDrawPixels variants */
>

Reviewed-by: Brian Paul <brianp@vmware.com>
Should this be tagged for the 11.1 / stable branches?

-Brian
On Mon, Dec 7, 2015 at 5:36 PM, Brian Paul <brianp@vmware.com> wrote:
> On 12/06/2015 04:34 PM, Marek Olšák wrote:
>>
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> Spotted by luck. The GLSL uniform storage is only associated once
>> in LinkShader and can't be reallocated afterwards, because that would
>> break the association.
>> ---
>>   src/mesa/state_tracker/st_cb_bitmap.c      |  6 +++++-
>>   src/mesa/state_tracker/st_cb_drawpixels.c  |  6 ------
>>   src/mesa/state_tracker/st_glsl_to_tgsi.cpp |  6 ++++++
>>   src/mesa/state_tracker/st_program.c        | 17 ++++-------------
>>   src/mesa/state_tracker/st_program.h        |  1 -
>>   5 files changed, 15 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/mesa/state_tracker/st_cb_bitmap.c
>> b/src/mesa/state_tracker/st_cb_bitmap.c
>> index cbc6845..a4a48a6 100644
>> --- a/src/mesa/state_tracker/st_cb_bitmap.c
>> +++ b/src/mesa/state_tracker/st_cb_bitmap.c
>> @@ -287,7 +287,8 @@ draw_bitmap_quad(struct gl_context *ctx, GLint x,
>> GLint y, GLfloat z,
>>         GLfloat colorSave[4];
>>         COPY_4V(colorSave, ctx->Current.Attrib[VERT_ATTRIB_COLOR0]);
>>         COPY_4V(ctx->Current.Attrib[VERT_ATTRIB_COLOR0], color);
>> -      st_upload_constants(st, fpv->parameters, PIPE_SHADER_FRAGMENT);
>> +      st_upload_constants(st, st->fp->Base.Base.Parameters,
>> +                          PIPE_SHADER_FRAGMENT);
>>         COPY_4V(ctx->Current.Attrib[VERT_ATTRIB_COLOR0], colorSave);
>>      }
>>
>> @@ -404,6 +405,9 @@ draw_bitmap_quad(struct gl_context *ctx, GLint x,
>> GLint y, GLfloat z,
>>      cso_restore_stream_outputs(cso);
>>
>>      pipe_resource_reference(&vbuf, NULL);
>> +
>> +   /* We uploaded modified constants, need to invalidate them. */
>> +   st->dirty.mesa |= _NEW_PROGRAM_CONSTANTS;
>>   }
>>
>>
>> diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c
>> b/src/mesa/state_tracker/st_cb_drawpixels.c
>> index 262ad80..e295f54 100644
>> --- a/src/mesa/state_tracker/st_cb_drawpixels.c
>> +++ b/src/mesa/state_tracker/st_cb_drawpixels.c
>> @@ -1109,9 +1109,6 @@ st_DrawPixels(struct gl_context *ctx, GLint x, GLint
>> y,
>>
>> st->pixel_xfer.pixelmap_sampler_view);
>>            num_sampler_view++;
>>         }
>> -
>> -      /* update fragment program constants */
>> -      st_upload_constants(st, fpv->parameters, PIPE_SHADER_FRAGMENT);
>>      }
>>
>>      /* Put glDrawPixels image into a texture */
>> @@ -1462,9 +1459,6 @@ st_CopyPixels(struct gl_context *ctx, GLint srcx,
>> GLint srcy,
>>
>> st->pixel_xfer.pixelmap_sampler_view);
>>            num_sampler_view++;
>>         }
>> -
>> -      /* update fragment program constants */
>> -      st_upload_constants(st, fpv->parameters, PIPE_SHADER_FRAGMENT);
>>      }
>>      else {
>>         assert(type == GL_DEPTH);
>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> index 40c7725..a32c4cf 100644
>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> @@ -5640,6 +5640,12 @@ get_mesa_program(struct gl_context *ctx,
>>
>>      _mesa_reference_program(ctx, &shader->Program, prog);
>>
>> +   /* Avoid reallocation of the program parameter list, because the
>> uniform
>> +    * storage is only associated with the original parameter list.
>> +    * This should be enough for Bitmap and DrawPixels constants.
>> +    */
>> +   _mesa_reserve_parameter_storage(prog->Parameters, 8);
>> +
>>      /* This has to be done last.  Any operation the can cause
>>       * prog->ParameterValues to get reallocated (e.g., anything that adds
>> a
>>       * program constant) has to happen before creating this linkage.
>> diff --git a/src/mesa/state_tracker/st_program.c
>> b/src/mesa/state_tracker/st_program.c
>> index 75ccaf2..39c54c2 100644
>> --- a/src/mesa/state_tracker/st_program.c
>> +++ b/src/mesa/state_tracker/st_program.c
>> @@ -112,8 +112,6 @@ delete_fp_variant(struct st_context *st, struct
>> st_fp_variant *fpv)
>>   {
>>      if (fpv->driver_shader)
>>         cso_delete_fragment_shader(st->cso_context, fpv->driver_shader);
>> -   if (fpv->parameters)
>> -      _mesa_free_parameter_list(fpv->parameters);
>>      free(fpv);
>>   }
>>
>> @@ -914,8 +912,6 @@ st_create_fp_variant(struct st_context *st,
>>            if (tgsi.tokens != stfp->tgsi.tokens)
>>               tgsi_free_tokens(tgsi.tokens);
>>            tgsi.tokens = tokens;
>> -         variant->parameters =
>> -            _mesa_clone_parameter_list(stfp->Base.Base.Parameters);
>>         } else
>>            fprintf(stderr, "mesa: cannot create a shader for glBitmap\n");
>>      }
>> @@ -924,6 +920,7 @@ st_create_fp_variant(struct st_context *st,
>>      if (key->drawpixels) {
>>         const struct tgsi_token *tokens;
>>         unsigned scale_const = 0, bias_const = 0, texcoord_const = 0;
>> +      struct gl_program_parameter_list *params =
>> stfp->Base.Base.Parameters;
>>
>>         /* Find the first unused slot. */
>>         variant->drawpix_sampler = ffs(~stfp->Base.Base.SamplersUsed) - 1;
>> @@ -935,27 +932,21 @@ st_create_fp_variant(struct st_context *st,
>>            variant->pixelmap_sampler = ffs(~samplers_used) - 1;
>>         }
>>
>> -      variant->parameters =
>> -         _mesa_clone_parameter_list(stfp->Base.Base.Parameters);
>> -
>>         if (key->scaleAndBias) {
>>            static const gl_state_index scale_state[STATE_LENGTH] =
>>               { STATE_INTERNAL, STATE_PT_SCALE };
>>            static const gl_state_index bias_state[STATE_LENGTH] =
>>               { STATE_INTERNAL, STATE_PT_BIAS };
>>
>> -         scale_const = _mesa_add_state_reference(variant->parameters,
>> -                                                 scale_state);
>> -         bias_const = _mesa_add_state_reference(variant->parameters,
>> -                                                bias_state);
>> +         scale_const = _mesa_add_state_reference(params, scale_state);
>> +         bias_const = _mesa_add_state_reference(params, bias_state);
>>         }
>>
>>         {
>>            static const gl_state_index state[STATE_LENGTH] =
>>               { STATE_INTERNAL, STATE_CURRENT_ATTRIB, VERT_ATTRIB_TEX0 };
>>
>> -         texcoord_const = _mesa_add_state_reference(variant->parameters,
>> -                                                    state);
>> +         texcoord_const = _mesa_add_state_reference(params, state);
>>         }
>>
>>         tokens = st_get_drawpix_shader(tgsi.tokens,
>> diff --git a/src/mesa/state_tracker/st_program.h
>> b/src/mesa/state_tracker/st_program.h
>> index d9b53ac..a8571f0 100644
>> --- a/src/mesa/state_tracker/st_program.h
>> +++ b/src/mesa/state_tracker/st_program.h
>> @@ -80,7 +80,6 @@ struct st_fp_variant
>>      void *driver_shader;
>>
>>      /** For glBitmap variants */
>> -   struct gl_program_parameter_list *parameters;
>>      uint bitmap_sampler;
>>
>>      /** For glDrawPixels variants */
>>
>
> Reviewed-by: Brian Paul <brianp@vmware.com>
> Should this be tagged for the 11.1 / stable branches?

Yes, I'll tag them for stable.

Marek