From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH] drm/i915: Drop the msleep parameter to wait_for() Date: Tue, 24 Aug 2010 00:33:18 +0100 Message-ID: <8u3s94$gajv98@orsmga002.jf.intel.com> References: <1282475148-15951-10-git-send-email-chris@chris-wilson.co.uk> <1282582569-24138-1-git-send-email-chris@chris-wilson.co.uk> <1282605397.29375.7.camel@pcjc2lap> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id C77EF9F04F for ; Mon, 23 Aug 2010 16:33:21 -0700 (PDT) In-Reply-To: <1282605397.29375.7.camel@pcjc2lap> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Peter Clifton , "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Tue, 24 Aug 2010 00:16:37 +0100, Peter Clifton wrote: > I noticed that the patch changes the semantics of some of the wait_for > calls. Previously, many were called with a zero msleep parameter - > meaning the call would not msleep. With this patch, the cases below will > now msleep(1), rather than not msleep'ing at all. Intentionally. The choices I made when adding the wait_for() were fairly arbitrary. I'd err on the side of sleeping the extra milliseconds rather than spend 500 microseconds busy-spinning. (I've have a different opinion if these ever become the rate-limiting step in modesetting... ;-) > > /* Wait for compressing bit to clear */ > > - if (wait_for((I915_READ(FBC_STATUS) & FBC_STAT_COMPRESSING) == 0, 10, 0)) { > > + if (wait_for((I915_READ(FBC_STATUS) & FBC_STAT_COMPRESSING) == 0, 10)) { > ^___ wait_for_atomic? This is perhaps the most debatable as I have no feel for what the compression delay is and the spin may only be on the order of a few hundred microseconds. However, I think the wait_for() here is entirely superfluous and have removed it in my drm-testing. A couple of the later patches increase those short timeouts you highlighted to fix reported issues, so those are poor candidates for busy-spinning. So outside of kdb, I don't see a reason where we need to be continuously polling the register. -- Chris Wilson, Intel Open Source Technology Centre