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

[Kazlauskas, Nicholas]

On 10/11/2018 10:07 AM, Michel Dänzer wrote:
> On 2018-10-11 2:40 p.m., Kazlauskas, Nicholas wrote:
>> On 10/11/2018 03:57 AM, Michel Dänzer wrote:
>>> On 2018-10-05 10: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. For some displays this means there is
>>>>       less time spent watching flickering occur during application load.
>>>>
>>>> For now this property is only set on the glx DRI3 backend. 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>
>>>>
>>>> [...]
>>>>    @@ -331,16 +360,25 @@ loader_dri3_drawable_init(xcb_connection_t
>>>> *conn,
>>>>       draw->have_back = 0;
>>>>       draw->have_fake_front = 0;
>>>>       draw->first_init = true;
>>>> +   draw->adaptive_sync_enable = false;
>>>> +   draw->adaptive_sync_active = false;
>>>>         draw->cur_blit_source = -1;
>>>>       draw->back_format = __DRI_IMAGE_FORMAT_NONE;
>>>>       mtx_init(&draw->mtx, mtx_plain);
>>>>       cnd_init(&draw->event_cnd);
>>>>    -   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;
>>>> +   }
>>>> +
>>>>       switch (vblank_mode) {
>>>>       case DRI_CONF_VBLANK_NEVER:
>>>>       case DRI_CONF_VBLANK_DEF_INTERVAL_0:
>>>> @@ -879,6 +917,12 @@ loader_dri3_swap_buffers_msc(struct
>>>> loader_dri3_drawable *draw,
>>>>       back = dri3_find_back_alloc(draw);
>>>>         mtx_lock(&draw->mtx);
>>>> +
>>>> +   if (draw->adaptive_sync_enable && !draw->adaptive_sync_active) {
>>>> +      set_adaptive_sync_property(draw->conn,
>>>> (xcb_window_t)draw->drawable, true);
>>>> +      draw->adaptive_sync_active = true;
>>>> +   }
>>>
>>> The window property should also either be deleted, or its value set to
>>> 0, when adaptive_sync_enable is false, to address the following scenario:
>>>
>>> 1. Xorg is running without a compositor, and the user runs an OpenGL app
>>> which draws to the root window, e.g. an xscreensaver hack.
>>>
>>> 2. Later, the user starts a compositor which draws to the root window.
>>>
>>> Step one sets the window property to 1, so without the above, the
>>> compositor will use variable refresh even if it's blacklisted.
>>
>> Good point. I'll go with setting the property to 0 only if the property
>> is already on the window so we don't end up adding it to windows that
>> don't need it.
> 
> Deleting the property would be easier, and doesn't require a server
> round-trip. If the property doesn't exist, the request to delete it is
> simply ignored.
> 
> 

I just went through updating the patches and ended up with the delete 
like you suggest. I had thought the only_if_exists flag was on the 
change property func but it was on the intern atom instead.

I'd rather delete than have to go through the query/set roundtrip.

Nicholas Kazlauskas