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

[Marek Olšák]

On Mon, May 2, 2011 at 11:35 PM, Roland Scheidegger <sroland at vmware.com> wrote:
> Am 02.05.2011 20:26, schrieb Marek Olšák:
>> On Mon, May 2, 2011 at 7:49 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>>> 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).
>>
>> Sorry, I wasn't clear enough. I do want to shift the work from drivers
>> to state trackers, because this mess is strictly OpenGL-specific. The
>> idea is:
>>
>> If PIPE_CAP_SEAMLESS_CUBE_MAP_PER_TEXTURE is 1, the driver should
>> follow the per-sampler state and *ignore* the global one.
>> Otherwise, if PIPE_CAP_SEAMLESS_CUBE_MAP is 1, the driver should
>> follow the global state. We can go as far as to require that only one
>> of the two CAPs can be exposed.
>>
>> The Mesa state tracker will have to set the states accordingly.
>>
>> Anything else will just make hardware drivers messy.
>
> Ah I guess that's an option too. Can't say I like it too much but then
> again I don't like the other options too much neither - though I'd
> probably prefer just having it in the sampler state even if it means
> some drivers have to get the information in an awkward way. This looks
> like it fits well alongside other texture filtering options - just
> because some hw can't do it per sampler and some apis don't require it

Actually, *all* APIs don't require it per sampler. It's just the
*optional* AMD extension which exposes it and which we don't have to
implement, and I seriously doubt any app will use that.

BTW, I have pushed all the patches which had been acked. I take back
all the patches related to seamless_cube_map in gallium and am going
to make new ones.

Marek