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

Sun Apr 21 06:56:46 PDT 2013

----- Original Message -----
> I have managed to make the triangle-rasterization test pass. Let's
> forget about what the origin is, because it's not really important.
> What is actually important is what happens when an edge falls exactly
> on a sample point. Radeons have a state which determines what happens
> for the left, right, top, and bottom edge, and it does not affect the
> coordinate system, which is always top-left. So the issue is fixable
> for radeon drivers as long as the origin is always top-left (i.e. no
> changes are made to the viewport and scissor states).
> 
> Registers:
> r300 - SC_EDGERULE
> r600 - PA_SC_EDGERULE
> 
> Marek
> 

That's great.

Spite the name, this was precisely the intent of "lower_left_origin" -- whether top edges (horizontal edges exactly along the sample point) are inclusive or exclusive.  I'm open for better name suggestions.

Jose

> On Sun, Apr 21, 2013 at 2:35 PM, Jose Fonseca <jfonseca at vmware.com> wrote:
> >
> >
> > ----- 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
> 