[Mesa-dev] [RFC] mesa: drop current draw/read buffer when ctx is released

[Rob Clark]

On Fri, Oct 28, 2016 at 1:24 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
> On 10/27/2016 01:48 PM, Rob Clark wrote:
>>
>> On Thu, Oct 27, 2016 at 2:59 AM, Tapani Pälli <tapani.palli at intel.com>
>> wrote:
>>>
>>> On 10/27/2016 12:16 AM, Rob Clark wrote:
>>>>
>>>> So, not quite sure if this is the *correct* solution, but it is at least
>>>> *a* solution to a problem with android wallpaper vs mesa that I've been
>>>> debugging.  Basically, what happens is:
>>>
>>>
>>> Could you tell more how to trigger this, is this with some particular
>>> live
>>> wallpaper and has this been working before (regression)? For me at least
>>> these default wallpapers have been working ok.
>>
>> Actually, it is the default static wallpaper that is problematic.  And
>> IIRC, it has never worked, at least not with any of the gallium
>> drivers (freedreno, virgl, vc4, etc).  Live-wallpaper did work, but
>> does not appear to exist in AOSP builds anymore.
>>
>> If this works with i965 on android, I'd be curious how.  Or if you're
>> android build had some mesa patches that are not upstream?
>
>
> I can confirm that default wallpaper is working on i965. Our Mesa tree
> currently looks like this:
>
> https://github.com/android-ia/external-mesa
>
> It's now quite a bit behind though because we were dealing with some
> non-graphics issues and are planning to rebase soon.

I suppose it is possible that it is triggered by something different
that mesa/st does vs dri/i965?  Although I find it odd that
dri2_make_current() would drop the reference to the EGLSurface when
unbinding ctx, but _mesa_make_current() would not drop the reference
to the corresponding gl_framebuffer.

Maybe the classic driver is holding an extra reference to the
EGLSurface so the _eglPutSurface() call in dri2_make_current() does
not actually drop the last refcnt?  Which would ensure when the window
surface is created it ends up with a different address..

but, I wonder if you could try w/ Rob Herring's tree.. this is what we
are using for the upstream kernel + AOSP builds[1]:

  https://github.com/robherring/mesa/commits/android-m

I guess in theory we should both need the same patches on top of
mesa..  although also I guess we should also just try to get to the
point where we can both use upstream mesa tree directly.

[1] see: https://github.com/robherring/generic_device/wiki/KConfig-based-Multi-platform-Android-Device-(and-Mesa-graphics)

Btw, I guess in theory the qemu/x86 build from rob's generic_device
stuff should also work on real hw, so I think we should be able to
test exact same build that is failing with gallium/virgl with i965.
But I don't have any real hw for that.

BR,
-R

>
>> BR,
>> -R
>>
>>>>      EGLSurface tmpSurface = mEgl.eglCreatePbufferSurface(mEglDisplay,
>>>> mEglConfig, attribs);
>>>>      mEgl.eglMakeCurrent(mEglDisplay, tmpSurface, tmpSurface,
>>>> mEglContext);
>>>>
>>>>      int[] maxSize = new int[1];
>>>>      Rect frame = surfaceHolder.getSurfaceFrame();
>>>>      glGetIntegerv(GL_MAX_TEXTURE_SIZE, maxSize, 0);
>>>>
>>>>      mEgl.eglMakeCurrent(mEglDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE,
>>>> EGL_NO_CONTEXT);
>>>>      mEgl.eglDestroySurface(mEglDisplay, tmpSurface);
>>>>
>>>>      ... check maxSize vs frame size and bail if needed ...
>>>>
>>>>      mEglSurface = mEgl.eglCreateWindowSurface(mEglDisplay, mEglConfig,
>>>> surfaceHolder, null);
>>>>      ... error checking ...
>>>>      mEgl.eglMakeCurrent(mEglDisplay, mEglSurface, mEglSurface,
>>>> mEglContext);
>>>>
>>>> When the window-surface is created, it ends up with the same ptr address
>>>> as the recently freed tmpSurface pbuffer surface.  Which after many
>>>> levels of indirection, results in st_framebuffer_validate() ending up
>>>> with
>>>> the same/old framebuffer object, and in the end never calling the
>>>> DRIimageLoaderExtension::getBuffers().  Then in droid_swap_buffers(),
>>>> the
>>>> dri2_surf is still the old pbuffer surface (with dri2_surf->buffer being
>>>> NULL, obviously, so when wallpaper app calls eglSwapBuffers() nothing
>>>> gets enqueued to the compositor).
>>>>
>>>> Maybe instead, eglDestroySurface() should clear any references the ctx
>>>> has to the surface.  Not sure how that would work.  Did I mention there
>>>> are many levels of indirection?
>>>> ---
>>>>    src/mesa/main/context.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
>>>> index 5e52065..83b8cc1 100644
>>>> --- a/src/mesa/main/context.c
>>>> +++ b/src/mesa/main/context.c
>>>> @@ -1646,6 +1646,10 @@ ALOGE("%s:%d, drawBuffer=%p, readBuffer=%p",
>>>> __func__, __LINE__, drawBuffer, rea
>>>>         if (!newCtx) {
>>>>          _glapi_set_dispatch(NULL);  /* none current */
>>>> +      if (curCtx) {
>>>> +         _mesa_reference_framebuffer(&curCtx->WinSysDrawBuffer, NULL);
>>>> +         _mesa_reference_framebuffer(&curCtx->WinSysReadBuffer, NULL);
>>>> +      }
>>>>       }
>>>>       else {
>>>>          _glapi_set_dispatch(newCtx->CurrentDispatch);
>>>
>>>
>>>
>