[Mesa-dev] [PATCH 1/3] draw: clamp the viewports to always be between 0 and max

[Brian Paul]

On 05/25/2013 12:12 AM, Zack Rusin wrote:
> The spec says that everything beyond last set viewport
> is reset to zero so make sure to preserve that behavior.
>
> Signed-off-by: Zack Rusin <zackr at vmware.com>
> ---
>   src/gallium/auxiliary/draw/draw_cliptest_tmp.h |    8 ++++----
>   src/gallium/auxiliary/draw/draw_context.c      |   14 ++++++++++++++
>   src/gallium/auxiliary/draw/draw_pipe_clip.c    |    8 ++++----
>   src/gallium/auxiliary/draw/draw_private.h      |    8 ++++++++
>   src/gallium/auxiliary/draw/draw_vs_variant.c   |    5 ++---
>   5 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/src/gallium/auxiliary/draw/draw_cliptest_tmp.h b/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
> index 09e1fd7..6e4a247 100644
> --- a/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
> +++ b/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
> @@ -25,8 +25,6 @@
>    *
>    **************************************************************************/
>
> -
> -
>   static boolean TAG(do_cliptest)( struct pt_post_vs *pvs,
>                                    struct draw_vertex_info *info )
>   {
> @@ -57,8 +55,10 @@ static boolean TAG(do_cliptest)( struct pt_post_vs *pvs,
>            draw_current_shader_uses_viewport_index(pvs->draw) ?
>            *((unsigned*)out->data[viewport_index_output]): 0;
>         unsigned mask = 0x0;
> -      const float *scale = pvs->draw->viewports[viewport_index].scale;
> -      const float *trans = pvs->draw->viewports[viewport_index].translate;
> +      const float *scale = pvs->draw->viewports[
> +         DRAW_CLAMP_VIEWPORT_IDX(viewport_index) ].scale;
> +      const float *trans = pvs->draw->viewports[
> +         DRAW_CLAMP_VIEWPORT_IDX(viewport_index)].translate;
>
>         initialize_vertex_header(out);
>
> diff --git a/src/gallium/auxiliary/draw/draw_context.c b/src/gallium/auxiliary/draw/draw_context.c
> index 63ccf38..bc35926 100644
> --- a/src/gallium/auxiliary/draw/draw_context.c
> +++ b/src/gallium/auxiliary/draw/draw_context.c
> @@ -317,6 +317,7 @@ void draw_set_viewport_states( struct draw_context *draw,
>                                  const struct pipe_viewport_state *vps )
>   {
>      const struct pipe_viewport_state *viewport = vps;
> +   unsigned i;
>      draw_do_flush(draw, DRAW_FLUSH_PARAMETER_CHANGE);
>
>      if (start_slot > PIPE_MAX_VIEWPORTS)
> @@ -327,6 +328,19 @@ void draw_set_viewport_states( struct draw_context *draw,
>
>      memcpy(draw->viewports + start_slot, vps,
>             sizeof(struct pipe_viewport_state) * num_viewports);
> +
> +   /* Zero out the remaining viewports */
> +   for (i = start_slot + num_viewports; i < PIPE_MAX_VIEWPORTS; ++i) {
> +      draw->viewports[i].scale[0] = 0.0f;
> +      draw->viewports[i].scale[1] = 0.0f;
> +      draw->viewports[i].scale[2] = 0.0f;
> +      draw->viewports[i].scale[3] = 0.0f;
> +      draw->viewports[i].translate[0] = 0.0f;
> +      draw->viewports[i].translate[1] = 0.0f;
> +      draw->viewports[i].translate[2] = 0.0f;
> +      draw->viewports[i].translate[3] = 0.0f;
> +   }

Would a memset(0) be worthwhile there since the most common case is 
setting one viewport then zeroing 15 others.


> +
>      draw->identity_viewport = (num_viewports == 1) &&
>         (viewport->scale[0] == 1.0f &&
>          viewport->scale[1] == 1.0f &&
> diff --git a/src/gallium/auxiliary/draw/draw_pipe_clip.c b/src/gallium/auxiliary/draw/draw_pipe_clip.c
> index b01e519..3b82be9 100644
> --- a/src/gallium/auxiliary/draw/draw_pipe_clip.c
> +++ b/src/gallium/auxiliary/draw/draw_pipe_clip.c
> @@ -112,8 +112,6 @@ static void copy_flat( struct draw_stage *stage,
>      }
>   }
>
> -
> -
>   /* Interpolate between two vertices to produce a third.
>    */
>   static void interp( const struct clip_stage *clip,
> @@ -152,9 +150,11 @@ static void interp( const struct clip_stage *clip,
>            *((unsigned*)in->data[viewport_index_output]) : 0;
>         const float *pos = dst->pre_clip_pos;
>         const float *scale =
> -         clip->stage.draw->viewports[viewport_index].scale;
> +         clip->stage.draw->viewports[
> +            DRAW_CLAMP_VIEWPORT_IDX(viewport_index)].scale;
>         const float *trans =
> -         clip->stage.draw->viewports[viewport_index].translate;
> +         clip->stage.draw->viewports[
> +            DRAW_CLAMP_VIEWPORT_IDX(viewport_index)].translate;
>         const float oow = 1.0f / pos[3];
>
>         dst->data[pos_attr][0] = pos[0] * oow * scale[0] + trans[0];
> diff --git a/src/gallium/auxiliary/draw/draw_private.h b/src/gallium/auxiliary/draw/draw_private.h
> index e5f192b..0a6ff7d 100644
> --- a/src/gallium/auxiliary/draw/draw_private.h
> +++ b/src/gallium/auxiliary/draw/draw_private.h
> @@ -469,4 +469,12 @@ draw_get_rasterizer_no_cull( struct draw_context *draw,
>   #define DRAW_GET_IDX(_elts, _i)                   \
>      (((_i) >= draw->pt.user.eltMax) ? 0 : (_elts)[_i])
>
> +/**
> + * Return index of the given viewport clamping it
> + * to be between 0 <= and < PIPE_MAX_VIEWPORTS
> + */
> +#define DRAW_CLAMP_VIEWPORT_IDX(idx)         \
> +   ((PIPE_MAX_VIEWPORTS > idx || idx < 0) ? idx : 0)

inline function?


> +
> +
>   #endif /* DRAW_PRIVATE_H */
> diff --git a/src/gallium/auxiliary/draw/draw_vs_variant.c b/src/gallium/auxiliary/draw/draw_vs_variant.c
> index 0387eaf..e83a8b5 100644
> --- a/src/gallium/auxiliary/draw/draw_vs_variant.c
> +++ b/src/gallium/auxiliary/draw/draw_vs_variant.c
> @@ -91,9 +91,8 @@ find_viewport(struct draw_context *draw,
>      int viewport_index =
>         draw_current_shader_uses_viewport_index(draw) ?
>         data[viewport_index_output * 4] : 0;
> -
> -   debug_assert(viewport_index < PIPE_MAX_VIEWPORTS);
> -   viewport_index = MIN2(viewport_index, PIPE_MAX_VIEWPORTS - 1);
> +
> +   viewport_index = DRAW_CLAMP_VIEWPORT_IDX(viewport_index);
>
>      return &draw->viewports[viewport_index];
>   }
>

Reviewed-by: Brian Paul <brianp at vmware.com>