On Fri, 2010-06-11 at 06:04 -0700, Roland Scheidegger wrote: > 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.
"flush_frontbuffer" is such a strange function. It's really meant to mean "present", and having it take a resource and subresource information would be entirely appropriate. Keith _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
