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@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
>
> Hello,
>
> The flush_delayed_work() calls were added in:
> "commit 5c9669cee534cbb834d51aae115267f5e561b622
> Author: Tom O'Rourke <Tom.O'Rourke@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
