Re: [Mesa-dev] [PATCH 07/25] mesa/i965: eliminate gl_tess_ctrl_program and use new shared shader_info

2016-10-18 Thread Kenneth Graunke 
On Tuesday, October 18, 2016 8:28:02 AM PDT Jason Ekstrand wrote:
> On Tue, Oct 18, 2016 at 8:14 AM, Jason Ekstrand 
> wrote:
> 
> > I want to make a few comments on how this series is structured.  This is
> > not the way I would have done it and I think the way you structured it
> > makes it substantially less rebasable than it could be and a bit harder to
> > review.  The way *I* would have done this would be something like the
> > following:
> >
> >  1) Move shader_info to common code (patches 1-2)
> >  2) Add a shader_info pointer to gl_program (patch 6), break the fill
> > shader_info stuff from glsl_to_nir into its own function, and call it from
> > somewhere such that it always gets filled out.
> >  3) Add new fields to shader_info *and* make sure they get filled out from
> > other GLSL information
> >  4) Convert i965 over to the new shader_info
> >  5) Convert gallium over to the new shader_info
> >  6) Make GLSL fill out shader_info directly and nuke the old shader
> > metadata.
> >  7) Delete the shader_info fill-out function.
> >
> > Something along these lines would go a long way towards avoiding the "mega
> > patch" problem where each patch touches 4 or 5 different components.  It
> > also makes it clearer to review because you don't add fields and then the
> > reviewer goes "Wait, where does this get set?  Oh, in another patch".  I'm
> > not necessarily saying that you have to go back and change your patches.
> > It's more a suggestion for if you end up doing a v3 or another refactor
> > along these lines in the future.
> >
> 
> On the review side, splitting out as I described above would make it much
> easier to review since it would be more-or-less one type of refactor per
> patch.  In this patch, we have several different kinds of refactors:
> 
>  1) Move consumers over to reading shader_info
>  2) Remove gl_tess_ctrl_program and related refactors
>  3) Move producer over to writing shader_info
> 
> Normally, when reviewing, I would just skim (2) and give (1) a (3) more
> effort.  Having them mixed together means I have to pay constant attention
> to what's going on.  Also, having (2) mixed in makes it harder to verify
> (3) because there's a lot of code motion only some of which matters.

I agree with Jason.  This could be structured a lot more cleanly, and it
would make it much easier to review.

For example, patches 3-5 add a bunch of new structure fields.  But they
aren't populated by anything.  The CS local_size_variable field finally
gets populated in patch 12 (a whole 7 patches later!)...and by the end
of the series...I don't see a single consumer of that field.

So, the field is useless.  But I had to use 'git log -p' on a branch and
search through your entire series to determine that.  There's far too
much context to keep in my head while reading, and it means I have to
abandon my usual read-emails-mostly-in-order review process.

I actually added TES shader info a little while back (but hadn't sent
them out yet as Vulkan tessellation isn't quite ready yet).  Here's what
my patches looked like:

1) Convert spacing from GLenums to a TESS_SPACING_* enum
https://cgit.freedesktop.org/~kwg/mesa/commit/?h=vktess=8b49a8485dd37eb405efcaaecd55244a8f63f213
   (simple cleanup I did across the whole codebase)

2) Introduce nir_shader_info fields and populate them in glsl_to_nir and
   spirv_to_nir.
https://cgit.freedesktop.org/~kwg/mesa/commit/?h=vktess=3142efa913965324ad21c3cefc792ab83e1a1390
   (fields are at least populated in all frontends, but may be useless)

3) Convert i965 over to use nir_shader_info for fields
https://cgit.freedesktop.org/~kwg/mesa/commit/?h=vktess=a518388acc7a6db88c7e21829e7a15b15b9304ad
   (now the fields are used.  admittedly I did some bonus code motion in
this patch...if I'm being pedantic, I should have made that a fourth
patch to make the prog_data fields be populated in Vulkan paths)

The first patch stands alone, and patches 2-3 stand together.  All are
very small.  You need no additional context to answer questions, and can
say "those look good" and move on rather quickly.

With that in mind, I'd like to ask you to please try and rework this
series along the lines that Jason suggested.  I know it's a bunch of
work, but being disciplined in how we organize our code is a really
useful skill that pays off in the long run.  When reviewers can look
at your code and quickly give a thumbs up, you get to land your patches
a lot more quickly, and that extra effort ultimately saves you (and
others) a whole lot of time.

