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

[Roland Scheidegger]

Am 20.08.2014 18:12, schrieb Jose Fonseca:
> On 20/08/14 17:02, Roland Scheidegger wrote:
>> Am 20.08.2014 17:47, schrieb Jose Fonseca:
>>> 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
>>> http://msdn.microsoft.com/en-us/library/windows/desktop/ff476202.aspx ,
>>> 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,
>> I think you meant PIPE_TXTURE_2D there?
> 
> No, I expclitely left PIPE_TEXTURE_CUBE due to the reasons I explained
> below.
> 
>>
>>>     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.
>>
>> Yes, but that's not what I'm talking about.
>> Even d3d10, which does not have any distinct cube/array texture layouts
> 
> Precisely.  D3D10 uses D3D10_RESOURCE_DIMENSION_TEXTURE2D for cubes plus
> the D3D10_RESOURCE_MISC_TEXTURECUBE misc flag.  Which is precisely what
> I was talking about when I said " We could also remove PIPE_TEXTURE_CUBE
> and have cube-maps be PIPE_TEXTURE_2D with a flag".
> 
>> (actually 10 still had a separate one for cubes, because there was hw
>> which really required a different layout afaik, but it got abandoned in
>> 10.1),
> 
> No D3D10 doesn't have D3D10_RESOURCE_DIMENSION_TEXTURECUBE.  D3D 10,
> 10.1, and 11, they all use RESOURCE_DIMENSION_TEXTURE2D +
> RESOURCE_MISC_TEXTURECUBE for cubemaps or cubemaps arrays.
You are right that d3d10 did not rally have a cube layout - they
required the misc_texturecube flag as you said (this is really the same
thing to me if you have a 2d + flag or explicit cube).
But this flag is more or less dead and buried with newer d3d versions,
it is there for compatibility reasons (with api level 10_0 you still
need it for cube maps, since it didn't allow the target casting from
cube to 2d array or vice versa later in the resource view). It was never
used for cube map arrays.


> 
>> still requires shader resource views to have them (and they must
>> match what's declared in the shader):
>> http://msdn.microsoft.com/en-us/library/windows/desktop/ff476211%28v=vs.85%29.aspx
>>
> 
> Right, there are different enums for resource types and view types.
> 
>> So, my guess is we should do the same - just have that type in the
>> sampler view (and drivers wishing to support the extension must take the
>> type from the view, and not the underlying resource - or they could get
>> it from the shader itself, presumably, if they really wanted, this is
>> actually what we do for texture size queries in llvmpipe, but it's more
>> of a necessary hack).
>>
>> You are right though we would not really require distinct types at the
>> resource level, but they don't really get in the way neither.
> 
> Yes, we could do the same. But I do think that in that case we should
> have a separate enum for views different from pipe_texture_target.  And
> pipe_texture_target would be slim down.
Yes that would make sense. I'm not sure it's worth the trouble of
changing the code though.

> 
> 
> But if you remove PIPE_TEXTURE_CUBE from pipe_texture_target you'll need
> to pass that info in a new flag (like D3D10_RESOURCE_MISC_TEXTURECUBE).
>  I don't feel strongly, but I'm not sure this is much more elegant than
> keeping PIPE_TEXTURE_CUBE around.
Ok understood PIPE_TEXTURE_CUBE would have to stay (as there's hw which
needs to know the distinction to an array).

So I guess we could do:
1) slim the pipe_texture_target enum down and use a different
pipe_view_target (or whatever) in the sampler view which has all values,
and allow all required casts (those involving cubes are probably the
only ones which really need to be restricted to drivers supporting
ARB_texture_view). It requires changing all drivers, state trackers to
the new scheme though.
2) just keep the pipe_texture_target enum the same and use it for both
resource and sampler views. The advantage here is that you don't need to
rewrite any driver code for now, unless you want to expose the new
ARB_texture_view capabilities (and for old state tracker code to still
work it should be enough to fill in a default view_target just as is
done for format).
3) keep the pipe_texture_target enum for now but use a new
pipe_view_target, with the intention of slimming pipe_texture_target
down later (when just using it at the resource level, it is now
definitely misnamed).

Roland