[Mesa-dev] State tracker pipe_surface woes...

[Stéphane Marchesin]

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 :)
>>
>> 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