Re: [PATCH 5/5 v3] drm/i915: debugfs spring cleaning

From: David Weinehall <david.weinehall@linux.intel.com>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 5/5 v3] drm/i915: debugfs spring cleaning
Date: Fri, 12 Aug 2016 15:55:37 +0300	[thread overview]
Message-ID: <20160812125537.5j2h72rw5ips7tjg@boom> (raw)
In-Reply-To: <48df1721-df97-9e58-43f1-a3327e4c5c87@intel.com>

On Fri, Aug 12, 2016 at 01:43:52PM +0100, Dave Gordon wrote:
> On 12/08/16 12:20, David Weinehall wrote:
> > drm/i915: debugfs spring cleaning
> > 
> > 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.
> > 
> > v2: Don't introduce extra dev (Chris)
> > 
> > v3: Make pipe_crc_info have a pointer to drm_i915_private instead of
> >     drm_device. This saves a bit of space, since we never use
> >     drm_device anywhere in these functions.
> > 
> >     Also some minor fixup that I missed in the previous version.
> > 
> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 749 ++++++++++++++++--------------------
> >  drivers/gpu/drm/i915/i915_drv.c     |   2 +-
> >  drivers/gpu/drm/i915/i915_drv.h     |   8 +-
> >  3 files changed, 342 insertions(+), 417 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 3eee5d1255f6..07bceac64e6d 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);
> > +}
> 
> Alternatively (noting that almost the only use we make of this drm_info_node
> is to indirect multiple times to get dev_priv), we could change what is
> stored in (struct seq_file).private to make it more convenient and/or
> efficient. For example,
> 
> struct i915_debugfs_node {
> 	struct drm_i915_private *dev_priv;
> 	struct drm_info_node drm_info;	// if still required
> };
> 
> thus eliminating several memory cycles per use for a cost of one word extra
> data per debugfs node.

Well, the memory cycles / use isn't really that important for debugfs
(or sysfs, for that matter). Memory consumption, however, always
affects us. Still, I admit that having the nodes provide dev_priv
directly would definitely be nice. I'll give your suggestion a whirl and
see how big the overhead would be.

Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx