[Mesa-dev] State tracker pipe_surface woes...
Roland Scheidegger
sroland at vmware.com
Tue Sep 6 16:36:46 PDT 2011
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).
Roland
>
>>
>>
>> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list