Re: [PATCH 09/16] drm/i915: Limit ring synchronisation (sw sempahores) RPS boosts

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 09/16] drm/i915: Limit ring synchronisation (sw sempahores) RPS boosts
Date: Mon, 4 May 2015 16:46:56 +0200	[thread overview]
Message-ID: <20150504144656.GO30184@phenom.ffwll.local> (raw)
In-Reply-To: <20150504143802.GN30184@phenom.ffwll.local>

On Mon, May 04, 2015 at 04:38:02PM +0200, Daniel Vetter wrote:
> On Mon, Apr 27, 2015 at 01:41:20PM +0100, Chris Wilson wrote:
> > Ring switches can occur many times per frame, and are often out of
> > control, causing frequent RPS boosting for no practical benefit. Treat
> > the sw semaphore synchronisation as a separate client and only allow it
> > to boost once per busy/idle cycle.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c |  1 +
> >  drivers/gpu/drm/i915/i915_drv.h     | 34 ++++++++++++++++++----------------
> >  drivers/gpu/drm/i915/i915_gem.c     |  7 +++++--
> >  drivers/gpu/drm/i915/intel_pm.c     |  1 +
> >  4 files changed, 25 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 2cbb3e9266f0..1d68e3ecaa00 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2311,6 +2311,7 @@ static int i915_rps_boost_info(struct seq_file *m, void *data)
> >  			   list_empty(&file_priv->rps_boost) ? "" : ", active");
> >  		rcu_read_unlock();
> >  	}
> > +	seq_printf(m, "Semaphore boosts: %d\n", dev_priv->rps.semaphores.rps_boosts);
> >  	seq_printf(m, "Kernel boosts: %d\n", dev_priv->rps.boosts);
> >  
> >  	mutex_unlock(&dev_priv->rps.hw_lock);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index caee59bf94ba..415a8e756e48 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -272,6 +272,22 @@ struct drm_i915_private;
> >  struct i915_mm_struct;
> >  struct i915_mmu_object;
> >  
> > +struct drm_i915_file_private {
> > +	struct drm_i915_private *dev_priv;
> > +	struct drm_file *file;
> > +
> > +	struct {
> > +		spinlock_t lock;
> > +		struct list_head request_list;
> > +	} mm;
> > +	struct idr context_idr;
> > +
> > +	struct list_head rps_boost;
> > +	struct intel_engine_cs *bsd_ring;
> > +
> > +	unsigned rps_boosts;
> > +};
> > +
> >  enum intel_dpll_id {
> >  	DPLL_ID_PRIVATE = -1, /* non-shared dpll in use */
> >  	/* real shared dpll ids must be >= 0 */
> > @@ -1054,6 +1070,8 @@ struct intel_gen6_power_mgmt {
> >  	struct list_head clients;
> >  	unsigned boosts;
> >  
> > +	struct drm_i915_file_private semaphores;
> 
> Still not in favour of reusing the entire file_private here. Imo
> extracting the rps_* stuff from that into a separate struct would be
> better and much less confusing.

Ah it's in a follow-up so I'm all happy, except that all the patches kinda
depend on each another and not all of the seemingly required prereqs for
are reviewed yet. So unfortunately I need to pass on these for now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx