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

Younes Manton younes.m at gmail.com
Thu Jun 10 14:17:47 PDT 2010


On Thu, Jun 10, 2010 at 3:23 PM, Roland Scheidegger <sroland at vmware.com> wrote:
> On 10.06.2010 21:14, Keith Whitwell wrote:
>> On Thu, 2010-06-10 at 11:32 -0700, Roland Scheidegger wrote:
>>> 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.
>>
>> That's bogus & left over from some distant past.  Just remove it.
>
> It is used in a lot of places still. Granted if drivers want to
> precalculate that they should do that in a driver specific subclass of
> pipe_surface, but the change in fact is already huge as-is...
>
> Roland

Offset and pitch/stride are/were both useful to nouveau (or maybe just
nvfx). After pitch/stride was removed pipe_surface was subclassed to
get it back so it's not a big deal to add offset.


More information about the mesa-dev mailing list