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

[Christoph Bumiller]

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.

> 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 don't expect people to use many different combinations of values there
though.

>
> Marek
>