[Mesa-dev] [PATCH v4 5/5] loader/dri3: Enable adaptive sync via _VARIABLE_REFRESH property

[Michel Dänzer]

On 2018-10-11 6:43 p.m., Nicholas Kazlauskas wrote:
> The DDX driver can be notified of adaptive sync suitability by
> flagging the application's window with the _VARIABLE_REFRESH property.
> 
> This property is set on the first swap the application performs
> when adaptive_sync_enable is set to true in the drirc.
> 
> It's performed here instead of when the loader is initialized for
> two reasons:
> 
> (1) The window's drawable can be missing during loader init.
>     This can be observed during the Unigine Superposition benchmark.
> 
> (2) Adaptive sync will only be enabled closer to when the application
>     actually begins rendering.
> 
> If adaptive_sync_enable is false then the _VARIABLE_REFRESH property
> is deleted on loader init.
> 
> The property is only managed on the glx DRI3 backend for now. This
> should cover most common applications and games on modern hardware.
> 
> Vulkan support can be implemented in a similar manner but would likely
> require splitting the function out into a common helper function.
> 
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> ---
>  src/loader/loader_dri3_helper.c | 58 ++++++++++++++++++++++++++++++++-
>  src/loader/loader_dri3_helper.h |  2 ++
>  2 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
> index f641a34e6d..d4ff5abe41 100644
> --- a/src/loader/loader_dri3_helper.c
> +++ b/src/loader/loader_dri3_helper.c
> @@ -101,6 +101,39 @@ get_xcb_visualtype_for_depth(struct loader_dri3_drawable *draw, int depth)
>     return NULL;
>  }
>  
> +/* Sets the adaptive sync window property state. */
> +static void
> +set_adaptive_sync_property(xcb_connection_t *conn,
> +                           xcb_window_t window,
> +                           uint32_t state)
> +{
> +   static char const name[] = "_VARIABLE_REFRESH";
> +   xcb_intern_atom_cookie_t cookie;
> +   xcb_intern_atom_reply_t* reply;
> +
> +   cookie = xcb_intern_atom(conn, 0, sizeof(name), name);
> +   reply = xcb_intern_atom_reply(conn, cookie, NULL);
> +   if (reply == NULL)
> +      return;
> +
> +   if (state)
> +      xcb_change_property(conn,
> +                        XCB_PROP_MODE_REPLACE,
> +                        window,
> +                        reply->atom,
> +                        XCB_ATOM_CARDINAL,
> +                        32,
> +                        1,
> +                        &state);

The indentation of the second and later parameters doesn't seem to line
up with the opening parenthesis. Also, we usually put as many arguments
on the same line as can fit within ~80 columns.


> +   else
> +      xcb_delete_property(conn,
> +                          window,
> +                          reply->atom);

If the drawable isn't actually a window (can also be a pixmap), these
requests will raise a BadWindow protocol error. An easy way to deal with
that:

 cookie = xcb_change/delete_property_checked(conn, ...);
 xcb_discard_reply(conn, cookie.sequence);

Maybe also change the parameter of this function to xcb_drawable_t draw.


> +   xcb_flush(conn);

This isn't necessary, the Property request will be processed before a
buffer swap anyway.


> -   if (draw->ext->config)
> +   if (draw->ext->config) {
>        draw->ext->config->configQueryi(draw->dri_screen,
>                                        "vblank_mode", &vblank_mode);
>  
> +      draw->ext->config->configQueryb(draw->dri_screen,
> +                                      "adaptive_sync_enable",
> +                                      &adaptive_sync_enable);
> +
> +      draw->adaptive_sync_enable = adaptive_sync_enable;
> +
> +      if (!adaptive_sync_enable)
> +         set_adaptive_sync_property(conn,
> +                                    (xcb_window_t)draw->drawable,
> +                                    false);

This should be after the if (draw->ext->config) block:

      if (!draw->adaptive_sync_enable)
         set_adaptive_sync_property(conn, draw->drawable, false);


> diff --git a/src/loader/loader_dri3_helper.h b/src/loader/loader_dri3_helper.h
> index 0d18181312..86f994cb2a 100644
> --- a/src/loader/loader_dri3_helper.h
> +++ b/src/loader/loader_dri3_helper.h
> @@ -156,6 +156,8 @@ struct loader_dri3_drawable {
>     xcb_special_event_t *special_event;
>  
>     bool first_init;
> +   bool adaptive_sync_enable;

BTW, I think the option should be called just "adaptive_sync" instead of
"adaptive_sync_enable". But I can live with the latter.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer