On Thu, Jun 10, 2010 at 3:23 PM, Roland Scheidegger <[email protected]> wrote: > On 10.06.2010 21:14, Keith Whitwell wrote: >> On Thu, 2010-06-10 at 11:32 -0700, 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. >>> >>> There's something else which is a bit ugly currently. pipe_surface >>> includes an offset. >> >> That's bogus & left over from some distant past. Just remove it. > > It is used in a lot of places still. Granted if drivers want to > precalculate that they should do that in a driver specific subclass of > pipe_surface, but the change in fact is already huge as-is... > > Roland
Offset and pitch/stride are/were both useful to nouveau (or maybe just nvfx). After pitch/stride was removed pipe_surface was subclassed to get it back so it's not a big deal to add offset. _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
