[Intel-gfx] [PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Paulo Zanoni]

2014/1/17 Daniel Vetter <daniel at ffwll.ch>:
> On Fri, Jan 17, 2014 at 10:21 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> On Fri, Jan 17, 2014 at 07:11:14PM -0200, Paulo Zanoni wrote:
>>> 2014/1/17 Chris Wilson <chris at chris-wilson.co.uk>:
>>> > On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote:
>>> >> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>> >>
>>> >> The eDP code records a few timestamps containing the last time we took
>>> >> some actions, because we need to wait before doing some other actions.
>>> >> The problem is that if we store a timestamp when suspending and then
>>> >> look at it when resuming, we'll ignore the unknown amount of time we
>>> >> actually were suspended.
>>> >>
>>> >> This happens with the panel power cycle delay: it's 500ms on my
>>> >> machine, and it's delaying the resume sequence by 200ms due to a
>>> >> timestamp we recorded before suspending. This patch should solve this
>>> >> problem by resetting the timestamps.
>>> >
>>> > But you don't explain why this is safe. The code nerfs the timeouts so
>>> > that they are ignored, yet the delays are independent. Should this be
>>> > based on realtime rather than jiffies?
>>>
>>> I'm not sure I understand your question. What's the problem you see exactly?
>>
>> Given the fast suspend & resume, we will not have waited the required
>> panel off time before poking it again etc. What makes that safe?
>
> Even worse the kernel might abort the suspend due to some issue and
> we'll immediately resume. Also, and immediate thaw operation after
> freezing is how hibernate works. Iirc the hw always enforces the full
> power off delay after a power reset for exactly this reason (at least
> on current platforms afaik).

Oh, now I get it. My bad. A brief experiment here shows that
do_gettimeofday or current_kernel_time can probably be used to fix the
problem. But since that requires properly getting all the timestamps
in the correct units, maybe rewriting some functions, I'll wait a few
days before I come back to this problem.

> But with the minimal delays in patch 6
> that won't help any more, either.

Patch 6 should be independent of this. This patch is just to save us
some time in the resume cases, but patch 6 is to correct the amount of
time we wait while we disable the panel. I don't see a reason to block
patch 6 on this one.

Thanks for the reviews,
Paulo

>
> I fear we need a bit more smarts here using a realtime clock source to
> figure out whether actually sufficient time elapsed :(
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni