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

[Kenneth Graunke]

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...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150428/383e9195/attachment.sig>