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

[Jose Fonseca]

----- 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.


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.


Jose