Re: [PATCH] drm/vblank: Fix delta_ns to an absolute value

[Hoegeun Kwon]

On 09/04/2017 04:36 PM, Daniel Vetter wrote:
> On Fri, Sep 01, 2017 at 04:36:25PM +0300, Ville SyrjÃlà wrote:
> > On Fri, Sep 01, 2017 at 04:07:16PM +0900, Hoegeun Kwon wrote:
> > > If scanout started, we should reduce etime by delta_ns. But delta_ns
> > > is negative if scanout has not started. If delta_ns is negative,
> > > subtraction of delta_ns from etime increases etime. This is wrong, the
> > > etime should not be increased, so you have to make delta_ns an
> > > absolute value.
> > >
> > > Signed-off-by: Hoegeun Kwon <hoegeun.kwon@xxxxxxxxxxx>
> > > ---
> > >
> > > Hello all,
> > >
> > > I think that the etime should not be increased.
> > > In cases where delta_ns is negative, if you get time again after an
> > > interrupt call, there is a problem that the time obtained from the
> > > interrupt becomes the future time instead of the past time.
> > >
> > > Please let me know if this patch is wrong.
> > It is wrong. The timestamp corresponds to the first active pixel of the
> > frame/field. So while between vblank start and active start we expect
> > to get a timestamp that is in the future.
> A bit more strict, it's up to drivers when exactly the vblank interrupt
> fires. Some fire at vblank start, some at the end, some somewhen around
> that. The only rule is that the vblank must not fire before the point of
> no return for another page_flip call, i.e. after the vblank event goes
> out, the next page_flip (or atomic ioctl) must go to the next vblank.
>
> So yeah both positive and negative deltas make perfect sense.
> -Daniel

Thanks Ville and Daniel,

Your response has been really helpful.
Thanks so much.

Best regards,
Hoegeun

> > > Best regards,
> > > Hoegeun
> > >
> > >   drivers/gpu/drm/drm_vblank.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > index 70f2b95..a3e0176 100644
> > > --- a/drivers/gpu/drm/drm_vblank.c
> > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > @@ -684,7 +684,7 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> > >   	/* Subtract time delta from raw timestamp to get final
> > >   	 * vblank_time timestamp for end of vblank.
> > >   	 */
> > > -	etime = ktime_sub_ns(etime, delta_ns);
> > > +	etime = ktime_sub_ns(etime, abs(delta_ns));
> > >   	*vblank_time = ktime_to_timeval(etime);
> > >   
> > >   	DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n",
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > --
> > Ville SyrjÃlÃ
> > Intel OTC
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel