[Mesa-dev] [PATCH 2/2] gallium: Replaced gl_rasterization_rules with lower_left_origin and half_pixel_center.

[Jose Fonseca]

----- Original Message -----
> On 21.04.2013 13:18, Jose Fonseca wrote:
> >
> > ----- Original Message -----
> >> On 21.04.2013 09:36, Jose Fonseca wrote:
> >>> ----- Original Message -----
> >>>> Do we really need the lower_left_origin state? I think I can't
> >>>> implement it for radeon and it's the kind of stuff that should be
> >>>> taken care of by the state tracker anyway.
> >>> My understanding is that hardware had switches for this sort of thing.
> >>> It's
> >>> really hard to provide fully-conforming rasterization for opengl, dx9 &
> >>> dx10 without it.
> >>>
> >>> If your hardware allows to put a negative pitch on rendertargets, then
> >>> that
> >>> should also do it.
> >> I have a switch for the upside down thing, but maybe it could be
> >> framebuffer state instead of rasterizer state (since it's going to
> >> either not change (D3D)
> > You're right, they should never change at higher frequency than
> > per-framebuffer.
> >
> > But due to auxiliary modules like u_blit, u_blitter, u_gen_mipmap, this
> > state will eventually change even for D3D state trackers.  (This is
> > however fixable, if there are performance implications switching this
> > state, we could enhance these helper modules so that they switch it often.
> > But I doubt this is a problem in practice)
> >
> >> or only change with the famebuffer, and I have
> >> to set WINDOW_OFFSET_Y to 0 / fb height depending on the setting of Y
> >> direction (the latter won't work with MRTs, but that's the non-FBO case
> >> anyway)) ?
> > Yes, it could go in theory, and truth is rasterizer state is full of bits
> > that apply to other stages of the pipeline, but the practical hurdle of
> > moving this to pipe_framebuffer is that pipe_framebuffer has no discrete
> > state beyond surfaces so far (it is little more than a tuple of surfaces),
> > so a lot of code would need to be updated to fill, propagate, and consider
> > such state in pipe_framebuffer...
> >
> > I presume your concern is that rasterizer state changes frequently where as
> > framebuffer state changes infrequently, so adding a dependency would cause
> > framebuffer to be processed more often than desired.  You can avoid that
> > by keeping track of the lower_left_origin state independently at
> > nvc0_rasterizer_state_bind:
> >
> > diff --git a/src/gallium/drivers/nvc0/nvc0_state.c
> > b/src/gallium/drivers/nvc0/nvc0_state.c
> > index cba076f..2a6fabf 100644
> > --- a/src/gallium/drivers/nvc0/nvc0_state.c
> > +++ b/src/gallium/drivers/nvc0/nvc0_state.c
> > @@ -324,6 +324,12 @@ nvc0_rasterizer_state_bind(struct pipe_context *pipe,
> > void *hwcso)
> >  
> >     nvc0->rast = hwcso;
> >     nvc0->dirty |= NVC0_NEW_RASTERIZER;
> > +
> > +   if (nvc0->rast &&
> > +       nvc0->lower_left_origin != nvc0->rast->pipe.lower_left_origin) {
> > +      nvc0->lower_left_origin = nvc0->rast->pipe.lower_left_origin;
> > +      nvc0->dirty |= NVC0_NEW_FRAMEBUFFER;
> > +   }
> >  }
> >
> >  static void
> >
> > This means you won't need to validate framebuffer anymore often than
> > strictly necessary. You could also have a new NVC0_NEW_FRAMEBUFFER_ORIGIN
> > flag, just for tidyness.
> >
> >> R600 seems to have PA_SU_VTX_CNTL.PIX_CENTER but no state to change the
> >> window origin / direction ... and I'd rather not have to bother with it
> >> myself either.
> > I need to get this working flawlessly on llvmpipe, but I really see no much
> > need for hw driver developers to rush and get this handled properly.
> > There is probably much bigger fish to fry.
> >
> > If people care enough to devise a state tracker workaround, we could have
> > this on a PIPE_CAP.  I'd be all for it.  But even in that case, I think
> > that nudging the coordinates slightly would probably get the most bang for
> > buck.
> >
> >> Also, note that this state and the pixel center one might (or maybe I
> >> should say will) affect the values of hardware's gl_FragCoord and hence
> >> PIPE_CAP_TGSI_FS_COORD_ORIGIN/PIXEL_CENTER*, i.e. the shader
> >> transformation of that input must be adjusted according to this state.
> >> I'd probably be OK with making this the driver's task.
> > The FS_COORD_PIXEL_CENTER spec in src/gallium/docs/source/tgsi.rst already
> > stated that these are independent:
> >
> >   Note that this does not affect the set of fragments generated by
> >   rasterization, which is instead controlled by gl_rasterization_rules in
> >   the
> >   rasterizer.
> >
> > And I'm not changing the semantics.  That also seems the spirit of
> > GL_ARB_fragment_coord_conventions spec.
> >
> > I wouldn't object to add to Gallium a dependency betwen these state if it
> > helps hw driver developers, but I don't see how we could define it in such
> > way that it would work well for all cases. And I suspect that different
> > hardware probably handles this slightly differently (ie, what is
> > orthogonal to some is not to others).
> 
> I think that drivers can just report all 4 CAPs as supported and do the
> adjustment in the shader themselves (no need for recompilation, just use
> uniforms, the st already does it like that), provided that the state
> tracker actually uses the rasterizer origin bit instead of changing the
> viewport and applies no transformation to the fragment coordinate
> whatsoever.

I'm not sure how much that simplifies in the end. If the drivers need to resort to uniforms to deal with all combinations, then how will making the gl_Fragcoord/viewport transformation depend on lower_left_origin simplify things? 

Is it really true that for all hardware gl_FragCoord will depend on the lower_left_origin rasterizer state?

Finally, I think this is precisely what Marek was concerned; so to allow existing drivers to opt out from having to deal with this, we'll need a cap.


That said, I don't oppose any of this if it make HW driver implementer lives easier.

But how seriously/quickly are you and other hardware drivers maintainers actually aiming at implementing this? I don't wanna go through all that trouble if nobody will care.


Either way, I think that this patch series already is a good improvement over the ugly "one-bit-fit-all-needs" gl_rasterization_rules state, and should cause no regressions whatsoever.  I'd like to tackle the "entanglement" of lower_left_origin with other bits of state in a follow-on gallium change after there is a clearer understanding/consensus if/how will HW implement this.

Jose