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. Keith _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
