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?
>
>
> 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.
Stéphane
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
