Re: [Mesa-dev] [PATCH] dri3: Only update number of back buffers in loader_dri3_get_buffers
On 2018-05-05 12:59 PM, Jason Ekstrand wrote:
> On Wed, May 2, 2018 at 2:35 AM, Michel Dänzer wrote:
>
>> From: Michel Dänzer
>>
>> And only free no longer needed back buffers there as well.
>>
>> We want to stick to the same back buffer throughout a frame, otherwise
>> we can run into various issues.
>>
>> Bugzilla: https://bugs.freedesktop.org/105906
>> Fixes: 3160cb86aa92 "egl/x11: Re-allocate buffers if format is suboptimal"
>> Reported-by: Sergii Romantsov
>> Tested-by: Eero Tamminen
>> Acked-by: Daniel Stone
>> Signed-off-by: Michel Dänzer
>> ---
>> src/loader/loader_dri3_helper.c | 19 +++
>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_
>> helper.c
>> index 23729f7ecb2..6db8303d26d 100644
>> --- a/src/loader/loader_dri3_helper.c
>> +++ b/src/loader/loader_dri3_helper.c
>> @@ -420,13 +420,6 @@ dri3_handle_present_event(struct
>> loader_dri3_drawable *draw,
>>
>> if (buf && buf->pixmap == ie->pixmap)
>> buf->busy = 0;
>> -
>> - if (buf && draw->cur_blit_source != b && !buf->busy &&
>> - (buf->reallocate ||
>> - (draw->num_back <= b && b < LOADER_DRI3_MAX_BACK))) {
>> -dri3_free_render_buffer(draw, buf);
>> -draw->buffers[b] = NULL;
>> - }
>>}
>>break;
>> }
>> @@ -559,7 +552,6 @@ dri3_find_back(struct loader_dri3_drawable *draw)
>> /* Check whether we need to reuse the current back buffer as new back.
>> * In that case, wait until it's not busy anymore.
>> */
>> - dri3_update_num_back(draw);
>> num_to_consider = draw->num_back;
>> if (!loader_dri3_have_image_blit(draw) && draw->cur_blit_source !=
>> -1) {
>>num_to_consider = 1;
>> @@ -1815,6 +1807,7 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
>> {
>> struct loader_dri3_drawable *draw = loaderPrivate;
>> struct loader_dri3_buffer *front, *back;
>> + int buf_id;
>>
>> buffers->image_mask = 0;
>> buffers->front = NULL;
>> @@ -1826,6 +1819,16 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
>> if (!dri3_update_drawable(driDrawable, draw))
>>return false;
>>
>> + dri3_update_num_back(draw);
>> +
>> + /* Free no longer needed back buffers */
>> + for (buf_id = draw->num_back; buf_id < LOADER_DRI3_MAX_BACK; buf_id++)
>> {
>> + if (draw->cur_blit_source != buf_id && draw->buffers[buf_id]) {
>> + dri3_free_render_buffer(draw, draw->buffers[buf_id]);
>> + draw->buffers[buf_id] = NULL;
>> + }
>> + }
>>
>
> Moving this hear means that dri3_update_num_back is no longer getting
> called from dri3_find_back_alloc which is used by swapbuffers_msc and
> copy_sub_buffer. Is this intended?
Indeed, it is. E.g. in swap_buffers_msc, if dri3_update_num_back changes
draw->num_back from 3 to 2, we might throw away a finished frame and
present random other contents instead.
--
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
Re: [Mesa-dev] [PATCH] dri3: Only update number of back buffers in loader_dri3_get_buffers
On Wed, May 2, 2018 at 2:35 AM, Michel Dänzer wrote:
> From: Michel Dänzer
>
> And only free no longer needed back buffers there as well.
>
> We want to stick to the same back buffer throughout a frame, otherwise
> we can run into various issues.
>
> Bugzilla: https://bugs.freedesktop.org/105906
> Fixes: 3160cb86aa92 "egl/x11: Re-allocate buffers if format is suboptimal"
> Reported-by: Sergii Romantsov
> Tested-by: Eero Tamminen
> Acked-by: Daniel Stone
> Signed-off-by: Michel Dänzer
> ---
> src/loader/loader_dri3_helper.c | 19 +++
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_
> helper.c
> index 23729f7ecb2..6db8303d26d 100644
> --- a/src/loader/loader_dri3_helper.c
> +++ b/src/loader/loader_dri3_helper.c
> @@ -420,13 +420,6 @@ dri3_handle_present_event(struct
> loader_dri3_drawable *draw,
>
> if (buf && buf->pixmap == ie->pixmap)
> buf->busy = 0;
> -
> - if (buf && draw->cur_blit_source != b && !buf->busy &&
> - (buf->reallocate ||
> - (draw->num_back <= b && b < LOADER_DRI3_MAX_BACK))) {
> -dri3_free_render_buffer(draw, buf);
> -draw->buffers[b] = NULL;
> - }
>}
>break;
> }
> @@ -559,7 +552,6 @@ dri3_find_back(struct loader_dri3_drawable *draw)
> /* Check whether we need to reuse the current back buffer as new back.
> * In that case, wait until it's not busy anymore.
> */
> - dri3_update_num_back(draw);
> num_to_consider = draw->num_back;
> if (!loader_dri3_have_image_blit(draw) && draw->cur_blit_source !=
> -1) {
>num_to_consider = 1;
> @@ -1815,6 +1807,7 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
> {
> struct loader_dri3_drawable *draw = loaderPrivate;
> struct loader_dri3_buffer *front, *back;
> + int buf_id;
>
> buffers->image_mask = 0;
> buffers->front = NULL;
> @@ -1826,6 +1819,16 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
> if (!dri3_update_drawable(driDrawable, draw))
>return false;
>
> + dri3_update_num_back(draw);
> +
> + /* Free no longer needed back buffers */
> + for (buf_id = draw->num_back; buf_id < LOADER_DRI3_MAX_BACK; buf_id++)
> {
> + if (draw->cur_blit_source != buf_id && draw->buffers[buf_id]) {
> + dri3_free_render_buffer(draw, draw->buffers[buf_id]);
> + draw->buffers[buf_id] = NULL;
> + }
> + }
>
Moving this hear means that dri3_update_num_back is no longer getting
called from dri3_find_back_alloc which is used by swapbuffers_msc and
copy_sub_buffer. Is this intended?
> +
> /* pixmaps always have front buffers.
> * Exchange swaps also mandate fake front buffers.
> */
> --
> 2.17.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] dri3: Only update number of back buffers in loader_dri3_get_buffers
From: Michel Dänzer
And only free no longer needed back buffers there as well.
We want to stick to the same back buffer throughout a frame, otherwise
we can run into various issues.
Bugzilla: https://bugs.freedesktop.org/105906
Fixes: 3160cb86aa92 "egl/x11: Re-allocate buffers if format is suboptimal"
Reported-by: Sergii Romantsov
Tested-by: Eero Tamminen
Acked-by: Daniel Stone
Signed-off-by: Michel Dänzer
---
src/loader/loader_dri3_helper.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 23729f7ecb2..6db8303d26d 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -420,13 +420,6 @@ dri3_handle_present_event(struct loader_dri3_drawable
*draw,
if (buf && buf->pixmap == ie->pixmap)
buf->busy = 0;
-
- if (buf && draw->cur_blit_source != b && !buf->busy &&
- (buf->reallocate ||
- (draw->num_back <= b && b < LOADER_DRI3_MAX_BACK))) {
-dri3_free_render_buffer(draw, buf);
-draw->buffers[b] = NULL;
- }
}
break;
}
@@ -559,7 +552,6 @@ dri3_find_back(struct loader_dri3_drawable *draw)
/* Check whether we need to reuse the current back buffer as new back.
* In that case, wait until it's not busy anymore.
*/
- dri3_update_num_back(draw);
num_to_consider = draw->num_back;
if (!loader_dri3_have_image_blit(draw) && draw->cur_blit_source != -1) {
num_to_consider = 1;
@@ -1815,6 +1807,7 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
{
struct loader_dri3_drawable *draw = loaderPrivate;
struct loader_dri3_buffer *front, *back;
+ int buf_id;
buffers->image_mask = 0;
buffers->front = NULL;
@@ -1826,6 +1819,16 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
if (!dri3_update_drawable(driDrawable, draw))
return false;
+ dri3_update_num_back(draw);
+
+ /* Free no longer needed back buffers */
+ for (buf_id = draw->num_back; buf_id < LOADER_DRI3_MAX_BACK; buf_id++) {
+ if (draw->cur_blit_source != buf_id && draw->buffers[buf_id]) {
+ dri3_free_render_buffer(draw, draw->buffers[buf_id]);
+ draw->buffers[buf_id] = NULL;
+ }
+ }
+
/* pixmaps always have front buffers.
* Exchange swaps also mandate fake front buffers.
*/
--
2.17.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] dri3: Only update number of back buffers in loader_dri3_get_buffers
On 27 April 2018 at 16:56, Michel Dänzer wrote: > And only free no longer needed back buffers there as well. > > We want to stick to the same back buffer throughout a frame, otherwise > we can run into various issues. Thanks for dealing with this Michel! Acked-by: Daniel Stone ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] dri3: Only update number of back buffers in loader_dri3_get_buffers
Hi,
On 27.04.2018 18:56, Michel Dänzer wrote:
From: Michel Dänzer
And only free no longer needed back buffers there as well.
We want to stick to the same back buffer throughout a frame, otherwise
we can run into various issues.
Bugzilla: https://bugs.freedesktop.org/105906
Fixes: 3160cb86aa92 "egl/x11: Re-allocate buffers if format is suboptimal"
Signed-off-by: Michel Dänzer
---
Sergii, Eero, here's an alternative fix which I think is cleaner, can
you test it?
Tested on KBL GT2.
Without the patch, there are >30 compiz crashes within ~3h test run.
With the patch, same compiz process works fine for the whole run.
-> looks good to me.
Tested-by: Eero Tamminen
- Eero
src/loader/loader_dri3_helper.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 23729f7ecb2..6db8303d26d 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -420,13 +420,6 @@ dri3_handle_present_event(struct loader_dri3_drawable
*draw,
if (buf && buf->pixmap == ie->pixmap)
buf->busy = 0;
-
- if (buf && draw->cur_blit_source != b && !buf->busy &&
- (buf->reallocate ||
- (draw->num_back <= b && b < LOADER_DRI3_MAX_BACK))) {
-dri3_free_render_buffer(draw, buf);
-draw->buffers[b] = NULL;
- }
}
break;
}
@@ -559,7 +552,6 @@ dri3_find_back(struct loader_dri3_drawable *draw)
/* Check whether we need to reuse the current back buffer as new back.
* In that case, wait until it's not busy anymore.
*/
- dri3_update_num_back(draw);
num_to_consider = draw->num_back;
if (!loader_dri3_have_image_blit(draw) && draw->cur_blit_source != -1) {
num_to_consider = 1;
@@ -1815,6 +1807,7 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
{
struct loader_dri3_drawable *draw = loaderPrivate;
struct loader_dri3_buffer *front, *back;
+ int buf_id;
buffers->image_mask = 0;
buffers->front = NULL;
@@ -1826,6 +1819,16 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
if (!dri3_update_drawable(driDrawable, draw))
return false;
+ dri3_update_num_back(draw);
+
+ /* Free no longer needed back buffers */
+ for (buf_id = draw->num_back; buf_id < LOADER_DRI3_MAX_BACK; buf_id++) {
+ if (draw->cur_blit_source != buf_id && draw->buffers[buf_id]) {
+ dri3_free_render_buffer(draw, draw->buffers[buf_id]);
+ draw->buffers[buf_id] = NULL;
+ }
+ }
+
/* pixmaps always have front buffers.
* Exchange swaps also mandate fake front buffers.
*/
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] dri3: Only update number of back buffers in loader_dri3_get_buffers
On 2018-04-30 08:58 AM, Sergii Romantsov wrote: > Hello, > with my simple tests it works, Thanks for testing. > but have some remarks (but don't have much experience with dri3): > > 1. It looks little dangerous to move logic of destroying buffers and seems > in your version we are loosing some logic with 'busy' and 'num_back' The purpose of the busy field is to prevent re-using a buffer in dri3_find_back while it's still being presented. It's not directly relevant for destroying buffers. The new code in loader_dri3_get_buffers does take num_back into account, so I'm not sure what issue you're seeing related to that? > 2. And also looks like buffers just will not be destroyed on time for some > cases. While that's possible in theory, it's only if the application stops rendering any more frames, which is unlikely in practice. And even if it happens, it can only result in retaining three back buffers instead of two. > 3. From my opinion it will be safer to use in function 'dri3_get_buffer' > call (with my patch): > 'dri3_fence_await(draw->conn, NULL, buffer)' instead of > 'dri3_fence_await(draw->conn, draw, buffer)' Assuming that can be done without re-introducing the issues addressed by commit a727c804a2c17db306c68e259ae845aa6382d3b1 "loader/dri3: Process event after each fence wait" (you'd have to discuss that with Thomas), I think my patch would be the better basis for it anyway. I'm sorry that I only realized so late what really should be done here. Thank you for pointing at the issue. -- 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
Re: [Mesa-dev] [PATCH] dri3: Only update number of back buffers in loader_dri3_get_buffers
Hello,
with my simple tests it works, but have some remarks (but don't have much
experience with dri3):
1. It looks little dangerous to move logic of destroying buffers and seems
in your version we are loosing some logic with 'busy' and 'num_back'
2. And also looks like buffers just will not be destroyed on time for some
cases.
3. From my opinion it will be safer to use in function 'dri3_get_buffer'
call (with my patch):
'dri3_fence_await(draw->conn, NULL, buffer)' instead of
'dri3_fence_await(draw->conn, draw, buffer)'
On Fri, Apr 27, 2018 at 6:56 PM, Michel Dänzer wrote:
> From: Michel Dänzer
>
> And only free no longer needed back buffers there as well.
>
> We want to stick to the same back buffer throughout a frame, otherwise
> we can run into various issues.
>
> Bugzilla: https://bugs.freedesktop.org/105906
> Fixes: 3160cb86aa92 "egl/x11: Re-allocate buffers if format is suboptimal"
> Signed-off-by: Michel Dänzer
> ---
>
> Sergii, Eero, here's an alternative fix which I think is cleaner, can
> you test it?
>
> src/loader/loader_dri3_helper.c | 19 +++
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_
> helper.c
> index 23729f7ecb2..6db8303d26d 100644
> --- a/src/loader/loader_dri3_helper.c
> +++ b/src/loader/loader_dri3_helper.c
> @@ -420,13 +420,6 @@ dri3_handle_present_event(struct
> loader_dri3_drawable *draw,
>
> if (buf && buf->pixmap == ie->pixmap)
> buf->busy = 0;
> -
> - if (buf && draw->cur_blit_source != b && !buf->busy &&
> - (buf->reallocate ||
> - (draw->num_back <= b && b < LOADER_DRI3_MAX_BACK))) {
> -dri3_free_render_buffer(draw, buf);
> -draw->buffers[b] = NULL;
> - }
>}
>break;
> }
> @@ -559,7 +552,6 @@ dri3_find_back(struct loader_dri3_drawable *draw)
> /* Check whether we need to reuse the current back buffer as new back.
> * In that case, wait until it's not busy anymore.
> */
> - dri3_update_num_back(draw);
> num_to_consider = draw->num_back;
> if (!loader_dri3_have_image_blit(draw) && draw->cur_blit_source !=
> -1) {
>num_to_consider = 1;
> @@ -1815,6 +1807,7 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
> {
> struct loader_dri3_drawable *draw = loaderPrivate;
> struct loader_dri3_buffer *front, *back;
> + int buf_id;
>
> buffers->image_mask = 0;
> buffers->front = NULL;
> @@ -1826,6 +1819,16 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
> if (!dri3_update_drawable(driDrawable, draw))
>return false;
>
> + dri3_update_num_back(draw);
> +
> + /* Free no longer needed back buffers */
> + for (buf_id = draw->num_back; buf_id < LOADER_DRI3_MAX_BACK; buf_id++)
> {
> + if (draw->cur_blit_source != buf_id && draw->buffers[buf_id]) {
> + dri3_free_render_buffer(draw, draw->buffers[buf_id]);
> + draw->buffers[buf_id] = NULL;
> + }
> + }
> +
> /* pixmaps always have front buffers.
> * Exchange swaps also mandate fake front buffers.
> */
> --
> 2.17.0
>
>
--
Sergii Romantsov
GlobalLogic Inc.
www.globallogic.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] dri3: Only update number of back buffers in loader_dri3_get_buffers
From: Michel Dänzer
And only free no longer needed back buffers there as well.
We want to stick to the same back buffer throughout a frame, otherwise
we can run into various issues.
Bugzilla: https://bugs.freedesktop.org/105906
Fixes: 3160cb86aa92 "egl/x11: Re-allocate buffers if format is suboptimal"
Signed-off-by: Michel Dänzer
---
Sergii, Eero, here's an alternative fix which I think is cleaner, can
you test it?
src/loader/loader_dri3_helper.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 23729f7ecb2..6db8303d26d 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -420,13 +420,6 @@ dri3_handle_present_event(struct loader_dri3_drawable
*draw,
if (buf && buf->pixmap == ie->pixmap)
buf->busy = 0;
-
- if (buf && draw->cur_blit_source != b && !buf->busy &&
- (buf->reallocate ||
- (draw->num_back <= b && b < LOADER_DRI3_MAX_BACK))) {
-dri3_free_render_buffer(draw, buf);
-draw->buffers[b] = NULL;
- }
}
break;
}
@@ -559,7 +552,6 @@ dri3_find_back(struct loader_dri3_drawable *draw)
/* Check whether we need to reuse the current back buffer as new back.
* In that case, wait until it's not busy anymore.
*/
- dri3_update_num_back(draw);
num_to_consider = draw->num_back;
if (!loader_dri3_have_image_blit(draw) && draw->cur_blit_source != -1) {
num_to_consider = 1;
@@ -1815,6 +1807,7 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
{
struct loader_dri3_drawable *draw = loaderPrivate;
struct loader_dri3_buffer *front, *back;
+ int buf_id;
buffers->image_mask = 0;
buffers->front = NULL;
@@ -1826,6 +1819,16 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
if (!dri3_update_drawable(driDrawable, draw))
return false;
+ dri3_update_num_back(draw);
+
+ /* Free no longer needed back buffers */
+ for (buf_id = draw->num_back; buf_id < LOADER_DRI3_MAX_BACK; buf_id++) {
+ if (draw->cur_blit_source != buf_id && draw->buffers[buf_id]) {
+ dri3_free_render_buffer(draw, draw->buffers[buf_id]);
+ draw->buffers[buf_id] = NULL;
+ }
+ }
+
/* pixmaps always have front buffers.
* Exchange swaps also mandate fake front buffers.
*/
--
2.17.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
