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

[Roland Scheidegger]

Am 20.08.2014 22:27, schrieb 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...)
> 
There are subtle differences between that unfortunately, though I don't
know if it would matter for any GL version. For instance, the return
value of texture size queries is one for the layer parameter of an array
texture with one layer, but at least d3d10 wants to see a zero for a
non-array texture (view) (this is the reason why we use the target
parameter from the shader and not from the resource for texture size
queries in llvmpipe).
Also, it definitely matters for cube map arrays - granted if you'd
create them as a cube texture rather than a plain 2d array (as d3d10.1
and up do) you could still guess that.
I am not actually completely sure a view target is absolutely required,
some drivers might be able to just use the information from the shader
dcl as easily but I'm not convinced that's true for everybody, and it's
probably not a good idea to just rely on that.

Roland