On Thu, 2010-06-10 at 12:23 -0700, Roland Scheidegger 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...
OK - if you want to defer removing it, that's ok - it's not really related to array textures. Maybe just add a deprecated comment and get back to it later. Keith _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
