[Mesa-dev] State tracker pipe_surface woes...
stephane.marchesin at gmail.com
Tue Sep 6 15:01:05 PDT 2011
2011/9/3 Jose Fonseca <jfonseca at vmware.com>:
> ----- Original Message -----
>> 2011/9/2 Stéphane Marchesin <stephane.marchesin at gmail.com>:
>> > 2011/9/2 Jose Fonseca <jfonseca at 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 :)
>> > 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?
> Fixing st_renderbuffer will imply some extensive work, and I don't expect you or anybody to fix it immediately. But I hope at least to convince you of the pipe_surface design goals, so that we can make opportunistic small steps on that direction.
Well, I have to fix that bug, so I do want to fix it immediately.
More information about the mesa-dev