Sorry, Tim :(

--Ken


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 07/25] mesa/i965: eliminate gl_tess_ctrl_program and use new shared shader_info

2016-10-18 Thread Jason Ekstrand 
On Tue, Oct 18, 2016 at 8:28 AM, Jason Ekstrand 
wrote:

> On Tue, Oct 18, 2016 at 8:14 AM, Jason Ekstrand 
> wrote:
>
>> I want to make a few comments on how this series is structured.  This is
>> not the way I would have done it and I think the way you structured it
>> makes it substantially less rebasable than it could be and a bit harder to
>> review.  The way *I* would have done this would be something like the
>> following:
>>
>>  1) Move shader_info to common code (patches 1-2)
>>  2) Add a shader_info pointer to gl_program (patch 6), break the fill
>> shader_info stuff from glsl_to_nir into its own function, and call it from
>> somewhere such that it always gets filled out.
>>  3) Add new fields to shader_info *and* make sure they get filled out
>> from other GLSL information
>>  4) Convert i965 over to the new shader_info
>>  5) Convert gallium over to the new shader_info
>>  6) Make GLSL fill out shader_info directly and nuke the old shader
>> metadata.
>>  7) Delete the shader_info fill-out function.
>>
>
Oh, and one more step:

 8) Refactor to get rid of all of the gl_foo_program stuff.  (Maybe
multiple patches?)


>
>> Something along these lines would go a long way towards avoiding the
>> "mega patch" problem where each patch touches 4 or 5 different components.
>> It also makes it clearer to review because you don't add fields and then
>> the reviewer goes "Wait, where does this get set?  Oh, in another patch".
>> I'm not necessarily saying that you have to go back and change your
>> patches.  It's more a suggestion for if you end up doing a v3 or another
>> refactor along these lines in the future.
>>
>
> On the review side, splitting out as I described above would make it much
> easier to review since it would be more-or-less one type of refactor per
> patch.  In this patch, we have several different kinds of refactors:
>
>  1) Move consumers over to reading shader_info
>  2) Remove gl_tess_ctrl_program and related refactors
>  3) Move producer over to writing shader_info
>
> Normally, when reviewing, I would just skim (2) and give (1) a (3) more
> effort.  Having them mixed together means I have to pay constant attention
> to what's going on.  Also, having (2) mixed in makes it harder to verify
> (3) because there's a lot of code motion only some of which matters.
>
>
>>
>>
>> On Mon, Oct 17, 2016 at 11:12 PM, Timothy Arceri <
>> timothy.arc...@collabora.com> wrote:
>>
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_context.h   |  6 ++---
>>>  src/mesa/drivers/dri/i965/brw_draw.c  |  2 +-
>>>  src/mesa/drivers/dri/i965/brw_program.c   |  2 +-
>>>  src/mesa/drivers/dri/i965/brw_tcs.c   | 32
>>> ++-
>>>  src/mesa/drivers/dri/i965/brw_tcs_surface_state.c |  2 +-
>>>  src/mesa/drivers/dri/i965/brw_tes.c   | 20 +++---
>>>  src/mesa/drivers/dri/i965/gen7_hs_state.c |  4 +--
>>>  src/mesa/main/context.c   |  2 +-
>>>  src/mesa/main/mtypes.h| 12 +
>>>  src/mesa/main/shaderapi.c |  4 +--
>>>  src/mesa/main/state.c | 11 
>>>  src/mesa/program/prog_statevars.c |  2 +-
>>>  src/mesa/program/program.c|  4 +--
>>>  src/mesa/program/program.h| 23 
>>>  src/mesa/state_tracker/st_atom.c  |  2 +-
>>>  src/mesa/state_tracker/st_atom_constbuf.c |  2 +-
>>>  src/mesa/state_tracker/st_atom_sampler.c  |  2 +-
>>>  src/mesa/state_tracker/st_atom_shader.c   |  2 +-
>>>  src/mesa/state_tracker/st_atom_texture.c  |  2 +-
>>>  src/mesa/state_tracker/st_cb_program.c| 10 +++
>>>  src/mesa/state_tracker/st_program.c   |  6 ++---
>>>  src/mesa/state_tracker/st_program.h   |  6 ++---
>>>  22 files changed, 58 insertions(+), 100 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
>>> b/src/mesa/drivers/dri/i965/brw_context.h
>>> index c92bb9f..9b7e184 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_context.h
>>> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>>> @@ -337,7 +337,7 @@ struct brw_vertex_program {
>>>
>>>  /** Subclass of Mesa tessellation control program */
>>>  struct brw_tess_ctrl_program {
>>> -   struct gl_tess_ctrl_program program;
>>> +   struct gl_program program;
>>> unsigned id;  /**< serial no. to identify tess ctrl progs, never
>>> re-used */
>>>  };
>>>
>>> @@ -1008,7 +1008,7 @@ struct brw_context
>>>  */
>>> const struct gl_vertex_program *vertex_program;
>>> const struct gl_geometry_program *geometry_program;
>>> -   const struct gl_tess_ctrl_program *tess_ctrl_program;
>>> +   const struct gl_program *tess_ctrl_program;
>>> const struct gl_tess_eval_program *tess_eval_program;
>>> const struct gl_fragment_program *fragment_program;
>>>

Re: [Mesa-dev] [PATCH 07/25] mesa/i965: eliminate gl_tess_ctrl_program and use new shared shader_info

2016-10-18 Thread Jason Ekstrand 
On Tue, Oct 18, 2016 at 8:14 AM, Jason Ekstrand 
wrote:

> I want to make a few comments on how this series is structured.  This is
> not the way I would have done it and I think the way you structured it
> makes it substantially less rebasable than it could be and a bit harder to
> review.  The way *I* would have done this would be something like the
> following:
>
>  1) Move shader_info to common code (patches 1-2)
>  2) Add a shader_info pointer to gl_program (patch 6), break the fill
> shader_info stuff from glsl_to_nir into its own function, and call it from
> somewhere such that it always gets filled out.
>  3) Add new fields to shader_info *and* make sure they get filled out from
> other GLSL information
>  4) Convert i965 over to the new shader_info
>  5) Convert gallium over to the new shader_info
>  6) Make GLSL fill out shader_info directly and nuke the old shader
> metadata.
>  7) Delete the shader_info fill-out function.
>
> Something along these lines would go a long way towards avoiding the "mega
> patch" problem where each patch touches 4 or 5 different components.  It
> also makes it clearer to review because you don't add fields and then the
> reviewer goes "Wait, where does this get set?  Oh, in another patch".  I'm
> not necessarily saying that you have to go back and change your patches.
> It's more a suggestion for if you end up doing a v3 or another refactor
> along these lines in the future.
>

