From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 2/2] drm/i915: Embellish wait_end trace Date: Tue, 29 Jul 2014 23:33:43 -0700 Message-ID: <20140730063342.GA2332@bwidawsk.net> References: <1406664870-29970-1-git-send-email-benjamin.widawsky@intel.com> <1406664870-29970-2-git-send-email-benjamin.widawsky@intel.com> <20140730061926.GI21570@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 12A066E54B for ; Tue, 29 Jul 2014 23:33:56 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140730061926.GI21570@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 , Ben Widawsky , Intel GFX List-Id: intel-gfx@lists.freedesktop.org On Wed, Jul 30, 2014 at 07:19:26AM +0100, Chris Wilson wrote: > On Tue, Jul 29, 2014 at 01:14:30PM -0700, Ben Widawsky wrote: > > This adds two new data points to the trace event, wait time, and whether > > or not the event slept. Both of these should already be obtainable > > through various means. This patch just makes the data more accessible. > > Right, the key point is that since the advent of the wait_begin/_end > pair is that we now allow concurrent non-blocking waits. > > > Wait is obtainable with the current code by matching seqnos in > > begin/end. In simple cases where begin/ends are always paired, this is > > trivial. However, if you queue up multiple begins/ends, it can get > > confusing. We're already calculating wait time, so it's trivially added > > here. This patch also provides the slightly more accurate wait_time as > > opposed to the timestamps from the tracepoint. It's observable, but just > > noise. > > > > The second bit of information, whether or not the operation slept is > > helpful in determining where time went. This is probably also obtainable > > through the scheduler events. However, we have the information easily at > > our fingertips, we may as well give it out. > > > > This results in an event which looks like: > > gem_gtt_hog 409 [000] 32.012641: i915:i915_gem_request_wait_end: dev=0, ring=3, seqno=4294963203, duration=0.000368225 (slept=yes) > > > > While here, rename sleep_time to wait_time since the verb sleep hasn't > > been true for a long time (several conditions exist where it won't > > sleep). > > > > Signed-off-by: Ben Widawsky > > Other than the debate in the earlier patch, this looks fine. > -Chris > I actually don't think wait_begin is a terribly interesting event after this patch BTW, but I didn't want to rock the boat too much. If you agree, I can send that one as well. -- Ben Widawsky, Intel Open Source Technology Center