Re: [Mesa-dev] [PATCH] egl/dri2: dri2_make_current: Release previous context's display

[Александр]

On 10.08.2016 10:44, Michel Dänzer wrote:

On 10/08/16 03:00 PM, Nicolas Boichat wrote:

eglMakeCurrent can also be used to change the active display. In that
case, we need to decrement ref_count of the previous display (possibly
destroying it), and increment it on the next display.

Also, old_dsurf/old_rsurf cannot be non-NULL if old_ctx is NULL, so
we only need to test if old_ctx is non-NULL.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97214
Fixes: 9ee683f877 (egl/dri2: Add reference count for dri2_egl_display)
Cc: "12.0" 
Reported-by: Alexandr Zelinsky 
Tested-by: Alexandr Zelinsky 
Signed-off-by: Nicolas Boichat 
---
  src/egl/drivers/dri2/egl_dri2.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 3205a36..701e42a 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1285,8 +1285,10 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
  
if (!unbind)

   dri2_dpy->ref_count++;
-  if (old_dsurf || old_rsurf || old_ctx)
- dri2_display_release(disp);
+  if (old_ctx) {
+ EGLDisplay old_disp = _eglGetDisplayHandle(old_ctx->Resource.Display);
+ dri2_display_release(old_disp);
+  }

Unfortunately, this change breaks the piglit test "spec@egl
1.4@eglterminate then unbind context", because old_ctx != NULL but
old_ctx->Resource.Display == NULL. Modifying the test to

   if (old_ctx && old_ctx->Resource.Display) {

fixes the regression and doesn't seem to cause any other problems.
Alexandr, does the patch still fix your problem with that modification?


Nicolas, this regression is also reproducible with
LIBGL_ALWAYS_SOFTWARE=1 . Please get used to testing your changes like
that and only send out changes for review which don't cause any regressions.



yesstill fix the problem
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] egl/dri2: dri2_make_current: Release previous context's display

[Nicolas Boichat]

