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

Eric Engestrom eric.engestrom at imgtec.com
Fri Aug 4 15:42:47 UTC 2017


On Saturday, 2017-08-05 00:16:11 +0900, Gwan-gyeong Mun 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.
> 
> Note that as dri2_wl_get_buffers_with_format() have not make a __DRI_BUFFER_BACK_LEFT
> attachment buffer for local_buffers, new helper function, dri2_egl_surface_free_local_buffers(),
> will drop the __DRI_BUFFER_BACK_LEFT check.
> So if other platforms use new helper functions, we have to ensure not to make
> __DRI_BUFFER_BACK_LEFT attachment buffer for local_buffers.
> 
> v2: Fixes from Emil's review:
>    a) Make local_buffers variable, dri2_egl_surface_alloc_local_buffer() and
>       dri2_egl_surface_free_local_buffers() unconditionally.
>    b) Preserve the original codeflow for error_path and normal_path.
>    c) Add note on commit messages for dropping of __DRI_BUFFER_BACK_LEFT check.
>    c) Rollback the unrelated whitespace changes.

The dri2_drm_get_buffers_with_format() unrelated whitespace change is
still here :P
(don't bother unless you need to do a v3 for some other reason)

Once this lands, platform_drm (and probably others) could use
a full whitespace cleanup pass, if you want :)

>    d) Add a missing blank line.
> 
> Signed-off-by: Mun Gwan-gyeong <elongbug at gmail.com>
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

v2 looks good to me.
Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>

Thanks!

