[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 <eric.engestrom at imgtec.com> wrote:
> On Friday, 2017-10-06 21:38:35 +0000, Gwan-gyeong Mun wrote:
>> This deduplicates free routines of color_buffers array.
>>
>> Signed-off-by: Mun Gwan-gyeong <elongbug at gmail.com>
>> ---
>>  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 <emil.velikov at collabora.com>

Thanks
Emil