Re: [Mesa-dev] [PATCH v2 8/8] egl/wayland: add dri2_wl_free_buffers() helper

[Emil Velikov]

On 17 October 2017 at 15:07, Eric Engestrom  wrote:
> On Friday, 2017-10-06 21:38:35 +, Gwan-gyeong Mun wrote:
>> This deduplicates free routines of color_buffers array.
>>
>> Signed-off-by: Mun Gwan-gyeong 
>> ---
>>  src/egl/drivers/dri2/platform_wayland.c | 60 
>> +
>>  1 file changed, 31 insertions(+), 29 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/platform_wayland.c 
>> b/src/egl/drivers/dri2/platform_wayland.c
>> index 1518a24b7c..cfe474cf58 100644
>> --- a/src/egl/drivers/dri2/platform_wayland.c
>> +++ b/src/egl/drivers/dri2/platform_wayland.c
>> @@ -253,6 +253,35 @@ dri2_wl_create_pixmap_surface(_EGLDriver *drv, 
>> _EGLDisplay *disp,
>> return NULL;
>>  }
>>
>> +static void
>> +dri2_wl_free_buffers(struct dri2_egl_surface *dri2_surf, bool check_lock)
>> +{
>> +   struct dri2_egl_display *dri2_dpy =
>> +  dri2_egl_display(dri2_surf->base.Resource.Display);
>> +
>> +   for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
>> +  if (dri2_surf->color_buffers[i].native_buffer) {
>> + if (check_lock && !dri2_surf->color_buffers[i].locked)
>> +wl_buffer_destroy((struct wl_buffer 
>> *)dri2_surf->color_buffers[i].native_buffer);
>> + else
>> +wl_buffer_destroy((struct wl_buffer 
>> *)dri2_surf->color_buffers[i].native_buffer);
>
> Both branches have the same code, should be a hint :P
> I think this should be:
>
>if (!check_lock || !dri2_surf->color_buffers[i].locked) {
>   wl_buffer_destroy((struct wl_buffer 
> *)dri2_surf->color_buffers[i].native_buffer);
Drop the unneeded cast?

>   dri2_surf->color_buffers[i].native_buffer = NULL;
>}
>
> without an `else`. You also want to remove the `native_buffer = NULL`
> from below, to avoid leaking locked buffers :)
>
I'm not sure if that's an actual leak. In either case, I'd leave that
as follow-up.

There are a few of instances where this can be used:
 - dri2_wl_destroy_surface, done
 - dri2_wl_release_buffers, done
 - update_buffers, todo
-  swrast_update_buffers, todo

Just an idea that came to mind, don't bother if you don't want to.
 - keep the platform specific buffer destroy (wl_buffer_destroy here)
in the platform code moving the rest as helper
 - plug the platform_android/platform_drm leak (?) by using the helper

Patches 5-7 look ok and are
Reviewed-by: Emil Velikov 

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 8/8] egl/wayland: add dri2_wl_free_buffers() helper

[Eric Engestrom]

On Friday, 2017-10-06 21:38:35 +, Gwan-gyeong Mun wrote:
> This deduplicates free routines of color_buffers array.
> 
> Signed-off-by: Mun Gwan-gyeong 
> ---
>  src/egl/drivers/dri2/platform_wayland.c | 60 
> +
>  1 file changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_wayland.c 
> b/src/egl/drivers/dri2/platform_wayland.c
> index 1518a24b7c..cfe474cf58 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -253,6 +253,35 @@ dri2_wl_create_pixmap_surface(_EGLDriver *drv, 
> _EGLDisplay *disp,
> return NULL;
>  }
>  
> +static void
> +dri2_wl_free_buffers(struct dri2_egl_surface *dri2_surf, bool check_lock)
> +{
> +   struct dri2_egl_display *dri2_dpy =
> +  dri2_egl_display(dri2_surf->base.Resource.Display);
> +
> +   for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
> +  if (dri2_surf->color_buffers[i].native_buffer) {
> + if (check_lock && !dri2_surf->color_buffers[i].locked)
> +wl_buffer_destroy((struct wl_buffer 
> *)dri2_surf->color_buffers[i].native_buffer);
> + else
> +wl_buffer_destroy((struct wl_buffer 
> *)dri2_surf->color_buffers[i].native_buffer);

Both branches have the same code, should be a hint :P
I think this should be:

   if (!check_lock || !dri2_surf->color_buffers[i].locked) {
  wl_buffer_destroy((struct wl_buffer 
*)dri2_surf->color_buffers[i].native_buffer);
  dri2_surf->color_buffers[i].native_buffer = NULL;
   }

without an `else`. You also want to remove the `native_buffer = NULL`
from below, to avoid leaking locked buffers :)

> +  }
> +  if (dri2_surf->color_buffers[i].dri_image)
> + 
> dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].dri_image);
> +  if (dri2_surf->color_buffers[i].linear_copy)
> + 
> dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].linear_copy);
> +  if (dri2_surf->color_buffers[i].data)
> + munmap(dri2_surf->color_buffers[i].data,
> +dri2_surf->color_buffers[i].data_size);
> +
> +  dri2_surf->color_buffers[i].native_buffer = NULL;
> +  dri2_surf->color_buffers[i].dri_image = NULL;
> +  dri2_surf->color_buffers[i].linear_copy = NULL;
> +  dri2_surf->color_buffers[i].data = NULL;
> +  dri2_surf->color_buffers[i].locked = false;
> +   }
> +}
> +
>  /**
>   * Called via eglDestroySurface(), drv->API.DestroySurface().
>   */
> @@ -266,17 +295,7 @@ dri2_wl_destroy_surface(_EGLDriver *drv, _EGLDisplay 
> *disp, _EGLSurface *surf)
>  
> dri2_dpy->core->destroyDrawable(dri2_surf->dri_drawable);
>  
> -   for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
> -  if (dri2_surf->color_buffers[i].native_buffer)
> - wl_buffer_destroy((struct wl_buffer 
> *)dri2_surf->color_buffers[i].native_buffer);
> -  if (dri2_surf->color_buffers[i].dri_image)
> - 
> dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].dri_image);
> -  if (dri2_surf->color_buffers[i].linear_copy)
> - 
> dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].linear_copy);
> -  if (dri2_surf->color_buffers[i].data)
> - munmap(dri2_surf->color_buffers[i].data,
> -dri2_surf->color_buffers[i].data_size);
> -   }
> +   dri2_wl_free_buffers(dri2_surf, false);
>  
> if (dri2_dpy->dri2)
>dri2_egl_surface_free_local_buffers(dri2_surf);
> @@ -308,24 +327,7 @@ dri2_wl_release_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->color_buffers); i++) {
> -  if (dri2_surf->color_buffers[i].native_buffer &&
> -  !dri2_surf->color_buffers[i].locked)
> - wl_buffer_destroy((struct wl_buffer 
> *)dri2_surf->color_buffers[i].native_buffer);
> -  if (dri2_surf->color_buffers[i].dri_image)
> - 
> dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].dri_image);
> -  if (dri2_surf->color_buffers[i].linear_copy)
> - 
> dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].linear_copy);
> -  if (dri2_surf->color_buffers[i].data)
> - munmap(dri2_surf->color_buffers[i].data,
> -dri2_surf->color_buffers[i].data_size);
> -
> -  dri2_surf->color_buffers[i].native_buffer = NULL;
> -  dri2_surf->color_buffers[i].dri_image = NULL;
> -  dri2_surf->color_buffers[i].linear_copy = NULL;
> -  dri2_surf->color_buffers[i].data = NULL;
> -  dri2_surf->color_buffers[i].locked = false;
> -   }
> +   dri2_wl_free_buffers(dri2_surf, true);
>  
> if (dri2_dpy->dri2)
>dri2_egl_surface_free_local_buffers(dri2_surf);
> -- 
> 2.14.2
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org

[Mesa-dev] [PATCH v2 8/8] egl/wayland: add dri2_wl_free_buffers() helper

[Gwan-gyeong Mun]

This deduplicates free routines of color_buffers array.

