On 10.06.2010 16:16, 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. Ok will add that then.
Roland _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
