[Intel-gfx] [PATCH] drm/i915: Finish enabling rps before use by sysfs or debugfs

[Daniel Vetter]

On Tue, Sep 17, 2013 at 06:05:27PM +0200, Daniel Vetter wrote:
> On Tue, Sep 17, 2013 at 12:12 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Mon, Sep 16, 2013 at 02:56:43PM -0700, Tom.O'Rourke at intel.com wrote:
> >> From: Tom O'Rourke <Tom.O'Rourke at intel.com>
> >>
> >> Enabling rps (turbo setup) was put in a work queue because it may
> >> take quite awhile.  This change flushes the work queue to initialize
> >> rps values before use by sysfs or debugfs.  Specifically,
> >> rps.delayed_resume_work is flushed before using rps.hw_max,
> >> rps.max_delay, rps.min_delay, or rps.cur_delay.
> >>
> >> This change fixes a problem in sysfs where show functions using
> >> uninitialized values show incorrect values and store functions
> >> using uninitialized values in range checks incorrectly fail to
> >> store valid input values.  This change also addresses similar use
> >> before initialized problems in debugfs.
> >>
> >> Change-Id: Ib9c4f2066b65013094cb9278fc17958a964836e7
> >> Signed-off-by: Tom O'Rourke <Tom.O'Rourke at intel.com>
> >
> > I think we can exercise this by going through a suspend/resume cycle and
> > racing concurrent reads/writes of the sysfs files in a 2nd process. With
> > the igt_fork and igt_system_suspend_autoresume helpers in our kernel
> > testsuite repro this should be a really quick testcase to write ...
> >
> > I'd go for a generic "read all sysfs files" approach to increase the
> > chances of catching other similar fallout in the future.
> 
> To clarify: I'd like to see a testcase for this so that we can make
> sure it keeps working. I guess we unfortunately can't test for
> functional correctness by just resuming since we won't see
> uninitialized data. But at least we can test for the
> flush_delayed_work deadlock implications, which have bitten us badly
> in the past on numerous occasions. And maybe we can provoke hangs if
> we adjust the rps values while the setup hasn't completed yet, so
> might also give us weak coverage there.

I've stitched something together with little enthusiasm ...

*insert maintainer rant*

Patch merged.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch