Re: [PATCH 07/26] drm/i915: clean up PPGTT init error path

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>,
	Ben Widawsky <benjamin.widawsky@intel.com>
Subject: Re: [PATCH 07/26] drm/i915: clean up PPGTT init error path
Date: Sat, 22 Mar 2014 20:58:29 +0000	[thread overview]
Message-ID: <20140322205829.GB4366@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <20140322194328.GA18765@bwidawsk.net>

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 <ben@bwidawsk.net>
> > > ---
> > >  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