* [PATCH] drm/vblank: Fix delta_ns to an absolute value [not found] <CGME20170901070725epcas1p288a5be80c3a3edb02433d24f5beed338@epcas1p2.samsung.com> @ 2017-09-01 7:07 ` Hoegeun Kwon 0 siblings, 0 replies; 6+ messages in thread From: Hoegeun Kwon @ 2017-09-01 7:07 UTC (permalink / raw) To: daniel.vetter, jani.nikula, seanpaul, airlied Cc: linux-kernel, dri-devel, Hoegeun Kwon 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@samsung.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. 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] drm/vblank: Fix delta_ns to an absolute value @ 2017-09-01 7:07 ` Hoegeun Kwon 0 siblings, 0 replies; 6+ messages in thread From: Hoegeun Kwon @ 2017-09-01 7:07 UTC (permalink / raw) To: daniel.vetter, jani.nikula, seanpaul, airlied Cc: dri-devel, linux-kernel, Hoegeun Kwon 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@samsung.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. 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/vblank: Fix delta_ns to an absolute value 2017-09-01 7:07 ` Hoegeun Kwon (?) @ 2017-09-01 13:36 ` Ville Syrjälä 2017-09-04 7:36 ` Daniel Vetter -1 siblings, 1 reply; 6+ messages in thread From: Ville Syrjälä @ 2017-09-01 13:36 UTC (permalink / raw) To: Hoegeun Kwon Cc: daniel.vetter, jani.nikula, seanpaul, airlied, linux-kernel, dri-devel 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@samsung.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. > > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/vblank: Fix delta_ns to an absolute value 2017-09-01 13:36 ` Ville Syrjälä @ 2017-09-04 7:36 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2017-09-04 7:36 UTC (permalink / raw) To: Ville Syrjälä Cc: daniel.vetter, Hoegeun Kwon, dri-devel, linux-kernel 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@samsung.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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/vblank: Fix delta_ns to an absolute value @ 2017-09-04 7:36 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2017-09-04 7:36 UTC (permalink / raw) To: Ville Syrjälä Cc: Hoegeun Kwon, linux-kernel, dri-devel, daniel.vetter 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@samsung.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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/vblank: Fix delta_ns to an absolute value 2017-09-04 7:36 ` Daniel Vetter (?) @ 2017-09-05 6:36 ` Hoegeun Kwon -1 siblings, 0 replies; 6+ messages in thread From: Hoegeun Kwon @ 2017-09-05 6:36 UTC (permalink / raw) To: Ville Syrjälä, linux-kernel, dri-devel, daniel.vetter Cc: 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@samsung.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 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@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> -- >> Ville Syrjälä >> Intel OTC >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-05 6:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20170901070725epcas1p288a5be80c3a3edb02433d24f5beed338@epcas1p2.samsung.com>
2017-09-01 7:07 ` [PATCH] drm/vblank: Fix delta_ns to an absolute value Hoegeun Kwon
2017-09-01 7:07 ` Hoegeun Kwon
2017-09-01 13:36 ` Ville Syrjälä
2017-09-04 7:36 ` Daniel Vetter
2017-09-04 7:36 ` Daniel Vetter
2017-09-05 6:36 ` Hoegeun Kwon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.