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

[Marek Olšák]

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.

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);

Marek