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

Sun Apr 21 07:30:58 PDT 2013

Some suggestions for the name:

lower_left_edge_rule
lower_left_rasterization_edge_rule
gl_edge_rule
gl_rasterization_edge_rule

In this case, the name is not as important as the documentation which
defines the behavior of the state.

Marek

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