[PATCH v2 08/18] mt trace: track GL context create/destroy calls

[Imre Deak]

On Fri, 2012-05-18 at 16:03 +0300, Pauli Nieminen wrote:
> On Tue, May 15, 2012 at 05:11:03PM +0300, Imre Deak wrote:
> > This is needed for muti-threaded tracing to work properly.
> > 
> > Tracing multi-threaded GL apps requires that we track the lifetime of
> > the GL context objects and store thread specific state like the currently
> > active GL context on thread-local memory area.
> > 
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  wrappers/egltrace.py       |    5 +++
> >  wrappers/gltrace.hpp       |   17 +++++++++-
> >  wrappers/gltrace_state.cpp |   71 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 91 insertions(+), 2 deletions(-)
> >  create mode 100644 wrappers/gltrace_state.cpp
> > 
> > diff --git a/wrappers/egltrace.py b/wrappers/egltrace.py
> > index 4858e2f..b958a0c 100644
> > --- a/wrappers/egltrace.py
> > +++ b/wrappers/egltrace.py
> > @@ -52,6 +52,9 @@ class EglTracer(GlTracer):
> >      def traceFunctionImplBody(self, function):
> >          GlTracer.traceFunctionImplBody(self, function)
> >  
> > +        if function.name == 'eglCreateContext':
> > +            print '    gltrace::createContext((unsigned long)_result);'
> > +
> >          if function.name == 'eglMakeCurrent':
> >              print '    // update the profile'
> >              print '    if (ctx != EGL_NO_CONTEXT) {'
> > @@ -67,6 +70,8 @@ class EglTracer(GlTracer):
> >              print '            tr->profile = gltrace::PROFILE_ES2;'
> >              print '    }'
> >  
> > +        if function.name == 'eglDestroyContext':
> > +            print '    gltrace::destroyContext((unsigned long)_result);'
> >  
> 
> Do we need same handling for glX counter parts?

Yes, we can easily do this for glX, cgl, wgl.

> >  if __name__ == '__main__':
> >      print '#include <stdlib.h>'
> > diff --git a/wrappers/gltrace.hpp b/wrappers/gltrace.hpp
> > index cd602cb..627b2f7 100644
> > --- a/wrappers/gltrace.hpp
> > +++ b/wrappers/gltrace.hpp
> > @@ -39,13 +39,26 @@ enum Profile {
> >      PROFILE_ES2,
> >  };
> >  
> > -struct Context {
> > +class Context {
> > +public:
> >      enum Profile profile;
> >      bool user_arrays;
> >      bool user_arrays_arb;
> >      bool user_arrays_nv;
> > +
> > +    Context(void) : profile(PROFILE_COMPAT), user_arrays(false),
> > +                    user_arrays_arb(false), user_arrays_nv(false) { }
> >  };
> > -    
> > +
> > +void
> > +createContext(unsigned long context_id);
> > +
> > +void
> > +destroyContext(unsigned long context_id);
> > +
> > +void
> > +setContext(unsigned long context_id);
> > +
> 
> How about using uintptr_t?
> 
> That would make sure we always have at least enough bits to hold pointer. Just
> in case we have in future more bits in cotext id that in unsigned
> long.

True, using unsigned long could break at least on Win64.

> >  Context *
> >  getContext(void);
> >  
> > diff --git a/wrappers/gltrace_state.cpp b/wrappers/gltrace_state.cpp
> > new file mode 100644
> > index 0000000..cce4e14
> > --- /dev/null
> > +++ b/wrappers/gltrace_state.cpp
> > @@ -0,0 +1,71 @@
> > +#include <gltrace.hpp>
> > +#include <os_thread.hpp>
> > +#include <assert.h>
> > +
> > +namespace gltrace {
> > +
> > +static std::map<unsigned long, Context *> context_map;
> > +
> > +class ThreadState {
> > +public:
> > +	Context *current_context;
> > +	Context dummy_context;
> > +
> > +	ThreadState(void) : current_context(NULL) { }
> > +};
> > +
> > +static os::thread_specific_ptr<struct ThreadState> thread_state;
> > +
> > +static ThreadState *get_ts(void)
> > +{
> > +	struct ThreadState *ts = thread_state.get();
> > +
> > +	if (!ts) {
> > +		ts = new ThreadState;
> > +		thread_state.reset(ts);
> > +	}
> > +
> > +	return ts;
> > +}
> > +
> > +void createContext(unsigned long context_id)
> > +{
> > +	assert(context_map.find(context_id) == context_map.end());
> > +	context_map[context_id] = new Context();
> > +}
> > +
> > +void destroyContext(unsigned long context_id)
> > +{
> > +	struct ThreadState *ts = get_ts();
> > +	Context *ctx;
> > +
> > +	ctx = context_map[context_id];
> > +	if (ctx == ts->current_context)
> > +		ts->current_context = NULL;
> > +
> > +	context_map.erase(context_id);
> > +	delete ctx;
> 
> eglDestroyContext is immediate only if context isn't current in any
> thread. In case context is current in any thread destruction is
> delayed untill all threads have released the context with MakeCurrent.
> 
> Implementing that would be simple with std::shared_ptr (c++11) or
> boost::shared_ptr. You could keep shared_ptr in context_map and
> ThreadState. In context destruction simple erasing the context from
> context_map would work correctly.
> 
> Later on when setContext replaces the current context we will
> automatically free the deleted context.

Though at the moment we don't do anything with the shared context
parameter, we would need it to handle buffer lookups when the manual
tracking implemented in this patchset is enabled. So yes, this needs
fixing along the buffer lookup code.

> > +}
> > +
> > +void setContext(unsigned long context_id)
> > +{
> > +	struct ThreadState *ts = get_ts();
> > +
> > +	ts->current_context = context_map[context_id];
> 
> Do we need special handing for EGL_NO_CONTEXT?
> 
> That can be passed to eglMakeCurrent to make the context non-active.
> But in that case we should return dummy_context if application calls
> gl functions.

Good point. In fact I also forgot to call setContext from the
eglMakeCurrent wrapper :/ It happened to work only because dummy_context
is also thread specific.

Thanks for the review, I'll resubmit the patches with the fixes.

--Imre

> > +}
> > +
> > +Context *getContext(void)
> > +{
> > +	struct ThreadState *ts = get_ts();
> > +	Context *ctx = ts->current_context;
> > +
> > +	/*
> > +	 * We are called from some gl* function at which point the behaviour
> > +	 * is undefined if there is no active context. So in this case return
> > +	 * a dummy context so that at least the apitrace internal book-keeping
> > +	 * - which is context dependent - won't crash.
> > +	 */
> > +	return ctx ? ctx : &ts->dummy_context;
> > +}
> > +
> > +}