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

[Emil Velikov]

On 17 October 2017 at 21:49, Gurchetan Singh
 wrote:
> Can you wrap color_buffers around:
>
> #if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM) ||
> defined(HAVE_ANDROID_PLATFORM)
>
> flags?  This is because platform_surfaceless has no native surfaces and it's
> good to be explicit in that regard.
>
Analogous to my reply in 1/8, I'd vote to keep it.

Sure surfaceless does not have native surfaces, yet
 - one should be able to swap the separate __DRIimage *{front,back?}
with the struct
 - using the locked/age variables + associated helpers one could
implement EXT_buffer_age for the platform

Just an idea for the future and I think it sounds reasonable?
-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Gurchetan Singh]

Can you wrap color_buffers around:

#if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM)
|| defined(HAVE_ANDROID_PLATFORM)

flags?  This is because platform_surfaceless has no native surfaces and
it's good to be explicit in that regard.

On Fri, Oct 6, 2017 at 2:38 PM, Gwan-gyeong Mun  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 
> ---
>  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)
> +#define COLOR_BUFFERS_SIZE 4
> +#else
> +   /* Usually Android uses at most triple buffers in ANativeWindow
> +* so hardcode the number of color_buffers to 3.
> +*/
> +#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
> -  struct wl_buffer   *wl_buffer;
>__DRIimage *dri_image;
>/* for is_different_gpu case. NULL else */
>__DRIimage *linear_copy;
>/* for swrast */
>void *data;
>int data_size;
> -#endif
> -#ifdef HAVE_DRM_PLATFORM
> -  struct gbm_bo   *bo;
>  #endif
>boollocked;
>int age;
> -   } color_buffers[4], *back, *current;
> -#endif
> +   } color_buffers[COLOR_BUFFERS_SIZE], *back, *current;
>
>  #ifdef HAVE_ANDROID_PLATFORM
> struct ANativeWindow *window;
> struct ANativeWindowBuffer *buffer;
> __DRIimage *dri_image_back;
> __DRIimage *dri_image_front;
> -
> -   /* 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.
> -*/
> -   struct {
> -  struct ANativeWindowBuffer *buffer;
> -  int age;
> -   } color_buffers[3], *back;
>  #endif
>
>  #if defined(HAVE_SURFACELESS_PLATFORM)
> diff --git a/src/egl/drivers/dri2/platform_android.c
> b/src/egl/drivers/dri2/platform_android.c
> index 0acbb38bd8..67e739c1fc 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -193,10 +193,10 @@ droid_window_dequeue_buffer(struct dri2_egl_surface
> *dri2_surf)
>  */
> EGLBoolean updated = EGL_FALSE;
> for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
> -  if (!dri2_surf->color_buffers[i].buffer) {
> - dri2_surf->color_buffers[i].buffer = dri2_surf->buffer;
> +  if (!dri2_surf->color_buffers[i].native_buffer) {
> + dri2_surf->color_buffers[i].native_buffer = (void
> *)dri2_surf->buffer;
>}
> -  if (dri2_surf->color_buffers[i].buffer == dri2_surf->buffer) {
> +  if (dri2_surf->color_buffers[i].native_buffer == (void
> *)dri2_surf->buffer) {
>   dri2_surf->back = _surf->color_buffers[i];
>   updated = EGL_TRUE;
>   break;
> @@ -208,10 +208,10 @@ droid_window_dequeue_buffer(struct dri2_egl_surface
> *dri2_surf)
> * the color_buffers
> */
>for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
> - dri2_surf->color_buffers[i].buffer = NULL;
> + dri2_surf->color_buffers[i].native_buffer = NULL;
>   

Re: [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  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 
> ---
>  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(_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(_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(_dpy->gbm_dri->base,
> - surf->base.width,
> - surf->base.height,
> - 

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

[Gwan-gyeong Mun]

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 
---
 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)
+#define COLOR_BUFFERS_SIZE 4
+#else
+   /* Usually Android uses at most triple buffers in ANativeWindow
+* so hardcode the number of color_buffers to 3.
+*/
+#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
-  struct wl_buffer   *wl_buffer;
   __DRIimage *dri_image;
   /* for is_different_gpu case. NULL else */
   __DRIimage *linear_copy;
   /* for swrast */
   void *data;
   int data_size;
-#endif
-#ifdef HAVE_DRM_PLATFORM
-  struct gbm_bo   *bo;
 #endif
   boollocked;
   int age;
-   } color_buffers[4], *back, *current;
-#endif
+   } color_buffers[COLOR_BUFFERS_SIZE], *back, *current;
 
 #ifdef HAVE_ANDROID_PLATFORM
struct ANativeWindow *window;
struct ANativeWindowBuffer *buffer;
__DRIimage *dri_image_back;
__DRIimage *dri_image_front;
-
-   /* 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.
-*/
-   struct {
-  struct ANativeWindowBuffer *buffer;
-  int age;
-   } color_buffers[3], *back;
 #endif
 
 #if defined(HAVE_SURFACELESS_PLATFORM)
diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index 0acbb38bd8..67e739c1fc 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -193,10 +193,10 @@ droid_window_dequeue_buffer(struct dri2_egl_surface 
*dri2_surf)
 */
EGLBoolean updated = EGL_FALSE;
for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
-  if (!dri2_surf->color_buffers[i].buffer) {
- dri2_surf->color_buffers[i].buffer = dri2_surf->buffer;
+  if (!dri2_surf->color_buffers[i].native_buffer) {
+ dri2_surf->color_buffers[i].native_buffer = (void *)dri2_surf->buffer;
   }
-  if (dri2_surf->color_buffers[i].buffer == dri2_surf->buffer) {
+  if (dri2_surf->color_buffers[i].native_buffer == (void 
*)dri2_surf->buffer) {
  dri2_surf->back = _surf->color_buffers[i];
  updated = EGL_TRUE;
  break;
@@ -208,10 +208,10 @@ droid_window_dequeue_buffer(struct dri2_egl_surface 
*dri2_surf)
* the color_buffers
*/
   for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
- dri2_surf->color_buffers[i].buffer = NULL;
+ dri2_surf->color_buffers[i].native_buffer = NULL;
  dri2_surf->color_buffers[i].age = 0;
   }
-  dri2_surf->color_buffers[0].buffer = dri2_surf->buffer;
+  dri2_surf->color_buffers[0].native_buffer = (void *)dri2_surf->buffer;
   dri2_surf->back = _surf->color_buffers[0];
}
 
diff --git a/src/egl/drivers/dri2/platform_drm.c 
b/src/egl/drivers/dri2/platform_drm.c
index 9005f5dd9e..3527352bab 100644
--- 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 =