[Mesa-dev] [PATCH] egl/dri2: refactor dri2_query_surface, swrastGetDrawableInfo

[Eric Engestrom]

On Tuesday, 2017-08-08 11:31:36 +0300, Tapani Pälli wrote:
> Currently swrastGetDrawableInfo always initializes w and h, patch
> refactors function as x11_get_drawable_info that returns success and
> sets the values only if no error happened. Add swrastGetDrawableInfo
> wrapper function as expected by DRI extension.
> 
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> Reported-by: Emil Velikov <emil.velikov at collabora.com>

Couple notes below, but with that:
Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>

> ---
>  src/egl/drivers/dri2/platform_x11.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_x11.c b/src/egl/drivers/dri2/platform_x11.c
> index 4ce819f..80f2477 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -99,8 +99,8 @@ swrastDestroyDrawable(struct dri2_egl_display * dri2_dpy,
>     xcb_free_gc(dri2_dpy->conn, dri2_surf->swapgc);
>  }
>  
> -static void
> -swrastGetDrawableInfo(__DRIdrawable * draw,
> +static bool
> +x11_get_drawable_info(__DRIdrawable * draw,
>                        int *x, int *y, int *w, int *h,
>                        void *loaderPrivate)
>  {
> @@ -110,12 +110,12 @@ swrastGetDrawableInfo(__DRIdrawable * draw,
>     xcb_get_geometry_cookie_t cookie;
>     xcb_get_geometry_reply_t *reply;
>     xcb_generic_error_t *error;
> +   bool ret = false;

Don't initialise `ret` here...

>  
> -   *x = *y = *w = *h = 0;
>     cookie = xcb_get_geometry (dri2_dpy->conn, dri2_surf->drawable);
>     reply = xcb_get_geometry_reply (dri2_dpy->conn, cookie, &error);
>     if (reply == NULL)
> -      return;
> +      return false;
>  
>     if (error != NULL) {
>        _eglLog(_EGL_WARNING, "error in xcb_get_geometry");

but set it here:

+   ret = false;

> @@ -125,8 +125,18 @@ swrastGetDrawableInfo(__DRIdrawable * draw,
>        *y = reply->y;
>        *w = reply->width;
>        *h = reply->height;
> +      ret = true;
>     }
>     free(reply);
> +   return ret;
> +}
> +
> +static void
> +swrastGetDrawableInfo(__DRIdrawable * draw,
> +                      int *x, int *y, int *w, int *h,
> +                      void *loaderPrivate)
> +{

+   *x = *y = *w = *h = 0;

All the callers (swrast & drisw) expect these to be set unconditionally,
so we should initialise them here.

(Note that __glXDRIGetDrawableInfo() is the only impl to not honour that;
might need to be fixed.)

> +   x11_get_drawable_info(draw, x, y, w, h, loaderPrivate);
>  }
>  
>  static void
> @@ -412,15 +422,14 @@ dri2_query_surface(_EGLDriver *drv, _EGLDisplay *dpy,
>  {
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
>     struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> -   int x, y, w = -1, h = -1;
> +   int x, y, w, h;
>  
>     __DRIdrawable *drawable = dri2_dpy->vtbl->get_dri_drawable(surf);
>  
>     switch (attribute) {
>     case EGL_WIDTH:
>     case EGL_HEIGHT:
> -      swrastGetDrawableInfo(drawable, &x, &y, &w, &h, dri2_surf);
> -      if (w != -1 && h != -1) {
> +      if (x11_get_drawable_info(drawable, &x, &y, &w, &h, dri2_surf)) {
>           surf->Width = w;
>           surf->Height = h;
>        }
> -- 
> 2.9.4
>