[Mesa-dev] [PATCH] egl: deduplicate allocations of local buffer over each platform backend.

[Emil Velikov]

Hi Gwan-gyeong Mun,

Thanks for sorting this out. Having a single copy instead of 3+ is always a win.

On 2 August 2017 at 10:23, Gwan-gyeong Mun <elongbug at gmail.com> wrote:
> platform_drm, platform_wayland and platform_android have similiar local buffer
> allocation routines. For deduplicating, it unifies dri2_egl_surface's
> local buffer allocation routines. And it polishes inconsistent indentations.
>
> Signed-off-by: Mun Gwan-gyeong <elongbug at gmail.com>
> ---
>  src/egl/drivers/dri2/egl_dri2.c         | 40 ++++++++++++++++++++
>  src/egl/drivers/dri2/egl_dri2.h         | 16 ++++++--
>  src/egl/drivers/dri2/platform_android.c | 40 ++------------------
>  src/egl/drivers/dri2/platform_drm.c     | 65 +++++++++++----------------------
>  src/egl/drivers/dri2/platform_wayland.c | 52 ++++++--------------------
>  5 files changed, 89 insertions(+), 124 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index a197e0456f..6ee3b36739 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -972,6 +972,46 @@ dri2_display_destroy(_EGLDisplay *disp)
>     disp->DriverData = NULL;
>  }
>
> +__DRIbuffer *
> +dri2_egl_surface_alloc_local_buffer(struct dri2_egl_surface *dri2_surf,
> +                                    unsigned int att, unsigned int format)
> +{
> +#if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM) || defined(HAVE_ANDROID_PLATFORM)
I think we can make this unconditional - a few bytes of extra code [in
the corner case], should be that bad.

[...]

> +void
> +dri2_egl_surface_free_local_buffers(struct dri2_egl_surface *dri2_surf)
> +{
> +#if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM) || defined(HAVE_ANDROID_PLATFORM)
Ditto.

[...]

> +#if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM) || defined(HAVE_ANDROID_PLATFORM)
> +   /* EGL-owned buffers */
> +   __DRIbuffer           *local_buffers[__DRI_BUFFER_COUNT];
> +#endif
> +
... and here.


[...]

>  static __DRIbuffer *
>  dri2_drm_get_buffers_with_format(__DRIdrawable *driDrawable,
> -                            int *width, int *height,
> -                            unsigned int *attachments, int count,
> -                            int *out_count, void *loaderPrivate)
> +                                 int *width, int *height,
> +                                 unsigned int *attachments, int count,
> +                                 int *out_count, void *loaderPrivate)
>  {
>     struct dri2_egl_surface *dri2_surf = loaderPrivate;
>     int i, j;
>
>     for (i = 0, j = 0; i < 2 * count; i += 2, j++) {
> +      __DRIbuffer *local;
> +
>        assert(attachments[i] < __DRI_BUFFER_COUNT);
>        assert(j < ARRAY_SIZE(dri2_surf->buffers));
>

>        switch (attachments[i]) {
>        case __DRI_BUFFER_BACK_LEFT:
> -        if (get_back_bo(dri2_surf) < 0) {
> -           _eglError(EGL_BAD_ALLOC, "failed to allocate color buffer");
> -           return NULL;
> -        }
> +         if (get_back_bo(dri2_surf) < 0) {
> +            _eglError(EGL_BAD_ALLOC, "failed to allocate color buffer");
> +            return NULL;
> +         }
>           back_bo_to_dri_buffer(dri2_surf, &dri2_surf->buffers[j]);
> -        break;
> +         break;
Unrelated whitespace changes?

>        default:
> -        if (get_aux_bo(dri2_surf, attachments[i], attachments[i + 1],
> -                       &dri2_surf->buffers[j]) < 0) {
> -           _eglError(EGL_BAD_ALLOC, "failed to allocate aux buffer");
> -           return NULL;
> -        }
> -        break;
> +         local = dri2_egl_surface_alloc_local_buffer(dri2_surf, attachments[i],
> +                                                     attachments[i + 1]);
> +
> +         if (local)
> +            dri2_surf->buffers[j] = *local;
> +         else {
> +            _eglError(EGL_BAD_ALLOC, "failed to allocate local buffer");
> +            return NULL;
> +         }
Please preserve the original codeflow:

if (condition) {
   error_path
   return
}
normal_path

[...]

> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -287,13 +287,8 @@ dri2_wl_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
>                  dri2_surf->color_buffers[i].data_size);
>     }
>
> -   if (dri2_dpy->dri2) {
> -      for (int i = 0; i < __DRI_BUFFER_COUNT; i++)
> -         if (dri2_surf->dri_buffers[i] &&
> -             dri2_surf->dri_buffers[i]->attachment != __DRI_BUFFER_BACK_LEFT)
> -            dri2_dpy->dri2->releaseBuffer(dri2_dpy->dri_screen,
> -                                          dri2_surf->dri_buffers[i]);
> -   }
> +   if (dri2_dpy->dri2)
> +      dri2_egl_surface_free_local_buffers(dri2_surf);
This seems like a functionality change, since the new helper is
missing the __DRI_BUFFER_BACK_LEFT check.
In reality that should not cause issues, since
dri2_wl_get_buffers_with_format() makes sure we don't have such buffer
in local_buffers.

Other platforms are more relaxed - missing the check all togethre.


Tl;Dr; Things should be fine, but elaborate why/how in the commit message.

Thinking out loud:
How can we ensure nobody overflows dri2_surf->buffers[], should we bother?

[...]

>        return NULL;
>
>     for (i = 0, j = 0; i < 2 * count; i += 2, j++) {
> +      __DRIbuffer *local;
Add blank line.

>        switch (attachments[i]) {
>        case __DRI_BUFFER_BACK_LEFT:
>           back_bo_to_dri_buffer(dri2_surf, &dri2_surf->buffers[j]);
>           break;
>        default:
> -         if (get_aux_bo(dri2_surf, attachments[i], attachments[i + 1],
> -                        &dri2_surf->buffers[j]) < 0) {
> -            _eglError(EGL_BAD_ALLOC, "failed to allocate aux buffer");
> +         local = dri2_egl_surface_alloc_local_buffer(dri2_surf, attachments[i],
> +                                                     attachments[i + 1]);
> +
> +         if (local)
> +            dri2_surf->buffers[j] = *local;
> +         else {
> +            _eglError(EGL_BAD_ALLOC, "failed to allocate local buffer");
Please preserve original codeflow.

With the above addressed the patch is
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

Thanks
Emil