Re: [PATCH 4/3] drm/i915: Read RPS frequencies earlier on non-VLV/CHV

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "O'Rourke, Tom" <Tom.O'Rourke@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 4/3] drm/i915: Read RPS frequencies earlier on non-VLV/CHV
Date: Tue, 8 Mar 2016 15:34:09 +0200	[thread overview]
Message-ID: <20160308133409.GK10446@intel.com> (raw)
In-Reply-To: <20160308004914.GA236841@torourke-desk1>

On Mon, Mar 07, 2016 at 04:49:14PM -0800, O'Rourke, Tom wrote:
> On Mon, Mar 07, 2016 at 08:57:09PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Read out the RPS frequencies already in intel_init_gt_powersave()
> > on all the platforms. So far we only did that on VLV/CHV, and the
> > rest of the platforms read them out at rps enable time, which happens
> > asynchronously from a workqueue. Reading them out earlier prevents
> > userspace from reading out invalid (zero) values via the relevant
> > sysfs files before the rps enable work has been executed.
> > 
> > This used to be prevented by the flush_delayed_work() + locking
> > in the sysfs code, but now that we no longer do that, we run the
> > risk of letting userspace see the initial zeroed values.
> > 
> > Note that it's still possible to read out cur_freq as 0, since that
> > only gets initialized from the delayed rps enable. Should that pose
> > a real problem, I guess we could always add the flush+locking back
> > for the cur_freq read.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> 
> Hello,
> 
> The flush_delayed_work() calls were added in:
> "commit 5c9669cee534cbb834d51aae115267f5e561b622
> Author: Tom O'Rourke <Tom.O'Rourke@intel.com>
> Date:   Mon Sep 16 14:56:43 2013 -0700
> 
>     drm/i915: Finish enabling rps before use by sysfs or debugfs"
> 
> This change was made to address use before initialization
> problems in min/max/cur frequency values.  The work queue
> being flushed was added in:
> "commit 1a01ab3b2dc4394c46c4c3230805748f632f6f74
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Fri Nov 2 11:14:00 2012 -0700
> 
>     drm/i915: put ring frequency and turbo setup into a work queue v5
> 
>     Communicating via the mailbox registers with the PCU can take quite
>     awhile.  And updating the ring frequency or enabling turbo is not
>     something that needs to happen synchronously, so take it out of our init
>     and resume paths to speed things up (~200ms on my T420)."
> 
> This change was made to reduce driver load times.
> 
> The two concerns I have are:
> 1) Similar changes would be needed in debugfs files.
> 2) This change may increase driver load times due to pcode
> mailbox command used in gen6_rps_init_frequencies() that
> is now in the init path.
> 
> I am not objecting to these changes but we should
> be aware of the impact to driver load latency.

If a single pcode command takes singificant amount of time there must
be something seriously wrong. TBH I never really bought the whole two
stage rps enable thing. Seemed like extra complexity for little gain.
Someone should definitely measure it again though, especially after
commit 9848de082ff4 ("drm/i915: Use usleep_range() in wait_for()")

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx