[Mesa-dev] [PATCH 14/18] i965/wm/gen6: Refactor program offset setup

[Pohjolainen, Topi]

On Tue, Apr 28, 2015 at 03:01:43PM -0700, Kenneth Graunke wrote:
> On Wednesday, April 22, 2015 11:47:34 PM Topi Pohjolainen wrote:
> > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_state.h     |  8 +++++
> >  src/mesa/drivers/dri/i965/gen6_wm_state.c | 56 ++++++++++++++++++-------------
> >  2 files changed, 41 insertions(+), 23 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_state.h b/src/mesa/drivers/dri/i965/brw_state.h
> > index 23f36c0..ca3274d 100644
> > --- a/src/mesa/drivers/dri/i965/brw_state.h
> > +++ b/src/mesa/drivers/dri/i965/brw_state.h
> > @@ -292,6 +292,14 @@ void brw_update_sampler_state(struct brw_context *brw,
> >                                uint32_t *sampler_state,
> >                                uint32_t batch_offset_for_sampler_state);
> >  
> > +/* gen6_wm_state.c */
> > +void
> > +gen6_wm_state_set_programs(const struct brw_wm_prog_data *prog_data,
> > +                           const struct brw_stage_state *stage_state,
> > +                           int min_inv_per_frag,
> > +                           uint32_t *ksp0, uint32_t *ksp2,
> > +                           uint32_t *dw4, uint32_t *dw5, uint32_t *dw6);
> > +
> >  /* gen6_sf_state.c */
> >  void
> >  calculate_attr_overrides(const struct brw_context *brw,
> > diff --git a/src/mesa/drivers/dri/i965/gen6_wm_state.c b/src/mesa/drivers/dri/i965/gen6_wm_state.c
> > index 8e673a4..bc921e5 100644
> > --- a/src/mesa/drivers/dri/i965/gen6_wm_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen6_wm_state.c
> > @@ -65,6 +65,37 @@ const struct brw_tracked_state gen6_wm_push_constants = {
> >     .emit = gen6_upload_wm_push_constants,
> >  };
> >  
> > +void
> > +gen6_wm_state_set_programs(const struct brw_wm_prog_data *prog_data,
> > +                           const struct brw_stage_state *stage_state,
> > +                           int min_inv_per_frag,
> > +                           uint32_t *ksp0, uint32_t *ksp2,
> > +                           uint32_t *dw4, uint32_t *dw5, uint32_t *dw6)
> > +{
> > +   if (prog_data->prog_offset_16 || prog_data->no_8) {
> > +      *dw5 |= GEN6_WM_16_DISPATCH_ENABLE;
> > +
> > +      if (!prog_data->no_8 && min_inv_per_frag == 1) {
> > +         *dw5 |= GEN6_WM_8_DISPATCH_ENABLE;
> > +         *dw4 |= (prog_data->base.dispatch_grf_start_reg <<
> > +                  GEN6_WM_DISPATCH_START_GRF_SHIFT_0);
> > +         *dw4 |= (prog_data->dispatch_grf_start_reg_16 <<
> > +                  GEN6_WM_DISPATCH_START_GRF_SHIFT_2);
> > +         *ksp0 = stage_state->prog_offset;
> > +         *ksp2 = stage_state->prog_offset + prog_data->prog_offset_16;
> > +      } else {
> > +         *dw4 |= (prog_data->dispatch_grf_start_reg_16 <<
> > +                  GEN6_WM_DISPATCH_START_GRF_SHIFT_0);
> > +         *ksp0 = stage_state->prog_offset + prog_data->prog_offset_16;
> > +      }
> > +   } else {
> > +      *dw5 |= GEN6_WM_8_DISPATCH_ENABLE;
> > +      *dw4 |= (prog_data->base.dispatch_grf_start_reg <<
> > +               GEN6_WM_DISPATCH_START_GRF_SHIFT_0);
> > +      *ksp0 = stage_state->prog_offset;
> > +   }
> > +}
> > +
> 
> This split feels awkward to me - the code to emit 3DSTATE_WM is now
> split across multiple functions...and it has 5 out parameters.  I really
> prefer keeping the code to fill out a packet's DWords together in one
> function.
> 
> Could we keep it in one function, but instead make upload_wm_state()
> take additional parameters, rather than poking at brw-> directly?
> 
> Sorry for the trouble...

I'll take another look. I can't remember all the details but I started that
way, it became ugly and so I decided to do this instead.