On the review side, splitting out as I described above would make it much
easier to review since it would be more-or-less one type of refactor per
patch.  In this patch, we have several different kinds of refactors:

 1) Move consumers over to reading shader_info
 2) Remove gl_tess_ctrl_program and related refactors
 3) Move producer over to writing shader_info

Normally, when reviewing, I would just skim (2) and give (1) a (3) more
effort.  Having them mixed together means I have to pay constant attention
to what's going on.  Also, having (2) mixed in makes it harder to verify
(3) because there's a lot of code motion only some of which matters.


>
>
> On Mon, Oct 17, 2016 at 11:12 PM, Timothy Arceri <
> timothy.arc...@collabora.com> wrote:
>
>> ---
>>  src/mesa/drivers/dri/i965/brw_context.h   |  6 ++---
>>  src/mesa/drivers/dri/i965/brw_draw.c  |  2 +-
>>  src/mesa/drivers/dri/i965/brw_program.c   |  2 +-
>>  src/mesa/drivers/dri/i965/brw_tcs.c   | 32
>> ++-
>>  src/mesa/drivers/dri/i965/brw_tcs_surface_state.c |  2 +-
>>  src/mesa/drivers/dri/i965/brw_tes.c   | 20 +++---
>>  src/mesa/drivers/dri/i965/gen7_hs_state.c |  4 +--
>>  src/mesa/main/context.c   |  2 +-
>>  src/mesa/main/mtypes.h| 12 +
>>  src/mesa/main/shaderapi.c |  4 +--
>>  src/mesa/main/state.c | 11 
>>  src/mesa/program/prog_statevars.c |  2 +-
>>  src/mesa/program/program.c|  4 +--
>>  src/mesa/program/program.h| 23 
>>  src/mesa/state_tracker/st_atom.c  |  2 +-
>>  src/mesa/state_tracker/st_atom_constbuf.c |  2 +-
>>  src/mesa/state_tracker/st_atom_sampler.c  |  2 +-
>>  src/mesa/state_tracker/st_atom_shader.c   |  2 +-
>>  src/mesa/state_tracker/st_atom_texture.c  |  2 +-
>>  src/mesa/state_tracker/st_cb_program.c| 10 +++
>>  src/mesa/state_tracker/st_program.c   |  6 ++---
>>  src/mesa/state_tracker/st_program.h   |  6 ++---
>>  22 files changed, 58 insertions(+), 100 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
>> b/src/mesa/drivers/dri/i965/brw_context.h
>> index c92bb9f..9b7e184 100644
>> --- a/src/mesa/drivers/dri/i965/brw_context.h
>> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>> @@ -337,7 +337,7 @@ struct brw_vertex_program {
>>
>>  /** Subclass of Mesa tessellation control program */
>>  struct brw_tess_ctrl_program {
>> -   struct gl_tess_ctrl_program program;
>> +   struct gl_program program;
>> unsigned id;  /**< serial no. to identify tess ctrl progs, never
>> re-used */
>>  };
>>
>> @@ -1008,7 +1008,7 @@ struct brw_context
>>  */
>> const struct gl_vertex_program *vertex_program;
>> const struct gl_geometry_program *geometry_program;
>> -   const struct gl_tess_ctrl_program *tess_ctrl_program;
>> +   const struct gl_program *tess_ctrl_program;
>> const struct gl_tess_eval_program *tess_eval_program;
>> const struct gl_fragment_program *fragment_program;
>> const struct gl_compute_program *compute_program;
>> @@ -1730,7 +1730,7 @@ brw_vertex_program_const(const struct
>> gl_vertex_program *p)
>>  }
>>
>>  static inline struct brw_tess_ctrl_program *
>> -brw_tess_ctrl_program(struct gl_tess_ctrl_program *p)
>> +brw_tess_ctrl_program(struct gl_program *p)
>>  {

Re: [Mesa-dev] [PATCH 07/25] mesa/i965: eliminate gl_tess_ctrl_program and use new shared shader_info

2016-10-18 Thread Jason Ekstrand 
On Mon, Oct 17, 2016 at 11:12 PM, Timothy Arceri <
timothy.arc...@collabora.com> wrote:

> ---
>  src/mesa/drivers/dri/i965/brw_context.h   |  6 ++---
>  src/mesa/drivers/dri/i965/brw_draw.c  |  2 +-
>  src/mesa/drivers/dri/i965/brw_program.c   |  2 +-
>  src/mesa/drivers/dri/i965/brw_tcs.c   | 32
> ++-
>  src/mesa/drivers/dri/i965/brw_tcs_surface_state.c |  2 +-
>  src/mesa/drivers/dri/i965/brw_tes.c   | 20 +++---
>  src/mesa/drivers/dri/i965/gen7_hs_state.c |  4 +--
>  src/mesa/main/context.c   |  2 +-
>  src/mesa/main/mtypes.h| 12 +
>  src/mesa/main/shaderapi.c |  4 +--
>  src/mesa/main/state.c | 11 
>  src/mesa/program/prog_statevars.c |  2 +-
>  src/mesa/program/program.c|  4 +--
>  src/mesa/program/program.h| 23 
>  src/mesa/state_tracker/st_atom.c  |  2 +-
>  src/mesa/state_tracker/st_atom_constbuf.c |  2 +-
>  src/mesa/state_tracker/st_atom_sampler.c  |  2 +-
>  src/mesa/state_tracker/st_atom_shader.c   |  2 +-
>  src/mesa/state_tracker/st_atom_texture.c  |  2 +-
>  src/mesa/state_tracker/st_cb_program.c| 10 +++
>  src/mesa/state_tracker/st_program.c   |  6 ++---
>  src/mesa/state_tracker/st_program.h   |  6 ++---
>  22 files changed, 58 insertions(+), 100 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> index c92bb9f..9b7e184 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -337,7 +337,7 @@ struct brw_vertex_program {
>
>  /** Subclass of Mesa tessellation control program */
>  struct brw_tess_ctrl_program {
> -   struct gl_tess_ctrl_program program;
> +   struct gl_program program;
> unsigned id;  /**< serial no. to identify tess ctrl progs, never
> re-used */
>  };
>
> @@ -1008,7 +1008,7 @@ struct brw_context
>  */
> const struct gl_vertex_program *vertex_program;
> const struct gl_geometry_program *geometry_program;
> -   const struct gl_tess_ctrl_program *tess_ctrl_program;
> +   const struct gl_program *tess_ctrl_program;
> const struct gl_tess_eval_program *tess_eval_program;
> const struct gl_fragment_program *fragment_program;
> const struct gl_compute_program *compute_program;
> @@ -1730,7 +1730,7 @@ brw_vertex_program_const(const struct
> gl_vertex_program *p)
>  }
>
>  static inline struct brw_tess_ctrl_program *
> -brw_tess_ctrl_program(struct gl_tess_ctrl_program *p)
> +brw_tess_ctrl_program(struct gl_program *p)
>  {
> return (struct brw_tess_ctrl_program *) p;
>  }
> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> index 5d176ef..5485de3 100644
> --- a/src/mesa/drivers/dri/i965/brw_draw.c
> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> @@ -458,7 +458,7 @@ brw_try_draw_prims(struct gl_context *ctx,
> brw->tes.base.sampler_count = ctx->TessEvalProgram._Current ?
>util_last_bit(ctx->TessEvalProgram._Current->Base.SamplersUsed) :
> 0;
> brw->tcs.base.sampler_count = ctx->TessCtrlProgram._Current ?
> -  util_last_bit(ctx->TessCtrlProgram._Current->Base.SamplersUsed) :
> 0;
> +  util_last_bit(ctx->TessCtrlProgram._Current->SamplersUsed) : 0;
> brw->vs.base.sampler_count =
>util_last_bit(ctx->VertexProgram._Current->Base.SamplersUsed);
>
> diff --git a/src/mesa/drivers/dri/i965/brw_program.c
> b/src/mesa/drivers/dri/i965/brw_program.c
> index a41f36e..4e0df5a 100644
> --- a/src/mesa/drivers/dri/i965/brw_program.c
> +++ b/src/mesa/drivers/dri/i965/brw_program.c
> @@ -168,7 +168,7 @@ static struct gl_program *brwNewProgram( struct
> gl_context *ctx,
>if (prog) {
>   prog->id = get_new_program_id(brw->screen);
>
> - return _mesa_init_gl_program(>program.Base, target, id);
> + return _mesa_init_gl_program(>program, target, id);
>} else {
>   return NULL;
>}
> diff --git a/src/mesa/drivers/dri/i965/brw_tcs.c
> b/src/mesa/drivers/dri/i965/brw_tcs.c
> index 0f03fab..08cf413 100644
> --- a/src/mesa/drivers/dri/i965/brw_tcs.c
> +++ b/src/mesa/drivers/dri/i965/brw_tcs.c
> @@ -178,7 +178,7 @@ brw_codegen_tcs_prog(struct brw_context *brw,
> double start_time = 0;
>
> if (tcp) {
> -  nir = tcp->program.Base.nir;
> +  nir = tcp->program.nir;
> } else {
>/* Create a dummy nir_shader.  We won't actually use NIR code to
> * generate assembly (it's easier to generate assembly directly),
> @@ -211,14 +211,14 @@ brw_codegen_tcs_prog(struct brw_context *brw,
>
> if (tcs) {
>brw_assign_common_binding_table_offsets(MESA_SHADER_TESS_CTRL,
> devinfo,
> -

Re: [Mesa-dev] [PATCH 07/25] mesa/i965: eliminate gl_tess_ctrl_program and use new shared shader_info

2016-10-18 Thread Jason Ekstrand 
I want to make a few comments on how this series is structured.  This is
not the way I would have done it and I think the way you structured it
makes it substantially less rebasable than it could be and a bit harder to
review.  The way *I* would have done this would be something like the
following:

 1) Move shader_info to common code (patches 1-2)
 2) Add a shader_info pointer to gl_program (patch 6), break the fill
shader_info stuff from glsl_to_nir into its own function, and call it from
somewhere such that it always gets filled out.
 3) Add new fields to shader_info *and* make sure they get filled out from
other GLSL information
 4) Convert i965 over to the new shader_info
 5) Convert gallium over to the new shader_info
 6) Make GLSL fill out shader_info directly and nuke the old shader
metadata.
 7) Delete the shader_info fill-out function.

Something along these lines would go a long way towards avoiding the "mega
patch" problem where each patch touches 4 or 5 different components.  It
also makes it clearer to review because you don't add fields and then the
reviewer goes "Wait, where does this get set?  Oh, in another patch".  I'm
not necessarily saying that you have to go back and change your patches.
It's more a suggestion for if you end up doing a v3 or another refactor
along these lines in the future.

On Mon, Oct 17, 2016 at 11:12 PM, Timothy Arceri <
timothy.arc...@collabora.com> wrote:

> ---
>  src/mesa/drivers/dri/i965/brw_context.h   |  6 ++---
>  src/mesa/drivers/dri/i965/brw_draw.c  |  2 +-
>  src/mesa/drivers/dri/i965/brw_program.c   |  2 +-
>  src/mesa/drivers/dri/i965/brw_tcs.c   | 32
> ++-
>  src/mesa/drivers/dri/i965/brw_tcs_surface_state.c |  2 +-
>  src/mesa/drivers/dri/i965/brw_tes.c   | 20 +++---
>  src/mesa/drivers/dri/i965/gen7_hs_state.c |  4 +--
>  src/mesa/main/context.c   |  2 +-
>  src/mesa/main/mtypes.h| 12 +
>  src/mesa/main/shaderapi.c |  4 +--
>  src/mesa/main/state.c | 11 
>  src/mesa/program/prog_statevars.c |  2 +-
>  src/mesa/program/program.c|  4 +--
>  src/mesa/program/program.h| 23 
>  src/mesa/state_tracker/st_atom.c  |  2 +-
>  src/mesa/state_tracker/st_atom_constbuf.c |  2 +-
>  src/mesa/state_tracker/st_atom_sampler.c  |  2 +-
>  src/mesa/state_tracker/st_atom_shader.c   |  2 +-
>  src/mesa/state_tracker/st_atom_texture.c  |  2 +-
>  src/mesa/state_tracker/st_cb_program.c| 10 +++
>  src/mesa/state_tracker/st_program.c   |  6 ++---
>  src/mesa/state_tracker/st_program.h   |  6 ++---
>  22 files changed, 58 insertions(+), 100 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> index c92bb9f..9b7e184 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -337,7 +337,7 @@ struct brw_vertex_program {
>
>  /** Subclass of Mesa tessellation control program */
>  struct brw_tess_ctrl_program {
> -   struct gl_tess_ctrl_program program;
> +   struct gl_program program;
> unsigned id;  /**< serial no. to identify tess ctrl progs, never
> re-used */
>  };
>
> @@ -1008,7 +1008,7 @@ struct brw_context
>  */
> const struct gl_vertex_program *vertex_program;
> const struct gl_geometry_program *geometry_program;
> -   const struct gl_tess_ctrl_program *tess_ctrl_program;
> +   const struct gl_program *tess_ctrl_program;
> const struct gl_tess_eval_program *tess_eval_program;
> const struct gl_fragment_program *fragment_program;
> const struct gl_compute_program *compute_program;
> @@ -1730,7 +1730,7 @@ brw_vertex_program_const(const struct
> gl_vertex_program *p)
>  }
>
>  static inline struct brw_tess_ctrl_program *
> -brw_tess_ctrl_program(struct gl_tess_ctrl_program *p)
> +brw_tess_ctrl_program(struct gl_program *p)
>  {
> return (struct brw_tess_ctrl_program *) p;
>  }
> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> index 5d176ef..5485de3 100644
> --- a/src/mesa/drivers/dri/i965/brw_draw.c
> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> @@ -458,7 +458,7 @@ brw_try_draw_prims(struct gl_context *ctx,
> brw->tes.base.sampler_count = ctx->TessEvalProgram._Current ?
>util_last_bit(ctx->TessEvalProgram._Current->Base.SamplersUsed) :
> 0;
> brw->tcs.base.sampler_count = ctx->TessCtrlProgram._Current ?
> -  util_last_bit(ctx->TessCtrlProgram._Current->Base.SamplersUsed) :
> 0;
> +  util_last_bit(ctx->TessCtrlProgram._Current->SamplersUsed) : 0;
> brw->vs.base.sampler_count =
>util_last_bit(ctx->VertexProgram._Current->Base.SamplersUsed);
>
> diff --git