| Message ID | 1395121738-29126-8-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
On Sat, Mar 22, 2014 at 08:58:29PM +0000, Chris Wilson wrote: > 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 Ah, of course. Yes, there was a shadowed dev_priv. I think it was merge/rebase fail either by myself or Daniel when the original patches were merged. > > -- > Chris Wilson, Intel Open Source Technology Centre
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); + + return 0; } static void
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(-)