Re: [Mesa-dev] [PATCH 10/30] egl/dri2: rework dri2_make_current code flow

[Emil Velikov]

On 25 September 2016 at 04:24, Eric Engestrom  wrote:
> On Thu, Aug 25, 2016 at 05:18:32PM +0100, Emil Velikov wrote:
>> From: Emil Velikov 
>>
>> Duplicate a few lines at the expense of making the code-flow clearer
>> while adding a few extra comments ;-)
>>
>> Signed-off-by: Emil Velikov 
>> ---
>>
>> It makes things clearer the way on this end, but if people have better
>> suggestions I would love to hear them.
>>
>> Emil
>> ---
>>  src/egl/drivers/dri2/egl_dri2.c | 43 
>> ++---
>>  1 file changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.c 
>> b/src/egl/drivers/dri2/egl_dri2.c
>> index a481236..d53c9b9 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.c
>> +++ b/src/egl/drivers/dri2/egl_dri2.c
>> @@ -1248,11 +1248,12 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay 
>> *disp, _EGLSurface *dsurf,
>> struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>> struct dri2_egl_context *dri2_ctx = dri2_egl_context(ctx);
>> _EGLContext *old_ctx;
>> +   _EGLDisplay *old_disp = NULL;
>> _EGLSurface *old_dsurf, *old_rsurf;
>> _EGLSurface *tmp_dsurf, *tmp_rsurf;
>> -   __DRIdrawable *ddraw, *rdraw;
>> -   __DRIcontext *cctx;
>> -   EGLBoolean unbind;
>> +   __DRIdrawable *ddraw = (dsurf) ? dri2_dpy->vtbl->get_dri_drawable(dsurf) 
>> : NULL;
>> +   __DRIdrawable *rdraw = (rsurf) ? dri2_dpy->vtbl->get_dri_drawable(rsurf) 
>> : NULL;
>> +   __DRIcontext *cctx = (dri2_ctx) ? dri2_ctx->dri_context : NULL;
>>
>> if (!dri2_dpy)
>>return _eglError(EGL_NOT_INITIALIZED, "eglMakeCurrent");
>> @@ -1263,32 +1264,34 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay 
>> *disp, _EGLSurface *dsurf,
>>return EGL_FALSE;
>> }
>>
>> -   /* flush before context switch */
>> -   if (old_ctx)
>> -  dri2_drv->glFlush();
>> -
>> -   ddraw = (dsurf) ? dri2_dpy->vtbl->get_dri_drawable(dsurf) : NULL;
>> -   rdraw = (rsurf) ? dri2_dpy->vtbl->get_dri_drawable(rsurf) : NULL;
>> -   cctx = (dri2_ctx) ? dri2_ctx->dri_context : NULL;
>> -
>> if (old_ctx) {
>>__DRIcontext *old_cctx = dri2_egl_context(old_ctx)->dri_context;
>> +
>> +  /* flush before context switch */
>> +  dri2_drv->glFlush();
>>dri2_dpy->core->unbindContext(old_cctx);
>> +
>> +  /* Keep track of the old dpy as we'll need it for resource cleanup */
>> +  old_disp = old_ctx->Resource.Display;
>> }
>>
>> -   unbind = (cctx == NULL && ddraw == NULL && rdraw == NULL);
>> +/* There's no new ctx to bind, so just destroy the old resources */
>> +   if (cctx == NULL) {
>> +  dri2_destroy_surface(drv, disp, old_dsurf);
>> +  dri2_destroy_surface(drv, disp, old_rsurf);
>> +  dri2_destroy_context(drv, disp, old_ctx);
>> +  dri2_display_release(old_disp);
>>
>> -   if (unbind || dri2_dpy->core->bindContext(cctx, ddraw, rdraw)) {
>> +  return EGL_TRUE;
>> +   }
>> +   if (dri2_dpy->core->bindContext(cctx, ddraw, rdraw)) {
>
> I'm pretty sure dropping `unbind` changes the behaviour, which means
> this isn't just a refactor.
>
IIRC there was a bunch of hidden assumptions which made the "unbind"
part identical. Then again as-is it's not clear, so adding the
ddraw/rdraw check is better.

> (Also, nit: the grouping on that diff would have been better if you had
> left an empty line between the two `if` :)
>
>>dri2_destroy_surface(drv, disp, old_dsurf);
>>dri2_destroy_surface(drv, disp, old_rsurf);
>> +  dri2_destroy_context(drv, disp, old_ctx);
>> +  dri2_display_release(old_disp);
>
> I think `dri2_display_release()` needs to be in an `if (old_disp)` (or
> `if (old_ctx)`, they're equivalent at this point), as it will
> dereference old_disp.
>
Patch 5/30 should cover that ?

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 10/30] egl/dri2: rework dri2_make_current code flow

[Eric Engestrom]

On Thu, Aug 25, 2016 at 05:18:32PM +0100, Emil Velikov wrote:
> From: Emil Velikov 
> 
> Duplicate a few lines at the expense of making the code-flow clearer
> while adding a few extra comments ;-)
> 
> Signed-off-by: Emil Velikov 
> ---
> 
> It makes things clearer the way on this end, but if people have better
> suggestions I would love to hear them.
> 
> Emil
> ---
>  src/egl/drivers/dri2/egl_dri2.c | 43 
> ++---
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index a481236..d53c9b9 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1248,11 +1248,12 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
> _EGLSurface *dsurf,
> struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> struct dri2_egl_context *dri2_ctx = dri2_egl_context(ctx);
> _EGLContext *old_ctx;
> +   _EGLDisplay *old_disp = NULL;
> _EGLSurface *old_dsurf, *old_rsurf;
> _EGLSurface *tmp_dsurf, *tmp_rsurf;
> -   __DRIdrawable *ddraw, *rdraw;
> -   __DRIcontext *cctx;
> -   EGLBoolean unbind;
> +   __DRIdrawable *ddraw = (dsurf) ? dri2_dpy->vtbl->get_dri_drawable(dsurf) 
> : NULL;
> +   __DRIdrawable *rdraw = (rsurf) ? dri2_dpy->vtbl->get_dri_drawable(rsurf) 
> : NULL;
> +   __DRIcontext *cctx = (dri2_ctx) ? dri2_ctx->dri_context : NULL;
>  
> if (!dri2_dpy)
>return _eglError(EGL_NOT_INITIALIZED, "eglMakeCurrent");
> @@ -1263,32 +1264,34 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
> _EGLSurface *dsurf,
>return EGL_FALSE;
> }
>  
> -   /* flush before context switch */
> -   if (old_ctx)
> -  dri2_drv->glFlush();
> -
> -   ddraw = (dsurf) ? dri2_dpy->vtbl->get_dri_drawable(dsurf) : NULL;
> -   rdraw = (rsurf) ? dri2_dpy->vtbl->get_dri_drawable(rsurf) : NULL;
> -   cctx = (dri2_ctx) ? dri2_ctx->dri_context : NULL;
> -
> if (old_ctx) {
>__DRIcontext *old_cctx = dri2_egl_context(old_ctx)->dri_context;
> +
> +  /* flush before context switch */
> +  dri2_drv->glFlush();
>dri2_dpy->core->unbindContext(old_cctx);
> +
> +  /* Keep track of the old dpy as we'll need it for resource cleanup */
> +  old_disp = old_ctx->Resource.Display;
> }
>  
> -   unbind = (cctx == NULL && ddraw == NULL && rdraw == NULL);
> +/* There's no new ctx to bind, so just destroy the old resources */
> +   if (cctx == NULL) {
> +  dri2_destroy_surface(drv, disp, old_dsurf);
> +  dri2_destroy_surface(drv, disp, old_rsurf);
> +  dri2_destroy_context(drv, disp, old_ctx);
> +  dri2_display_release(old_disp);
>  
> -   if (unbind || dri2_dpy->core->bindContext(cctx, ddraw, rdraw)) {
> +  return EGL_TRUE;
> +   }
> +   if (dri2_dpy->core->bindContext(cctx, ddraw, rdraw)) {

I'm pretty sure dropping `unbind` changes the behaviour, which means
this isn't just a refactor.

(Also, nit: the grouping on that diff would have been better if you had
left an empty line between the two `if` :)

>dri2_destroy_surface(drv, disp, old_dsurf);
>dri2_destroy_surface(drv, disp, old_rsurf);
> +  dri2_destroy_context(drv, disp, old_ctx);
> +  dri2_display_release(old_disp);

I think `dri2_display_release()` needs to be in an `if (old_disp)` (or
`if (old_ctx)`, they're equivalent at this point), as it will
dereference old_disp.

Cheers,
  Eric

>  
> -  if (!unbind)
> - dri2_dpy->ref_count++;
> -  if (old_ctx) {
> - EGLDisplay old_disp = 
> _eglGetDisplayHandle(old_ctx->Resource.Display);
> - dri2_destroy_context(drv, disp, old_ctx);
> - dri2_display_release(old_disp);
> -  }
> +  /* Refcount the new dpy */
> +  dri2_dpy->ref_count++;
>  
>return EGL_TRUE;
> } else {
> -- 
> 2.9.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev