[Mesa-dev] [PATCH 05/22] i965: Move data from brw->vs into a base class if gs will also need it.

[Chad Versace]

On 08/29/2013 03:51 PM, Paul Berry wrote:
> On 29 August 2013 15:46, Eric Anholt <eric at anholt.net> wrote:
>
>> Kenneth Graunke <kenneth at whitecape.org> writes:
>>
>>> On 08/26/2013 03:12 PM, Paul Berry wrote:
>>>> This paves the way for sharing the code that will set up the vertex
>>>> and geometry shader pipeline state.
>>>> ---
>>>>    src/mesa/drivers/dri/i965/brw_context.h          | 47
>> ++++++++++++++----------
>>>>    src/mesa/drivers/dri/i965/brw_draw.c             |  3 +-
>>>>    src/mesa/drivers/dri/i965/brw_misc_state.c       |  6 +--
>>>>    src/mesa/drivers/dri/i965/brw_vs.c               | 12 +++---
>>>>    src/mesa/drivers/dri/i965/brw_vs_state.c         | 24 ++++++------
>>>>    src/mesa/drivers/dri/i965/brw_vs_surface_state.c | 43
>> ++++++++++++----------
>>>>    src/mesa/drivers/dri/i965/brw_vtbl.c             |  2 +-
>>>>    src/mesa/drivers/dri/i965/brw_wm_sampler_state.c |  8 ++--
>>>>    src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  4 +-
>>>>    src/mesa/drivers/dri/i965/gen6_sampler_state.c   |  2 +-
>>>>    src/mesa/drivers/dri/i965/gen6_vs_state.c        | 23 +++++++-----
>>>>    src/mesa/drivers/dri/i965/gen7_vs_state.c        | 18 +++++----
>>>>    12 files changed, 107 insertions(+), 85 deletions(-)
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
>> b/src/mesa/drivers/dri/i965/brw_context.h
>>>> index dcd4c9a..9784956 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_context.h
>>>> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>>>> @@ -818,6 +818,32 @@ struct brw_query_object {
>>>>
>>>>
>>>>    /**
>>>> + * Data shared between brw_context::vs and brw_context::gs
>>>> + */
>>>> +struct brw_vec4_context_base
>>>> +{
>>>> +   drm_intel_bo *scratch_bo;
>>>> +   drm_intel_bo *const_bo;
>>>> +   /** Offset in the program cache to the program */
>>>> +   uint32_t prog_offset;
>>>> +   uint32_t state_offset;
>>>> +
>>>> +   uint32_t push_const_offset; /* Offset in the batchbuffer */
>>>> +   int push_const_size; /* in 256-bit register increments */
>>>> +
>>>> +   uint32_t bind_bo_offset;
>>>> +   uint32_t surf_offset[BRW_MAX_VEC4_SURFACES];
>>>> +
>>>> +   /** SAMPLER_STATE count and table offset */
>>>> +   uint32_t sampler_count;
>>>> +   uint32_t sampler_offset;
>>>> +
>>>> +   /** Offsets in the batch to sampler default colors (texture border
>> color) */
>>>> +   uint32_t sdc_offset[BRW_MAX_TEX_UNIT];
>>>> +};
>>>
>>> I like what this patch is doing, but I really don't like the names.
>>>
>>> With the exception of ralloc, "context"/"ctx" generally always mean the
>>> global GL context: gl_context or a subclass like brw_context.  (For
>>> ralloc, we inherited the "context" terminology from talloc, so it kind
>>> of stuck.)  vec4_ctx/brw_vec4_context_base are something totally
>> different.
>>>
>>> This is a structure that represents the shader program state for a
>>> particular pipeline stage.  Also, other than BRW_MAX_VEC4_SURFACES,
>>> there's nothing vec4 specific about this at all.  The pixel shader could
>>> use every one of these fields (and should eventually).  So I dislike
>>> "vec4" in the name - we're just going to have to change it.
>>>
>>> I had suggested names like brw_shader_state or brw_pipeline_state...I'm
>>> open to other ideas.
>>
>> brw_stage_state[_base]?  "stage" is the terminology used in ARB_sso (and
>> in our conversations), and this looks like it's the common state among
>> all of vs/gs/fs.  pipeline is for a group of the things, for sure.
>>
>> On the other hand, we've been using "shader" as the variable name to
>> store the per-stage thing, and shader_program for the whole-pipeline
>> thing, so brw_shader_state seems fine to me too.
>>
>
> Ken and I like brw_stage_state[_base].  Ken prefers without _base, I have a
> minor preference for _base, but we both feel like that's a detail that can
> be decided on the fly when I get around to doing the renaming.
>
> I'll plan on changing the name to brw_state_state[_base] tomorrow morning
> unless I hear objections.

I also don't like vec4 in the name.

With the new name brw_{stage,shader}_state[_base], patch 5 is
Reviewed-by: Chad Versace <chad.versace at linux.intel.com>

I prefer brw_stage_state without _base. I don't think _base adds anything
useful to the name, and it just makes the name longer. Also, _base hints at some sort
of OO inheritance, but what you're doing smells more like composition than
inheritance to me.