[Mesa-dev] [PATCH 4/4] i965/clip: Removing scissor atom

[Kenneth Graunke]

On Monday, August 04, 2014 12:24:04 PM Ben Widawsky wrote:
> On GEN8, a change in scissor state does not effect anything for the
> clipper/sf hardware state. The hardware will always do the right thing
> once the viewport extents are programmed. We can therefore remove the
> unecessary state emission.
> 
> Ken originally spotted this.

Scissoring state affects the value of ctx->DrawBuffer->_Xmin, _Xmax, _Ymin, and _Ymax.  So, _NEW_SCISSORS was actually necessary prior to your patch #2.

Perhaps reword the commit message to be something like this:

Now that we no longer use ctx->DrawBuffer->_Xmin and related fields to program the screen-space viewport extents, we don't depend on any scissoring state.  So we can drop the _NEW_SCISSOR dependency.

> ---
>  src/mesa/drivers/dri/i965/gen8_viewport_state.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/gen8_viewport_state.c b/src/mesa/drivers/dri/i965/gen8_viewport_state.c
> index 04a4530..d7e06c4 100644
> --- a/src/mesa/drivers/dri/i965/gen8_viewport_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_viewport_state.c
> @@ -100,13 +100,17 @@ 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_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 (viewport_Ymax < ctx->DrawBuffer->_Ymax ||
> +            viewport_Xmax < ctx->DrawBuffer->_Xmax) {
> +            perf_debug("Using viewport extents for savings\n");
> +      }

I suspect you didn't mean to add this :)  Please drop it, as using ctx->DrawBuffer->_Xmax actually introduces a dependency on _NEW_SCISSOR again. :)

With those fixed, this would get a:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

>        if (render_to_fbo) {
>           vp[12] = ctx->ViewportArray[i].X;
>           vp[13] = viewport_Xmax - 1;
> @@ -130,7 +134,7 @@ gen8_upload_sf_clip_viewport(struct brw_context *brw)
>  
>  const struct brw_tracked_state gen8_sf_clip_viewport = {
>     .dirty = {
> -      .mesa = _NEW_BUFFERS | _NEW_SCISSOR | _NEW_VIEWPORT,
> +      .mesa = _NEW_BUFFERS | _NEW_VIEWPORT,
>        .brw = BRW_NEW_BATCH,
>        .cache = 0,
>     },
> 
-------------- 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/3a0a1306/attachment-0001.sig>