[Mesa-dev] RFC: array textures in gallium and assorted cleanups

[Keith Whitwell]

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