[Mesa-dev] [PATCH 09/53] r600: use macros for updating the various stages.

[Oded Gabbay]

On Mon, Nov 30, 2015 at 8:20 AM, Dave Airlie <airlied at gmail.com> wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> These macros will make things easier to see when tess
> is added to the mix.
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>  src/gallium/drivers/r600/r600_state_common.c | 40 +++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/src/gallium/drivers/r600/r600_state_common.c b/src/gallium/drivers/r600/r600_state_common.c
> index 36afbd6..bc9c3b6 100644
> --- a/src/gallium/drivers/r600/r600_state_common.c
> +++ b/src/gallium/drivers/r600/r600_state_common.c
> @@ -1294,6 +1294,26 @@ static void r600_update_clip_state(struct r600_context *rctx,
>                         return false;                                   \
>         } while(0)
>
> +#define UPDATE_SHADER(hw, sw) do {                                     \
> +               if (sw##_dirty || (rctx->hw_shader_stages[(hw)].shader != rctx->sw##_shader->current)) \
> +                       update_shader_atom(ctx, &rctx->hw_shader_stages[(hw)], rctx->sw##_shader->current); \
> +       } while(0)
> +
> +#define UPDATE_SHADER_CLIP(hw, sw) do {                                        \
> +               if (sw##_dirty || (rctx->hw_shader_stages[(hw)].shader != rctx->sw##_shader->current)) { \
> +                       update_shader_atom(ctx, &rctx->hw_shader_stages[(hw)], rctx->sw##_shader->current); \
> +                       clip_so_current = rctx->sw##_shader->current;   \
> +               }                                                       \
> +       } while(0)
> +
> +#define UPDATE_SHADER_GS(hw, hw2, sw) do {                             \
> +               if (sw##_dirty || (rctx->hw_shader_stages[(hw)].shader != rctx->sw##_shader->current)) { \
> +                       update_shader_atom(ctx, &rctx->hw_shader_stages[(hw)], rctx->sw##_shader->current); \
> +                       update_shader_atom(ctx, &rctx->hw_shader_stages[(hw2)], rctx->sw##_shader->current->gs_copy_shader); \
> +                       clip_so_current = rctx->sw##_shader->current->gs_copy_shader; \
> +               }                                                       \
> +       } while(0)
> +

Why did you drop the "unlikely" from all the ifs ?

>  #define SET_NULL_SHADER(hw) do {                                               \
>                 if (rctx->hw_shader_stages[(hw)].shader)        \
>                         update_shader_atom(ctx, &rctx->hw_shader_stages[(hw)], NULL); \
> @@ -1338,17 +1358,11 @@ static bool r600_update_derived_state(struct r600_context *rctx)
>                 }
>
>                 /* gs_shader provides GS and VS (copy shader) */
> -               if (unlikely(rctx->hw_shader_stages[R600_HW_STAGE_GS].shader != rctx->gs_shader->current)) {
> -                       update_shader_atom(ctx, &rctx->hw_shader_stages[R600_HW_STAGE_GS], rctx->gs_shader->current);
> -                       update_shader_atom(ctx, &rctx->hw_shader_stages[R600_HW_STAGE_VS], rctx->gs_shader->current->gs_copy_shader);
> -
> -                       clip_so_current = rctx->gs_shader->current->gs_copy_shader;
> -               }
> +               UPDATE_SHADER_GS(R600_HW_STAGE_GS, R600_HW_STAGE_VS, gs);
>
>                 /* vs_shader is used as ES */
> -               if (unlikely(vs_dirty || rctx->hw_shader_stages[R600_HW_STAGE_ES].shader != rctx->vs_shader->current)) {
> -                       update_shader_atom(ctx, &rctx->hw_shader_stages[R600_HW_STAGE_ES], rctx->vs_shader->current);
> -               }
> +               UPDATE_SHADER(R600_HW_STAGE_ES, vs);
> +
>         } else {
>                 if (unlikely(rctx->hw_shader_stages[R600_HW_STAGE_GS].shader)) {
>                         SET_NULL_SHADER(R600_HW_STAGE_GS);
> @@ -1357,11 +1371,7 @@ static bool r600_update_derived_state(struct r600_context *rctx)
>                         r600_mark_atom_dirty(rctx, &rctx->shader_stages.atom);
>                 }
>
> -               if (unlikely(vs_dirty || rctx->hw_shader_stages[R600_HW_STAGE_VS].shader != rctx->vs_shader->current)) {
> -                       update_shader_atom(ctx, &rctx->hw_shader_stages[R600_HW_STAGE_VS], rctx->vs_shader->current);
> -
> -                       clip_so_current = rctx->vs_shader->current;
> -               }
> +               UPDATE_SHADER_CLIP(R600_HW_STAGE_VS, vs);
>         }
>
>         /* Update clip misc state. */
> @@ -1399,8 +1409,8 @@ static bool r600_update_derived_state(struct r600_context *rctx)
>                 }
>
>                 r600_mark_atom_dirty(rctx, &rctx->shader_stages.atom);
> -               update_shader_atom(ctx, &rctx->hw_shader_stages[R600_HW_STAGE_PS], rctx->ps_shader->current);
>         }
> +       UPDATE_SHADER(R600_HW_STAGE_PS, ps);

I'm not sure moving it outside the if is equal to the original code,
because in the large if statement before the macro call, there are
four conditions which are OR'ed between themselves. In the macro, you
have only two out of those four conditions. So, if those 2 conditions
in the macro are false, but at least one of the other two conditions
are true, you wouldn't update the PS shader in this patch, while in
the original code, the PS shader would have gotten updated.

btw, reading the original code, I'm not sure it was correct, because
calling update_shader_atom increments some memory accounting variables
unconditionally, so if you updated the shader with itself (i.e. they
weren't different), it may have resulted in wrong memory accounting.

>
>         if (rctx->b.chip_class >= EVERGREEN) {
>                 evergreen_update_db_shader_control(rctx);
> --
> 2.5.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Without reading yet the rest of the patch series, I feel this patch is
overkill. But I guess it would make sense when you add the tess stuff.