[Mesa-dev] [PROPOSAL] gallium: move state enable bits from clip_state to rasterizer_state

[Jose Fonseca]

----- Original Message -----
> On 17.12.2011 20:54, Marek Olšák wrote:
> > On Sat, Dec 17, 2011 at 4:45 PM, Christoph Bumiller
> > <e0425955 at student.tuwien.ac.at> wrote:
> >> On 17.12.2011 15:45, Marek Olšák wrote:
> >>> ---
> >>>
> >>> This was suggested by Keith Whitwell in this email addressed to
> >>> me on 8/6/2010:
> >>> http://lists.freedesktop.org/archives/mesa-dev/2010-August/001810.html
> >>>
> >>>
> >>>  src/gallium/include/pipe/p_defines.h |    2 +-
> >>>  src/gallium/include/pipe/p_state.h   |   14 ++++++++++++--
> >>>  2 files changed, 13 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/src/gallium/include/pipe/p_defines.h
> >>> b/src/gallium/include/pipe/p_defines.h
> >>> index 800a04c..c441a1f 100644
> >>> --- a/src/gallium/include/pipe/p_defines.h
> >>> +++ b/src/gallium/include/pipe/p_defines.h
> >>> @@ -453,7 +453,7 @@ enum pipe_cap {
> >>>     PIPE_CAP_TGSI_FS_COORD_ORIGIN_LOWER_LEFT = 38,
> >>>     PIPE_CAP_TGSI_FS_COORD_PIXEL_CENTER_HALF_INTEGER = 39,
> >>>     PIPE_CAP_TGSI_FS_COORD_PIXEL_CENTER_INTEGER = 40,
> >>> -   PIPE_CAP_DEPTH_CLAMP = 41,
> >>> +   PIPE_CAP_DEPTH_CLIP_DISABLE = 41,
> >>>     PIPE_CAP_SHADER_STENCIL_EXPORT = 42,
> >>>     PIPE_CAP_TGSI_INSTANCEID = 43,
> >>>     PIPE_CAP_VERTEX_ELEMENT_INSTANCE_DIVISOR = 44,
> >>> diff --git a/src/gallium/include/pipe/p_state.h
> >>> b/src/gallium/include/pipe/p_state.h
> >>> index f943ca5..3aedada 100644
> >>> --- a/src/gallium/include/pipe/p_state.h
> >>> +++ b/src/gallium/include/pipe/p_state.h
> >>> @@ -127,6 +127,18 @@ struct pipe_rasterizer_state
> >>>      */
> >>>     unsigned rasterizer_discard:1;
> >>>
> >>> +   /**
> >>> +    * When false, depth clipping is disabled and the depth value
> >>> will be
> >>> +    * clamped later at the per-pixel level before depth testing.
> >>> +    * This depends on PIPE_CAP_DEPTH_CLIP_DISABLE.
> >>> +    */
> >>> +   unsigned depth_clip:1;
> >>> +
> >>> +   /**
> >>> +    * Enable bits for user clip planes.
> >>> +    */
> >>> +   unsigned user_clip_plane_enable:PIPE_MAX_CLIP_PLANES;
> >>> +
> >> The first is fine, but I don't like having user clip plane enables
> >> here,
> >> can't make them part of a hardware state buffer since whether a
> >> clip
> >> plane is enabled or not also depends on the vertex (or domain or
> >> geometry) shader, using both UCPs and gl_ClipDistance at the same
> >> time
> >> doesn't work.
> > I don't expect anyone to use both. Anyway, the user clip planes are
> > fixed-function and neither r300 nor r600 clip state has any
> > dependency
> > on shaders. The UCP state is pretty much separate from everything
> > else. I'd like to keep depth_clip and UCP enables together, because
> > they are set via the same register on r600.
> 
> *Sigh*.
> Fine then, I think it doesn't really matter for nv50, so I'm not
> opposing the change, do it if it helps r600.

I don't oppose either. I don't think it will hamper any driver.

> > The problem with pipe_clip_state is that if I want to enable UCPs,
> > I
> > must also set the clip planes = 32 floats at most. I can't re-use
> > the
> > 32 floats which were set last time.
> >
> >> The rasterizer cso is already so large ... if we keep going like
> >> this at
> >> some point you have re-validate everything when the rasterizer CSO
> >> changes, because of interdependencies, and we'll get larger and
> >> larger
> >> numbers of rasterizer CSOs with slow lookup and hashing.
> > I double-checked and there should be no change in the size of
> > pipe_rasterizer_state with the patch. The state currently has 3
> > dwords
> > + 5 floats. However, some or all of the floats could be put in its
> > own
> > state struct, because they are changed at a pretty low frequency.
> > For
> > example:
> >
> > struct pipe_poly_offset_state {
> >    float offset_units;
> >    float offset_scale;
> >    float offset_clamp;
> > };
> >
> > pipe->set_poly_offset_state(pipe, &poly_offset_state);
> 
> And keeping the enable bits in the rasterizer ? Yes, that would be
> the
> same way we treat stencil ref, and speed up the hashing.
> Also, depth_clip and poly_offset are somewhat related (at least more
> than depth_clip and clip planes), so keeping the enable bits of both
> in
> the rasterizer state seems nicer.

I think this is a good idea in general too. As Keith said, having floating point status being parameters of CSOs.

> I don't expect people to use many different combinations of values
> there
> though.

Normal apps don't, but test suites do. llvmpipe internally converts fixed function parameters to LLVM IR, but floating point state is passed as parameters to generated code, to avoid recompilations.

Jose