[Mesa-stable] [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: <mesa-stable at lists.freedesktop.org>
>>> Signed-off-by: Deepak Rawat <drawat at vmware.com>
>>> Reviewed-by: Thomas Hellstrom <thellstrom at vmware.com>
>>> ---
>>>   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