Re: [Mesa-dev] [PATCH] egl: refactor color_buffers structure for deduplicating (v2)

2017-11-20 Thread Rob Herring 
On Wed, Nov 15, 2017 at 9:27 AM, 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.
>
> v2: Fixes from Emil's review
>   a) change ifdef macro of "HAVE_WAYLAND_PLATFORM or HAVE_DRM_PLATFORM" to
>  "HAVE_ANDROID_PLATFORM" for COLOR_BUFFERS_SIZE.
>   b) drop the unneeded indentation of comment.
>   c) drop ifdef macro of HAVE_WAYLAND_PLATFORM from color_buffers structure
>  for more generic and widespread helpers.
>   d) drop unneeded $native_type -> void * cast and viceversa.
>   e) create the local native_buffer of $native_type and cast on assignment.
>
> Signed-off-by: Mun Gwan-gyeong 
> ---
>  src/egl/drivers/dri2/egl_dri2.h | 32 --
>  src/egl/drivers/dri2/platform_android.c | 10 +++---
>  src/egl/drivers/dri2/platform_drm.c | 60 
> ++---
>  src/egl/drivers/dri2/platform_wayland.c | 41 +++---
>  4 files changed, 73 insertions(+), 70 deletions(-)

Reviewed-by: Rob Herring 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] egl: refactor color_buffers structure for deduplicating (v2)

2017-11-16 Thread Tomasz Figa 
Hi Gwan-gyeong,

Thanks for the patch!

On Wed, Nov 15, 2017 at 11:27 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.
>
> v2: Fixes from Emil's review
>   a) change ifdef macro of "HAVE_WAYLAND_PLATFORM or HAVE_DRM_PLATFORM" to
>  "HAVE_ANDROID_PLATFORM" for COLOR_BUFFERS_SIZE.
>   b) drop the unneeded indentation of comment.
>   c) drop ifdef macro of HAVE_WAYLAND_PLATFORM from color_buffers structure
>  for more generic and widespread helpers.
>   d) drop unneeded $native_type -> void * cast and viceversa.
>   e) create the local native_buffer of $native_type and cast on assignment.
>
> Signed-off-by: Mun Gwan-gyeong 

The Android part looks good to me:

Reviewed-by: Tomasz Figa 

I think this patch actually enables us to do some further cleanup in
Android part. I'll try to send follow up patches in near future.

Best regards,
Tomasz

> ---
>  src/egl/drivers/dri2/egl_dri2.h | 32 --
>  src/egl/drivers/dri2/platform_android.c | 10 +++---
>  src/egl/drivers/dri2/platform_drm.c | 60 
> ++---
>  src/egl/drivers/dri2/platform_wayland.c | 41 +++---
>  4 files changed, 73 insertions(+), 70 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index 0ec8f44dce..cbeedadd4b 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 */
>
> +#ifdef HAVE_ANDROID_PLATFORM
> +/* Usually Android uses at most triple buffers in ANativeWindow so hardcode
> + * the number of color_buffers to 3.
> + */
> +#define COLOR_BUFFERS_SIZE 3
> +#else
> +#define COLOR_BUFFERS_SIZE 4
> +#endif
> +
>  #include "eglconfig.h"
>  #include "eglcontext.h"
>  #include "egldisplay.h"
> @@ -280,39 +289,26 @@ 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 {
> -#ifdef HAVE_WAYLAND_PLATFORM
> -  struct wl_buffer   *wl_buffer;
> +  void *native_buffer; // aka wl_buffer/gbm_bo/ANativeWindowBuffer
>__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 63223e9a69..24e5ddebf9 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 = dri2_surf->buffer;
>}
> -  if (dri2_surf->color_buffers[i].buffer == dri2_surf->buffer) {
> +  if (dri2_surf->color_buffers[i].native_buffer == dri2_surf->buffer) {
>

[Mesa-dev] [PATCH] egl: refactor color_buffers structure for deduplicating (v2)

2017-11-15 Thread 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.

v2: Fixes from Emil's review
  a) change ifdef macro of "HAVE_WAYLAND_PLATFORM or HAVE_DRM_PLATFORM" to
 "HAVE_ANDROID_PLATFORM" for COLOR_BUFFERS_SIZE.
  b) drop the unneeded indentation of comment.
  c) drop ifdef macro of HAVE_WAYLAND_PLATFORM from color_buffers structure
 for more generic and widespread helpers.
  d) drop unneeded $native_type -> void * cast and viceversa.
  e) create the local native_buffer of $native_type and cast on assignment.

Signed-off-by: Mun Gwan-gyeong 
---
 src/egl/drivers/dri2/egl_dri2.h | 32 --
 src/egl/drivers/dri2/platform_android.c | 10 +++---
 src/egl/drivers/dri2/platform_drm.c | 60 ++---
 src/egl/drivers/dri2/platform_wayland.c | 41 +++---
 4 files changed, 73 insertions(+), 70 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
index 0ec8f44dce..cbeedadd4b 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 */
 
+#ifdef HAVE_ANDROID_PLATFORM
+/* Usually Android uses at most triple buffers in ANativeWindow so hardcode
+ * the number of color_buffers to 3.
+ */
+#define COLOR_BUFFERS_SIZE 3
+#else
+#define COLOR_BUFFERS_SIZE 4
+#endif
+
 #include "eglconfig.h"
 #include "eglcontext.h"
 #include "egldisplay.h"
@@ -280,39 +289,26 @@ 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 {
-#ifdef HAVE_WAYLAND_PLATFORM
-  struct wl_buffer   *wl_buffer;
+  void *native_buffer; // aka wl_buffer/gbm_bo/ANativeWindowBuffer
   __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 63223e9a69..24e5ddebf9 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 = dri2_surf->buffer;
   }
-  if (dri2_surf->color_buffers[i].buffer == dri2_surf->buffer) {
+  if (dri2_surf->color_buffers[i].native_buffer == 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 =