[Mesa-dev] [PATCH v2] egl/android: Implement the eglSwapinterval for Android.

[Wu, Zhongmin]

Hi Eric:
Sorry, but what is the status about this patch.  I saw you said you was going to help to modify the comments and push it in the last mail. Is this patch merged in to lasts codes ?
Or do I miss something?

-----Original Message-----
From: Eric Engestrom [mailto:eric at engestrom.ch] 
Sent: Friday, January 19, 2018 3:46 
To: Emil Velikov <emil.l.velikov at gmail.com>
Cc: Wu, Zhongmin <zhongmin.wu at intel.com>; ML mesa-dev <mesa-dev at lists.freedesktop.org>; Tomasz Figa <tfiga at chromium.org>; Rob Herring <robh at kernel.org>; Liu, Zhiquan <zhiquan.liu at intel.com>; Long, Zhifang <zhifang.long at intel.com>; Kondapally, Kalyan <kalyan.kondapally at intel.com>; Palli, Tapani <tapani.palli at intel.com>; Xu, Randy <randy.xu at intel.com>; Bhardwaj, MunishX <munishx.bhardwaj at intel.com>; Kps, Harish Krupo <harish.krupo.kps at intel.com>; Chad Versace <chadversary at chromium.org>
Subject: Re: [PATCH v2] egl/android: Implement the eglSwapinterval for Android.

On Thursday, 2018-01-18 19:07:06 +0000, Emil Velikov wrote:
> On 18 January 2018 at 17:43, Eric Engestrom <eric at engestrom.ch> wrote:
> > On Thursday, 2018-01-18 07:52:42 +0000, Zhongmin Wu wrote:
> >> Implement the eglSwapinterval for Android platform to enable the 
> >> async mode for some GFX benchmarks such as Daimler C217, CityBench.
> >>
> >> Signed-off-by: Zhongmin Wu <zhongmin.wu at intel.com>
> >> ---
> >>  src/egl/drivers/dri2/platform_android.c | 21 +++++++++++++++++++++
> >>  1 file changed, 21 insertions(+)
> >>
> >> diff --git a/src/egl/drivers/dri2/platform_android.c 
> >> b/src/egl/drivers/dri2/platform_android.c
> >> index f6a24cd..3a64689 100644
> >> --- a/src/egl/drivers/dri2/platform_android.c
> >> +++ b/src/egl/drivers/dri2/platform_android.c
> >> @@ -476,6 +476,20 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
> >>     return EGL_TRUE;
> >>  }
> >>
> >> +static EGLBoolean
> >> +droid_swap_interval(_EGLDriver *drv, _EGLDisplay *dpy,
> >> +                   _EGLSurface *surf, EGLint interval) {
> >> +   struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> >> +   struct ANativeWindow *window = dri2_surf->window;
> >> +
> >> +   if (window->setSwapInterval(window, interval))
> >> +      return EGL_FALSE;
> >> +
> >> +   surf->SwapInterval = interval;
> >> +   return EGL_TRUE;
> >> +}
> >> +
> >>  static int
> >>  update_buffers(struct dri2_egl_surface *dri2_surf)  { @@ -1300,6 
> >> +1314,7 @@ static const struct dri2_egl_display_vtbl droid_display_vtbl = {
> >>     .swap_buffers = droid_swap_buffers,
> >>     .swap_buffers_with_damage = dri2_fallback_swap_buffers_with_damage, /* Android implements the function */
> >>     .swap_buffers_region = dri2_fallback_swap_buffers_region,
> >> +   .swap_interval = droid_swap_interval,
> >>  #if ANDROID_API_LEVEL >= 23
> >>     .set_damage_region = droid_set_damage_region,  #else @@ -1443,6 
> >> +1458,12 @@ dri2_initialize_android(_EGLDriver *drv, _EGLDisplay 
> >> *dpy)
> >>
> >>     dri2_setup_screen(dpy);
> >>
> >> +   /* we set the maximum swap interval as 1 for Android platform, Since
> >> +   it is the maximum value supported by Android according to the
> >> +   value of ANativeWindow::maxSwapInterval.
> >> +   */
> >> +   dri2_setup_swap_interval(dpy, 1);
> >
> > My C<->C++ interaction skills are more than rusty, but is it 
> > possible to use `ANativeWindow.maxSwapInterval` or something like this?
> >
> > Given that the dEQP tests all pass, whether with the current comment 
> > or the class field directly used in the code, this patch is:
> > Reviewed-by: Eric Engestrom <eric at engestrom.ch>
> >
> > I'll push it for you in a couple days if no one objects :)
> >
> My current checkout shows the struct is nicely separated for inclusion 
> in both C and C++ sources.
> The C++ vfuncs are wrapped in a ifdef _cplusplus block, so the ABI 
> should be identical across both.
> 
> I'm suspecting that maxSwapInterval > 1 will require additional 
> changes, so I'd keep it as-is

That's actually a good point; this should indeed stay hard coded, with this comment.

> (nit the comment style and adding dEQP stats in the commit message).

I was going to do this before pushing, but you're right to point it out :)

> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
> 
> -Emil