MIME-version: 1.0
Content-type: text/plain; charset="utf-8"; format="flowed"
Subject: Re: [PATCH] drm/vblank: Fix delta_ns to an absolute value
To: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= <ville.syrjala@linux.intel.com>,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        daniel.vetter@intel.com
From: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Cc: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Message-id: <b54fc435-5ffd-389d-2c51-0bcb0b524a49@samsung.com>
Date: Tue, 05 Sep 2017 15:36:52 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
        Thunderbird/52.2.1
In-reply-to: <20170904073620.pmnlxghc4xe2xqvp@phenom.ffwll.local>
Content-transfer-encoding: 8bit
Content-language: en-US
CMS-TYPE: 102P
DLP-Filter: Pass
References: <CGME20170901070725epcas1p288a5be80c3a3edb02433d24f5beed338@epcas1p2.samsung.com>
        <1504249636-11681-1-git-send-email-hoegeun.kwon@samsung.com>
        <20170901133625.GT4914@intel.com>
        <20170904073620.pmnlxghc4xe2xqvp@phenom.ffwll.local>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2807
Lines: 73

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