[Mesa-dev] [PATCH 2/7] egl/x11: check for dri2_dpy->flush before using the flush extension

[Emil Velikov]

On 16 May 2017 at 18:32, Chad Versace <chadversary at chromium.org> wrote:
> On Mon 15 May 2017, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov at collabora.com>
>>
>> Analogous to previous commit.
>>
>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>> ---
>
>> If people prefer I can split the whitespace/indent changes in
>> this/previous path from the rest. Don't feel too strongly either way.
>
> I do think the patches would be easier to review if you did that. As
> written, it's difficult to see what's behavioral change vs cosmetic
> cleanups.
>
Ack, I will be less for the future.

>>
>>  src/egl/drivers/dri2/platform_x11.c | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/platform_x11.c b/src/egl/drivers/dri2/platform_x11.c
>> index 3bce0bb3f21..becf00547e6 100644
>> --- a/src/egl/drivers/dri2/platform_x11.c
>> +++ b/src/egl/drivers/dri2/platform_x11.c
>> @@ -817,8 +817,7 @@ dri2_copy_region(_EGLDriver *drv, _EGLDisplay *disp,
>>     if (draw->Type == EGL_PIXMAP_BIT || draw->Type == EGL_PBUFFER_BIT)
>>        return EGL_TRUE;
>>
>> -   if (dri2_dpy->flush)
>> -      dri2_dpy->flush->flush(dri2_surf->dri_drawable);
>> +   dri2_dpy->flush->flush(dri2_surf->dri_drawable);
>>
>>     if (dri2_surf->have_fake_front)
>>        render_attachment = XCB_DRI2_ATTACHMENT_BUFFER_FAKE_FRONT_LEFT;
>> @@ -880,8 +879,7 @@ dri2_x11_swap_buffers_msc(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw,
>>      * happened.  The driver should still be using the viewport hack to catch
>>      * window resizes.
>>      */
>> -   if (dri2_dpy->flush &&
>> -       dri2_dpy->flush->base.version >= 3 && dri2_dpy->flush->invalidate)
>> +   if (dri2_dpy->flush->base.version >= 3 && dri2_dpy->flush->invalidate)
>>        dri2_dpy->flush->invalidate(dri2_surf->dri_drawable);
>>
>>     return swap_count;
>> @@ -893,19 +891,17 @@ dri2_x11_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw)
>>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>>     struct dri2_egl_surface *dri2_surf = dri2_egl_surface(draw);
>>
>> -   if (dri2_dpy->dri2) {
>> -      if (dri2_x11_swap_buffers_msc(drv, disp, draw, 0, 0, 0) != -1) {
>> -          return EGL_TRUE;
>> -      }
>> +   if (!dri2_dpy->flush) {
>> +      dri2_dpy->core->swapBuffers(dri2_surf->dri_drawable);
>> +      return EGL_TRUE;
>
> Yes, thank you.
>
> I'll say it again to anyone who's reading. The Go style guide insists on
> this, for good reason.
>
>     https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow
>
>     Try to keep the normal code path at a minimal indentation, and indent
>     the error handling, dealing with it first. This improves the readability
>     of the code by permitting visually scanning the normal path quickly.
>
>> +   }
>> +
>> +   if (dri2_x11_swap_buffers_msc(drv, disp, draw, 0, 0, 0) == -1) {
>>        /* Swap failed with a window drawable. */
>>        _eglError(EGL_BAD_NATIVE_WINDOW, __func__);
>>        return EGL_FALSE;
>> -   } else {
>> -      assert(dri2_dpy->swrast);
>> -
>> -      dri2_dpy->core->swapBuffers(dri2_surf->dri_drawable);
>> -      return EGL_TRUE;
>>     }
>> +   return EGL_TRUE;
>>  }
>
> There's a lot of hidden complexity in platform_x11.c, and I'm unsure if
> this patch preserves proper extension checks. I defer to someone who
> knows this code better.

Just sent out updated/split patches adding a bit of description about these.

-Emil