| Message ID | 20200220174155.2162242-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] i915/i915_pm_rpm: Only check for suspend failures after each debugfs entry | expand |
On 2020-02-20 19:41, Chris Wilson wrote: > Since we check before and then after each debugfs entry, we do not need > to check before each time as well. We will error out as soon as it does > fail, at all other times we know the system to be idle. > > No impact on runtime for glk (which apparently is one of the better > behaving systems). > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Martin Peres <martin.peres@linux.intel.com> I don't like this patch because the first read might not have the gpu suspended, and there shouldn't be much overhead in checking twice rather than once. What's your rationale here? To me, the issue is that some platforms suspend in milliseconds while some take seconds, and that might be indicative a real bug in the driver. Martin > --- > tests/i915/i915_pm_rpm.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c > index 0c2821122..bf412b5cc 100644 > --- a/tests/i915/i915_pm_rpm.c > +++ b/tests/i915/i915_pm_rpm.c > @@ -932,9 +932,6 @@ static int read_entry(const char *filepath, > int fd; > int rc; > > - igt_assert_f(wait_for_suspended(), "Before opening: %s (%s)\n", > - filepath + pathinfo->base, filepath); > - > fd = open(filepath, O_RDONLY | O_NONBLOCK); > if (fd < 0) { > igt_debug("Failed to open '%s': %m\n", filepath); >
Quoting Martin Peres (2020-02-21 07:33:59) > On 2020-02-20 19:41, Chris Wilson wrote: > > Since we check before and then after each debugfs entry, we do not need > > to check before each time as well. We will error out as soon as it does > > fail, at all other times we know the system to be idle. > > > > No impact on runtime for glk (which apparently is one of the better > > behaving systems). > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Martin Peres <martin.peres@linux.intel.com> > > I don't like this patch because the first read might not have the gpu > suspended, and there shouldn't be much overhead in checking twice rather > than once. > > What's your rationale here? We always do a check before after each file. We start in a known state, and expect to be able to return to that suspended state, and the _real_ guts of the test is that any device access is accounted for. assert(suspended) would be a better check for non-interference. > To me, the issue is that some platforms suspend in milliseconds while > some take seconds, and that might be indicative a real bug in the driver. Exactly. -Chris
On 2020-02-21 10:21, Chris Wilson wrote: > Quoting Martin Peres (2020-02-21 07:33:59) >> On 2020-02-20 19:41, Chris Wilson wrote: >>> Since we check before and then after each debugfs entry, we do not need >>> to check before each time as well. We will error out as soon as it does >>> fail, at all other times we know the system to be idle. >>> >>> No impact on runtime for glk (which apparently is one of the better >>> behaving systems). >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Martin Peres <martin.peres@linux.intel.com> >> >> I don't like this patch because the first read might not have the gpu >> suspended, and there shouldn't be much overhead in checking twice rather >> than once. >> >> What's your rationale here? > > We always do a check before after each file. We start in a known state, > and expect to be able to return to that suspended state, and the _real_ > guts of the test is that any device access is accounted for. > > assert(suspended) would be a better check for non-interference. I would feel better with assert(suspended) added, but would it really speed anything up since I assume wait_for_suspended() should be instantaneous if we are already suspended, right? > >> To me, the issue is that some platforms suspend in milliseconds while >> some take seconds, and that might be indicative a real bug in the driver. > > Exactly. Good to hear :) > -Chris >
Quoting Martin Peres (2020-02-21 08:28:16) > On 2020-02-21 10:21, Chris Wilson wrote: > > Quoting Martin Peres (2020-02-21 07:33:59) > >> On 2020-02-20 19:41, Chris Wilson wrote: > >>> Since we check before and then after each debugfs entry, we do not need > >>> to check before each time as well. We will error out as soon as it does > >>> fail, at all other times we know the system to be idle. > >>> > >>> No impact on runtime for glk (which apparently is one of the better > >>> behaving systems). > >>> > >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >>> Cc: Martin Peres <martin.peres@linux.intel.com> > >> > >> I don't like this patch because the first read might not have the gpu > >> suspended, and there shouldn't be much overhead in checking twice rather > >> than once. > >> > >> What's your rationale here? > > > > We always do a check before after each file. We start in a known state, > > and expect to be able to return to that suspended state, and the _real_ > > guts of the test is that any device access is accounted for. > > > > assert(suspended) would be a better check for non-interference. > > I would feel better with assert(suspended) added, but would it really > speed anything up since I assume wait_for_suspended() should be > instantaneous if we are already suspended, right? No, only if there was some random bug, we would now get a failure rather than papering it over. -Chris
diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c index 0c2821122..bf412b5cc 100644 --- a/tests/i915/i915_pm_rpm.c +++ b/tests/i915/i915_pm_rpm.c @@ -932,9 +932,6 @@ static int read_entry(const char *filepath, int fd; int rc; - igt_assert_f(wait_for_suspended(), "Before opening: %s (%s)\n", - filepath + pathinfo->base, filepath); - fd = open(filepath, O_RDONLY | O_NONBLOCK); if (fd < 0) { igt_debug("Failed to open '%s': %m\n", filepath);
Since we check before and then after each debugfs entry, we do not need to check before each time as well. We will error out as soon as it does fail, at all other times we know the system to be idle. No impact on runtime for glk (which apparently is one of the better behaving systems). Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Martin Peres <martin.peres@linux.intel.com> --- tests/i915/i915_pm_rpm.c | 3 --- 1 file changed, 3 deletions(-)