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

[Brian Paul]

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().

-Brian