[Mesa-dev] [PATCH 2/4] i965: Viewport extents on GEN8

[Kenneth Graunke]

On Monday, August 04, 2014 12:24:02 PM 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.

I'm not sure about cache effects...pretty sure it doesn't save PS cycles.

> It also permits the use of guardband clipping in all cases (see later
> patch). The actual pixels drawn to the screen are an intersection of the
> drawing rectangle, the viewport extents, and the scissor rectangle.
> 
> Scissor rectangle is not super important this discussion as it should
                                         ^^^ to/for?
> always help do the right thing provided the programmer uses it.
> 
> switch (viewport dimensions, drawrect dimension) {
>    case viewport > drawing rectangle: no effects; break;
>    case viewport == drawing rectangle: no effects; break;
>    case viewport < drawing rectangle:
>       Pixels (after the viewport transformation but before expensive
>       rastersizing and shading operations) which are outside of the
>       viewport are discarded.

As we discussed, the 3D clipper normally gets involved and trims off any geometry outside of the viewport, but within the drawing rectangle.  So, expensive pixel shading operations would not happen regardless.

I think the main point of this patch is your earlier comment:
"The actual pixels drawn to the screen are an intersection of the
 drawing rectangle, the viewport extents, and the scissor rectangle."

The previous code programmed the viewport extents to the intersection of the viewport, drawing rectangle, and scissor rectangle.  This is unnecessary, because the hardware does that intersection for us.  So we should simply program it to the viewport.

Also, please do change the title to include some sort of verb.  For example,

   i965: Simplify Gen8+ viewport extents programming.

Commit messages aside!  Your code looks good, and is:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Thank you for doing this!

> }
> 
> I am unable to find a test case where this improves performance, but in
> all my testing it doesn't hurt performance, and intuitively, it should
> not ever hurt performance. It also permits us to use the guardband more
> freely (see upcoming patch).
> 
> v2: Updating commit message.
> ---
>  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 b5171e0..04a4530 100644
> --- a/src/mesa/drivers/dri/i965/gen8_viewport_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_viewport_state.c
> @@ -100,17 +100,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;
>        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;
> 
-------------- 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/20140809/0247d6e9/attachment.sig>