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. Wondering if there's a good reason why it both stores the context and still needs one passed upon destruction. In any case, the former tex_surface_destroy() didn't have a (then screen) parameter. > > Otherwise, it looks like a nice cleanup in addition to the new > functionality. Thanks for the review, Roland _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
