Re: [Intel-gfx] [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt

2014-08-06 Thread Thierry, Michel 


 -Original Message-
 From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch]
 Sent: Monday, August 04, 2014 3:19 PM
 To: Intel Graphics Development
 Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä
 Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt
 
 Stuffing this into the context setup code doesn't make a lot of sense.
 Also reusing the real ppgtt setup code makes even less sense since the
 aliasing ppgtt isn't a real address space. Leaving all that stuff
 unitialized will make sure that we catch any abusers promptly.
 
 This is also a prep work to clean up the context-ppgtt link.
 
 v2: Fix up the logic fail, I've fumbled it so badly to completely
 disable ppgtt on gen6. Spotted by Ville and Michel. Also move around
 the pde write into the gen6 init function, since otherwise it won't
 work at all.
 
 Cc: Thierry, Michel michel.thie...@intel.com
 Cc: Ville Syrjälä ville.syrj...@linux.intel.com
 Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
 ---
  drivers/gpu/drm/i915/i915_gem_context.c | 13 +-
  drivers/gpu/drm/i915/i915_gem_gtt.c | 42 +++-
 -
  2 files changed, 31 insertions(+), 24 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
 b/drivers/gpu/drm/i915/i915_gem_context.c
 index 3b8367aa8404..7a455fcee3a7 100644
 --- a/drivers/gpu/drm/i915/i915_gem_context.c
 +++ b/drivers/gpu/drm/i915/i915_gem_context.c
 @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev,
   goto err_unpin;
   } else
   ctx-vm = ppgtt-base;
 -
 - /* This case is reserved for the global default context and
 -  * should only happen once. */
 - if (is_global_default_ctx) {
 - if (WARN_ON(dev_priv-mm.aliasing_ppgtt)) {
 - ret = -EEXIST;
 - goto err_unpin;
 - }
 -
 - dev_priv-mm.aliasing_ppgtt = ppgtt;
 - }

I expect some problems with full ppgtt  this (in some places, the driver is 
still making some decisions based on dev_priv-mm.aliasing_ppgtt, which now 
will be null). 
Should I address these problems in a subsequent patch?

   } else if (USES_PPGTT(dev)) {
   /* For platforms which only have aliasing PPGTT, we fake the
* address space and refcounting. */
 @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
   }
   }
 
 - ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
 + ctx = i915_gem_create_context(dev, NULL,
 USES_FULL_PPGTT(dev));
   if (IS_ERR(ctx)) {
   DRM_ERROR(Failed to create default global context (error
 %ld)\n,
 PTR_ERR(ctx));
  }
 
 --
 1.9.3



smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt

2014-08-06 Thread Daniel Vetter 
On Wed, Aug 06, 2014 at 08:18:52AM +, Thierry, Michel wrote:
 
 
  -Original Message-
  From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch]
  Sent: Monday, August 04, 2014 3:19 PM
  To: Intel Graphics Development
  Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä
  Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global 
  gtt
  
  Stuffing this into the context setup code doesn't make a lot of sense.
  Also reusing the real ppgtt setup code makes even less sense since the
  aliasing ppgtt isn't a real address space. Leaving all that stuff
  unitialized will make sure that we catch any abusers promptly.
  
  This is also a prep work to clean up the context-ppgtt link.
  
  v2: Fix up the logic fail, I've fumbled it so badly to completely
  disable ppgtt on gen6. Spotted by Ville and Michel. Also move around
  the pde write into the gen6 init function, since otherwise it won't
  work at all.
  
  Cc: Thierry, Michel michel.thie...@intel.com
  Cc: Ville Syrjälä ville.syrj...@linux.intel.com
  Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
  ---
   drivers/gpu/drm/i915/i915_gem_context.c | 13 +-
   drivers/gpu/drm/i915/i915_gem_gtt.c | 42 +++-
  -
   2 files changed, 31 insertions(+), 24 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
  b/drivers/gpu/drm/i915/i915_gem_context.c
  index 3b8367aa8404..7a455fcee3a7 100644
  --- a/drivers/gpu/drm/i915/i915_gem_context.c
  +++ b/drivers/gpu/drm/i915/i915_gem_context.c
  @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev,
  goto err_unpin;
  } else
  ctx-vm = ppgtt-base;
  -
  -   /* This case is reserved for the global default context and
  -* should only happen once. */
  -   if (is_global_default_ctx) {
  -   if (WARN_ON(dev_priv-mm.aliasing_ppgtt)) {
  -   ret = -EEXIST;
  -   goto err_unpin;
  -   }
  -
  -   dev_priv-mm.aliasing_ppgtt = ppgtt;
  -   }
 
 I expect some problems with full ppgtt  this (in some places, the
 driver is still making some decisions based on
 dev_priv-mm.aliasing_ppgtt, which now will be null).  Should I address
 these problems in a subsequent patch?

