[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 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).

Jose