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

[O'Rourke, Tom]

On Mon, Mar 07, 2016 at 08:57:09PM +0200, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at 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 at linux.intel.com>
> ---

Hello,

The flush_delayed_work() calls were added in:
"commit 5c9669cee534cbb834d51aae115267f5e561b622
Author: Tom O'Rourke <Tom.O'Rourke at 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 at 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.

Thanks,
Tom