On 10.06.2010 20:32, 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.
Actually, this turns out to be quite problematic. It really doesn't look like the right abstraction at some places. Things like flush_frontbuffer use pipe_surface, looks to me like they should just use the pipe_resource instead (maybe hardcoded for level 0, layer 0 - I'm sceptical anything else ever worked anyway)? Or is something else entirely needed (that is a different "screen->get_dumb_2d_surface" function)? It just seems like the hacks to use a context for getting a pipe_surface which shouldn't really be needed in the first place are just making things a mess. Roland _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
