[Mesa-dev] [PATCH] [AMD] dri3: Add adaptive_sync_enable driconf option

[Nicolai Hähnle]

On 17.10.2017 12:07, Michel Dänzer wrote:
> On 17/10/17 11:33 AM, Nicolai Hähnle wrote:
>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>
>> When enabled, this will request FreeSync via the hybrid amdgpu DDX's
>> AMDGPU X11 protocol extension.
>>
>> Due to limitations in the DDX this will only work for applications
>> that cover the entire X screen (which is important to keep in mind when
>> you have a multi-monitor setup).
> 
> This limitation already applies to page flipping in general, it's not
> specific to FreeSync.

Maybe it shouldn't apply in general? After all, what if you have one 
monitor with 60Hz and one with 90Hz?


>> @@ -269,25 +315,32 @@ loader_dri3_drawable_init(xcb_connection_t *conn,
>>      draw->drawable = drawable;
>>      draw->dri_screen = dri_screen;
>>      draw->is_different_gpu = is_different_gpu;
>>   
>>      draw->have_back = 0;
>>      draw->have_fake_front = 0;
>>      draw->first_init = true;
>>   
>>      draw->cur_blit_source = -1;
>>      draw->back_format = __DRI_IMAGE_FORMAT_NONE;
>> +   draw->adaptive_sync = false;
>>   
>> -   if (draw->ext->config)
>> +   if (draw->ext->config) {
>>         draw->ext->config->configQueryi(draw->dri_screen,
>>                                         "vblank_mode", &vblank_mode);
>>   
>> +      unsigned char adaptive_sync_enable = 1;
> 
> Adaptive sync shouldn't be enabled by default. (Maybe that effectively
> isn't the case, but then initializing this variable to 1 is just
> confusing :)

You're right, I'll change it :)


>> @@ -767,20 +820,25 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable *draw,
>>                                bool force_copy)
>>   {
>>      struct loader_dri3_buffer *back;
>>      int64_t ret = 0;
>>      uint32_t options = XCB_PRESENT_OPTION_NONE;
>>   
>>      draw->vtable->flush_drawable(draw, flush_flags);
>>   
>>      back = dri3_find_back_alloc(draw);
>>   
>> +   if (draw->adaptive_sync) {
>> +      if (!loader_dri3_amdgpu_freesync_enable(draw->conn, draw->drawable))
>> +         draw->adaptive_sync = false;
>> +   }
> 
> The AMDGPUFreesyncCapability request only has to be submitted once per
> window, not for every buffer swap.

Unfortunately, that's not true due to the implementation in the DDX.

For one,  we *definitely* have to re-submit the request at least each 
time the window is moved or resized, because the request doesn't 
register unless the window is already fullscreen.

For another, some applications create multiple windows, but the DDX can 
only keep track of one.

There's perhaps an argument to be made that both of these issues would 
better be fixed in the DDX. However, since this is a non-upstream 
interim solution anyway, and the final upstream solution will likely 
also do something in swap_buffers, I think this approach is reasonable.

Correct me if I got some of the xcb code wrong, but from what I 
understand there should be no stalls associated with it, just a bunch of 
additional bytes sent over the socket on each swap buffers, which 
doesn't seem too bad to me.

(For what it's worth, the closed GL driver sends this request on 
glXMakeCurrent, which makes even less sense to me :-))

Cheers,
Nicolai

> 
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.