Re: [Intel-gfx] [PATCH 5/5] drm/i915: debugfs spring cleaning

2016-08-09 Thread David Weinehall 
On Mon, Aug 08, 2016 at 04:28:56PM +0100, Chris Wilson wrote:
> On Mon, Aug 08, 2016 at 04:20:01PM +0300, David Weinehall wrote:
> > @@ -136,13 +140,14 @@ static void
> >  describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
> >  {
> > struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> > +   struct drm_device *dev = _priv->drm;
> > struct intel_engine_cs *engine;
> > struct i915_vma *vma;
> > unsigned int frontbuffer_bits;
> > int pin_count = 0;
> > enum intel_engine_id id;
> >  
> > -   lockdep_assert_held(>base.dev->struct_mutex);
> > +   lockdep_assert_held(>struct_mutex);
> 
> This is not a good tradeoff however. lockdep_assert_held() is
> conditional code that should be compiled out,
> 
> >  
> > seq_printf(m, "%pK: %c%c%c%c%c %8zdKiB %02x %02x [ ",
> >>base,
> > @@ -157,13 +162,13 @@ describe_obj(struct seq_file *m, struct 
> > drm_i915_gem_object *obj)
> > for_each_engine_id(engine, dev_priv, id)
> > seq_printf(m, "%x ",
> >i915_gem_active_get_seqno(>last_read[id],
> > -
> > >base.dev->struct_mutex));
> > +>struct_mutex));
> 
> Same again here.
> 
> > seq_printf(m, "] %x %x%s%s%s",
> >i915_gem_active_get_seqno(>last_write,
> > ->base.dev->struct_mutex),
> > +>struct_mutex),
> >i915_gem_active_get_seqno(>last_fence,
> > ->base.dev->struct_mutex),
> > -  i915_cache_level_str(to_i915(obj->base.dev), 
> > obj->cache_level),
> > +>struct_mutex),
> > +  i915_cache_level_str(dev_priv, obj->cache_level),
> >obj->dirty ? " dirty" : "",
> >obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
> > if (obj->base.name)
> > @@ -201,7 +206,7 @@ describe_obj(struct seq_file *m, struct 
> > drm_i915_gem_object *obj)
> > }
> >  
> > engine = i915_gem_active_get_engine(>last_write,
> > -   >base.dev->struct_mutex);
> > +   >struct_mutex);
> 
> and again.
> 
> I'm quite happy with dev_priv->drm and need a strong argument to
> introduce dev = _priv->drm locals. dev_priv->drm should avoid the
> need for the compiler to emit any locals should they go out of scope.

Thanks for the feedback.  Will fix.


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

Re: [Intel-gfx] [PATCH 5/5] drm/i915: debugfs spring cleaning

2016-08-08 Thread Chris Wilson 
On Mon, Aug 08, 2016 at 04:20:01PM +0300, David Weinehall wrote:
> @@ -136,13 +140,14 @@ static void
>  describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>  {
>   struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> + struct drm_device *dev = _priv->drm;
>   struct intel_engine_cs *engine;
>   struct i915_vma *vma;
>   unsigned int frontbuffer_bits;
>   int pin_count = 0;
>   enum intel_engine_id id;
>  
> - lockdep_assert_held(>base.dev->struct_mutex);
> + lockdep_assert_held(>struct_mutex);

This is not a good tradeoff however. lockdep_assert_held() is
conditional code that should be compiled out,

>  
>   seq_printf(m, "%pK: %c%c%c%c%c %8zdKiB %02x %02x [ ",
>  >base,
> @@ -157,13 +162,13 @@ describe_obj(struct seq_file *m, struct 
> drm_i915_gem_object *obj)
>   for_each_engine_id(engine, dev_priv, id)
>   seq_printf(m, "%x ",
>  i915_gem_active_get_seqno(>last_read[id],
> -  
> >base.dev->struct_mutex));
> +  >struct_mutex));

Same again here.

>   seq_printf(m, "] %x %x%s%s%s",
>  i915_gem_active_get_seqno(>last_write,
> -  >base.dev->struct_mutex),
> +  >struct_mutex),
>  i915_gem_active_get_seqno(>last_fence,
> -  >base.dev->struct_mutex),
> -i915_cache_level_str(to_i915(obj->base.dev), 
> obj->cache_level),
> +  >struct_mutex),
> +i915_cache_level_str(dev_priv, obj->cache_level),
>  obj->dirty ? " dirty" : "",
>  obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
>   if (obj->base.name)
> @@ -201,7 +206,7 @@ describe_obj(struct seq_file *m, struct 
> drm_i915_gem_object *obj)
>   }
>  
>   engine = i915_gem_active_get_engine(>last_write,
> - >base.dev->struct_mutex);
> + >struct_mutex);

and again.

I'm quite happy with dev_priv->drm and need a strong argument to
introduce dev = _priv->drm locals. dev_priv->drm should avoid the
need for the compiler to emit any locals should they go out of scope.
-Chris

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

