Re: [Mesa-dev] [PATCH] egl/x11: Send invalidate to the driver on dri2_copy_region

[Thomas Hellstrom]

On 04/26/2018 11:56 AM, Michel Dänzer wrote:

On 2018-04-26 11:33 AM, Thomas Hellstrom wrote:

On 04/26/2018 10:30 AM, Michel Dänzer wrote:

On 2018-04-25 07:49 PM, Deepak Rawat wrote:

Similar to what is done in dri2_x11_swap_buffers_msc send invalidate
to the driver because egl/X11 is not watching for for server's
invalidate events. The dri2_copy_region path is trigerred when
server supports DRI2 version minor 1.

Tested with piglit egl tests for regression.

Cc: 
Signed-off-by: Deepak Rawat 
Reviewed-by: Thomas Hellstrom 
---
   src/egl/drivers/dri2/platform_x11.c | 7 +++
   1 file changed, 7 insertions(+)

diff --git a/src/egl/drivers/dri2/platform_x11.c
b/src/egl/drivers/dri2/platform_x11.c
index 6c287b4d06..e99434ea3a 100644
--- a/src/egl/drivers/dri2/platform_x11.c
+++ b/src/egl/drivers/dri2/platform_x11.c
@@ -841,6 +841,13 @@ dri2_copy_region(_EGLDriver *drv, _EGLDisplay
*disp,
  render_attachment);
  free(xcb_dri2_copy_region_reply(dri2_dpy->conn, cookie, NULL));
   +   /*
+    * Just like as done in dri2_x11_swap_buffers_msc we aren't
watching for
+    * server's invalidate events, so just send invalidate to driver.
+    */
+   if (dri2_dpy->flush->base.version >= 3 &&
dri2_dpy->flush->invalidate)
+  dri2_dpy->flush->invalidate(dri2_surf->dri_drawable);
+
  return EGL_TRUE;
   }

Why is invalidate needed after copy_region? That's surprising, I suspect
it just papers over the real problem.



I had a quick look into the platform_x11 code. dri2_copy_region is
called only from the various swap_buffers() paths,
dri2_x11_swap_buffers() and dri2_x11_swap_buffers_region(). Explicit
invalidation is, before this patch, done only if the server supports
dri2 swaps. Probably because most drivers do, vmware does not, so we can
hit swapbuffers without doing explicit invalidation.

In comparison, GLX does explicit invalidation on swapbuffers,
bind_context and bind_texture for the same protocol version, so this
patch should only bring the EGL swapbuffer paths closer to what GLX is
doing, while still not addressing bind_context and bind_texture.

FWIW, EGL platform_x11 (dri2) seems to not parse server invalidate
events for the higher protocol versions, relying solely on explicit
invalidation.

The purpose of the invalidate callback is to trigger updating the DRI2
buffers before drawing the next frame. This isn't necessary after a
copy_region operation, so you seem to be relying on some kind of side
effect of the invalidate callback. Which may be okay, but I think it
would be clearer not to put this code in dri2_copy_region itself.


The purpose in this case is to update the dri2 buffers after a 
swapbuffer operation, which *is* needed, but I agree that there might be 
cases where the dri2_copy_region could theoretically be used without 
requiring an invalidation.


So we could of course move out the invalidation to the swapbuffer functions.

/Thomas


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

Re: [Mesa-dev] [PATCH] egl/x11: Send invalidate to the driver on dri2_copy_region

[Michel Dänzer]

