<div dir="ltr">On 29 August 2013 15:46, Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5">Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> 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. </div></div>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. </blockquote></div> </div><div class="gmail_extra">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. </div><div class="gmail_extra">I'll plan on changing the name to brw_state_state[_base] tomorrow morning unless I hear objections. </div></div>