Re: [Intel-gfx] [PATCH 06/11] drm/i915: Store owning file on the i915_address_space

2015-12-17 Thread Chris Wilson 
On Thu, Dec 17, 2015 at 11:52:06AM +, Tvrtko Ursulin wrote:
> >-ret = __hw_ppgtt_init(dev, ppgtt);
> >+ret = __hw_ppgtt_init(ppgtt, dev_priv);
> > if (ret == 0) {
> > kref_init(>ref);
> > i915_address_space_init(>base, dev_priv);
> >+ppgtt->base.file = file_priv;
> 
> I would keep using file_priv since that's what's it's called all
> over the place but whatever.

Personally I have been working towards dropping the _priv from our
nomenclature inside our driver (only on the boundaries from drm do we
care about translating from drm objects to ours) - in a parallel fashion
to using crtc and crtc->base instead of intel_crtc / crtc.

> >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h 
> >b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >index bae005a62cfc..4e9553ace33f 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >@@ -273,12 +273,11 @@ struct i915_pml4 {
> >  struct i915_address_space {
> > struct drm_mm mm;
> > struct drm_device *dev;
> >+struct drm_i915_file_private *file;
> 
> Suggest putting a comment documenting when it is NULL and when it is
> valid. Commit says so, but I think comment is also needed.

+   /* Every address space belongs to a struct file - except for the global
+* GTT that is owned by the driver (and so @file is set to NULL). In
+* principle, no information should leak from one context to another
+* (or between files/processes etc) unless explicitly shared by the
+* owner. Tracking the owner is important in order to free up per-file
+* objects along with the file, to aide resource tracking, and to
+* assign blame.
+*/
-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 06/11] drm/i915: Store owning file on the i915_address_space

2015-12-17 Thread Tvrtko Ursulin 


Hi,

On 14/12/15 11:36, Chris Wilson wrote:

For the global GTT (and aliasing GTT), the address space is owned by the
device (it is a global resource) and so the per-file owner field is
NULL. For per-process GTT (where we create an address space per
context), each is owned by the opening file. We can use this ownership
information to both distinguish GGTT and ppGTT address spaces, as well
as occasionally inspect the owner.

Signed-off-by: Chris Wilson 
---
  drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
  drivers/gpu/drm/i915/i915_drv.h |  1 -
  drivers/gpu/drm/i915/i915_gem_context.c |  3 ++-
  drivers/gpu/drm/i915/i915_gem_gtt.c | 25 +
  drivers/gpu/drm/i915/i915_gem_gtt.h | 13 ++---
  5 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 9ec133f5af00..179e3c5c5022 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -352,7 +352,7 @@ static int per_file_stats(int id, void *ptr, void *data)
= container_of(vma->vm,
   struct i915_hw_ppgtt,
   base);
-   if (ppgtt->file_priv != stats->file_priv)
+   if (ppgtt->base.file != stats->file_priv)
continue;
}

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6ce163a681f2..b32a00f60e98 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2924,7 +2924,6 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
return container_of(vm, struct i915_hw_ppgtt, base);
  }

