[Mesa-dev] RFC: array textures in gallium and assorted cleanups

[Roland Scheidegger]

On 10.06.2010 17:12, Keith Whitwell wrote:
> On Thu, 2010-06-10 at 07:29 -0700, Brian Paul wrote:
>> Keith Whitwell wrote:
>>> On Thu, 2010-06-10 at 07:08 -0700, Roland Scheidegger wrote:
>>>> On 10.06.2010 11:30, Keith Whitwell wrote:
>>>>> On Thu, 2010-06-03 at 13:26 -0700, Roland Scheidegger wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I've created a new branch gallium-array-textures which has some more
>>>>>> interface changes, this time to support array textures basically.
>>>>>> Nothing has been adapted to these changes yet (I'll do that it should be
>>>>>> mostly trivial as long as array textures aren't actually supported by
>>>>>> the driver or even mesa state tracker), but now would be a good time if
>>>>>> you have some comments for the proposed interface changes.
>>>>>>
>>>>>> Roland
>>>>> Roland,
>>>>>
>>>>> This looks great!
>>>>>
>>>>> Couple of comments -- you're now using the term "layer" in various
>>>>> places, but there is no strong definition of what that means - except in
>>>>> the patch comment, which isn't useful once the patch is committed.  Can
>>>>> you define this term somewhere in the documentation?
>>>> Ok will do.
>>>>
>>>>> Also, there are a couple of things that look like typos in the interface
>>>>> change diff, but I'm sure you'll find those the first time you try to
>>>>> compile this.  eg:
>>>>>
>>>>>     void (*resource_copy_region)(struct pipe_context *pipe,
>>>>>                                  struct pipe_resource *dst,
>>>>> -                                struct pipe_subresource subdst,
>>>>> +                                unsigned level,
>>>>>                                  unsigned dstx, unsigned dsty, unsigned dstz,
>>>>>                                  struct pipe_resource *src,
>>>>> -                                struct pipe_subresource subsrc,
>>>>> -                                unsigned srcx, unsigned srcy, unsigned srcz,
>>>>> -                                unsigned width, unsigned height);
>>>>> +                                unsigned level,
>>>>> +                                const struct pipe_box *);
>>>>>  
>>>>> It seems like you end up with two parameters named "level" ??
>>>> Yes, I had already fixed this locally.
>>>> create_surface also had a bug (still got passed pipe_screen instead of
>>>> pipe_context since it moved to context), as well as I need to store the
>>>> context itself in pipe_surface (much like pipe_sampler_view does).
>>>> That actually was a bit non-trivial since some state trackers don't
>>>> really have a context handy when they called the former
>>>> get_tex_surface() (glx, wgl and so on statetrackers not the rendering
>>>> ones). Some of them did, though, already have their own context (for
>>>> resource_copy_region, for instance) so I'm about to do this in a similar
>>>> fashion.
>>>> Actually, I was wondering if surface_destroy() should also get passed in
>>>> a context - seems strange since it already stores the context, but this
>>>> is exactly what sampler_view_destroy() does, which I'd like to see as a
>>>> very analogous function.
>>> Yes, it should take a context, mainly for consistency.  It helps when
>>> wrapping/unwrapping these functions to have a consistent interface.  
>> Yes.  The other reason is you have to be careful with objects that 
>> save context pointers when those objects might be shared among 
>> multiple contexts.
>>
>> If object A is created by context C1 and shared with context C2 and C1 
>> gets destroyed, we're in trouble if we use A's stale context pointer. 
>>   It's safer to use the context pointer that's passed to the function.
>>
>> I fixed a bug along those lines a couple months ago.  See 
>> st_DeleteTextureObject().
> 
> Anything created by a context in gallium is private to that context.
> The shareable entities are created in the screen.  In effect, Roland's
> change makes surfaces private to the context.
> 
> That may have effects elsewhere, eg in the mesa state tracker, which may
> be relying on sharing surfaces (aka render_target_views,
> depth_stencil_views) between contexts.

I am actually wondering if we should have some different abstraction for
"surfaces" which are used for presents etc. Clearly, the glx etc. state
trackers have no intention for using these pipe_resources as render
attachment points, hence it's not really the right abstraction.
But I guess that can be figured out later.

There's something else which is a bit ugly currently. pipe_surface
includes an offset. Clearly, when talking about surfaces that span
several layers, that doesn't really make a whole lot of sense - the view
itself includes all information to figure out what the offset should
finally be when actually rendering to the surface (either the hardware
does this fully on its own or the driver can do it as necessary). The
layout member in pipe_surface also looks a bit out of place if
pipe_surface is just a view into a resource for rendering purposes.

I actually wondered a long time ago why the i965 driver (both gallium
and classic) goes to great length figuring out offsets, tiling bits,
etc. when rendering to a specific mip level, face, zslice (ok it fails
at zslices, and might fail at some mip levels on some hw). Since you can
apparently just program the hardware to do this all on its own - much
easier and should also handle 3d textures just fine. But ultimately it
can't work since in "old" OpenGL rendering to a slice of a 3d texture
really turns this into a 2d surface, hence you can attach it alongside
other 2d surfaces (for example, "normal" 2d depth buffer and slice of 3d
texture as color attachment). But the hardware requires all resources to
be of the same type (and same width, height,...) for this to work... As
far as I can tell, this is something which should not be an issue in
DX10 though documentation is somewhat vague on some points. With OpenGL
layered attachments, same resource type is required too - though I can't
see any requirement of same size, hence the i965 probably can't do this
neither. Anyway, the exact details of what a driver is expected to
handle in terms of multiple pipe_surface attachments if it supports
array textures still need to be figured out.

Roland