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

[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
};

In struct pipe_context:

void *(*create_texture_misc_state)(struct pipe_context*, struct
pipe_texture_misc_state*);
void (*bind_texture_misc_state)(struct pipe_context*, void*);
void (*delete_texture_misc_state)(struct pipe_context*, void*);

New caps:
PIPE_CAP_SEAMLESS_CUBE_MAP
PIPE_CAP_SEAMLESS_CUBE_MAP_PER_TEXTURE

What do you think?

Best regards
Marek