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

[Tapani Pälli]

On 08/08/2017 12:00 PM, Eric Engestrom wrote:
> 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;

ok, will change

>> @@ -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.

argh this initialization was the plan but I forgot it, thanks! :)

> (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
>>