On Thu, Aug 11, 2016 at 12:10 AM, Nicolas Boichat  wrote:
> On Wed, Aug 10, 2016 at 9:44 AM, Michel Dänzer  wrote:
>> On 10/08/16 03:00 PM, Nicolas Boichat wrote:
>>> eglMakeCurrent can also be used to change the active display. In that
>>> case, we need to decrement ref_count of the previous display (possibly
>>> destroying it), and increment it on the next display.
>>>
>>> Also, old_dsurf/old_rsurf cannot be non-NULL if old_ctx is NULL, so
>>> we only need to test if old_ctx is non-NULL.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97214
>>> Fixes: 9ee683f877 (egl/dri2: Add reference count for dri2_egl_display)
>>> Cc: "12.0" 
>>> Reported-by: Alexandr Zelinsky 
>>> Tested-by: Alexandr Zelinsky 
>>> Signed-off-by: Nicolas Boichat 
>>> ---
>>>  src/egl/drivers/dri2/egl_dri2.c | 6 --
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/egl/drivers/dri2/egl_dri2.c 
>>> b/src/egl/drivers/dri2/egl_dri2.c
>>> index 3205a36..701e42a 100644
>>> --- a/src/egl/drivers/dri2/egl_dri2.c
>>> +++ b/src/egl/drivers/dri2/egl_dri2.c
>>> @@ -1285,8 +1285,10 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay 
>>> *disp, _EGLSurface *dsurf,
>>>
>>>if (!unbind)
>>>   dri2_dpy->ref_count++;
>>> -  if (old_dsurf || old_rsurf || old_ctx)
>>> - dri2_display_release(disp);
>>> +  if (old_ctx) {
>>> + EGLDisplay old_disp = 
>>> _eglGetDisplayHandle(old_ctx->Resource.Display);
>>> + dri2_display_release(old_disp);
>>> +  }
>>
>> Unfortunately, this change breaks the piglit test "spec@egl
>> 1.4@eglterminate then unbind context", because old_ctx != NULL but
>> old_ctx->Resource.Display == NULL. Modifying the test to
>>
>>   if (old_ctx && old_ctx->Resource.Display) {
>>
>> fixes the regression and doesn't seem to cause any other problems.
>
> This is probably wrong as the display will leak (it definitely should
> be freed after calling eglTerminate + eglMakeCurrent(NULL)).
>
> I think I know where the problem is (the call to
> _eglReleaseDisplayResources happens too early), I'm not sure what's
> the best solution. I'll have a look.

Turns out we destroy old_ctx just above, and then use it again here,
fix is simple (swap a few lines).

Patch v2 to follow.

Thanks!

>> Alexandr, does the patch still fix your problem with that modification?
>>
>>
>> Nicolas, this regression is also reproducible with
>> LIBGL_ALWAYS_SOFTWARE=1 . Please get used to testing your changes like
>> that and only send out changes for review which don't cause any regressions.
>
> Managed to build piglit, and run it using a locally built mesa. Are
> the commands below what you would use?
>
> export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$MESA_DIR/lib
> export LIBGL_DRIVERS_PATH=$MESA_DIR/lib/gallium
> export EGL_DRIVERS_PATH=$MESA_DIR/lib
> export EGL_LOG_LEVEL=debug
> export LIBGL_ALWAYS_SOFTWARE=1
> ./piglit run -p x11_egl -t "spec@egl.*" all results/master

I realized that running with --valgrind is quite useful (it's slow,
but since there are only 20+ tests, it's fine).
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] egl/dri2: dri2_make_current: Release previous context's display

[Nicolas Boichat]

On Wed, Aug 10, 2016 at 9:44 AM, Michel Dänzer  wrote:
> On 10/08/16 03:00 PM, Nicolas Boichat wrote:
>> eglMakeCurrent can also be used to change the active display. In that
>> case, we need to decrement ref_count of the previous display (possibly
>> destroying it), and increment it on the next display.
>>
>> Also, old_dsurf/old_rsurf cannot be non-NULL if old_ctx is NULL, so
>> we only need to test if old_ctx is non-NULL.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97214
>> Fixes: 9ee683f877 (egl/dri2: Add reference count for dri2_egl_display)
>> Cc: "12.0" 
>> Reported-by: Alexandr Zelinsky 
>> Tested-by: Alexandr Zelinsky 
>> Signed-off-by: Nicolas Boichat 
>> ---
>>  src/egl/drivers/dri2/egl_dri2.c | 6 --
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.c 
>> b/src/egl/drivers/dri2/egl_dri2.c
>> index 3205a36..701e42a 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.c
>> +++ b/src/egl/drivers/dri2/egl_dri2.c
>> @@ -1285,8 +1285,10 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
>> _EGLSurface *dsurf,
>>
>>if (!unbind)
>>   dri2_dpy->ref_count++;
>> -  if (old_dsurf || old_rsurf || old_ctx)
>> - dri2_display_release(disp);
>> +  if (old_ctx) {
>> + EGLDisplay old_disp = 
>> _eglGetDisplayHandle(old_ctx->Resource.Display);
>> + dri2_display_release(old_disp);
>> +  }
>
> Unfortunately, this change breaks the piglit test "spec@egl
> 1.4@eglterminate then unbind context", because old_ctx != NULL but
> old_ctx->Resource.Display == NULL. Modifying the test to
>
>   if (old_ctx && old_ctx->Resource.Display) {
>
> fixes the regression and doesn't seem to cause any other problems.

This is probably wrong as the display will leak (it definitely should
be freed after calling eglTerminate + eglMakeCurrent(NULL)).

I think I know where the problem is (the call to
_eglReleaseDisplayResources happens too early), I'm not sure what's
the best solution. I'll have a look.

> Alexandr, does the patch still fix your problem with that modification?
>
>
> Nicolas, this regression is also reproducible with
> LIBGL_ALWAYS_SOFTWARE=1 . Please get used to testing your changes like
> that and only send out changes for review which don't cause any regressions.

Managed to build piglit, and run it using a locally built mesa. Are
the commands below what you would use?

export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$MESA_DIR/lib
export LIBGL_DRIVERS_PATH=$MESA_DIR/lib/gallium
export EGL_DRIVERS_PATH=$MESA_DIR/lib
export EGL_LOG_LEVEL=debug
export LIBGL_ALWAYS_SOFTWARE=1
./piglit run -p x11_egl -t "spec@egl.*" all results/master

Best,

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

Re: [Mesa-dev] [PATCH] egl/dri2: dri2_make_current: Release previous context's display

[Michel Dänzer]

On 10/08/16 03:00 PM, Nicolas Boichat wrote:
> eglMakeCurrent can also be used to change the active display. In that
> case, we need to decrement ref_count of the previous display (possibly
> destroying it), and increment it on the next display.
> 
> Also, old_dsurf/old_rsurf cannot be non-NULL if old_ctx is NULL, so
> we only need to test if old_ctx is non-NULL.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97214
> Fixes: 9ee683f877 (egl/dri2: Add reference count for dri2_egl_display)
> Cc: "12.0" 
> Reported-by: Alexandr Zelinsky 
> Tested-by: Alexandr Zelinsky 
> Signed-off-by: Nicolas Boichat 
> ---
>  src/egl/drivers/dri2/egl_dri2.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 3205a36..701e42a 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1285,8 +1285,10 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
> _EGLSurface *dsurf,
>  
>if (!unbind)
>   dri2_dpy->ref_count++;
> -  if (old_dsurf || old_rsurf || old_ctx)
> - dri2_display_release(disp);
> +  if (old_ctx) {
> + EGLDisplay old_disp = 
> _eglGetDisplayHandle(old_ctx->Resource.Display);
> + dri2_display_release(old_disp);
> +  }

Unfortunately, this change breaks the piglit test "spec@egl
1.4@eglterminate then unbind context", because old_ctx != NULL but
old_ctx->Resource.Display == NULL. Modifying the test to

  if (old_ctx && old_ctx->Resource.Display) {

fixes the regression and doesn't seem to cause any other problems.
Alexandr, does the patch still fix your problem with that modification?


Nicolas, this regression is also reproducible with
LIBGL_ALWAYS_SOFTWARE=1 . Please get used to testing your changes like
that and only send out changes for review which don't cause any regressions.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] egl/dri2: dri2_make_current: Release previous context's display

[Nicolas Boichat]

eglMakeCurrent can also be used to change the active display. In that
case, we need to decrement ref_count of the previous display (possibly
destroying it), and increment it on the next display.

Also, old_dsurf/old_rsurf cannot be non-NULL if old_ctx is NULL, so
we only need to test if old_ctx is non-NULL.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97214
Fixes: 9ee683f877 (egl/dri2: Add reference count for dri2_egl_display)
Cc: "12.0" 
Reported-by: Alexandr Zelinsky 
Tested-by: Alexandr Zelinsky 
Signed-off-by: Nicolas Boichat 
---
 src/egl/drivers/dri2/egl_dri2.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 3205a36..701e42a 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1285,8 +1285,10 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
 
   if (!unbind)
  dri2_dpy->ref_count++;
-  if (old_dsurf || old_rsurf || old_ctx)
- dri2_display_release(disp);
+  if (old_ctx) {
+ EGLDisplay old_disp = _eglGetDisplayHandle(old_ctx->Resource.Display);
+ dri2_display_release(old_disp);
+  }
 
   return EGL_TRUE;
} else {
-- 
2.8.0.rc3.226.g39d4020

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