From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 18/19] drm/i915: save some time when waiting the eDP timings Date: Mon, 25 Nov 2013 18:38:53 -0800 Message-ID: <20131126023853.GA27598@bwidawsk.net> References: <1385048853-1579-1-git-send-email-przanoni@gmail.com> <1385048853-1579-19-git-send-email-przanoni@gmail.com> <20131121160017.GD9515@nuc-i3427.alporthouse.com> <20131125221755.GA19602@bwidawsk.net> <20131125232510.GC31545@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail.bwidawsk.net (bwidawsk.net [166.78.191.112]) by gabe.freedesktop.org (Postfix) with ESMTP id 29A60FADD8 for ; Mon, 25 Nov 2013 18:39:05 -0800 (PST) Content-Disposition: inline In-Reply-To: <20131125232510.GC31545@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Chris Wilson , Paulo Zanoni , intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Mon, Nov 25, 2013 at 11:25:10PM +0000, Chris Wilson wrote: > On Mon, Nov 25, 2013 at 02:17:55PM -0800, Ben Widawsky wrote: > > On Thu, Nov 21, 2013 at 04:00:17PM +0000, Chris Wilson wrote: > > > On Thu, Nov 21, 2013 at 01:47:32PM -0200, Paulo Zanoni wrote: > > > > From: Paulo Zanoni > > > > > > > > The eDP spec defines some points where after you do action A, you have > > > > to wait some time before action B. The thing is that in our driver > > > > action B does not happen exactly after action A, but we still use > > > > msleep() calls directly. What this patch happens is that we record the > > > > timestamp of when action A happened, then, just before action B, we > > > > look at how much time has passed and only sleep the remaining amount > > > > needed. > > > > > > > > With this change, I am able to save about 5-20ms (out of the total > > > > 200ms) of the backlight_off delay and completely skip the 1ms > > > > backlight_on delay. The 600ms vdd_off delay doesn't happen during > > > > normal usage anymore due to a previous patch. > > > > > > > > Signed-off-by: Paulo Zanoni > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 38 +++++++++++++++++++++++++++++++++++--- > > > > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > > > > 2 files changed, 38 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > > index b438e76..3a1ca80 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -1051,12 +1051,41 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp) > > > > ironlake_wait_panel_status(intel_dp, IDLE_OFF_MASK, IDLE_OFF_VALUE); > > > > } > > > > > > > > +static void ironlake_wait_jiffies_delay(unsigned long timestamp, > > > > + int to_wait_ms) > > > This is not hw specific, so just > > > intel_wait_until_after(timestamp_jiffies, to_wait_ms) > > > > Can't we do this with our existing wait_for, and get all the other junk > > we've crammed in there? > > wait_for(false, timestamp + to_wait_ms) > > > > Or do I have this all wrong? > > It would be > wait_for(false, jiffies_to_ms(max(ms_to_jiffies(timestamp+to_wait_ms) - jiffies, 0)) > or something pretty similar. Definitely macro abuse as you would be > hoping that the compiler turned > while (!time_after(jiffies, timeout)) msleep(1); > into > msleep(to_ms(timeout-jiffies)); > > So perhaps clearer would be: > intel_wait_until_after(unsigned long timestamp_jiffies, > int to_wait_ms) > { > timestamp_jiffies += msec_to_jiffes(to_wait_ms) + 1; > if (time_after(timestamp_jiffies, jiffies) { > timestamp_jiffies -= jiffies; > while (timestamp_jiffies) > timestamp_jiffies = schedule_timeout(timestamp_jiffies); > } > } > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre Oops, I meant "timestamp + to_wait_ms - jiffies_to_msecs(jiffies)" I agree it does get a bit messy, but I think there is likely a cleaner way that what you propose if we store things as jiffies. I hate dealing with jiffies, and I feel like even your function has a race with jiffies though. let timestamp_jiffies: 2 let jiffies: 1 if jiffies increments by 2 after your timestamp_jiffies -=, you'll have a problem. > if (time_after(timestamp_jiffies, jiffies) { time_after(2, 1) // jiffies increments by 2 > timestamp_jiffies -= jiffies; timestamp_jiffies -= 3 > while (timestamp_jiffies) while (-1) > timestamp_jiffies = schedule_timeout(timestamp_jiffies); schedule_timeout(-1) -- Ben Widawsky, Intel Open Source Technology Center