On 2018-04-26 11:33 AM, Thomas Hellstrom wrote:
> On 04/26/2018 10:30 AM, Michel Dänzer wrote:
>> On 2018-04-25 07:49 PM, Deepak Rawat wrote:
>>> Similar to what is done in dri2_x11_swap_buffers_msc send invalidate
>>> to the driver because egl/X11 is not watching for for server's
>>> invalidate events. The dri2_copy_region path is trigerred when
>>> server supports DRI2 version minor 1.
>>>
>>> Tested with piglit egl tests for regression.
>>>
>>> Cc: 
>>> Signed-off-by: Deepak Rawat 
>>> Reviewed-by: Thomas Hellstrom 
>>> ---
>>>   src/egl/drivers/dri2/platform_x11.c | 7 +++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/src/egl/drivers/dri2/platform_x11.c
>>> b/src/egl/drivers/dri2/platform_x11.c
>>> index 6c287b4d06..e99434ea3a 100644
>>> --- a/src/egl/drivers/dri2/platform_x11.c
>>> +++ b/src/egl/drivers/dri2/platform_x11.c
>>> @@ -841,6 +841,13 @@ dri2_copy_region(_EGLDriver *drv, _EGLDisplay
>>> *disp,
>>>  render_attachment);
>>>  free(xcb_dri2_copy_region_reply(dri2_dpy->conn, cookie, NULL));
>>>   +   /*
>>> +    * Just like as done in dri2_x11_swap_buffers_msc we aren't
>>> watching for
>>> +    * server's invalidate events, so just send invalidate to driver.
>>> +    */
>>> +   if (dri2_dpy->flush->base.version >= 3 &&
>>> dri2_dpy->flush->invalidate)
>>> +  dri2_dpy->flush->invalidate(dri2_surf->dri_drawable);
>>> +
>>>  return EGL_TRUE;
>>>   }
>> Why is invalidate needed after copy_region? That's surprising, I suspect
>> it just papers over the real problem.
>>
>>
> I had a quick look into the platform_x11 code. dri2_copy_region is
> called only from the various swap_buffers() paths,
> dri2_x11_swap_buffers() and dri2_x11_swap_buffers_region(). Explicit
> invalidation is, before this patch, done only if the server supports
> dri2 swaps. Probably because most drivers do, vmware does not, so we can
> hit swapbuffers without doing explicit invalidation.
> 
> In comparison, GLX does explicit invalidation on swapbuffers,
> bind_context and bind_texture for the same protocol version, so this
> patch should only bring the EGL swapbuffer paths closer to what GLX is
> doing, while still not addressing bind_context and bind_texture.
> 
> FWIW, EGL platform_x11 (dri2) seems to not parse server invalidate
> events for the higher protocol versions, relying solely on explicit
> invalidation.

The purpose of the invalidate callback is to trigger updating the DRI2
buffers before drawing the next frame. This isn't necessary after a
copy_region operation, so you seem to be relying on some kind of side
effect of the invalidate callback. Which may be okay, but I think it
would be clearer not to put this code in dri2_copy_region itself.


-- 
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] egl/x11: Send invalidate to the driver on dri2_copy_region

[Thomas Hellstrom]

On 04/26/2018 10:30 AM, Michel Dänzer wrote:

On 2018-04-25 07:49 PM, Deepak Rawat wrote:

Similar to what is done in dri2_x11_swap_buffers_msc send invalidate
to the driver because egl/X11 is not watching for for server's
invalidate events. The dri2_copy_region path is trigerred when
server supports DRI2 version minor 1.

Tested with piglit egl tests for regression.

Cc: 
Signed-off-by: Deepak Rawat 
Reviewed-by: Thomas Hellstrom 
---
  src/egl/drivers/dri2/platform_x11.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/src/egl/drivers/dri2/platform_x11.c 
b/src/egl/drivers/dri2/platform_x11.c
index 6c287b4d06..e99434ea3a 100644
--- a/src/egl/drivers/dri2/platform_x11.c
+++ b/src/egl/drivers/dri2/platform_x11.c
@@ -841,6 +841,13 @@ dri2_copy_region(_EGLDriver *drv, _EGLDisplay *disp,
   render_attachment);
 free(xcb_dri2_copy_region_reply(dri2_dpy->conn, cookie, NULL));
  
+   /*

+* Just like as done in dri2_x11_swap_buffers_msc we aren't watching for
+* server's invalidate events, so just send invalidate to driver.
+*/
+   if (dri2_dpy->flush->base.version >= 3 && dri2_dpy->flush->invalidate)
+  dri2_dpy->flush->invalidate(dri2_surf->dri_drawable);
+
 return EGL_TRUE;
  }