Signed-off-by: Mun Gwan-gyeong 
---
 src/egl/drivers/dri2/platform_wayland.c | 60 +
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_wayland.c 
b/src/egl/drivers/dri2/platform_wayland.c
index 1518a24b7c..cfe474cf58 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -253,6 +253,35 @@ dri2_wl_create_pixmap_surface(_EGLDriver *drv, _EGLDisplay 
*disp,
return NULL;
 }
 
+static void
+dri2_wl_free_buffers(struct dri2_egl_surface *dri2_surf, bool check_lock)
+{
+   struct dri2_egl_display *dri2_dpy =
+  dri2_egl_display(dri2_surf->base.Resource.Display);
+
+   for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
+  if (dri2_surf->color_buffers[i].native_buffer) {
+ if (check_lock && !dri2_surf->color_buffers[i].locked)
+wl_buffer_destroy((struct wl_buffer 
*)dri2_surf->color_buffers[i].native_buffer);
+ else
+wl_buffer_destroy((struct wl_buffer 
*)dri2_surf->color_buffers[i].native_buffer);
+  }
+  if (dri2_surf->color_buffers[i].dri_image)
+ dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].dri_image);
+  if (dri2_surf->color_buffers[i].linear_copy)
+ 
dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].linear_copy);
+  if (dri2_surf->color_buffers[i].data)
+ munmap(dri2_surf->color_buffers[i].data,
+dri2_surf->color_buffers[i].data_size);
+
+  dri2_surf->color_buffers[i].native_buffer = NULL;
+  dri2_surf->color_buffers[i].dri_image = NULL;
+  dri2_surf->color_buffers[i].linear_copy = NULL;
+  dri2_surf->color_buffers[i].data = NULL;
+  dri2_surf->color_buffers[i].locked = false;
+   }
+}
+
 /**
  * Called via eglDestroySurface(), drv->API.DestroySurface().
  */
@@ -266,17 +295,7 @@ dri2_wl_destroy_surface(_EGLDriver *drv, _EGLDisplay 
*disp, _EGLSurface *surf)
 
dri2_dpy->core->destroyDrawable(dri2_surf->dri_drawable);
 
-   for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
-  if (dri2_surf->color_buffers[i].native_buffer)
- wl_buffer_destroy((struct wl_buffer 
*)dri2_surf->color_buffers[i].native_buffer);
-  if (dri2_surf->color_buffers[i].dri_image)
- dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].dri_image);
-  if (dri2_surf->color_buffers[i].linear_copy)
- 
dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].linear_copy);
-  if (dri2_surf->color_buffers[i].data)
- munmap(dri2_surf->color_buffers[i].data,
-dri2_surf->color_buffers[i].data_size);
-   }
+   dri2_wl_free_buffers(dri2_surf, false);
 
if (dri2_dpy->dri2)
   dri2_egl_surface_free_local_buffers(dri2_surf);
@@ -308,24 +327,7 @@ dri2_wl_release_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->color_buffers); i++) {
-  if (dri2_surf->color_buffers[i].native_buffer &&
-  !dri2_surf->color_buffers[i].locked)
- wl_buffer_destroy((struct wl_buffer 
*)dri2_surf->color_buffers[i].native_buffer);
-  if (dri2_surf->color_buffers[i].dri_image)
- dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].dri_image);
-  if (dri2_surf->color_buffers[i].linear_copy)
- 
dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].linear_copy);
-  if (dri2_surf->color_buffers[i].data)
- munmap(dri2_surf->color_buffers[i].data,
-dri2_surf->color_buffers[i].data_size);
-
-  dri2_surf->color_buffers[i].native_buffer = NULL;
-  dri2_surf->color_buffers[i].dri_image = NULL;
-  dri2_surf->color_buffers[i].linear_copy = NULL;
-  dri2_surf->color_buffers[i].data = NULL;
-  dri2_surf->color_buffers[i].locked = false;
-   }
+   dri2_wl_free_buffers(dri2_surf, true);
 
if (dri2_dpy->dri2)
   dri2_egl_surface_free_local_buffers(dri2_surf);
-- 
2.14.2

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