From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: use hrtimer in wait for vblank Date: Mon, 24 Mar 2014 10:55:44 +0100 Message-ID: <20140324095544.GE26878@phenom.ffwll.local> References: <1395648818-14310-1-git-send-email-arun.r.murthy@intel.com> <20140324085134.GJ4366@nuc-i3427.alporthouse.com> <8761n4vtjl.fsf@intel.com> <20140324094809.GP4366@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f54.google.com (mail-ee0-f54.google.com [74.125.83.54]) by gabe.freedesktop.org (Postfix) with ESMTP id 423036E872 for ; Mon, 24 Mar 2014 02:55:49 -0700 (PDT) Received: by mail-ee0-f54.google.com with SMTP id d49so4196799eek.41 for ; Mon, 24 Mar 2014 02:55:48 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140324094809.GP4366@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , "Murthy, Arun R" , Jani Nikula , "daniel.vetter@ffwll.ch" , "intel-gfx@lists.freedesktop.org" , "airlied@linux.ie" List-Id: intel-gfx@lists.freedesktop.org On Mon, Mar 24, 2014 at 09:48:09AM +0000, Chris Wilson wrote: > On Mon, Mar 24, 2014 at 09:34:35AM +0000, Murthy, Arun R wrote: > > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h > > >> b/drivers/gpu/drm/i915/intel_drv.h > > >> index 44067bc..079280a 100644 > > >> --- a/drivers/gpu/drm/i915/intel_drv.h > > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > > >> @@ -52,7 +52,7 @@ > > >> break; \ > > >> } \ > > >> if (W && drm_can_sleep()) { \ > > >> - msleep(W); \ > > >> + usleep_range(W * 1000, W * 2 * 1000); \ > > >> } else { \ > > >> cpu_relax(); \ > > >> } \ > > > > > > Ok. But W is still just a random value we picked for being the mininum > > > legal value for msleep(). So just usleep_range(500, 2000) or somesuch > > > will be fine. We can rename W to CAN_SLEEP it that helps. > > > > We do use _wait_for directly from intel_dp.c with W == 10 to not retry so many times on what's expected to be a long wait. > > Hmm, missed that. We can fallback to the fallback plan of switching > based on W between msleep and usleep_range. Using -1, would be best. > > if (W && drm_can_sleep()) { > if (__builtin_constant_p(W) && W < 0) > usleep_range(500, 2000); > else > msleep(W); > } > > > Its not expected to be too long, we tend to get vblank every 16ms. With using usleep_range > > with min and max as 1-2ms, we have observed that this function consuming 4ms to 17ms. > > vs msleep()? That would be nice information to capture in the changelog. > We expect the average wait here to be 8ms, ofc. > > > Having 10ms sleep timer, we might tend to be blocking for 6ms in some cases.(from the above data) > > Moreover 10ms usleep doesn't guarantee that we get a chance to execute after 10ms, it might > > get increased due to scheduling. So ideal solution would be to use useep_range which uses hrtimers. > > Ideal for what? We only use the sleeping path where we do not care too > great about the latency, otherwise we use the busy-wait path. Improving > the latency of the sleep without adversely affecting overall system > performance/power is ideal. We've used it in a few places where we'd care a bit more about latency (like edid reads) but on most platforms those switched to interrupts now. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch