On 17 October 2017 at 15:07, Eric Engestrom <eric.engest...@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 <elong...@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.veli...@collabora.com>
Thanks
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
