Hi Harish, There's a handful of comments, most of which style related nitpicks. Couple that apply through the patch: - rewrap the comments to utilise ~80 columns. - please use consistent variable names - fooBar is not used in src/egl.
Splitting the core implementation and Android code into separate patches, would be great but optional. On 5 June 2017 at 15:07, Harish Krupo <harish.krupo....@intel.com> wrote: > This patch adds support for the EGL_KHR_partial_update extension for > android platform. It passes 36/37 tests in dEQP for EGL_KHR_partial_update. > 1 test not supported. > > v2: add fallback for eglSetDamageRegionKHR (Tapani) > > v3: The native_window_set_surface_damage call is available only from > Android version 6.0. Reintroduce the ANDROID_VERSION guard and > advertise extension only if version is >= 6.0. (Emil Velikov) > > Signed-off-by: Harish Krupo <harish.krupo....@intel.com> > --- > Android.common.mk | 3 +- > Android.mk | 2 + > src/egl/drivers/dri2/egl_dri2.c | 12 +++++ > src/egl/drivers/dri2/egl_dri2.h | 4 ++ > src/egl/drivers/dri2/platform_android.c | 37 ++++++++++++++ > src/egl/main/eglapi.c | 87 > +++++++++++++++++++++++++++++++++ > src/egl/main/eglapi.h | 2 + > src/egl/main/egldisplay.h | 1 + > src/egl/main/eglentrypoint.h | 1 + > src/egl/main/eglfallbacks.c | 1 + > src/egl/main/eglsurface.c | 8 +++ > src/egl/main/eglsurface.h | 12 +++++ > 12 files changed, 169 insertions(+), 1 deletion(-) > > diff --git a/Android.common.mk b/Android.common.mk > index d9b198a956..4777ea8bd9 100644 > --- a/Android.common.mk > +++ b/Android.common.mk > @@ -39,7 +39,8 @@ LOCAL_CFLAGS += \ > -Wno-initializer-overrides \ > -Wno-mismatched-tags \ > -DPACKAGE_VERSION=\"$(MESA_VERSION)\" \ > - > -DPACKAGE_BUGREPORT=\"https://bugs.freedesktop.org/enter_bug.cgi?product=Mesa\"; > + > -DPACKAGE_BUGREPORT=\"https://bugs.freedesktop.org/enter_bug.cgi?product=Mesa\"; > \ > + > -DANDROID_VERSION=0x0$(MESA_ANDROID_MAJOR_VERSION)0$(MESA_ANDROID_MINOR_VERSION) > On second thought, this will be iffy with Android O and later, since the major is a letter. The newly introduced ANDROID_API_LEVEL might be a better fit? Apologies for misleading you :-\ > --- a/src/egl/drivers/dri2/egl_dri2.c > +++ b/src/egl/drivers/dri2/egl_dri2.c > @@ -1464,6 +1464,17 @@ dri2_swap_buffers_region(_EGLDriver *drv, _EGLDisplay > *dpy, _EGLSurface *surf, > } > > static EGLBoolean > +dri2_set_damage_region(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf, > + EGLint *rects, EGLint n_rects) > +{ > + struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy); > + if (dri2_dpy->vtbl->set_damage_region) > + return dri2_dpy->vtbl->set_damage_region(drv, dpy, surf, rects, > n_rects); > + > + return EGL_FALSE; Please add a fallback entry in egl_dri2_fallbacks.h use it in the other platforms. Then this check becomes obsolete. > --- a/src/egl/drivers/dri2/platform_android.c > +++ b/src/egl/drivers/dri2/platform_android.c > @@ -651,6 +651,39 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, > _EGLSurface *draw) > return EGL_TRUE; > } > > +static EGLBoolean > +droid_set_damage_region(_EGLDriver *drv, > + _EGLDisplay *disp, > + _EGLSurface *draw, const EGLint* rects, EGLint > n_rects) > +{ > + struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp); > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(draw); > + android_native_rect_t* droid_rects = NULL; Nit: add blank line. > + if (n_rects != 0) > + droid_rects = (android_native_rect_t *) Please drop the cast - this is not a C++ source. > + calloc(n_rects, sizeof(android_native_rect_t)); > + > + EGLint surfWidth = dri2_surf->base.Width; > + EGLint surfHeight = dri2_surf->base.Height; > + EGLint dIndex; > + dIndex is not needed - use "EGLint i = 0; i < n_rects; += 4" in the loop condition. > + for (dIndex = 0; dIndex < n_rects; dIndex++) { > + EGLint i = dIndex * 4; > + droid_rects[dIndex].left = rects[i]; // left == x > + droid_rects[dIndex].bottom = rects[i + 1]; // bottom == y > + droid_rects[dIndex].right = rects[i] + rects[i + 2]; // left + width > + droid_rects[dIndex].top = rects[i + 1] + rects[i + 3]; // bottom + > height Comments don't help much. Please drop. > + } > + > +#if ANDROID_VERSION >= 0x600 > + native_window_set_surface_damage(dri2_surf->window, droid_rects, n_rects); Isolating the most important function like this is a bad idea. Please wrap the whole of droid_set_damage_region. > +#endif > + > + free(droid_rects); > + > + return EGL_TRUE; > +} > + > static _EGLImage * > droid_create_image_from_prime_fd_yuv(_EGLDisplay *disp, _EGLContext *ctx, > struct ANativeWindowBuffer *buf, int fd) > @@ -1101,6 +1134,7 @@ static struct dri2_egl_display_vtbl droid_display_vtbl > = { > .swap_buffers = droid_swap_buffers, > .swap_buffers_with_damage = dri2_fallback_swap_buffers_with_damage, > .swap_buffers_region = dri2_fallback_swap_buffers_region, > + .set_damage_region = droid_set_damage_region, With the above suggestions this becomes #if ANDROID_API_LEVEL >= 23 .set_damage_region = droid_set_damage_region, #else .set_damage_region = dri2_fallback_set_damage_region, #endif > @@ -1281,6 +1300,74 @@ eglSwapBuffersWithDamageKHR(EGLDisplay dpy, EGLSurface > surface, > return _eglSwapBuffersWithDamageCommon(disp, surf, rects, n_rects); > } > > +/* Nit: make this /** > + * If the width of the passed rect is greater than the surface's > + * width then it is clamped to the width of the surface. Same with > + * height. > + */ > + > +static void > +_eglSetDamageRegionKHRClampRects(_EGLDisplay* disp, _EGLSurface* surf, > + EGLint *rects, EGLint n_rects) > +{ > + EGLint i; > + EGLint surfHeight = surf->Height; > + EGLint surfWidth = surf->Width; > + > + for (i = 0; i < (4 * n_rects); i += 4) { Do we really need the "4 *" here? > + EGLint x, y, rectWidth, rectHeight; > + x = rects[i]; > + y = rects[i + 1]; > + rectWidth = rects[i + 2]; > + rectHeight = rects[i + 3]; > + > + if (rectWidth + x > surfWidth) Math can overflow. > + rects[i + 2] = surfWidth - x; > + > + if (rectHeight + y > surfHeight) Ditto. > + rects[i + 3] = surfHeight - y; > + } > +} > + > +static EGLBoolean EGLAPIENTRY > +eglSetDamageRegionKHR(EGLDisplay dpy, EGLSurface surface, > + EGLint *rects, EGLint n_rects) > +{ > + _EGLDisplay *disp = _eglLockDisplay(dpy); > + _EGLSurface *surf = _eglLookupSurface(surface, disp); > + _EGL_FUNC_START(disp, EGL_OBJECT_SURFACE_KHR, surf, EGL_FALSE); > + _EGLContext *ctx = _eglGetCurrentContext(); > + _EGLDriver *drv; > + EGLBoolean ret; > + _EGL_CHECK_SURFACE(disp, surf, EGL_FALSE, drv); > + > + if (_eglGetContextHandle(ctx) == EGL_NO_CONTEXT || > + surf->Type != EGL_WINDOW_BIT || ctx->DrawSurface != surf || Nit: Group the error conditions if (not_current_surface || not_postable || not_buffer_destroyed) if (_eglGetContextHandle(ctx) == EGL_NO_CONTEXT || ctx->DrawSurface != surf || surf->Type != EGL_WINDOW_BIT || > + surf->SwapBehavior != EGL_BUFFER_DESTROYED) > + RETURN_EGL_ERROR(disp, EGL_BAD_MATCH, EGL_FALSE); > + > + /* If the damage region is already set between > + * frame boundaries, throw bad access error > + */ > + if (surf->SetDamageRegion) > + RETURN_EGL_ERROR(disp, EGL_BAD_ACCESS, EGL_FALSE); > + > + /* If the buffer age has not been queried before > + * setting the damage region, between > + * frame boundaries, throw bad access error > + */ Not sure these comments bring much. Please rewrap if you to prefer to keep. Regards, Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
