Mir

Code review comment for lp:~kdub/mir/egl-sync-fences

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> > I'm not very familiar with this code, but could gl_bind_to_texture() be
> made idempotent and called
> > in both branches (which might then be merged into one)?
>
> gl_bind_to_texture possibly uploads the texture, so calling it twice when not
> needed will waste texture uploads. We need the second function that adds the
> sync point without uploading, mostly for texture cache usage.

I was wondering if gl_bind_to_texture() had enough context to "know" of the upload was unnecessary.

>
> Ideally, I'd change it to:
> //uploads
> gl_bind_to_texture()
> //adds sync points
> secure_for_render()
>
> but this doesn't fix the bugs for downstreams.

Its also an easy to misuse API as it requires a specific order of calls. It is almost as though secure_for_render() should return an object (a future?) whose destructor does the wait for the gpu release.

> I think I'll go with:
> //deprecated/legacy, calls bind(), then secure_for_render()
> gl_bind_to_texture()
> //uploads
> bind()
> //adds sync points
> secure_for_render()

OK, I don't have strong feelings against this, it is better than the current proposal.

« Back to merge proposal