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

[Roland Scheidegger]

Am 07.09.2011 02:00, schrieb Stéphane Marchesin:
> 2011/9/6 Roland Scheidegger <sroland at vmware.com>:
>> Am 07.09.2011 00:01, schrieb 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?
>>
>> 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.
That said I'm not really sure why pipe_sampler_view and pipe_surface
actually have a context pointer in them, since they are only supposed to
be used with the context in which they were created I think those
shouldn't actually exist - surface_destroy and sampler_view_destroy will
have context as a parameter, and if they aren't shared between contexts
it's pointless to store the context pointer. Might be a relic from when
those structs were created/destroyed using screen functions...


Roland