[Intel-gfx] [PATCH 5/5] drm/i915: debugfs spring cleaning

2016-08-08 Thread David Weinehall 
Just like with sysfs, we do some major overhaul.

Pass dev_priv instead of dev to all feature macros (IS_, HAS_,
INTEL_, etc.). This has the side effect that a bunch of functions
now get dev_priv passed instead of dev.

All calls to INTEL_INFO()->gen have been replaced with
INTEL_GEN().

We want access to to_i915(node->minor->dev) in a lot of places,
so add the node_to_i915() helper to accomodate for this.

Finally, we have quite a few cases where we get a void * pointer,
and need to cast it to drm_device *, only to run to_i915() on it.
Add cast_to_i915() to do this.

Signed-off-by: David Weinehall 
---
 drivers/gpu/drm/i915/i915_debugfs.c | 699 
 drivers/gpu/drm/i915/i915_drv.c |   2 +-
 drivers/gpu/drm/i915/i915_drv.h |   8 +-
 3 files changed, 321 insertions(+), 388 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index ad4f7178667c..6d31d35cc89b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -46,6 +46,11 @@ enum {
PINNED_LIST,
 };
 
+static inline struct drm_i915_private *node_to_i915(struct drm_info_node *node)
+{
+   return to_i915(node->minor->dev);
+}
+
 /* As the drm_debugfs_init() routines are called before dev->dev_private is
  * allocated we need to hook into the minor for release. */
 static int
@@ -74,12 +79,11 @@ drm_add_fake_info_node(struct drm_minor *minor,
 
 static int i915_capabilities(struct seq_file *m, void *data)
 {
-   struct drm_info_node *node = m->private;
-   struct drm_device *dev = node->minor->dev;
-   const struct intel_device_info *info = INTEL_INFO(dev);
+   struct drm_i915_private *dev_priv = node_to_i915(m->private);
+   const struct intel_device_info *info = INTEL_INFO(dev_priv);
 
-   seq_printf(m, "gen: %d\n", info->gen);
-   seq_printf(m, "pch: %d\n", INTEL_PCH_TYPE(dev));
+   seq_printf(m, "gen: %d\n", INTEL_GEN(dev_priv));
+   seq_printf(m, "pch: %d\n", INTEL_PCH_TYPE(dev_priv));
 #define PRINT_FLAG(x)  seq_printf(m, #x ": %s\n", yesno(info->x))
 #define SEP_SEMICOLON ;
DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG, SEP_SEMICOLON);
@@ -136,13 +140,14 @@ static void
 describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 {
struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
+   struct drm_device *dev = _priv->drm;
struct intel_engine_cs *engine;
struct i915_vma *vma;
unsigned int frontbuffer_bits;
int pin_count = 0;
enum intel_engine_id id;
 
-   lockdep_assert_held(>base.dev->struct_mutex);
+   lockdep_assert_held(>struct_mutex);
 
seq_printf(m, "%pK: %c%c%c%c%c %8zdKiB %02x %02x [ ",
   >base,
@@ -157,13 +162,13 @@ describe_obj(struct seq_file *m, struct 
drm_i915_gem_object *obj)
for_each_engine_id(engine, dev_priv, id)
seq_printf(m, "%x ",
   i915_gem_active_get_seqno(>last_read[id],
-
>base.dev->struct_mutex));
+>struct_mutex));
seq_printf(m, "] %x %x%s%s%s",
   i915_gem_active_get_seqno(>last_write,
->base.dev->struct_mutex),
+>struct_mutex),
   i915_gem_active_get_seqno(>last_fence,
->base.dev->struct_mutex),
-  i915_cache_level_str(to_i915(obj->base.dev), 
obj->cache_level),
+>struct_mutex),
+  i915_cache_level_str(dev_priv, obj->cache_level),
   obj->dirty ? " dirty" : "",
   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
if (obj->base.name)
@@ -201,7 +206,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object 
*obj)
}
 
engine = i915_gem_active_get_engine(>last_write,
-   >base.dev->struct_mutex);
+   >struct_mutex);
if (engine)
seq_printf(m, " (%s)", engine->name);
 
@@ -215,8 +220,8 @@ static int i915_gem_object_list_info(struct seq_file *m, 
void *data)
struct drm_info_node *node = m->private;
uintptr_t list = (uintptr_t) node->info_ent->data;
struct list_head *head;
-   struct drm_device *dev = node->minor->dev;
-   struct drm_i915_private *dev_priv = to_i915(dev);
+   struct drm_i915_private *dev_priv = node_to_i915(node);
+   struct drm_device *dev = _priv->drm;
struct i915_ggtt *ggtt = _priv->ggtt;
struct i915_vma *vma;
u64 total_obj_size, total_gtt_size;
@@ -274,9 +279,8 @@ static int obj_rank_by_stolen(void *priv,
 
 static int i915_gem_stolen_list_info(struct seq_file *m, void *data)
 {
-