-
  static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
  {
return i915_gem_obj_ggtt_bound_view(obj, _ggtt_view_normal);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 25a8498f0b5a..dcb4603a7f03 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -296,7 +296,8 @@ i915_gem_create_context(struct drm_device *dev,
}

if (USES_FULL_PPGTT(dev)) {
-   struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv);
+   struct i915_hw_ppgtt *ppgtt =
+   i915_ppgtt_create(to_i915(dev), file_priv);

if (IS_ERR_OR_NULL(ppgtt)) {
DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n",
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index da150c27a76c..130ccefb2491 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2112,11 +2112,12 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
return 0;
  }

-static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
+static int __hw_ppgtt_init(struct i915_hw_ppgtt *ppgtt,
+  struct drm_i915_private *dev_priv)
  {
-   ppgtt->base.dev = dev;
+   ppgtt->base.dev = dev_priv->dev;

-   if (INTEL_INFO(dev)->gen < 8)
+   if (INTEL_INFO(dev_priv)->gen < 8)
return gen6_ppgtt_init(ppgtt);
else
return gen8_ppgtt_init(ppgtt);
@@ -2132,15 +2133,17 @@ static void i915_address_space_init(struct 
i915_address_space *vm,
list_add_tail(>global_link, _priv->vm_list);
  }

-int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
+int i915_ppgtt_init(struct i915_hw_ppgtt *ppgtt,
+   struct drm_i915_private *dev_priv,
+   struct drm_i915_file_private *file_priv)
  {
-   struct drm_i915_private *dev_priv = dev->dev_private;
int ret = 0;


Since you are using the opportunity to tidy whitespace maybe also tidy 
this needless init. ;)




-   ret = __hw_ppgtt_init(dev, ppgtt);
+   ret = __hw_ppgtt_init(ppgtt, dev_priv);
if (ret == 0) {
kref_init(>ref);
i915_address_space_init(>base, dev_priv);
+   ppgtt->base.file = file_priv;


I would keep using file_priv since that's what's it's called all over 
the place but whatever.



}

return ret;
@@ -2183,7 +2186,8 @@ int i915_ppgtt_init_ring(struct drm_i915_gem_request *req)
  }

  struct i915_hw_ppgtt *
-i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv)
+i915_ppgtt_create(struct drm_i915_private *dev_priv,
+ struct drm_i915_file_private *fpriv)
  {
struct i915_hw_ppgtt *ppgtt;
int ret;
@@ -2192,14 +2196,12 @@ i915_ppgtt_create(struct drm_device *dev, struct 
drm_i915_file_private *fpriv)
if (!ppgtt)
return ERR_PTR(-ENOMEM);

-   ret = i915_ppgtt_init(dev, ppgtt);
+   ret = i915_ppgtt_init(ppgtt, dev_priv, fpriv);

[Intel-gfx] [PATCH 06/11] drm/i915: Store owning file on the i915_address_space

2015-12-14 Thread Chris Wilson 
For the global GTT (and aliasing GTT), the address space is owned by the
device (it is a global resource) and so the per-file owner field is
NULL. For per-process GTT (where we create an address space per
context), each is owned by the opening file. We can use this ownership
information to both distinguish GGTT and ppGTT address spaces, as well
as occasionally inspect the owner.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
 drivers/gpu/drm/i915/i915_drv.h |  1 -
 drivers/gpu/drm/i915/i915_gem_context.c |  3 ++-
 drivers/gpu/drm/i915/i915_gem_gtt.c | 25 +
 drivers/gpu/drm/i915/i915_gem_gtt.h | 13 ++---
 5 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 9ec133f5af00..179e3c5c5022 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -352,7 +352,7 @@ static int per_file_stats(int id, void *ptr, void *data)
= container_of(vma->vm,
   struct i915_hw_ppgtt,
   base);
-   if (ppgtt->file_priv != stats->file_priv)
+   if (ppgtt->base.file != stats->file_priv)
continue;
}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6ce163a681f2..b32a00f60e98 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2924,7 +2924,6 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
return container_of(vm, struct i915_hw_ppgtt, base);
 }
 
-
 static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
 {
return i915_gem_obj_ggtt_bound_view(obj, _ggtt_view_normal);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 25a8498f0b5a..dcb4603a7f03 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -296,7 +296,8 @@ i915_gem_create_context(struct drm_device *dev,
}
 
if (USES_FULL_PPGTT(dev)) {
-   struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv);
+   struct i915_hw_ppgtt *ppgtt =
+   i915_ppgtt_create(to_i915(dev), file_priv);
 
if (IS_ERR_OR_NULL(ppgtt)) {
DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n",
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index da150c27a76c..130ccefb2491 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2112,11 +2112,12 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
return 0;
 }
 
-static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
+static int __hw_ppgtt_init(struct i915_hw_ppgtt *ppgtt,
+  struct drm_i915_private *dev_priv)
 {
-   ppgtt->base.dev = dev;
+   ppgtt->base.dev = dev_priv->dev;
 
-   if (INTEL_INFO(dev)->gen < 8)
+   if (INTEL_INFO(dev_priv)->gen < 8)
return gen6_ppgtt_init(ppgtt);
else
return gen8_ppgtt_init(ppgtt);
@@ -2132,15 +2133,17 @@ static void i915_address_space_init(struct 
i915_address_space *vm,
list_add_tail(>global_link, _priv->vm_list);
 }
 
-int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
+int i915_ppgtt_init(struct i915_hw_ppgtt *ppgtt,
+   struct drm_i915_private *dev_priv,
+   struct drm_i915_file_private *file_priv)
 {
-   struct drm_i915_private *dev_priv = dev->dev_private;
int ret = 0;
 
-   ret = __hw_ppgtt_init(dev, ppgtt);
+   ret = __hw_ppgtt_init(ppgtt, dev_priv);
if (ret == 0) {
kref_init(>ref);
i915_address_space_init(>base, dev_priv);
+   ppgtt->base.file = file_priv;
}
 
return ret;
@@ -2183,7 +2186,8 @@ int i915_ppgtt_init_ring(struct drm_i915_gem_request *req)
 }
 
 struct i915_hw_ppgtt *
-i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv)
+i915_ppgtt_create(struct drm_i915_private *dev_priv,
+ struct drm_i915_file_private *fpriv)
 {
struct i915_hw_ppgtt *ppgtt;
int ret;
@@ -2192,14 +2196,12 @@ i915_ppgtt_create(struct drm_device *dev, struct 
drm_i915_file_private *fpriv)
if (!ppgtt)
return ERR_PTR(-ENOMEM);
 
-   ret = i915_ppgtt_init(dev, ppgtt);
+   ret = i915_ppgtt_init(ppgtt, dev_priv, fpriv);
if (ret) {
kfree(ppgtt);
return ERR_PTR(ret);
}
 
-   ppgtt->file_priv = fpriv;
-
trace_i915_ppgtt_create(>base);
 
return ppgtt;
@@ -2717,7 +2719,7 @@ static int