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

[Marek Olšák]

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

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