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

Open Source and information security mailing list archives

Message-ID: <20170904073620.pmnlxghc4xe2xqvp@phenom.ffwll.local>
Date:   Mon, 4 Sep 2017 09:36:20 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:     Hoegeun Kwon <hoegeun.kwon@...sung.com>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        daniel.vetter@...el.com
Subject: Re: [PATCH] drm/vblank: Fix delta_ns to an absolute value

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@...sung.com>
> > ---
> > 
> > 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

> 
> > 
> > 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@...ts.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives