Re: [Mesa-dev] [PATCH 06/11] loader_dri3: Honour the request to preserve back buffer content

[Michel Dänzer]

On 15/08/17 09:45 PM, Thomas Hellstrom wrote:
> On 08/15/2017 09:33 AM, Michel Dänzer wrote:
>> On 11/08/17 11:14 PM, Thomas Hellstrom wrote:
>>> EGL uses the force_copy parameter to loader_dri3_swap_buffers_msc()
>>> to indicate
>>> that it wants to preserve back buffer contents across a buffer swap.
>>>
>>> While the loader then turns off server-side page-flipping there's
>>> nothing to
>>> guarantee that a new backbuffer isn't chosen when EGL starts to
>>> render again,
>>> and that buffer's content is of course undefined.
>>>
>>> So rework the functionality:
>>> If the client supports local blits, allow server-side page flipping
>>> and when
>>> a new back is grabbed, if needed, blit the old back's content to the
>>> new back.
>>> If the client doesn't support local blits, disallow server-side
>>> page-flipping
>>> to avoid a client deadlock and then, when grabbing a new back buffer,
>>> sleep
>>> until the old back is idle, which may take a substantial time
>>> depending on
>>> swap interval.
>>>
>>> Signed-off-by: Thomas Hellstrom 
>> [...]
>>
>>> @@ -475,12 +476,21 @@ dri3_find_back(struct loader_dri3_drawable *draw)
>>>  int b;
>>>  xcb_generic_event_t *ev;
>>>  xcb_present_generic_event_t *ge;
>>> +   int num_to_consider = draw->num_back;
>>>/* Increase the likelyhood of reusing current buffer */
>>>  dri3_flush_present_events(draw);
>>>   +   /* Check whether we need to reuse the current back buffer as
>>> new back.
>>> +* In that case, wait until it's not busy anymore.
>>> +*/
>>> +   if (!draw->have_image_blit && draw->cur_blit_source != -1) {
>>> +  num_to_consider = 1;
>>> +  draw->cur_blit_source = -1;
>>> +   }
>> Is it possible that dri3_find_back gets called with
>> draw->cur_blit_source != -1, entering this block (assuming
>> !draw->have_image_blit)...
>>
>>
>>>  for (;;) {
>>> -  for (b = 0; b < draw->num_back; b++) {
>>> +  for (b = 0; b < num_to_consider; b++) {
>>>int id = LOADER_DRI3_BACK_ID((b + draw->cur_back) %
>>> draw->num_back);
>>>struct loader_dri3_buffer *buffer = draw->buffers[id];
>> ... Then later gets called again with draw->cur_blit_source == -1,
>> choosing a different back buffer here, because the previous one is still
>> busy?
>>
>>
> I don't think that's possible. If dri3_find_back returns a back buffer,
> it should be idle, in the sense that it's not currently in the swap
> chain or being scanned out from. It may still have a non-signaled fence
> though. If a later call to dri_find_back() decides it's busy then
> someone must have called swapBuffer in between and it's time to change
> back anyway...

Okay, then this patch is also

Reviewed-by: Michel Dänzer 


-- 
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 06/11] loader_dri3: Honour the request to preserve back buffer content

[Thomas Hellstrom]

On 08/15/2017 09:33 AM, Michel Dänzer wrote:

On 11/08/17 11:14 PM, Thomas Hellstrom wrote:

EGL uses the force_copy parameter to loader_dri3_swap_buffers_msc() to indicate
that it wants to preserve back buffer contents across a buffer swap.

While the loader then turns off server-side page-flipping there's nothing to
guarantee that a new backbuffer isn't chosen when EGL starts to render again,
and that buffer's content is of course undefined.

So rework the functionality:
If the client supports local blits, allow server-side page flipping and when
a new back is grabbed, if needed, blit the old back's content to the new back.
If the client doesn't support local blits, disallow server-side page-flipping
to avoid a client deadlock and then, when grabbing a new back buffer, sleep
until the old back is idle, which may take a substantial time depending on
swap interval.

Signed-off-by: Thomas Hellstrom 

[...]


@@ -475,12 +476,21 @@ dri3_find_back(struct loader_dri3_drawable *draw)
 int b;
 xcb_generic_event_t *ev;
 xcb_present_generic_event_t *ge;
+   int num_to_consider = draw->num_back;
  
 /* Increase the likelyhood of reusing current buffer */

 dri3_flush_present_events(draw);
  
+   /* Check whether we need to reuse the current back buffer as new back.

+* In that case, wait until it's not busy anymore.
+*/
+   if (!draw->have_image_blit && draw->cur_blit_source != -1) {
+  num_to_consider = 1;
+  draw->cur_blit_source = -1;
+   }

Is it possible that dri3_find_back gets called with
draw->cur_blit_source != -1, entering this block (assuming
!draw->have_image_blit)...



 for (;;) {
-  for (b = 0; b < draw->num_back; b++) {
+  for (b = 0; b < num_to_consider; b++) {
   int id = LOADER_DRI3_BACK_ID((b + draw->cur_back) % draw->num_back);
   struct loader_dri3_buffer *buffer = draw->buffers[id];

... Then later gets called again with draw->cur_blit_source == -1,
choosing a different back buffer here, because the previous one is still
busy?


I don't think that's possible. If dri3_find_back returns a back buffer, 
it should be idle, in the sense that it's not currently in the swap 
chain or being scanned out from. It may still have a non-signaled fence 
though. If a later call to dri_find_back() decides it's busy then 
someone must have called swapBuffer in between and it's time to change 
back anyway...


/Thomas



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

Re: [Mesa-dev] [PATCH 06/11] loader_dri3: Honour the request to preserve back buffer content

[Michel Dänzer]

On 11/08/17 11:14 PM, Thomas Hellstrom wrote:
> EGL uses the force_copy parameter to loader_dri3_swap_buffers_msc() to 
> indicate
> that it wants to preserve back buffer contents across a buffer swap.
> 
> While the loader then turns off server-side page-flipping there's nothing to
> guarantee that a new backbuffer isn't chosen when EGL starts to render again,
> and that buffer's content is of course undefined.
> 
> So rework the functionality:
> If the client supports local blits, allow server-side page flipping and when
> a new back is grabbed, if needed, blit the old back's content to the new back.
> If the client doesn't support local blits, disallow server-side page-flipping
> to avoid a client deadlock and then, when grabbing a new back buffer, sleep
> until the old back is idle, which may take a substantial time depending on
> swap interval.
> 
> Signed-off-by: Thomas Hellstrom 

[...]

> @@ -475,12 +476,21 @@ dri3_find_back(struct loader_dri3_drawable *draw)
> int b;
> xcb_generic_event_t *ev;
> xcb_present_generic_event_t *ge;
> +   int num_to_consider = draw->num_back;
>  
> /* Increase the likelyhood of reusing current buffer */
> dri3_flush_present_events(draw);
>  
> +   /* Check whether we need to reuse the current back buffer as new back.
> +* In that case, wait until it's not busy anymore.
> +*/
> +   if (!draw->have_image_blit && draw->cur_blit_source != -1) {
> +  num_to_consider = 1;
> +  draw->cur_blit_source = -1;
> +   }

Is it possible that dri3_find_back gets called with
draw->cur_blit_source != -1, entering this block (assuming
!draw->have_image_blit)...


> for (;;) {
> -  for (b = 0; b < draw->num_back; b++) {
> +  for (b = 0; b < num_to_consider; b++) {
>   int id = LOADER_DRI3_BACK_ID((b + draw->cur_back) % draw->num_back);
>   struct loader_dri3_buffer *buffer = draw->buffers[id];

... Then later gets called again with draw->cur_blit_source == -1,
choosing a different back buffer here, because the previous one is still
busy?


-- 
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 06/11] loader_dri3: Honour the request to preserve back buffer content

[Thomas Hellstrom]

EGL uses the force_copy parameter to loader_dri3_swap_buffers_msc() to indicate
that it wants to preserve back buffer contents across a buffer swap.

While the loader then turns off server-side page-flipping there's nothing to
guarantee that a new backbuffer isn't chosen when EGL starts to render again,
and that buffer's content is of course undefined.

So rework the functionality:
If the client supports local blits, allow server-side page flipping and when
a new back is grabbed, if needed, blit the old back's content to the new back.
If the client doesn't support local blits, disallow server-side page-flipping
to avoid a client deadlock and then, when grabbing a new back buffer, sleep
until the old back is idle, which may take a substantial time depending on
swap interval.

Signed-off-by: Thomas Hellstrom 
---
 src/loader/loader_dri3_helper.c | 55 ++---
 src/loader/loader_dri3_helper.h |  1 +
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 6f84a43..812adcc 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -258,6 +258,7 @@ loader_dri3_drawable_init(xcb_connection_t *conn,
 
draw->have_image_blit = draw->ext->image->base.version >= 9 &&
   draw->ext->image->blitImage != NULL;
+   draw->cur_blit_source = -1;
 
if (draw->ext->config)
   draw->ext->config->configQueryi(draw->dri_screen,
@@ -475,12 +476,21 @@ dri3_find_back(struct loader_dri3_drawable *draw)
int b;
xcb_generic_event_t *ev;
xcb_present_generic_event_t *ge;
+   int num_to_consider = draw->num_back;
 
/* Increase the likelyhood of reusing current buffer */
dri3_flush_present_events(draw);
 
+   /* Check whether we need to reuse the current back buffer as new back.
+* In that case, wait until it's not busy anymore.
+*/
+   if (!draw->have_image_blit && draw->cur_blit_source != -1) {
+  num_to_consider = 1;
+  draw->cur_blit_source = -1;
+   }
+
for (;;) {
-  for (b = 0; b < draw->num_back; b++) {
+  for (b = 0; b < num_to_consider; b++) {
  int id = LOADER_DRI3_BACK_ID((b + draw->cur_back) % draw->num_back);
  struct loader_dri3_buffer *buffer = draw->buffers[id];
 
@@ -753,6 +763,13 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable 
*draw,
0, 0, __BLIT_FLAG_FLUSH);
}
 
+   /* If we need to preload the new back buffer, remember the source.
+* The force_copy parameter is used by EGL to attempt to preserve
+* the back buffer across a call to this function.
+*/
+   if (force_copy)
+  draw->cur_blit_source = LOADER_DRI3_BACK_ID(draw->cur_back);
+
dri3_flush_present_events(draw);
 
if (back && !draw->is_pixmap) {
@@ -791,8 +808,13 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable 
*draw,
*/
   if (draw->swap_interval == 0)
   options |= XCB_PRESENT_OPTION_ASYNC;
-  if (force_copy)
-  options |= XCB_PRESENT_OPTION_COPY;
+
+  /* If we need to populate the new back, but need to reuse the back
+   * buffer slot due to lack of local blit capabilities, make sure
+   * the server doesn't flip and we deadlock.
+   */
+  if (!draw->have_image_blit && draw->cur_blit_source != -1)
+ options |= XCB_PRESENT_OPTION_COPY;
 
   back->busy = 1;
   back->last_swap = draw->send_sbc;
@@ -1365,6 +1387,31 @@ dri3_get_buffer(__DRIdrawable *driDrawable,
}
dri3_fence_await(draw->conn, buffer);
 
+   /*
+* Do we need to preserve the content of a previous buffer?
+*
+* Note that this blit is needed only to avoid a wait for a buffer that
+* is currently in the flip chain or being scanned out from. That's really
+* a tradeoff. If we're ok with the wait we can reduce the number of back
+* buffers to 1 for SWAP_EXCHANGE, and 1 for SWAP_COPY,
+* but in the latter case we must disallow page-flipping.
+*/
+   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]) {
+
+  struct loader_dri3_buffer *source = draw->buffers[draw->cur_blit_source];
+
+  /* Avoid flushing here. Will propably do good for tiling hardware. */
+  (void) loader_dri3_blit_image(draw,
+buffer->image,
+source->image,
+0, 0, draw->width, draw->height,
+0, 0, 0);
+  buffer->last_swap = source->last_swap;
+  draw->cur_blit_source = -1;
+   }
/* Return the requested buffer */
return buffer;
 }
@@ -1431,7 +1478,7 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
   return false;
 
/* pixmaps always have front buffers */
-//   if (draw->is_pixmap)
+   if