[Mesa-dev] [PATCH v2 2/8] egl: refactor color_buffers structure for deduplicating

[Emil Velikov]

On 6 October 2017 at 22:38, Gwan-gyeong Mun <elongbug at gmail.com> wrote:
> This is added for preventing adding of new color buffers structure and back*
> when new platform backend is added.
> This refactoring separates out the common and platform specific bits.
> This makes odd casting in the platform_foo.c but it prevents adding of new
> ifdef magic.
> Because of color_buffers array size is different on android and wayland /drm,
> it adds COLOR_BUFFERS_SIZE macro.
>  - android's color buffers array size is 3.
>    drm & wayland's color buffers array size is 4.
>
> Fixes from Rob's review:
>  - refactor to separate out the common and platform specific bits.
>
> Fixes from Emil's review:
>  - use suggested color buffers structure shape.
>    it makes a buffer pointer of each platform to void pointer type.
>
> Signed-off-by: Mun Gwan-gyeong <elongbug at gmail.com>
> ---
>  src/egl/drivers/dri2/egl_dri2.h         | 30 +++++++++---------
>  src/egl/drivers/dri2/platform_android.c | 10 +++---
>  src/egl/drivers/dri2/platform_drm.c     | 55 +++++++++++++++++----------------
>  src/egl/drivers/dri2/platform_wayland.c | 46 +++++++++++++--------------
>  4 files changed, 71 insertions(+), 70 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index 017895f0d9..08ccf24410 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -65,6 +65,15 @@ struct zwp_linux_dmabuf_v1;
>
>  #endif /* HAVE_ANDROID_PLATFORM */
>
> +#if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM)
The guard should be if Android ->3, 4 otherwise.

> +#define COLOR_BUFFERS_SIZE 4
> +#else
> +       /* Usually Android uses at most triple buffers in ANativeWindow
> +        * so hardcode the number of color_buffers to 3.
> +        */
Props for keeping the comment around. Please drop the indentation.

> +#define COLOR_BUFFERS_SIZE 3
> +#endif
> +
>  #include "eglconfig.h"
>  #include "eglcontext.h"
>  #include "egldisplay.h"
> @@ -286,39 +295,28 @@ struct dri2_egl_surface
>     /* EGL-owned buffers */
>     __DRIbuffer           *local_buffers[__DRI_BUFFER_COUNT];
>
> -#if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM)
> +   /* Used to record all the buffers created by each platform's native window
> +    * and their ages.
> +    */
>     struct {
> +      void *native_buffer; // aka wl_buffer/gbm_bo/ANativeWindowBuffer
>  #ifdef HAVE_WAYLAND_PLATFORM
I would drop this guard. Sure it will make the struct tiny bit larger,
but it will allow us to have a more generic and widespread helpers.

The rest of the patch should use a handful of:
 - drop unneeded $native_type -> void * cast
 - create the local native_buffer of $native_type and cast on assignment

Some partial examples follow:

> --- a/src/egl/drivers/dri2/platform_drm.c
> +++ b/src/egl/drivers/dri2/platform_drm.c
> @@ -53,7 +53,7 @@ lock_front_buffer(struct gbm_surface *_surf)
>        return NULL;
>     }
>
> -   bo = dri2_surf->current->bo;
> +   bo = (struct gbm_bo *)dri2_surf->current->native_buffer;
>
Unneeded cast?

>     for (unsigned i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
> -      if (dri2_surf->color_buffers[i].bo)
> -        gbm_bo_destroy(dri2_surf->color_buffers[i].bo);
> +      if (dri2_surf->color_buffers[i].native_buffer)
> +        gbm_bo_destroy((struct gbm_bo *)dri2_surf->color_buffers[i].native_buffer);
Ditto.

>     }
>
>     dri2_egl_surface_free_local_buffers(dri2_surf);
> @@ -204,23 +204,24 @@ get_back_bo(struct dri2_egl_surface *dri2_surf)
>
>     if (dri2_surf->back == NULL)
>        return -1;
> -   if (dri2_surf->back->bo == NULL) {
> +   if (dri2_surf->back->native_buffer == NULL) {
>        if (surf->base.modifiers)
> -         dri2_surf->back->bo = gbm_bo_create_with_modifiers(&dri2_dpy->gbm_dri->base,
> -                                                            surf->base.width,
> -                                                            surf->base.height,
> -                                                            surf->base.format,
> -                                                            surf->base.modifiers,
> -                                                            surf->base.count);
> +         dri2_surf->back->native_buffer =
> +            (void *)gbm_bo_create_with_modifiers(&dri2_dpy->gbm_dri->base,
> +                                                 surf->base.width,
> +                                                 surf->base.height,
> +                                                 surf->base.format,
> +                                                 surf->base.modifiers,
> +                                                 surf->base.count);
>        else
> -         dri2_surf->back->bo = gbm_bo_create(&dri2_dpy->gbm_dri->base,
> -                                             surf->base.width,
> -                                             surf->base.height,
> -                                             surf->base.format,
> -                                             surf->base.flags);
> +         dri2_surf->back->native_buffer = (void *)gbm_bo_create(&dri2_dpy->gbm_dri->base,
> +                                                                surf->base.width,
> +                                                                surf->base.height,
> +                                                                surf->base.format,
> +                                                                surf->base.flags);
>
struct gbm_bo *bo;

if (foo)
   bo = gbm_bo...()
else
   bo = gbm_bo...()

if (bo == NULL)
   return -1;

dri2_surf->back->native_buffer = (void *)bo;


> @@ -1521,7 +1521,7 @@ static EGLBoolean
>  dri2_wl_swrast_allocate_buffer(struct dri2_egl_surface *dri2_surf,
>                                 int format, int w, int h,
>                                 void **data, int *size,
> -                               struct wl_buffer **buffer)
> +                               void **buffer)
No need to change the type?

-Emil