> ---
>  src/egl/drivers/dri2/egl_dri2.c         | 34 +++++++++++++++++++++
>  src/egl/drivers/dri2/egl_dri2.h         | 14 ++++++---
>  src/egl/drivers/dri2/platform_android.c | 40 ++-----------------------
>  src/egl/drivers/dri2/platform_drm.c     | 46 ++++++++---------------------
>  src/egl/drivers/dri2/platform_wayland.c | 52 ++++++++-------------------------
>  5 files changed, 71 insertions(+), 115 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 733659d547..129cc35872 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -972,6 +972,40 @@ 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)
> +{
> +   struct dri2_egl_display *dri2_dpy =
> +      dri2_egl_display(dri2_surf->base.Resource.Display);
> +
> +   if (att >= ARRAY_SIZE(dri2_surf->local_buffers))
> +      return NULL;
> +
> +   if (!dri2_surf->local_buffers[att]) {
> +      dri2_surf->local_buffers[att] =
> +         dri2_dpy->dri2->allocateBuffer(dri2_dpy->dri_screen, att, format,
> +                                        dri2_surf->base.Width, dri2_surf->base.Height);
> +   }
> +
> +   return dri2_surf->local_buffers[att];
> +}
> +
> +void
> +dri2_egl_surface_free_local_buffers(struct dri2_egl_surface *dri2_surf)
> +{
> +   struct dri2_egl_display *dri2_dpy =
> +      dri2_egl_display(dri2_surf->base.Resource.Display);
> +
> +   for (int i = 0; i < ARRAY_SIZE(dri2_surf->local_buffers); i++) {
> +      if (dri2_surf->local_buffers[i]) {
> +         dri2_dpy->dri2->releaseBuffer(dri2_dpy->dri_screen,
> +                                       dri2_surf->local_buffers[i]);
> +         dri2_surf->local_buffers[i] = NULL;
> +      }
> +   }
> +}
> +
>  /**
>   * Called via eglTerminate(), drv->API.Terminate().
>   *
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index ccfefef61f..6c7d75587a 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -283,8 +283,10 @@ struct dri2_egl_surface
>     struct gbm_dri_surface *gbm_surf;
>  #endif
>  
> +   /* EGL-owned buffers */
> +   __DRIbuffer           *local_buffers[__DRI_BUFFER_COUNT];
> +
>  #if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM)
> -   __DRIbuffer           *dri_buffers[__DRI_BUFFER_COUNT];
>     struct {
>  #ifdef HAVE_WAYLAND_PLATFORM
>        struct wl_buffer   *wl_buffer;
> @@ -309,9 +311,6 @@ struct dri2_egl_surface
>     __DRIimage *dri_image_back;
>     __DRIimage *dri_image_front;
>  
> -   /* EGL-owned buffers */
> -   __DRIbuffer           *local_buffers[__DRI_BUFFER_COUNT];
> -
>     /* Used to record all the buffers created by ANativeWindow and their ages.
>      * Usually Android uses at most triple buffers in ANativeWindow
>      * so hardcode the number of color_buffers to 3.
> @@ -451,4 +450,11 @@ dri2_set_WL_bind_wayland_display(_EGLDriver *drv, _EGLDisplay *disp)
>  void
>  dri2_display_destroy(_EGLDisplay *disp);
>  
> +__DRIbuffer *
> +dri2_egl_surface_alloc_local_buffer(struct dri2_egl_surface *dri2_surf,
> +                                    unsigned int att, unsigned int format);
> +
> +void
> +dri2_egl_surface_free_local_buffers(struct dri2_egl_surface *dri2_surf);
> +
>  #endif /* EGL_DRI2_INCLUDED */
> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
> index 50a8248695..71dd1594c7 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -271,40 +271,6 @@ droid_window_cancel_buffer(struct dri2_egl_surface *dri2_surf)
>     }
>  }
>  
> -static __DRIbuffer *
> -droid_alloc_local_buffer(struct dri2_egl_surface *dri2_surf,
> -                         unsigned int att, unsigned int format)
> -{
> -   struct dri2_egl_display *dri2_dpy =
> -      dri2_egl_display(dri2_surf->base.Resource.Display);
> -
> -   if (att >= ARRAY_SIZE(dri2_surf->local_buffers))
> -      return NULL;
> -
> -   if (!dri2_surf->local_buffers[att]) {
> -      dri2_surf->local_buffers[att] =
> -         dri2_dpy->dri2->allocateBuffer(dri2_dpy->dri_screen, att, format,
> -               dri2_surf->base.Width, dri2_surf->base.Height);
> -   }
> -
> -   return dri2_surf->local_buffers[att];
> -}
> -
> -static void
> -droid_free_local_buffers(struct dri2_egl_surface *dri2_surf)
> -{
> -   struct dri2_egl_display *dri2_dpy =
> -      dri2_egl_display(dri2_surf->base.Resource.Display);
> -
> -   for (int i = 0; i < ARRAY_SIZE(dri2_surf->local_buffers); i++) {
> -      if (dri2_surf->local_buffers[i]) {
> -         dri2_dpy->dri2->releaseBuffer(dri2_dpy->dri_screen,
> -               dri2_surf->local_buffers[i]);
> -         dri2_surf->local_buffers[i] = NULL;
> -      }
> -   }
> -}
> -
>  static _EGLSurface *
>  droid_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
>  		    _EGLConfig *conf, void *native_window,
> @@ -400,7 +366,7 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>     struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
>  
> -   droid_free_local_buffers(dri2_surf);
> +   dri2_egl_surface_free_local_buffers(dri2_surf);
>  
>     if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
>        if (dri2_surf->buffer)
> @@ -447,7 +413,7 @@ update_buffers(struct dri2_egl_surface *dri2_surf)
>     /* free outdated buffers and update the surface size */
>     if (dri2_surf->base.Width != dri2_surf->buffer->width ||
>         dri2_surf->base.Height != dri2_surf->buffer->height) {
> -      droid_free_local_buffers(dri2_surf);
> +      dri2_egl_surface_free_local_buffers(dri2_surf);
>        dri2_surf->base.Width = dri2_surf->buffer->width;
>        dri2_surf->base.Height = dri2_surf->buffer->height;
>     }
> @@ -970,7 +936,7 @@ droid_get_buffers_parse_attachments(struct dri2_egl_surface *dri2_surf,
>        case __DRI_BUFFER_ACCUM:
>        case __DRI_BUFFER_DEPTH_STENCIL:
>        case __DRI_BUFFER_HIZ:
> -         local = droid_alloc_local_buffer(dri2_surf,
> +         local = dri2_egl_surface_alloc_local_buffer(dri2_surf,
>                 attachments[i], attachments[i + 1]);
>  
>           if (local) {
> diff --git a/src/egl/drivers/dri2/platform_drm.c b/src/egl/drivers/dri2/platform_drm.c
> index a952aa5456..6895d2fbfe 100644
> --- a/src/egl/drivers/dri2/platform_drm.c
> +++ b/src/egl/drivers/dri2/platform_drm.c
> @@ -196,11 +196,7 @@ dri2_drm_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
>  	 gbm_bo_destroy(dri2_surf->color_buffers[i].bo);
>     }
>  
> -   for (unsigned i = 0; i < __DRI_BUFFER_COUNT; i++) {
> -      if (dri2_surf->dri_buffers[i])
> -         dri2_dpy->dri2->releaseBuffer(dri2_dpy->dri_screen,
> -                                       dri2_surf->dri_buffers[i]);
> -   }
> +   dri2_egl_surface_free_local_buffers(dri2_surf);
>  
>     free(surf);
>  
> @@ -291,39 +287,18 @@ back_bo_to_dri_buffer(struct dri2_egl_surface *dri2_surf, __DRIbuffer *buffer)
>     buffer->flags = 0;
>  }
>  
> -static int
> -get_aux_bo(struct dri2_egl_surface *dri2_surf,
> -	   unsigned int attachment, unsigned int format, __DRIbuffer *buffer)
> -{
> -   struct dri2_egl_display *dri2_dpy =
> -      dri2_egl_display(dri2_surf->base.Resource.Display);
> -   __DRIbuffer *b = dri2_surf->dri_buffers[attachment];
> -
> -   if (b == NULL) {
> -      b = dri2_dpy->dri2->allocateBuffer(dri2_dpy->dri_screen,
> -					 attachment, format,
> -					 dri2_surf->base.Width,
> -					 dri2_surf->base.Height);
> -      dri2_surf->dri_buffers[attachment] = b;
> -   }
> -   if (b == NULL)
> -      return -1;
> -
> -   memcpy(buffer, b, sizeof *buffer);
> -
> -   return 0;
> -}
> -
>  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));
>  
> @@ -336,11 +311,14 @@ dri2_drm_get_buffers_with_format(__DRIdrawable *driDrawable,
>           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) {
> +	    _eglError(EGL_BAD_ALLOC, "failed to allocate local buffer");
>  	    return NULL;
>  	 }
> +	 dri2_surf->buffers[j] = *local;
>  	 break;
>        }
>     }
> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
> index 73966b7c50..54de55ba4c 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -282,13 +282,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);
>  
>     if (dri2_surf->throttle_callback)
>        wl_callback_destroy(dri2_surf->throttle_callback);
> @@ -335,13 +330,8 @@ dri2_wl_release_buffers(struct dri2_egl_surface *dri2_surf)
>        dri2_surf->color_buffers[i].locked = false;
>     }
>  
> -   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);
>  }
>  
>  static int
> @@ -496,29 +486,6 @@ back_bo_to_dri_buffer(struct dri2_egl_surface *dri2_surf, __DRIbuffer *buffer)
>  }
>  
>  static int
> -get_aux_bo(struct dri2_egl_surface *dri2_surf,
> -           unsigned int attachment, unsigned int format, __DRIbuffer *buffer)
> -{
> -   struct dri2_egl_display *dri2_dpy =
> -      dri2_egl_display(dri2_surf->base.Resource.Display);
> -   __DRIbuffer *b = dri2_surf->dri_buffers[attachment];
> -
> -   if (b == NULL) {
> -      b = dri2_dpy->dri2->allocateBuffer(dri2_dpy->dri_screen,
> -                                         attachment, format,
> -                                         dri2_surf->base.Width,
> -                                         dri2_surf->base.Height);
> -      dri2_surf->dri_buffers[attachment] = b;
> -   }
> -   if (b == NULL)
> -      return -1;
> -
> -   memcpy(buffer, b, sizeof *buffer);
> -
> -   return 0;
> -}
> -
> -static int
>  update_buffers(struct dri2_egl_surface *dri2_surf)
>  {
>     struct dri2_egl_display *dri2_dpy =
> @@ -572,16 +539,21 @@ dri2_wl_get_buffers_with_format(__DRIdrawable * driDrawable,
>        return NULL;
>  
>     for (i = 0, j = 0; i < 2 * count; i += 2, j++) {
> +      __DRIbuffer *local;
> +
>        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) {
> +            _eglError(EGL_BAD_ALLOC, "failed to allocate local buffer");
>              return NULL;
>           }
> +         dri2_surf->buffers[j] = *local;
>           break;
>        }
>     }
> -- 
> 2.13.3
> 


More information about the mesa-dev mailing list