RE: [igt-dev] [PATCH i-g-t 2/6] i915/i915_pm_rpm.c: create PC state subtest group

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3638 bytes --]



From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Sent: Friday, January 22, 2021 1:26 AM
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; igt-dev@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t 2/6] i915/i915_pm_rpm.c: create PC state subtest group



On Thu, Dec 10, 2020 at 2:42 AM Chris Wilson <chris@chris-wilson.co.uk<mailto:chris@chris-wilson.co.uk>> wrote:
Quoting Anshuman Gupta (2020-12-10 05:02:48)
> On 2020-12-09 at 16:47:01 +0000, Chris Wilson wrote:
> > Quoting Anshuman Gupta (2020-12-09 16:25:02)
> > > On 2020-12-09 at 16:25:55 +0000, Chris Wilson wrote:
> > > > Quoting Anshuman Gupta (2020-12-09 16:06:38)
> > > > > Create a separate igt test group and move package C
> > > > > state in to this subgroup.
> > > > > Run powertop --auto-tune to tune SOC power configuration
> > > > > for package C state tests.
> > > > >
> > > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com<mailto:anshuman.gupta@intel.com>>
> > > > > ---
> > > > >  tests/i915/i915_pm_rpm.c | 35 +++++++++++++++++++++++++++++++----
> > > > >  1 file changed, 31 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
> > > > > index af55b569..42bc44d9 100644
> > > > > --- a/tests/i915/i915_pm_rpm.c
> > > > > +++ b/tests/i915/i915_pm_rpm.c
> > > > > @@ -832,6 +832,25 @@ static void basic_subtest(void)
> > > > >         /* XXX Also we can test wake up via exec nop */
> > > > >  }
> > > > >
> > > > > +static bool setup_powertop(void)
> > > > > +{
> > > > > +       FILE *fp;
> > > > > +       char tmp[512];
> > > > > +
> > > > > +       fp = popen("powertop --auto-tune", "r");
> > > >
> > > > Doesn't this defeat the point of having it work out of the box?
> May be misunderstood your comment, is it PC state or powertop should work
> out of box ?

Powermanagement should not require the user to configure it before it
can work.

powertop in particular may tweak the gfx, which is verboten.

Manually perform any configuration you think is required, and warn if the
initial configuration does not support reaching the lowest pc state the
system can. File bugs.

I understand why Anshuman did this. There are many many components
out there that are blocking the PC10 and this powertop --auto-tune
is trying to take care of all the corner cases to get the "best" of all the
components so we can try to garantee that we are not getting blocked by other components.

However this is very very dangerous. It toggles a lot of unsafe and untested cases.

I have lost a laptop right after a powertop --auto-tune and have never used that
in my life again. Coincidence? Maybe... but I don't want to take the risk in my
machine and I definitely cannot recommend its use.
Thanks Rodrigo for comment.
Actually as part of manual validation powertop being used by different PnP Val team.
Without using powertop surely we are not going to hit PC8/PC10 in that case I am afraid that
these test can’t be part of automatic validation in CI.
We may need to find alternative in some sort of OS level Power Management Policy decision maker,
which can decide conservative power saving oriented policy.
Runtime idle  with display on which leads to PC10 and s0ix effectively for client products,
Ubuntu OS looks like not really tuned for that.

Thanks,
Anshuman Gupta.

-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org<mailto:igt-dev@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[-- Attachment #2: Type: text/html, Size: 8326 bytes --]