[Mesa-dev] [PATCH 1/2] i965: Viewport extents on gen8

[Pohjolainen, Topi]

On Thu, Jul 03, 2014 at 11:23:13AM -0700, Ben Widawsky wrote:
> Viewport extents are a 3rd rectangle that defines which pixels get
> discarded as part of the rasterization process. This can potentially
> improve performance by reducing cache usage, and freeing up PS cycles.
> This will get hit if one's viewport is smaller than the full
> renderbuffer, and scissoring is not used.
> 
> The functionality itself very much resembles scissoring.
> ---
>  src/mesa/drivers/dri/i965/gen8_viewport_state.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/gen8_viewport_state.c b/src/mesa/drivers/dri/i965/gen8_viewport_state.c
> index b366246..2bf5fbb 100644
> --- a/src/mesa/drivers/dri/i965/gen8_viewport_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_viewport_state.c
> @@ -86,17 +86,23 @@ gen8_upload_sf_clip_viewport(struct brw_context *brw)
>        vp[10] = -gby; /* y-min */
>        vp[11] =  gby; /* y-max */
>  
> -      /* _NEW_SCISSOR | _NEW_VIEWPORT | _NEW_BUFFERS: Screen Space Viewport */
> +      /* _NEW_SCISSOR | _NEW_VIEWPORT | _NEW_BUFFERS: Screen Space Viewport
> +       * The hardware will take the intersection of the drawing rectangle,
> +       * scissor rectangle, and the viewport extents. We don't need to be
> +       * smart, and can therefore just program the viewport extents.
> +       */
> +      float viewport_Xmax = ctx->ViewportArray[i].X + ctx->ViewportArray[i].Width;
> +      float viewport_Ymax = ctx->ViewportArray[i].Y + ctx->ViewportArray[i].Height;

These could be both declared as constants and the right hand sides should go
to their own lines as they are now overflowing the 80-char limit.

Otherwise this looks to me as the right thing to do, and not only from
performance point of view. I'm thinking a case where the limits for the
drawbuffer are not set but where the viewport limits are - with the current
logic we wouldn't apply the limits, right? I guess we don't have any such
piglit test case.

But then I also realized that if we applied this, then gen8 wouldn't take the
drawbuffer limits into account anywhere else. So this should break some piglit
tests?

I tried to look at the earlier generations from six onwards and actually
couldn't find any state atom using the drawbuffer limits. (The only reference
is SF-scissor setting in brw_sf_state.c:upload_sf_vp() which is for gen < 6).
I guess I can say I'm confused.

>        if (render_to_fbo) {
> -         vp[12] = ctx->DrawBuffer->_Xmin;
> -         vp[13] = ctx->DrawBuffer->_Xmax - 1;
> -         vp[14] = ctx->DrawBuffer->_Ymin;
> -         vp[15] = ctx->DrawBuffer->_Ymax - 1;
> +         vp[12] = ctx->ViewportArray[i].X;
> +         vp[13] = viewport_Xmax - 1;
> +         vp[14] = ctx->ViewportArray[i].Y;
> +         vp[15] = viewport_Ymax - 1;
>        } else {
> -         vp[12] = ctx->DrawBuffer->_Xmin;
> -         vp[13] = ctx->DrawBuffer->_Xmax - 1;
> -         vp[14] = ctx->DrawBuffer->Height - ctx->DrawBuffer->_Ymax;
> -         vp[15] = ctx->DrawBuffer->Height - ctx->DrawBuffer->_Ymin - 1;
> +         vp[12] = ctx->ViewportArray[i].X;
> +         vp[13] = viewport_Xmax - 1;
> +         vp[14] = ctx->DrawBuffer->Height - viewport_Ymax;
> +         vp[15] = ctx->DrawBuffer->Height - ctx->ViewportArray[i].Y - 1;
>        }
>  
>        vp += 16;
> -- 
> 2.0.1
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev