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

[Ben Widawsky]

On Thu, Jul 24, 2014 at 10:29:11AM +0300, Pohjolainen, Topi wrote:
> 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.

Got it.

> 
> 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.

I'm new here. I don't understand. Can you explain what you mean by
drawbuffer limits not being set set? I wasn't really aware such a thing
was possible (if I followed your meaning).

> 
> 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 didn't run full, but quick didn't have any regressions.

> 
> 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;

I'll take a look, but Ken may have a more immediate answer. I was
distracted by other things for the 3 weeks, but I am back looking at
this now (and the GB clipping as well). My quick answer is that hardware
will just do the right thing (I only looked at GEN8), but I need to read
the docs further.

I've yet to find a perf win, though some benchmarks do hit this path.
I'll do some more piglit testing as well, and update with that info.

-- 
Ben Widawsky, Intel Open Source Technology Center