Re: [Mesa-dev] [PATCH] dri3: Prevent multiple freeing of buffers.

[Sergii Romantsov]

Hello (ping) Thomas and Daniel,
any more suggestions? As i can see Eero has done a test-run and v2 also
looks good.

As alternative solution we also may try to call '*dri3_fence_await(draw->conn,
draw, buffer)'* with '*draw*'-parameter as '*NULL'.*
What do you think?

On Tue, Apr 10, 2018 at 11:10 AM, Sergii Romantsov <
sergii.romant...@gmail.com> wrote:

> Hello,
> i've updated patch simply,
> but that seems requires additional checking because in call 
> *dri3_handle_present_event
> *potentially may happens reset of '*busy*' with condition '*buf->pixmap
> == ie->pixmap*'
>
> On Fri, Apr 6, 2018 at 9:03 PM, Thomas Hellstrom 
> wrote:
>
>> Hi,
>>
>>
>> On 04/06/2018 04:51 PM, Daniel Stone wrote:
>>
>>> Hi Sergii,
>>>
>>> On 6 April 2018 at 09:12, Sergii Romantsov 
>>> wrote:
>>>
 Commit 3160cb86aa92 adds optimization with flag 'reallocate'.
 Processing of flag causes buffers freeing while pointer
 is still hold in caller stack and than again used to be freed.

>>> Thanks a lot for writing this. I take it the core of the problem is
>>> that dri3_handle_present_event() can be called whilst we're inside
>>> dri3_get_buffer(), which wasn't the case before.
>>>
>>> This was only introduced as of a727c804a2c1, and I'm not sure I fully
>>> follow the rationale for that commit. Thomas, why do we need to
>>> process the events? I guess we could also fake it by turning 'busy'
>>> into a refcount, which would be incremented/decremented as it is today
>>> when posting buffers and getting Idle events, but also when we're
>>> holding a local pointer which we can't have stolen from under us.
>>>
>>> Cheers,
>>> Daniel
>>>
>>
>> The motivation for this commit IIRC was that with internal glretrace
>> automated tests, we typically would end up with corrupt rendering due to
>> invalid viewports after window resizes. The resize events were typically
>> not picked up as fast with dri3 as with dri2, so due to the lack of
>> documented strategy how to handle window- and viewport resizes with dri3
>> clients, I tried to make it mimic dri2 where we had no such issues. The
>> reason for the slow pick up was that dri3 was waiting for fences rather
>> than on X replies...
>>
>> /Thomas
>>
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] dri3: Prevent multiple freeing of buffers.

[Sergii Romantsov]

Hello,
i've updated patch simply,
but that seems requires additional checking because in call
*dri3_handle_present_event
*potentially may happens reset of '*busy*' with condition '*buf->pixmap ==
ie->pixmap*'

On Fri, Apr 6, 2018 at 9:03 PM, Thomas Hellstrom 
wrote:

> Hi,
>
>
> On 04/06/2018 04:51 PM, Daniel Stone wrote:
>
>> Hi Sergii,
>>
>> On 6 April 2018 at 09:12, Sergii Romantsov 
>> wrote:
>>
>>> Commit 3160cb86aa92 adds optimization with flag 'reallocate'.
>>> Processing of flag causes buffers freeing while pointer
>>> is still hold in caller stack and than again used to be freed.
>>>
>> Thanks a lot for writing this. I take it the core of the problem is
>> that dri3_handle_present_event() can be called whilst we're inside
>> dri3_get_buffer(), which wasn't the case before.
>>
>> This was only introduced as of a727c804a2c1, and I'm not sure I fully
>> follow the rationale for that commit. Thomas, why do we need to
>> process the events? I guess we could also fake it by turning 'busy'
>> into a refcount, which would be incremented/decremented as it is today
>> when posting buffers and getting Idle events, but also when we're
>> holding a local pointer which we can't have stolen from under us.
>>
>> Cheers,
>> Daniel
>>
>
> The motivation for this commit IIRC was that with internal glretrace
> automated tests, we typically would end up with corrupt rendering due to
> invalid viewports after window resizes. The resize events were typically
> not picked up as fast with dri3 as with dri2, so due to the lack of
> documented strategy how to handle window- and viewport resizes with dri3
> clients, I tried to make it mimic dri2 where we had no such issues. The
> reason for the slow pick up was that dri3 was waiting for fences rather
> than on X replies...
>
> /Thomas
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] dri3: Prevent multiple freeing of buffers.

[Thomas Hellstrom]

Hi,

On 04/06/2018 04:51 PM, Daniel Stone wrote:

Hi Sergii,

On 6 April 2018 at 09:12, Sergii Romantsov  wrote:

Commit 3160cb86aa92 adds optimization with flag 'reallocate'.
Processing of flag causes buffers freeing while pointer
is still hold in caller stack and than again used to be freed.

Thanks a lot for writing this. I take it the core of the problem is
that dri3_handle_present_event() can be called whilst we're inside
dri3_get_buffer(), which wasn't the case before.

