From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 4/5] drm/i915: save some time when waiting the eDP timings Date: Wed, 11 Dec 2013 11:06:04 +0100 Message-ID: <20131211100604.GW9804@phenom.ffwll.local> References: <1386358364-1539-1-git-send-email-przanoni@gmail.com> <1386358364-1539-5-git-send-email-przanoni@gmail.com> <20131210222859.GT9804@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wg0-f42.google.com (mail-wg0-f42.google.com [74.125.82.42]) by gabe.freedesktop.org (Postfix) with ESMTP id CA6CDFAC4E for ; Wed, 11 Dec 2013 02:05:15 -0800 (PST) Received: by mail-wg0-f42.google.com with SMTP id a1so5750062wgh.3 for ; Wed, 11 Dec 2013 02:05:13 -0800 (PST) Content-Disposition: inline In-Reply-To: <20131210222859.GT9804@phenom.ffwll.local> 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: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Tue, Dec 10, 2013 at 11:28:59PM +0100, Daniel Vetter wrote: > On Fri, Dec 06, 2013 at 05:32:43PM -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 does 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. > > > > v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and > > move it to intel_display.c > > - Fix the msleep call: diff is in jiffies > > v3: - Use "tmp_jiffies" so we don't need to worry about the value of > > "jiffies" advancing while we're doing the math. > > > > Signed-off-by: Paulo Zanoni > > --- > > drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++ > > drivers/gpu/drm/i915/intel_dp.c | 26 +++++++++++++++++++++++--- > > drivers/gpu/drm/i915/intel_drv.h | 4 ++++ > > 3 files changed, 45 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 3c59b67..0c238dd 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -806,6 +806,24 @@ void intel_wait_for_vblank(struct drm_device *dev, int pipe) > > DRM_DEBUG_KMS("vblank wait timed out\n"); > > } > > > > +/* If you need to wait X ms between events A and B, but event B doesn't happen > > + * exactly after event A, you record the timestamp (jiffies) of when event A > > + * happened, then just before event B you call intel_wait_until_after and pass > > + * the timestamp as the first argument, and X as the second argument. */ > > +void intel_wait_until_after(unsigned long timestamp, int to_wait_ms) > > +{ > > + unsigned long target = timestamp + msecs_to_jiffies(to_wait_ms); > > msec_to_jiffies_timeout is what we want here. Also I nowadays prefer > multiline comments with the /* and */ on their own line for more > consistency ... > -Daniel > > > + unsigned long diff; > > + /* Don't re-read the value of "jiffies" every time since it may change > > + * behind our back and break the math. */ > > + unsigned long tmp_jiffies = jiffies; > > + > > + if (time_after(target, tmp_jiffies)) { > > + diff = (long)target - (long)tmp_jiffies; > > + msleep(jiffies_to_msecs(diff)); > > This will add one more jiffy again, so for optimal results we need to use > something else here I think. schedule_timeout is what I think we should use here. Also, this function isn't really anything intel specific, so I think we should drop the intel_ prefix and shovel it as a static inline (it's really small after all) next to the other generic timeout helpers at the bottom of i915_drv.h. I'm also a bit unhappy still about the name - we should somehow make it clearer that the timeout is in ms. A few ideas: msleep_start_from_jiffies msleep_form_jiffies wait_after_until_ms wait_remaining_ms_from_jiffies Also mabye rename timestamp to ts_jiffies or so. All this bikeshedding here is because the mixing of time-units here irks me a bit (and I pretty much expect someone to botch it eventually). So spending a bit more time on coming up with a really good name would be good imo. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch