Re: [PATCH RESEND 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event

[veeras@codeaurora.org]

On 2020-11-13 12:45, Daniel Vetter wrote:
> On Thu, Nov 12, 2020 at 10:27:23AM -0800, Veera Sundaram Sankaran 
> wrote:
>> The explicit out-fences in crtc are signaled as part of vblank event,
>> indicating all framebuffers present on the Atomic Commit request are
>> scanned out on the screen. Though the fence signal and the vblank 
>> event
>> notification happens at the same time, triggered by the same hardware
>> vsync event, the timestamp set in both are different. With drivers
>> supporting precise vblank timestamp the difference between the two
>> timestamps would be even higher. This might have an impact on use-mode
>> frameworks using these fence timestamps for purposes other than simple
>> buffer usage. For instance, the Android framework uses the 
>> retire-fences
>> as an alternative to vblank when frame-updates are in progress Set the
>> fence timestamp during send vblank event to avoid discrepancies.
> 
> I think a reference to the exact source code in android that does this
> would be really useful. Something in drm_hwcomposer or whatever is 
> doing
> this.
> 

Thanks for the review. Sorry for not getting back earlier, was waiting
for the review on [patch 1/2], so that both comments can be addressed 
together.
Here is the reference for Android expecting retire-fence timestamp to
match exactly with hardware vsync as it is used for the dispsync model.

Usage: https://source.android.com/devices/graphics/implement-vsync
Code: 
https://android.googlesource.com/platform/frameworks/native/+/master/services/surfaceflinger/Scheduler/Scheduler.cpp#397
Will update the commit-text with the links as part of V2 patch.

Thanks,
Veera

> Aside from documenting why we want to do this I think this all looks
> reasonable.
> -Daniel
> 
>> 
>> Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org>
>> ---
>>  drivers/gpu/drm/drm_vblank.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/drm_vblank.c 
>> b/drivers/gpu/drm/drm_vblank.c
>> index b18e1ef..b38e50c 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -24,6 +24,7 @@
>>   * OTHER DEALINGS IN THE SOFTWARE.
>>   */
>> 
>> +#include <linux/dma-fence.h>
>>  #include <linux/export.h>
>>  #include <linux/kthread.h>
>>  #include <linux/moduleparam.h>
>> @@ -999,6 +1000,14 @@ static void send_vblank_event(struct drm_device 
>> *dev,
>>  		e->event.seq.time_ns = ktime_to_ns(now);
>>  		break;
>>  	}
>> +
>> +	/*
>> +	 * update fence timestamp with the same vblank timestamp as both
>> +	 * are signaled by the same event
>> +	 */
>> +	if (e->base.fence)
>> +		e->base.fence->timestamp = now;
>> +
>>  	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
>>  	drm_send_event_locked(dev, &e->base);
>>  }
>> --
>> 2.7.4
>>