Why is invalidate needed after copy_region? That's surprising, I suspect
it just papers over the real problem.


I had a quick look into the platform_x11 code. dri2_copy_region is 
called only from the various swap_buffers() paths, 
dri2_x11_swap_buffers() and dri2_x11_swap_buffers_region(). Explicit 
invalidation is, before this patch, done only if the server supports 
dri2 swaps. Probably because most drivers do, vmware does not, so we can 
hit swapbuffers without doing explicit invalidation.


In comparison, GLX does explicit invalidation on swapbuffers, 
bind_context and bind_texture for the same protocol version, so this 
patch should only bring the EGL swapbuffer paths closer to what GLX is 
doing, while still not addressing bind_context and bind_texture.


FWIW, EGL platform_x11 (dri2) seems to not parse server invalidate 
events for the higher protocol versions, relying solely on explicit 
invalidation.


/Thomas


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

Re: [Mesa-dev] [PATCH] egl/x11: Send invalidate to the driver on dri2_copy_region

[Michel Dänzer]

On 2018-04-25 07:49 PM, Deepak Rawat wrote:
> Similar to what is done in dri2_x11_swap_buffers_msc send invalidate
> to the driver because egl/X11 is not watching for for server's
> invalidate events. The dri2_copy_region path is trigerred when
> server supports DRI2 version minor 1.
> 
> Tested with piglit egl tests for regression.
> 
> Cc: 
> Signed-off-by: Deepak Rawat 
> Reviewed-by: Thomas Hellstrom 
> ---
>  src/egl/drivers/dri2/platform_x11.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/egl/drivers/dri2/platform_x11.c 
> b/src/egl/drivers/dri2/platform_x11.c
> index 6c287b4d06..e99434ea3a 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -841,6 +841,13 @@ dri2_copy_region(_EGLDriver *drv, _EGLDisplay *disp,
>  render_attachment);
> free(xcb_dri2_copy_region_reply(dri2_dpy->conn, cookie, NULL));
>  
> +   /*
> +* Just like as done in dri2_x11_swap_buffers_msc we aren't watching for
> +* server's invalidate events, so just send invalidate to driver.
> +*/
> +   if (dri2_dpy->flush->base.version >= 3 && dri2_dpy->flush->invalidate)
> +  dri2_dpy->flush->invalidate(dri2_surf->dri_drawable);
> +
> return EGL_TRUE;
>  }

Why is invalidate needed after copy_region? That's surprising, I suspect
it just papers over the real problem.


-- 
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/x11: Send invalidate to the driver on dri2_copy_region

[Deepak Rawat]

Similar to what is done in dri2_x11_swap_buffers_msc send invalidate
to the driver because egl/X11 is not watching for for server's
invalidate events. The dri2_copy_region path is trigerred when
server supports DRI2 version minor 1.

Tested with piglit egl tests for regression.

Cc: 
Signed-off-by: Deepak Rawat 
Reviewed-by: Thomas Hellstrom 
---
 src/egl/drivers/dri2/platform_x11.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/egl/drivers/dri2/platform_x11.c 
b/src/egl/drivers/dri2/platform_x11.c
index 6c287b4d06..e99434ea3a 100644
--- a/src/egl/drivers/dri2/platform_x11.c
+++ b/src/egl/drivers/dri2/platform_x11.c
@@ -841,6 +841,13 @@ dri2_copy_region(_EGLDriver *drv, _EGLDisplay *disp,
   render_attachment);
free(xcb_dri2_copy_region_reply(dri2_dpy->conn, cookie, NULL));
 
+   /*
+* Just like as done in dri2_x11_swap_buffers_msc we aren't watching for
+* server's invalidate events, so just send invalidate to driver.
+*/
+   if (dri2_dpy->flush->base.version >= 3 && dri2_dpy->flush->invalidate)
+  dri2_dpy->flush->invalidate(dri2_surf->dri_drawable);
+
return EGL_TRUE;
 }
 
-- 
2.17.0

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