Hi Gwan-gyeong Mun,
Thanks for sorting this out. Having a single copy instead of 3+ is always a win.
On 2 August 2017 at 10:23, Gwan-gyeong Mun <elong...@gmail.com> wrote:
> platform_drm, platform_wayland and platform_android have similiar local buffer
> allocation routines. For deduplicating, it unifies dri2_egl_surface's
> local buffer allocation routines. And it polishes inconsistent indentations.
>
> Signed-off-by: Mun Gwan-gyeong <elong...@gmail.com>
> ---
> src/egl/drivers/dri2/egl_dri2.c | 40 ++++++++++++++++++++
> src/egl/drivers/dri2/egl_dri2.h | 16 ++++++--
> src/egl/drivers/dri2/platform_android.c | 40 ++------------------
> src/egl/drivers/dri2/platform_drm.c | 65
> +++++++++++----------------------
> src/egl/drivers/dri2/platform_wayland.c | 52 ++++++--------------------
> 5 files changed, 89 insertions(+), 124 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index a197e0456f..6ee3b36739 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -972,6 +972,46 @@ dri2_display_destroy(_EGLDisplay *disp)
> disp->DriverData = NULL;
> }
>
> +__DRIbuffer *
> +dri2_egl_surface_alloc_local_buffer(struct dri2_egl_surface *dri2_surf,
> + unsigned int att, unsigned int format)
> +{
> +#if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM) ||
> defined(HAVE_ANDROID_PLATFORM)
I think we can make this unconditional - a few bytes of extra code [in
the corner case], should be that bad.
[...]
> +void
> +dri2_egl_surface_free_local_buffers(struct dri2_egl_surface *dri2_surf)
> +{
> +#if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM) ||
> defined(HAVE_ANDROID_PLATFORM)
Ditto.
[...]
> +#if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM) ||
> defined(HAVE_ANDROID_PLATFORM)
> + /* EGL-owned buffers */
> + __DRIbuffer *local_buffers[__DRI_BUFFER_COUNT];
> +#endif
> +
... and here.
[...]
> static __DRIbuffer *
> dri2_drm_get_buffers_with_format(__DRIdrawable *driDrawable,
> - int *width, int *height,
> - unsigned int *attachments, int count,
> - int *out_count, void *loaderPrivate)
> + int *width, int *height,
> + unsigned int *attachments, int count,
> + int *out_count, void *loaderPrivate)
> {
> struct dri2_egl_surface *dri2_surf = loaderPrivate;
> int i, j;
>
> for (i = 0, j = 0; i < 2 * count; i += 2, j++) {
> + __DRIbuffer *local;
> +
> assert(attachments[i] < __DRI_BUFFER_COUNT);
> assert(j < ARRAY_SIZE(dri2_surf->buffers));
>
> switch (attachments[i]) {
> case __DRI_BUFFER_BACK_LEFT:
> - if (get_back_bo(dri2_surf) < 0) {
> - _eglError(EGL_BAD_ALLOC, "failed to allocate color buffer");
> - return NULL;
> - }
> + if (get_back_bo(dri2_surf) < 0) {
> + _eglError(EGL_BAD_ALLOC, "failed to allocate color buffer");
> + return NULL;
> + }
> back_bo_to_dri_buffer(dri2_surf, &dri2_surf->buffers[j]);
> - break;
> + break;
Unrelated whitespace changes?
> default:
> - if (get_aux_bo(dri2_surf, attachments[i], attachments[i + 1],
> - &dri2_surf->buffers[j]) < 0) {
> - _eglError(EGL_BAD_ALLOC, "failed to allocate aux buffer");
> - return NULL;
> - }
> - break;
> + local = dri2_egl_surface_alloc_local_buffer(dri2_surf,
> attachments[i],
> + attachments[i + 1]);
> +
> + if (local)
> + dri2_surf->buffers[j] = *local;
> + else {
> + _eglError(EGL_BAD_ALLOC, "failed to allocate local buffer");
> + return NULL;
> + }
Please preserve the original codeflow:
if (condition) {
error_path
return
}
normal_path
[...]
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -287,13 +287,8 @@ dri2_wl_destroy_surface(_EGLDriver *drv, _EGLDisplay
> *disp, _EGLSurface *surf)
> dri2_surf->color_buffers[i].data_size);
> }
>
> - if (dri2_dpy->dri2) {
> - for (int i = 0; i < __DRI_BUFFER_COUNT; i++)
> - if (dri2_surf->dri_buffers[i] &&
> - dri2_surf->dri_buffers[i]->attachment != __DRI_BUFFER_BACK_LEFT)
> - dri2_dpy->dri2->releaseBuffer(dri2_dpy->dri_screen,
> - dri2_surf->dri_buffers[i]);
> - }
> + if (dri2_dpy->dri2)
> + dri2_egl_surface_free_local_buffers(dri2_surf);
This seems like a functionality change, since the new helper is
missing the __DRI_BUFFER_BACK_LEFT check.
In reality that should not cause issues, since
dri2_wl_get_buffers_with_format() makes sure we don't have such buffer
in local_buffers.
Other platforms are more relaxed - missing the check all togethre.
Tl;Dr; Things should be fine, but elaborate why/how in the commit message.
Thinking out loud:
How can we ensure nobody overflows dri2_surf->buffers[], should we bother?
[...]
> return NULL;
>
> for (i = 0, j = 0; i < 2 * count; i += 2, j++) {
> + __DRIbuffer *local;
Add blank line.
> switch (attachments[i]) {
> case __DRI_BUFFER_BACK_LEFT:
> back_bo_to_dri_buffer(dri2_surf, &dri2_surf->buffers[j]);
> break;
> default:
> - if (get_aux_bo(dri2_surf, attachments[i], attachments[i + 1],
> - &dri2_surf->buffers[j]) < 0) {
> - _eglError(EGL_BAD_ALLOC, "failed to allocate aux buffer");
> + local = dri2_egl_surface_alloc_local_buffer(dri2_surf,
> attachments[i],
> + attachments[i + 1]);
> +
> + if (local)
> + dri2_surf->buffers[j] = *local;
> + else {
> + _eglError(EGL_BAD_ALLOC, "failed to allocate local buffer");
Please preserve original codeflow.
With the above addressed the patch is
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
