----- Original Message -----
> Am 07.09.2011 02:00, schrieb Stéphane Marchesin:
> > 2011/9/6 Roland Scheidegger <srol...@vmware.com>:
> >> Am 07.09.2011 00:01, schrieb Stéphane Marchesin:
> >>> 2011/9/3 Jose Fonseca <jfons...@vmware.com>:
> >>>>
> >>>>
> >>>> ----- Original Message -----
> >>>>> 2011/9/2 Stéphane Marchesin <stephane.marche...@gmail.com>:
> >>>>>> 2011/9/2 Jose Fonseca <jfons...@vmware.com>:
> >>>>>>> ----- Original Message -----
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> While debugging some code I ran across the following
> >>>>>>>> situation:
> >>>>>>>>
> >>>>>>>> - pipe_context c1 is created
> >>>>>>>> - pipe_surface s1 is created
> >>>>>>>> - strb-> surface is set to s1 (s1's refcount goes up)
> >>>>>>>> - pipe_context c1 is destroyed
> >>>>>>>> - strb is destroyed
> >>>>>>>> - strb->surface is destroyed (so s1's refcount is now 0 and
> >>>>>>>> we
> >>>>>>>> want
> >>>>>>>> to
> >>>>>>>> destroy it)
> >>>>>>>>
> >>>>>>>> At that point s1 references c1 which is not available any
> >>>>>>>> more,
> >>>>>>>> so
> >>>>>>>> when we try to call ctx->surface_destroy to destroy s1 we
> >>>>>>>> crash.
> >>>>>>>>
> >>>>>>>> We discussed this a bit on IRC, and we agreed that the
> >>>>>>>> proper
> >>>>>>>> solution, since surfaces outlive their context, is to make
> >>>>>>>> surfaces
> >>>>>>>> screen-bound instead. I'm going to implement that unless
> >>>>>>>> someone
> >>>>>>>> objects.
> >>>>>>>>
> >>>>>>>> As a side note, the same issue will happen with
> >>>>>>>> sampler_views, so
> >>>>>>>> it'll get a similar fix.
> >>>>>>>
> >>>>>>> Sampler views and surfaces were previously objects bound to
> >>>>>>> screen, and we changed that because of poor multithreading
> >>>>>>> semantics. Per-context sampler views / render targets
> >>>>>>> actually
> >>>>>>> matches the 3D APIs semantics better, so I don't think that
> >>>>>>> reverting is the solution.
> >>>>>>>
> >>>>>>> It looks to me that the issue here is that pipe_context
> >>>>>>> should not
> >>>>>>> be destroyed before the surfaces. strb->surface should only
> >>>>>>> be
> >>>>>>> used by one context, and should be destroyed before that
> >>>>>>> context
> >>>>>>> is destroyed.
> >>>>>>>
> >>>>>>> IIUC, strb matches GL renderbuffer semantics and can be
> >>>>>>> shared by
> >>>>>>> multiple context. If so, strb is treating pipe_surfaces as a
> >>>>>>> entity shareable by contexts when really shouldn't.
> >>>>>>>
> >>>>>>> The solution is:
> >>>>>>> - strb can only have pipe_resources, plus the key for the
> >>>>>>> surface
> >>>>>>> (face, level, etc)
> >>>>>>> - the pipe_surfaces that are derived should be stored/cached
> >>>>>>> in
> >>>>>>> the GLcontext.
> >>>>>>> - when the GLcontext / pipe_context is being destroy, the
> >>>>>>> pipe
> >>>>>>> surfaces can be destroyed before
> >>>>>>>
> >>>>>>
> >>>>>> I don't understand some of it. From what I see, it should be
> >>>>>> enough,
> >>>>>> whenever strb binds a surface, to add a pointer to this strb
> >>>>>> to a
> >>>>>> list of strb's to the pipe_context. By doing that, we would be
> >>>>>> able
> >>>>>> to
> >>>>>> unbind the surfaces from the strb before we destroy the
> >>>>>> context.
> >>>>>> However, pipe_context structures can't reference gl
> >>>>>> structures, so
> >>>>>> how
> >>>>>> would you solve that?
> >>>>>>
> >>>>>
> >>>>> Alright, maybe I'm too tired, I just have to put it in strb...
> >>>>> My
> >>>>> other questions still stand though :)
> >>>>>
> >>>>> Stéphane
> >>>>>
> >>>>>
> >>>>>> Also, what difference does it make if strb's only have
> >>>>>> pipe_resources?
> >>>>
> >>>> Pipe resources can be shared between contexts, therefore they
> >>>> should not refer context or context data.
> >>>> So it is always safe to destroy pipe_resources pipe_contexts on
> >>>> any order.
> >>>>
> >>>> Using your example above, if you replace surface "s1" with
> >>>> resource "r1", a reference from r1 to c1 would be violating the
> >>>> semantics.
> >>>>
> >>>>>> And why do I need a key?
> >>>>
> >>>> "key" is a bad name perhaps. Unless the resource is always a
> >>>> single level 2d texture, if we replace the strb->surface with a
> >>>> strb->resource, we will need to specify separately which
> >>>> face/level is sought.
> >>>>
> >>>>>> This all is definitely more complex than it should be.
> >>>>
> >>>> May be I'm not understanding what you were proposing. When you
> >>>> said that
> >>>>
> >>>> "the proper solution, since surfaces outlive their context, is
> >>>> to make
> >>>> surfaces screen-bound instead"
> >>>>
> >>>> I interpreted this statement as moving the
> >>>> pipe_context::create_surface method to
> >>>> pipe_screen::create_surface. Was this really your plan or did
> >>>> you meant something else?
> >>>>
> >>>> Because, my understanding is that it should be the other way
> >>>> around: when we moved the create_surface method from
> >>>> pipe_screen to pipe_context, we forgot to fix the
> >>>> st_renderbuffer code to do this properly.
> >>>>
> >>>>
> >>>>
> >>>> The fix I proposed may seem a bit complex, but keeping
> >>>> pipe_surfaces as context objects actually helps pipe drivers
> >>>> that put derived data in pipe_surfaces to be much simpler, as
> >>>> they no longer need complicated locking to be thread safe --
> >>>> the state tracker guarantees that pipe_surfaces belong to that
> >>>> context, and that context alone.
> >>>>
> >>>> That is, moving stuff all into the screen sounds simple at
> >>>> first, but then it becomes a serious nightmare to make it
> >>>> thread safe.
> >>>>
> >>>>
> >>>>
> >>>> Note that if st_renderbuffer can't be shared between GL
> >>>> contexts, then the solution can be much simpler: all we need to
> >>>> do is ensure that the surfaces are destroyed before the
> >>>> context. I haven't looked at the Mesa state tracker code in a
> >>>> while, so I'm a bit rusty in this regard.
> >>>>
> >>>> ARB_framebuffer_object says that framebuffer objects are
> >>>> per-context, but renderbuffer objects like textures can be
> >>>> shared between contexts. So when one looks at st_renderbuffer
> >>>> definition:
> >>>>
> >>>> /**
> >>>> * Derived renderbuffer class. Just need to add a
> >>>> pointer to the
> >>>> * pipe surface.
> >>>> */
> >>>> struct st_renderbuffer
> >>>> {
> >>>> struct gl_renderbuffer Base;
> >>>> struct pipe_resource *texture;
> >>>> struct pipe_surface *surface; /* temporary view into
> >>>> texture */
> >>>> struct pipe_sampler_view *sampler_view;
> >>>> enum pipe_format format; /** preferred format, or
> >>>> PIPE_FORMAT_NONE */
> >>>> GLboolean defined; /**< defined contents? */
> >>>>
> >>>> /**
> >>>> * Used only when hardware accumulation buffers are
> >>>> not supported.
> >>>> */
> >>>> boolean software;
> >>>> size_t stride;
> >>>> void *data;
> >>>>
> >>>> struct st_texture_object *rtt; /**< GL render to
> >>>> texture's texture */
> >>>> int rtt_level, rtt_face, rtt_slice;
> >>>>
> >>>> /** Render to texture state */
> >>>> struct pipe_resource *texture_save;
> >>>> struct pipe_surface *surface_save;
> >>>> struct pipe_sampler_view *sampler_view_save;
> >>>> };
> >>>>
> >>>> There are several pipe_context specific state that shouldn't be
> >>>> here: all pipe_surface and pipe_sampler_view objects, which can
> >>>> potentially be used by a context other than the context that
> >>>> created them. Those pointer should be kept either in a
> >>>> framebuffer object, and/or the GL context: created/destroyed as
> >>>> needed, or maybe cached for performance.
> >>>
> >>> Ok, so those things "shouln't be here", but I still don't get
> >>> what you
> >>> want to replace that pipe_surface member with for example. You
> >>> need
> >>> the format/size in many places, how is that going to fly? Access
> >>> them
> >>> through the context somehow?
> >>
> >> Note that there's actually lots of unused stuff in there.
> >> surface_save, sampler_view_save (as well as texture_save though
> >> this is
> >> not a problem here) are never used anywhere so can trivially go.
> >> sampler_view in there is probably a mistake too it is only used
> >> for ref
> >> counting purposes outside of a (unused - even says so in a
> >> comment)
> >> function (st_get_renderbuffer_sampler_view), and I can't see why
> >> this
> >> would be needed. Probably a leftover from earlier code.
> >> So this leaves only the surface pointer as problematic.
> >> I'm not quite sure where you'd want to store the pipe_surface, but
> >> I
> >> think all the information you need to (re)create it can already be
> >> extracted from st_renderbuffer. You could for instance create a
> >> cache of
> >> surfaces in st_context maybe. Obviously this means if you share a
> >> st_renderbuffer you have to create "identical" pipe_surfaces in
> >> each
> >> context (should be pretty light-weight, though obviously could be
> >> a
> >> problem for some less capapble hw if the driver needs to make a
> >> copy of
> >> the actual resource data, in that case the driver probably would
> >> need to
> >> make sure it can recognize such cases and do deferred destruction
> >> of the
> >> copied data but this is a problem anyway).
> >> Or you could just put the surface pointer into another struct
> >> instead
> >> (which would also contain a pointer to the strb) and use that
> >> throughout
> >> most of the code (except for sharing, of course).
> >
> > How is that different from updating the ctx pointer in pipe_surface
> > to
> > the to the context still using the strb after the old context
> > vanishes?
>
> Well adjusting the ctx pointer seems like a hack, if the struct isn't
> supposed to be shared in the first place.
Precisely. The pipe driver may actually implement surfaces as
struct my_surface {
struct pipe_surface base;
void *pointer_to_another_context_owned_structure.
};
Jose
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
