[Mesa-stable] [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: <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.
>
>
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