[Mesa-dev] [PATCH 2/6] gallium: implement ARB_seamless_cube_map

[Roland Scheidegger]

Am 02.05.2011 19:20, schrieb Marek Olšák:
> On Mon, May 2, 2011 at 5:53 PM, Brian Paul <brianp at vmware.com> wrote:
>> On 05/02/2011 08:40 AM, Marek Olšák wrote:
>>>
>>> On Mon, May 2, 2011 at 3:47 PM, Brian Paul<brianp at vmware.com>  wrote:
>>>>
>>>> On 05/02/2011 07:03 AM, Marek Olšák wrote:
>>>>>
>>>>> ---
>>>>>  src/gallium/include/pipe/p_defines.h        |    1 +
>>>>>  src/gallium/include/pipe/p_state.h          |    1 +
>>>>>  src/mesa/state_tracker/st_atom_rasterizer.c |    6 +++++-
>>>>>  src/mesa/state_tracker/st_extensions.c      |    4 ++++
>>>>>  4 files changed, 11 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/src/gallium/include/pipe/p_defines.h
>>>>> b/src/gallium/include/pipe/p_defines.h
>>>>> index 431a7fb..176fee9 100644
>>>>> --- a/src/gallium/include/pipe/p_defines.h
>>>>> +++ b/src/gallium/include/pipe/p_defines.h
>>>>> @@ -465,6 +465,7 @@ enum pipe_cap {
>>>>>     PIPE_CAP_VERTEX_ELEMENT_INSTANCE_DIVISOR = 44,
>>>>>     PIPE_CAP_FRAGMENT_COLOR_CLAMP_CONTROL = 45,
>>>>>     PIPE_CAP_MIXED_COLORBUFFER_FORMATS = 46,
>>>>> +   PIPE_CAP_SEAMLESS_CUBE_MAP = 47
>>>>>  };
>>>>>
>>>>>  /* Shader caps not specific to any single stage */
>>>>> diff --git a/src/gallium/include/pipe/p_state.h
>>>>> b/src/gallium/include/pipe/p_state.h
>>>>> index 0c1f509..26e8a8e 100644
>>>>> --- a/src/gallium/include/pipe/p_state.h
>>>>> +++ b/src/gallium/include/pipe/p_state.h
>>>>> @@ -101,6 +101,7 @@ struct pipe_rasterizer_state
>>>>>     unsigned line_smooth:1;
>>>>>     unsigned line_stipple_enable:1;
>>>>>     unsigned line_last_pixel:1;
>>>>> +   unsigned seamless_cube_map:1;
>>>>
>>>> Shouldn't this be sampler state and not rasterizer state?  The AMD
>>>> extension
>>>> lets this option be set per texture.  Though, I don't know if non-AMD
>>>> hardware supports that mode.
>>>
>>> r600 and r700 hardware has one enable bit that affects the entire
>>> chip. There is no per-sampler enable bit.
>>>
>>> evergreen and later hardware indeed has a per-sampler enable bit.
>>>
>>> I wouldn't like to limit the extension to evergreen, so
>>> pipe_rasterizer_state seemed to be the only choice.
>>
>> Well, we could still put the bit in pipe_sampler_state and make sure it's
>> either set or unset in all active sampler objects.  It shouldn't be a
>> problem in the state tracker.
>>
>> It just feels wrong to put sampler-related state in the rasterizer struct.
>>  And when the day comes that we do want per-texture/sampler seamless cube
>> map, the flag will already be in the right place and not in two different
>> places.
> 
> Even though the state only affects samplers, it's a global GPU state
> on r600. (so maybe pipe_texture_misc_state?) It would be pretty
> awkward to read one field from e.g. fragment sampler state 0 and use
> it for all vertex, geometry, and fragment samplers.
> 
> It would be best for all GPUs to have both the per-sampler state of
> seamless cubemap (evergreen and i965 friendly) and the global state
> bit (r600 and nvidia friendly). So my alternative solution is:
> 
> unsigned pipe_sampler_state::seamless_cube_map:1; // for the AMD extension
> 
> // for the ARB extension (because people think
> // putting it in rasterizer_state is awkward)
> struct pipe_texture_misc_state {
>    unsigned seamless_cube_map:1
> };

I don't like that. If you want to make that global state, sticking that
into rasterizer state is just fine imho even though it isn't exactly
rasterizer state.
But I don't like the idea of having that state in two different places,
you just shift the work from the state tracker to the drivers (for those
supporting both).
I guess in some ways that's similar to point sprite coords which also
have local and global enables. But I don't think just sticking
PIPE_MAX_SAMPLERS seamless_cube_map bits in pipe_rasterizer state is the
solution neither...

Roland