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

[Roland Scheidegger]

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
doesn't look like a very strong argument to me (especially since some
apis expose it and some hw supports it per sampler).

Roland