From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 5/9] drm/i915/bdw: Reorganize PT allocations Date: Wed, 12 Feb 2014 15:52:20 -0800 Message-ID: <20140212235220.GB4269@bwidawsk.net> References: <1392244132-6806-1-git-send-email-benjamin.widawsky@intel.com> <1392244132-6806-6-git-send-email-benjamin.widawsky@intel.com> <20140212234559.GI14909@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 68952FB00E for ; Wed, 12 Feb 2014 15:52:26 -0800 (PST) Content-Disposition: inline In-Reply-To: <20140212234559.GI14909@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 , Ben Widawsky , Intel GFX List-Id: intel-gfx@lists.freedesktop.org On Wed, Feb 12, 2014 at 11:45:59PM +0000, Chris Wilson wrote: > On Wed, Feb 12, 2014 at 02:28:48PM -0800, Ben Widawsky wrote: > > - for (i = first_pte; i < last_pte; i++) > > + for (i = which_pte; i < last_pte; i++) { > > pt_vaddr[i] = scratch_pte; > > + num_entries--; > > + BUG_ON(num_entries < 0); > > + } > > > > kunmap_atomic(pt_vaddr); > > > > - num_entries -= last_pte - first_pte; > > I'm going to moan about this being replaced by a BUG_ON inside the inner > loop. > I'm fine with removing it. I guess doing any sort of perf with BUG_ON is ill advised, but running without BUG_ON is perhaps equally ill advised. > > - first_pte = 0; > > - act_pt++; > > + which_pte = 0; > > > + if (which_pde + 1 == GEN8_PDES_PER_PAGE) > > + which_pdpe++; > > + which_pde = (which_pde + 1) & GEN8_PDE_MASK; > > I think this would be clearer written as > if (++which_pde == GEN8_PDES_PER_PAGE) { > which_pdpe++; > which_pde = 0; > } I'm fine with that change. > as then the relationship between pdpe and pde is much more apparent to > me. Do we feel that which_pte, which_pde, which_pdpe are really any > better than pte, pde, pdpe? Or is it important to question ourselves > every step of the way? I actually just don't like act_, and first_, dropping the "which_" is perfectly acceptable to me. > > And I may as well moan about having to preallocate everything. ;-) > -Chris > Deferring allocation is an important but separate step. > -- > Chris Wilson, Intel Open Source Technology Centre I'll give Imre a bit to leave comments and then respin. -- Ben Widawsky, Intel Open Source Technology Center