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

[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?

Stéphane