This was only introduced as of a727c804a2c1, and I'm not sure I fully
follow the rationale for that commit. Thomas, why do we need to
process the events? I guess we could also fake it by turning 'busy'
into a refcount, which would be incremented/decremented as it is today
when posting buffers and getting Idle events, but also when we're
holding a local pointer which we can't have stolen from under us.

Cheers,
Daniel


The motivation for this commit IIRC was that with internal glretrace 
automated tests, we typically would end up with corrupt rendering due to 
invalid viewports after window resizes. The resize events were typically 
not picked up as fast with dri3 as with dri2, so due to the lack of 
documented strategy how to handle window- and viewport resizes with dri3 
clients, I tried to make it mimic dri2 where we had no such issues. The 
reason for the slow pick up was that dri3 was waiting for fences rather 
than on X replies...


/Thomas

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

Re: [Mesa-dev] [PATCH] dri3: Prevent multiple freeing of buffers.

[Daniel Stone]

Hi Sergii,

On 6 April 2018 at 09:12, Sergii Romantsov  wrote:
> Commit 3160cb86aa92 adds optimization with flag 'reallocate'.
> Processing of flag causes buffers freeing while pointer
> is still hold in caller stack and than again used to be freed.

Thanks a lot for writing this. I take it the core of the problem is
that dri3_handle_present_event() can be called whilst we're inside
dri3_get_buffer(), which wasn't the case before.

This was only introduced as of a727c804a2c1, and I'm not sure I fully
follow the rationale for that commit. Thomas, why do we need to
process the events? I guess we could also fake it by turning 'busy'
into a refcount, which would be incremented/decremented as it is today
when posting buffers and getting Idle events, but also when we're
holding a local pointer which we can't have stolen from under us.

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

Re: [Mesa-dev] [PATCH] dri3: Prevent multiple freeing of buffers.

[Eero Tamminen]

Hi,

I tested this on KBL GT2.  Compiz crashes during 3h test run dropped 
from 30 to none.  There were couple of percent changes in few synthetic 
tests, but I think that's just because Compiz is now running properly.


-> looks good!


- Eero

Tested-by: Eero Tamminen 


On 06.04.2018 11:12, Sergii Romantsov wrote:

Commit 3160cb86aa92 adds optimization with flag 'reallocate'.
Processing of flag causes buffers freeing while pointer
is still hold in caller stack and than again used to be freed.

Fixes: 3160cb86aa92 "egl/x11: Re-allocate buffers if format is suboptimal"

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105906
Signed-off-by: Sergii Romantsov 
Tested-by: Andriy Khulap 
---
  src/loader/loader_dri3_helper.c | 7 +--
  src/loader/loader_dri3_helper.h | 1 +
  2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index fe17df1..5f9cc42 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -422,7 +422,7 @@ dri3_handle_present_event(struct loader_dri3_drawable *draw,
  buf->busy = 0;
  
   if (buf && draw->cur_blit_source != b && !buf->busy &&

- (buf->reallocate ||
+ ((buf->reallocate && !buf->in_use_to_destroy) ||
   (draw->num_back <= b && b < LOADER_DRI3_MAX_BACK))) {
  dri3_free_render_buffer(draw, buf);
  draw->buffers[b] = NULL;
@@ -1688,6 +1688,7 @@ dri3_get_buffer(__DRIdrawable *driDrawable,
 (buffer_type == loader_dri3_buffer_front && draw->have_fake_front))
&& buffer) {
  
+ buffer->in_use_to_destroy = true;

   /* Fill the new buffer with data from an old buffer */
   dri3_fence_await(draw->conn, draw, buffer);
   if (!loader_dri3_blit_image(draw,
@@ -1731,6 +1732,7 @@ dri3_get_buffer(__DRIdrawable *driDrawable,
draw->buffers[buf_id] = buffer;
 }
 dri3_fence_await(draw->conn, draw, buffer);
+   buffer = draw->buffers[buf_id];
  
 /*

  * Do we need to preserve the content of a previous buffer?
@@ -1744,7 +1746,8 @@ dri3_get_buffer(__DRIdrawable *driDrawable,
 if (buffer_type == loader_dri3_buffer_back &&
 draw->cur_blit_source != -1 &&
 draw->buffers[draw->cur_blit_source] &&
-   buffer != draw->buffers[draw->cur_blit_source]) {
+   buffer != draw->buffers[draw->cur_blit_source] &&
+   buffer != NULL) {
  
struct loader_dri3_buffer *source = draw->buffers[draw->cur_blit_source];
  
diff --git a/src/loader/loader_dri3_helper.h b/src/loader/loader_dri3_helper.h

index 7e3d829..9232d61 100644
--- a/src/loader/loader_dri3_helper.h
+++ b/src/loader/loader_dri3_helper.h
@@ -62,6 +62,7 @@ struct loader_dri3_buffer {
 bool busy;   /* Set on swap, cleared on IdleNotify */
 bool own_pixmap; /* We allocated the pixmap ID, free on 
destroy */
 bool reallocate; /* Buffer should be reallocated and not 
reused */
+   bool in_use_to_destroy; /* Buffer is in use and will be 
destroyed soon */
  
 uint32_t num_planes;

 uint32_t size;



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