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

[Pohjolainen, Topi]

On Thu, Jul 24, 2014 at 10:22:25AM -0700, Ben Widawsky wrote:
> 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).

I'm asking for the same reason, I don't know how these things are set in the
core. I meant the DrawBuffer::_Xmin/Xmax/Ymin/Ymax. So I'm thinking a case
where DrawBuffer::_Xmin/Xmax/Ymin/Ymax do not impose any limits but where
ViewportArray[] do. Current logic wouldn't take them into account but your
version would.
But then on the other hand after your change I don't see how the constraints
in DrawBuffer::_Xmin/Xmax/Ymin/Ymax would be taken into account.

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