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

2014-03-23 Thread Ben Widawsky 
On Sat, Mar 22, 2014 at 08:58:29PM +, 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 +, 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 b...@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

-- 
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2014-03-22 Thread Ben Widawsky 
On Tue, Mar 18, 2014 at 08:44:28AM +, 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 b...@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.

-- 
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2014-03-22 Thread Chris Wilson 
On Sat, Mar 22, 2014 at 12:43:28PM -0700, Ben Widawsky wrote:
 On Tue, Mar 18, 2014 at 08:44:28AM +, 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 b...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2014-03-18 Thread Chris Wilson 
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 b...@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

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 07/26] drm/i915: clean up PPGTT init error path

2014-03-17 Thread Ben Widawsky 
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 b...@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);
+
+   return 0;
 }
 
 static void
-- 
1.9.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx