[Mesa-dev] [PATCH 2/2] mesa/st: add ARB_texture_view support

[Ilia Mirkin]

On Wed, Aug 20, 2014 at 4:12 PM, Roland Scheidegger <sroland at vmware.com> wrote:
> Am 20.08.2014 18:48, schrieb Roland Scheidegger:
>> Am 20.08.2014 18:33, schrieb Ilia Mirkin:
>>> On Wed, Aug 20, 2014 at 12:22 PM, Jose Fonseca <jfonseca at vmware.com> wrote:
>>>> On 20/08/14 17:14, Roland Scheidegger wrote:
>>>>>
>>>>> Am 20.08.2014 17:55, schrieb Ilia Mirkin:
>>>>>>
>>>>>> On Wed, Aug 20, 2014 at 11:47 AM, Jose Fonseca <jfonseca at vmware.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> On 20/08/14 16:31, Ilia Mirkin wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hm, it's not tested. And you're right, that would (most likely) mess
>>>>>>>> up, since it would only have the pipe_resource's target. Any
>>>>>>>> suggestions on how to fix it? Should the target be added to
>>>>>>>> pipe_sampler_view?
>>>>>>>>
>>>>>>>> On Wed, Aug 20, 2014 at 11:25 AM, Roland Scheidegger
>>>>>>>> <sroland at vmware.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Didn't look at it that closely, but I'm pretty surprised this really
>>>>>>>>> works. One things ARB_texture_view can do is cast cube maps (and cube
>>>>>>>>> map arrays) to 2d arrays and vice versa (also 1d/2d to the respective
>>>>>>>>> array type), and we cannot express that in sampler views (yet) (we
>>>>>>>>> can't
>>>>>>>>> express it in surfaces neither but there it should not matter). Which
>>>>>>>>> means the type used in the shader for sampling will not match the
>>>>>>>>> sampler view, which sounds quite broken to me.
>>>>>>>>>
>>>>>>>>> Roland
>>>>>>>>>
>>>>>>>
>>>>>>> Probably the only sane thing to do eliminate the disctinction between
>>>>>>> PIPE_TEXTURE_FOO and PIPE_TEXTURE_FOO_ARRAY like  in
>>>>>>>
>>>>>>> https://urldefense.proofpoint.com/v1/url?u=http://msdn.microsoft.com/en-us/library/windows/desktop/ff476202.aspx&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=x03OmuVWAQgfFbsFB2SLMLwSYavxkU8Zsypu9lEIkpg%3D%0A&s=4b0ca75d91e6d53d92658d9e334cf2a73a01efe5667464c969f5c085409052ff
>>>>>>> ,
>>>>>>> e.g.,:
>>>>>>>
>>>>>>> enum pipe_texture_target {
>>>>>>>     PIPE_BUFFER           = 0,
>>>>>>>     PIPE_TEXTURE_1D       = 1,
>>>>>>>     PIPE_TEXTURE_2D       = 2,
>>>>>>>     PIPE_TEXTURE_3D       = 3,
>>>>>>>     PIPE_TEXTURE_CUBE     = 4, // Must have same layout as
>>>>>>> PIPE_TEXTURE_2D
>>>>>>>     PIPE_TEXTURE_RECT     = 5,
>>>>>>>     PIPE_TEXTURE_1D_ARRAY = PIPE_TEXTURE_1D,
>>>>>>>     PIPE_TEXTURE_2D_ARRAY = PIPE_TEXTURE_2D,
>>>>>>>     PIPE_TEXTURE_CUBE_ARRAY = PIPE_TEXTURE_CUBE,
>>>>>>>     PIPE_MAX_TEXTURE_TYPES
>>>>>>> };
>>>>>>>
>>>>>>>
>>>>>>> We could also remove PIPE_TEXTURE_CUBE and have cube-maps be
>>>>>>> PIPE_TEXTURE_2D
>>>>>>> with a flag, but that's probably a lot of work. Instead, drivers that
>>>>>>> want
>>>>>>> to be able to support ARB_texture_view will need to ensure
>>>>>>> PIPE_TEXTURE_CUBE/PIPE_TEXTURE_2D layout match.
>>>>>>
>>>>>>
>>>>>> Another quick + cheap alternative (at least looking at nv50/nvc0 code)
>>>>>> would be to pass a separate target parameter to
>>>>>> ->create_sampler_view(). That would be enough for nouveau, but perhaps
>>>>>> not more generally? Take a look at nv50_tex.c:nv50_create_texture_view
>>>>>> -- it also needs to work out the depth of the texture (presumably to
>>>>>> deal with out-of-bounds accesses) and that is written to the texture
>>>>>> info structure.
>>>>>
>>>>> Well that should be enough, but I don't think it fits out design.
>>>>
>>>>
>>>>
>>>>> We've
>>>>>
>>>>> encapsulated other "override" information like the format in the view
>>>>> already, and I see no reason why the target "cast" should be treated any
>>>>> different.
>>>>
>>>>
>>>> In other words, you're arguing for:
>>>>
>>>> diff --git a/src/gallium/include/pipe/p_state.h
>>>> b/src/gallium/include/pipe/p_state.h
>>>> index a82686b..c87ac4e 100644
>>>> --- a/src/gallium/include/pipe/p_state.h
>>>> +++ b/src/gallium/include/pipe/p_state.h
>>>> @@ -333,6 +333,7 @@ struct pipe_surface
>>>
>>> On struct pipe_sampler_view, I thought... unless I'm misunderstanding.
>>> This was also my first thought about fixing this after Roland pointed
>>> out the issue.
>> Yes definitely for pipe_sampler_view - d3d10 also has it on the render
>> target / depth stencil views, though so far I'm not convinced there's
>> any value in that (the addressing of cube maps / arrays, 1d / 1d arrays
>> is entirely the same in all cases, what matters is really the first and
>> last layer only).
>>
>>>
>>>>     struct pipe_reference reference;
>>>>     struct pipe_resource *texture; /**< resource into which this is a view
>>>> */
>>>>     struct pipe_context *context; /**< context this surface belongs to */
>>>> +   enum pipe_texture target;
>> Make it pipe_texture_target target ;-)
>>
>>
>>>>     enum pipe_format format;
>>>>
>>>>     /* XXX width/height should be removed */
>>>>
>>>>
>>>> It's a fair point.  And I don't object that solution.
>>>>
>>>> Of course, for this to work, drivers will need to treat the _ARRAY and non
>>>> _ARRAY targets the same when determining the texture layout for this to
>>>> work.
>>>>
>>>>
>>>> I just felt this would be a good oportunity to slim down pipe_texture_target
>>>> too.  I'm not sure the _ARRAY distinction still matters at this level, but I
>>>> suppose it doesn't hurt.
>>>
>>> Such a cleanup would probably have to be done by someone with a better
>>> understanding of gallium than me. OTOH if you guys feel like doing it
>>> the sampler_view way will accrue too much technical debt, that's fine
>>> too. Unless I hear otherwise, I'm going to try to do it the
>>> pipe_sampler_view way tonight.
>>>
>>
>> Yes I think it would be a nice cleanup to split it up into two enums. I
>> was mostly proposing just reusing the same enum and keeping
>> pipe_texture_target the same because it would require less code change.
>> But maybe that could come back haunting us later, I agree it would be
>> cleaner if they are separated.
>>
>>
> OTOH I guess if we'd kill the arrays on the resource side, at least for
> gl we could not figure out the default view without poking into the
> corresponding gl object. Might not be much of a problem though.

You could look at ->array_size, no? (And the difference between a 2d
texture and a 2d array texture with 1 array element is somewhat
academic...)