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. Clearly, when talking about surfaces that span several layers, that doesn't really make a whole lot of sense - the view itself includes all information to figure out what the offset should finally be when actually rendering to the surface (either the hardware does this fully on its own or the driver can do it as necessary). The layout member in pipe_surface also looks a bit out of place if pipe_surface is just a view into a resource for rendering purposes. I actually wondered a long time ago why the i965 driver (both gallium and classic) goes to great length figuring out offsets, tiling bits, etc. when rendering to a specific mip level, face, zslice (ok it fails at zslices, and might fail at some mip levels on some hw). Since you can apparently just program the hardware to do this all on its own - much easier and should also handle 3d textures just fine. But ultimately it can't work since in "old" OpenGL rendering to a slice of a 3d texture really turns this into a 2d surface, hence you can attach it alongside other 2d surfaces (for example, "normal" 2d depth buffer and slice of 3d texture as color attachment). But the hardware requires all resources to be of the same type (and same width, height,...) for this to work... As far as I can tell, this is something which should not be an issue in DX10 though documentation is somewhat vague on some points. With OpenGL layered attachments, same resource type is required too - though I can't see any requirement of same size, hence the i965 probably can't do this neither. Anyway, the exact details of what a driver is expected to handle in terms of multiple pipe_surface attachments if it supports array textures still need to be figured out. Roland _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