Oh, good catch. I've done a bit a review and found two cases:
- Driver unload code. Already fairly broken, made much worse by my series
  here. I've fixed that up yesterday and will resend the series with those
  additional patches and revised patches.
- cmd parser. I guess that should be a fixup patch on top. I'll also do
  that.

Have you spotted any other places?

Thanks, Daniel

 
  } else if (USES_PPGTT(dev)) {
  /* For platforms which only have aliasing PPGTT, we fake the
   * address space and refcounting. */
  @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
  }
  }
  
  -   ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
  +   ctx = i915_gem_create_context(dev, NULL,
  USES_FULL_PPGTT(dev));
  if (IS_ERR(ctx)) {
  DRM_ERROR(Failed to create default global context (error
  %ld)\n,
PTR_ERR(ctx));
   }
  
  --
  1.9.3
 



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt

2014-08-06 Thread Thierry, Michel 


 -Original Message-
 From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
 Vetter
 Sent: Wednesday, August 06, 2014 9:31 AM
 To: Thierry, Michel
 Cc: Daniel Vetter; Intel Graphics Development
 Subject: Re: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of
global gtt
 
 On Wed, Aug 06, 2014 at 08:18:52AM +, Thierry, Michel wrote:
 
 
   -Original Message-
   From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch]
   Sent: Monday, August 04, 2014 3:19 PM
   To: Intel Graphics Development
   Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä
   Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of
global gtt
  
   Stuffing this into the context setup code doesn't make a lot of sense.
   Also reusing the real ppgtt setup code makes even less sense since the
   aliasing ppgtt isn't a real address space. Leaving all that stuff
   unitialized will make sure that we catch any abusers promptly.
  
   This is also a prep work to clean up the context-ppgtt link.
  
   v2: Fix up the logic fail, I've fumbled it so badly to completely
   disable ppgtt on gen6. Spotted by Ville and Michel. Also move around
   the pde write into the gen6 init function, since otherwise it won't
   work at all.
  
   Cc: Thierry, Michel michel.thie...@intel.com
   Cc: Ville Syrjälä ville.syrj...@linux.intel.com
   Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
   ---
drivers/gpu/drm/i915/i915_gem_context.c | 13 +-
drivers/gpu/drm/i915/i915_gem_gtt.c | 42
 +++-
   -
2 files changed, 31 insertions(+), 24 deletions(-)
  
   diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
   b/drivers/gpu/drm/i915/i915_gem_context.c
   index 3b8367aa8404..7a455fcee3a7 100644
   --- a/drivers/gpu/drm/i915/i915_gem_context.c
   +++ b/drivers/gpu/drm/i915/i915_gem_context.c
   @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device
 *dev,
 goto err_unpin;
 } else
 ctx-vm = ppgtt-base;
   -
   - /* This case is reserved for the global default context and
   -  * should only happen once. */
   - if (is_global_default_ctx) {
   - if (WARN_ON(dev_priv-mm.aliasing_ppgtt)) {
   - ret = -EEXIST;
   - goto err_unpin;
   - }
   -
   - dev_priv-mm.aliasing_ppgtt = ppgtt;
   - }
 
  I expect some problems with full ppgtt  this (in some places, the
  driver is still making some decisions based on
  dev_priv-mm.aliasing_ppgtt, which now will be null).  Should I address
  these problems in a subsequent patch?
 
 Oh, good catch. I've done a bit a review and found two cases:
 - Driver unload code. Already fairly broken, made much worse by my series
   here. I've fixed that up yesterday and will resend the series with those
   additional patches and revised patches.
 - cmd parser. I guess that should be a fixup patch on top. I'll also do
   that.
 
 Have you spotted any other places?
The other place is gen8_ring_dispatch_execbuffer, it won't set the ppgtt
flag in MI_BATCH_BUFFER_START.

 
 Thanks, Daniel
 
 
 } else if (USES_PPGTT(dev)) {
 /* For platforms which only have aliasing PPGTT, we fake the
  * address space and refcounting. */
   @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device
 *dev)
 }
 }
  
   - ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
   + ctx = i915_gem_create_context(dev, NULL,
   USES_FULL_PPGTT(dev));
 if (IS_ERR(ctx)) {
 DRM_ERROR(Failed to create default global context (error
   %ld)\n,
   PTR_ERR(ctx));
}
  
   --
   1.9.3
 
 
 
 
 --
 Daniel Vetter
 Software Engineer, Intel Corporation
 +41 (0) 79 365 57 48 - http://blog.ffwll.ch


smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt

2014-08-06 Thread Daniel Vetter 
On Wed, Aug 06, 2014 at 08:44:34AM +, Thierry, Michel wrote:
 
 
  -Original Message-
  From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
  Vetter
  Sent: Wednesday, August 06, 2014 9:31 AM
  To: Thierry, Michel
  Cc: Daniel Vetter; Intel Graphics Development
  Subject: Re: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of
 global gtt
  
  On Wed, Aug 06, 2014 at 08:18:52AM +, Thierry, Michel wrote:
  
  
-Original Message-
From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch]
Sent: Monday, August 04, 2014 3:19 PM
To: Intel Graphics Development
Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä
Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of
 global gtt
   
Stuffing this into the context setup code doesn't make a lot of sense.
Also reusing the real ppgtt setup code makes even less sense since the
aliasing ppgtt isn't a real address space. Leaving all that stuff
unitialized will make sure that we catch any abusers promptly.
   
This is also a prep work to clean up the context-ppgtt link.
   
v2: Fix up the logic fail, I've fumbled it so badly to completely
disable ppgtt on gen6. Spotted by Ville and Michel. Also move around
the pde write into the gen6 init function, since otherwise it won't
work at all.
   
Cc: Thierry, Michel michel.thie...@intel.com
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
 drivers/gpu/drm/i915/i915_gem_context.c | 13 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c | 42
  +++-
-
 2 files changed, 31 insertions(+), 24 deletions(-)
   
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 3b8367aa8404..7a455fcee3a7 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device
  *dev,
goto err_unpin;
} else
ctx-vm = ppgtt-base;
-
-   /* This case is reserved for the global default context 
and
-* should only happen once. */
-   if (is_global_default_ctx) {
-   if (WARN_ON(dev_priv-mm.aliasing_ppgtt)) {
-   ret = -EEXIST;
-   goto err_unpin;
-   }
-
-   dev_priv-mm.aliasing_ppgtt = ppgtt;
-   }
  
   I expect some problems with full ppgtt  this (in some places, the
   driver is still making some decisions based on
   dev_priv-mm.aliasing_ppgtt, which now will be null).  Should I address
   these problems in a subsequent patch?
  
  Oh, good catch. I've done a bit a review and found two cases:
  - Driver unload code. Already fairly broken, made much worse by my series
here. I've fixed that up yesterday and will resend the series with those
additional patches and revised patches.
  - cmd parser. I guess that should be a fixup patch on top. I'll also do
that.
  
  Have you spotted any other places?
 The other place is gen8_ring_dispatch_execbuffer, it won't set the ppgtt
 flag in MI_BATCH_BUFFER_START.

Indeed. I've also found a few other really fishy places. Will follow up
with revised patches.
-Daniel

 
  
  Thanks, Daniel
  
  
} else if (USES_PPGTT(dev)) {
/* For platforms which only have aliasing PPGTT, we 
fake the
 * address space and refcounting. */
@@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device
  *dev)
}
}
   
-   ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
+   ctx = i915_gem_create_context(dev, NULL,
USES_FULL_PPGTT(dev));
if (IS_ERR(ctx)) {
DRM_ERROR(Failed to create default global context 
(error
%ld)\n,
  PTR_ERR(ctx));
 }
   
--
1.9.3
  
  
  
  
  --
  Daniel Vetter
  Software Engineer, Intel Corporation
  +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt

2014-08-04 Thread Daniel Vetter 
Stuffing this into the context setup code doesn't make a lot of sense.
Also reusing the real ppgtt setup code makes even less sense since the
aliasing ppgtt isn't a real address space. Leaving all that stuff
unitialized will make sure that we catch any abusers promptly.

This is also a prep work to clean up the context-ppgtt link.

v2: Fix up the logic fail, I've fumbled it so badly to completely
disable ppgtt on gen6. Spotted by Ville and Michel. Also move around
the pde write into the gen6 init function, since otherwise it won't
work at all.

Cc: Thierry, Michel michel.thie...@intel.com
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
 drivers/gpu/drm/i915/i915_gem_context.c | 13 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c | 42 +++--
 2 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 3b8367aa8404..7a455fcee3a7 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev,
goto err_unpin;
} else
ctx-vm = ppgtt-base;
-
-   /* This case is reserved for the global default context and
-* should only happen once. */
-   if (is_global_default_ctx) {
-   if (WARN_ON(dev_priv-mm.aliasing_ppgtt)) {
-   ret = -EEXIST;
-   goto err_unpin;
-   }
-
-   dev_priv-mm.aliasing_ppgtt = ppgtt;
-   }
} else if (USES_PPGTT(dev)) {
/* For platforms which only have aliasing PPGTT, we fake the
 * address space and refcounting. */
@@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
}
}
 
-   ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
+   ctx = i915_gem_create_context(dev, NULL, USES_FULL_PPGTT(dev));
if (IS_ERR(ctx)) {
DRM_ERROR(Failed to create default global context (error 
%ld)\n,
  PTR_ERR(ctx));
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4fa7807ed4d5..a4c1740d79be 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1188,35 +1188,38 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 ppgtt-node.size  20,
 ppgtt-node.start / PAGE_SIZE);
 
+   gen6_write_pdes(ppgtt);
+   DRM_DEBUG(Adding PPGTT at offset %x\n,
+ ppgtt-pd_offset  10);
+
return 0;
 }
 
-int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
+int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 {
struct drm_i915_private *dev_priv = dev-dev_private;
-   int ret = 0;
 
ppgtt-base.dev = dev;
ppgtt-base.scratch = dev_priv-gtt.base.scratch;
 
if (INTEL_INFO(dev)-gen  8)
-   ret = gen6_ppgtt_init(ppgtt);
+   return gen6_ppgtt_init(ppgtt);
else if (IS_GEN8(dev))
-   ret = gen8_ppgtt_init(ppgtt, dev_priv-gtt.base.total);
+   return gen8_ppgtt_init(ppgtt, dev_priv-gtt.base.total);
else
BUG();
+}
+int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
+{
+   struct drm_i915_private *dev_priv = dev-dev_private;
+   int ret = 0;
 
-   if (!ret) {
-   struct drm_i915_private *dev_priv = dev-dev_private;
+   ret = __hw_ppgtt_init(dev, ppgtt);
+   if (ret == 0) {
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);
-   }
}
 
return ret;
@@ -1728,6 +1731,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
struct drm_mm_node *entry;
struct drm_i915_gem_object *obj;
unsigned long hole_start, hole_end;
+   int ret;
 
BUG_ON(mappable_end  end);
 
@@ -1739,7 +1743,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
/* Mark any preallocated objects as occupied */
list_for_each_entry(obj, dev_priv-mm.bound_list, global_list) {
struct i915_vma *vma = i915_gem_obj_to_vma(obj, ggtt_vm);
-   int ret;
+
DRM_DEBUG_KMS(reserving preallocated space: %lx + %zx\n,
  i915_gem_obj_ggtt_offset(obj), obj-base.size);
 
@@ -1766,6