Re: [Mesa-dev] [PATCH] egl/dri2: refactor dri2_query_surface, swrastGetDrawableInfo
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
Reported-by: Emil Velikov
Couple notes below, but with that:
Reviewed-by: Eric Engestrom
---
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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/dri2: refactor dri2_query_surface, swrastGetDrawableInfo
On 8 August 2017 at 10:00, 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
>> Reported-by: Emil Velikov
>
> Couple notes below, but with that:
> Reviewed-by: Eric Engestrom
>
>> ---
>> 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.
>
Initialize preemptively or set when x11_get_drawable_info() fails -
either one will do.
With Eric's suggestions
Reviewed-by: Emil Velikov
> (Note that __glXDRIGetDrawableInfo() is the only impl to not honour that;
> might need to be fixed.)
>
AFAICT the function returns false on error, we should be fine.
Plus it's a legacy DRI1 codepath, so personally I will be a bit
cautious about subtle changes in behaviour
-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/dri2: refactor dri2_query_surface, swrastGetDrawableInfo
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
> Reported-by: Emil Velikov
Couple notes below, but with that:
Reviewed-by: Eric Engestrom
> ---
> 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
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
