[Mesa-dev] [PATCH 03/22] i965/meta: Use explicit uniform locations in fast clear shader

[Iago Toral]

On Thu, 2016-02-18 at 10:34 -0800, Ian Romanick wrote:
> On 02/18/2016 07:01 AM, Iago Toral wrote:
> > On Wed, 2016-02-17 at 17:57 -0800, Ian Romanick wrote:
> >> From: Ian Romanick <ian.d.romanick at intel.com>
> >>
> >> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c | 14 ++++++++------
> >>  1 file changed, 8 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> >> index 37dffb9..3917df1 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> >> @@ -62,7 +62,6 @@ struct brw_fast_clear_state {
> >>     struct gl_vertex_array_object *array_obj;
> >>     GLuint vao;
> >>     GLuint shader_prog;
> >> -   GLint color_location;
> >>  };
> >>  
> >>  static bool
> >> @@ -118,7 +117,13 @@ brw_bind_rep_write_shader(struct brw_context *brw, float *color)
> >>        "   gl_Position = position;\n"
> >>        "}\n";
> >>     const char *fs_source =
> >> -      "uniform vec4 color;\n"
> >> +      /* GL_ARB_explicit_uniform_location requires either GLSL 3.30 or
> >> +       * GL_ARB_explicit_attrib_location, and only the later is universally
> >> +       * supported.
> >> +       */
> >> +      "#extension GL_ARB_explicit_attrib_location: require\n"
> >> +      "#extension GL_ARB_explicit_uniform_location: require\n"
> > 
> > We only use fast clears since gen6 so right now anything that goes
> > through this code  is going to meet this requirements. I wonder if it
> > would be worth it to assert on the supported GL version here, or at
> > least that the FS compile worked though.
> 
> We support both of these extensions on all hardware used by the i965
> driver.  In fact, every driver that supports GLSL enables these
> extensions.  i965, i915, and Gallium state tracker unconditionally
> enable these extensions.
> 
> We may want to add checks that the compile and link succeed, but I don't
> think that's necessarily related to this change.  What do you think?

If we support these on all hardware then it is really not an issue and
in that case I agree that adding checks for successful compile/link
isn't really related to this change.

> >> +      "layout(location=0) uniform vec4 color;\n"
> >>        "void main()\n"
> >>        "{\n"
> >>        "   gl_FragColor = color;\n"
> >> @@ -141,13 +146,10 @@ brw_bind_rep_write_shader(struct brw_context *brw, float *color)
> >>        _mesa_BindAttribLocation(clear->shader_prog, 0, "position");
> >>        _mesa_ObjectLabel(GL_PROGRAM, clear->shader_prog, -1, "meta repclear");
> >>        _mesa_LinkProgram(clear->shader_prog);
> >> -
> >> -      clear->color_location =
> >> -         _mesa_GetUniformLocation(clear->shader_prog, "color");
> >>     }
> >>  
> >>     _mesa_UseProgram(clear->shader_prog);
> >> -   _mesa_Uniform4fv(clear->color_location, 1, color);
> >> +   _mesa_Uniform4fv(0, 1, color);
> >>  }
> >>  
> >>  void
> > 
> > 
> 
>