From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 07/26] drm/i915: clean up PPGTT init error path Date: Sat, 22 Mar 2014 20:58:29 +0000 Message-ID: <20140322205829.GB4366@nuc-i3427.alporthouse.com> References: <1395121738-29126-1-git-send-email-benjamin.widawsky@intel.com> <1395121738-29126-8-git-send-email-benjamin.widawsky@intel.com> <20140318084428.GD13504@nuc-i3427.alporthouse.com> <20140322194328.GA18765@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from fireflyinternet.com (mail.fireflyinternet.com [87.106.93.118]) by gabe.freedesktop.org (Postfix) with ESMTP id A2D5B6E5C2 for ; Sat, 22 Mar 2014 13:58:34 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140322194328.GA18765@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ben Widawsky Cc: Intel GFX , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Sat, Mar 22, 2014 at 12:43:28PM -0700, Ben Widawsky wrote: > On Tue, Mar 18, 2014 at 08:44:28AM +0000, Chris Wilson wrote: > > On Mon, Mar 17, 2014 at 10:48:39PM -0700, Ben Widawsky wrote: > > > The old code (I'm having trouble finding the commit) had a reason for > > > doing things when there was an error, and would continue on, thus the > > > !ret. For the newer code however, this looks completely silly. > > > > > > Follow the normal idiom of if (ret) return ret. > > > > > > Also, put the pde wiring in the gen specific init, now that GEN8 exists. > > > > > > Signed-off-by: Ben Widawsky > > > --- > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 22 +++++++++------------- > > > 1 file changed, 9 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > index 1620211..5f73284 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > @@ -1202,6 +1202,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > > > ppgtt->pd_offset = > > > ppgtt->node.start / PAGE_SIZE * sizeof(gen6_gtt_pte_t); > > > > > > + gen6_write_pdes(ppgtt); > > > + > > > ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true); > > > > > > DRM_DEBUG_DRIVER("Allocated pde space (%ldM) at GTT entry: %lx\n", > > > @@ -1226,20 +1228,14 @@ int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) > > > else > > > BUG(); > > > > > > - if (!ret) { > > > - struct drm_i915_private *dev_priv = dev->dev_private; > > > - kref_init(&ppgtt->ref); > > > - drm_mm_init(&ppgtt->base.mm, ppgtt->base.start, > > > - ppgtt->base.total); > > > - i915_init_vm(dev_priv, &ppgtt->base); > > > - if (INTEL_INFO(dev)->gen < 8) { > > > - gen6_write_pdes(ppgtt); > > > - DRM_DEBUG("Adding PPGTT at offset %x\n", > > > - ppgtt->pd_offset << 10); > > > - } > > > - } > > > + if (ret) > > > + return ret; > > > > > > - return ret; > > > + kref_init(&ppgtt->ref); > > > + drm_mm_init(&ppgtt->base.mm, ppgtt->base.start, ppgtt->base.total); > > > + i915_init_vm(dev_priv, &ppgtt->base); > > > > Didn't we just delete the dev_priv local variable? > > -Chris > > The important part is that the pde writes moved. (The DRM debug is also > dropped). As for this code, I just wanted to get rid of the if (!ret) > block. It looks weird. > > Maybe I didn't get what you're asking though. I was wondering if this patch compiles because of the removal of the dev_priv local variable. (Or if the original was a shadow.) -Chris -- Chris Wilson, Intel Open Source Technology Centre