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

[Kazlauskas, Nicholas]

On 10/12/2018 05:25 AM, Michel Dänzer wrote:
> 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.

Noted, will fix.

> 
> 
>> +   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.

I've seen this happen for a few applications I've tested, actually. This 
seems like a reasonable solution.

> 
> 
>> +   xcb_flush(conn);
> 
> This isn't necessary, the Property request will be processed before a
> buffer swap anyway.

It's not necessary for setting the value at least. I'm not sure about 
deleting the property, but there should be a flush at some point?

> 
> 
>> -   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.
> 
> 

I think this was mostly just following the convention set by previously 
proposed patches. I'm open to shortening this.

Nicholas Kazlauskas