Re: [Intel-gfx] [PATCH i-g-t] RFC: split PM workarounds into separate lib
On Thu, Dec 10, 2015 at 06:01:28PM +0200, David Weinehall wrote: > On Tue, Dec 08, 2015 at 03:42:27PM +0200, Ville Syrjälä wrote: > > On Tue, Dec 08, 2015 at 10:50:39AM +0200, David Weinehall wrote: > > > Since the defaults for some external power management related settings > > > prevents us from testing our power management functionality properly, > > > we have to work around it. Currently this is done from the individual > > > test cases, but this is sub-optimal. This patch moves the PM-related > > > workarounds into a separate library, and adds some code to restore the > > > previous settings for the SATA link power management while at it. > > > > Why is it called "workarounds"? That gives me the impression we're > > working around something that's supposed to work but doesn't. That's not > > the case here. > > Workarounds was because we are working around "imperfect" settings > in other components. At least to me power management should be enabled > out of the box, not something that requires admin-level workarounds. > Since we're not in control of said defaults, we have to modify the > settings when we run our tests, hence workarounds. Fully agreed that power tuning should be applied by default, but that's a loong process to convince all the other kernel maintainers. And we need to get our own house in order first too, but that's in progress. > That said, as I've replied to a later post, igt_pm is fine by me. One more: Please namespace all the library functions you're adding and exporting to tests with igt_pm_. Static/internal functions can still be named however you feel like. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] RFC: split PM workarounds into separate lib
On Tue, Dec 08, 2015 at 01:22:14PM +, Zanoni, Paulo R wrote: > Em Ter, 2015-12-08 às 10:50 +0200, David Weinehall escreveu: > > Since the defaults for some external power management related > > settings > > prevents us from testing our power management functionality properly, > > we have to work around it. Currently this is done from the individual > > test cases, but this is sub-optimal. This patch moves the PM-related > > workarounds into a separate library, and adds some code to restore > > the > > previous settings for the SATA link power management while at it. > > > > This patch should be seen as a RFC; there might be other workarounds > > for external issues that should be moved into the library, and if > > those > > workarounds aren't related to power management it might be better to > > choose a different name for the library. > > I didn't deeply look the implementation, but you have my Acked-by on > the idea. > > You may also consider adding a function to just run "sudo powertop -- > auto-tune" in addition to the other things, but you can't undo this > later. > > But in the end, it all depends on your machine. A bad machine will > never reach the deepest expected PC states. That's the problem when > automating things... > > Since you're interested in PM, you may also want to look at: > http://patchwork.freedesktop.org/patch/66392/ > maybe there's some code there that you may want to take. Thanks! At the very least it seems like a very useful tool that I can use as an independent test to verify my own tests against. :) Kind regards, David ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] RFC: split PM workarounds into separate lib
On Tue, Dec 08, 2015 at 03:42:27PM +0200, Ville Syrjälä wrote: > On Tue, Dec 08, 2015 at 10:50:39AM +0200, David Weinehall wrote: > > Since the defaults for some external power management related settings > > prevents us from testing our power management functionality properly, > > we have to work around it. Currently this is done from the individual > > test cases, but this is sub-optimal. This patch moves the PM-related > > workarounds into a separate library, and adds some code to restore the > > previous settings for the SATA link power management while at it. > > Why is it called "workarounds"? That gives me the impression we're > working around something that's supposed to work but doesn't. That's not > the case here. Workarounds was because we are working around "imperfect" settings in other components. At least to me power management should be enabled out of the box, not something that requires admin-level workarounds. Since we're not in control of said defaults, we have to modify the settings when we run our tests, hence workarounds. That said, as I've replied to a later post, igt_pm is fine by me. Kind regards, David ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] RFC: split PM workarounds into separate lib
On Thu, Dec 10, 2015 at 11:09:42AM +0100, Daniel Vetter wrote: > On Tue, Dec 08, 2015 at 05:05:12PM -0200, Paulo Zanoni wrote: > > 2015-12-08 11:42 GMT-02:00 Ville Syrjälä : > > > On Tue, Dec 08, 2015 at 10:50:39AM +0200, David Weinehall wrote: > > >> Since the defaults for some external power management related settings > > >> prevents us from testing our power management functionality properly, > > >> we have to work around it. Currently this is done from the individual > > >> test cases, but this is sub-optimal. This patch moves the PM-related > > >> workarounds into a separate library, and adds some code to restore the > > >> previous settings for the SATA link power management while at it. > > > > > > Why is it called "workarounds"? That gives me the impression we're > > > working around something that's supposed to work but doesn't. That's not > > > the case here. > > > > Well, in theory they could be considered workarounds since IMHO the > > machine is supposed to be saving as much power as it can on an idle > > state, but it isn't. But this more of a philosophical discussion and > > we can debate forever. > > > > Anyway, if we rename the file to something like lib/igt_pm.c we'll be > > able to move the residency-checking code and possibly more common > > things there, so I'm not against the rename either. > > I like the ring of lib/igt_pm.c. No reservations from me, igt_pm it is. Kind regards, David ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] RFC: split PM workarounds into separate lib
On Tue, Dec 08, 2015 at 05:05:12PM -0200, Paulo Zanoni wrote: > 2015-12-08 11:42 GMT-02:00 Ville Syrjälä : > > On Tue, Dec 08, 2015 at 10:50:39AM +0200, David Weinehall wrote: > >> Since the defaults for some external power management related settings > >> prevents us from testing our power management functionality properly, > >> we have to work around it. Currently this is done from the individual > >> test cases, but this is sub-optimal. This patch moves the PM-related > >> workarounds into a separate library, and adds some code to restore the > >> previous settings for the SATA link power management while at it. > > > > Why is it called "workarounds"? That gives me the impression we're > > working around something that's supposed to work but doesn't. That's not > > the case here. > > Well, in theory they could be considered workarounds since IMHO the > machine is supposed to be saving as much power as it can on an idle > state, but it isn't. But this more of a philosophical discussion and > we can debate forever. > > Anyway, if we rename the file to something like lib/igt_pm.c we'll be > able to move the residency-checking code and possibly more common > things there, so I'm not against the rename either. I like the ring of lib/igt_pm.c. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] RFC: split PM workarounds into separate lib
2015-12-08 11:42 GMT-02:00 Ville Syrjälä : > On Tue, Dec 08, 2015 at 10:50:39AM +0200, David Weinehall wrote: >> Since the defaults for some external power management related settings >> prevents us from testing our power management functionality properly, >> we have to work around it. Currently this is done from the individual >> test cases, but this is sub-optimal. This patch moves the PM-related >> workarounds into a separate library, and adds some code to restore the >> previous settings for the SATA link power management while at it. > > Why is it called "workarounds"? That gives me the impression we're > working around something that's supposed to work but doesn't. That's not > the case here. Well, in theory they could be considered workarounds since IMHO the machine is supposed to be saving as much power as it can on an idle state, but it isn't. But this more of a philosophical discussion and we can debate forever. Anyway, if we rename the file to something like lib/igt_pm.c we'll be able to move the residency-checking code and possibly more common things there, so I'm not against the rename either. > >> >> This patch should be seen as a RFC; there might be other workarounds >> for external issues that should be moved into the library, and if those >> workarounds aren't related to power management it might be better to >> choose a different name for the library. >> >> David Weinehall (1): >> lib/pm_workarounds: Lib for PM workarounds >> >> lib/Makefile.sources | 2 + >> lib/igt.h| 1 + >> lib/igt_aux.c| 15 +--- >> lib/pm_workarounds.c | 233 >> +++ >> lib/pm_workarounds.h | 31 +++ >> tests/pm_lpsp.c | 25 +- >> tests/pm_rpm.c | 29 ++- >> 7 files changed, 279 insertions(+), 57 deletions(-) >> create mode 100644 lib/pm_workarounds.c >> create mode 100644 lib/pm_workarounds.h >> >> -- >> 2.6.2 >> >> ___ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] RFC: split PM workarounds into separate lib
On Tue, Dec 08, 2015 at 10:50:39AM +0200, David Weinehall wrote: > Since the defaults for some external power management related settings > prevents us from testing our power management functionality properly, > we have to work around it. Currently this is done from the individual > test cases, but this is sub-optimal. This patch moves the PM-related > workarounds into a separate library, and adds some code to restore the > previous settings for the SATA link power management while at it. Why is it called "workarounds"? That gives me the impression we're working around something that's supposed to work but doesn't. That's not the case here. > > This patch should be seen as a RFC; there might be other workarounds > for external issues that should be moved into the library, and if those > workarounds aren't related to power management it might be better to > choose a different name for the library. > > David Weinehall (1): > lib/pm_workarounds: Lib for PM workarounds > > lib/Makefile.sources | 2 + > lib/igt.h| 1 + > lib/igt_aux.c| 15 +--- > lib/pm_workarounds.c | 233 > +++ > lib/pm_workarounds.h | 31 +++ > tests/pm_lpsp.c | 25 +- > tests/pm_rpm.c | 29 ++- > 7 files changed, 279 insertions(+), 57 deletions(-) > create mode 100644 lib/pm_workarounds.c > create mode 100644 lib/pm_workarounds.h > > -- > 2.6.2 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] RFC: split PM workarounds into separate lib
Em Ter, 2015-12-08 às 10:50 +0200, David Weinehall escreveu: > Since the defaults for some external power management related > settings > prevents us from testing our power management functionality properly, > we have to work around it. Currently this is done from the individual > test cases, but this is sub-optimal. This patch moves the PM-related > workarounds into a separate library, and adds some code to restore > the > previous settings for the SATA link power management while at it. > > This patch should be seen as a RFC; there might be other workarounds > for external issues that should be moved into the library, and if > those > workarounds aren't related to power management it might be better to > choose a different name for the library. I didn't deeply look the implementation, but you have my Acked-by on the idea. You may also consider adding a function to just run "sudo powertop -- auto-tune" in addition to the other things, but you can't undo this later. But in the end, it all depends on your machine. A bad machine will never reach the deepest expected PC states. That's the problem when automating things... Since you're interested in PM, you may also want to look at: http://patchwork.freedesktop.org/patch/66392/ maybe there's some code there that you may want to take. > > David Weinehall (1): > lib/pm_workarounds: Lib for PM workarounds > > lib/Makefile.sources | 2 + > lib/igt.h| 1 + > lib/igt_aux.c| 15 +--- > lib/pm_workarounds.c | 233 > +++ > lib/pm_workarounds.h | 31 +++ > tests/pm_lpsp.c | 25 +- > tests/pm_rpm.c | 29 ++- > 7 files changed, 279 insertions(+), 57 deletions(-) > create mode 100644 lib/pm_workarounds.c > create mode 100644 lib/pm_workarounds.h > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t] RFC: split PM workarounds into separate lib
Since the defaults for some external power management related settings prevents us from testing our power management functionality properly, we have to work around it. Currently this is done from the individual test cases, but this is sub-optimal. This patch moves the PM-related workarounds into a separate library, and adds some code to restore the previous settings for the SATA link power management while at it. This patch should be seen as a RFC; there might be other workarounds for external issues that should be moved into the library, and if those workarounds aren't related to power management it might be better to choose a different name for the library. David Weinehall (1): lib/pm_workarounds: Lib for PM workarounds lib/Makefile.sources | 2 + lib/igt.h| 1 + lib/igt_aux.c| 15 +--- lib/pm_workarounds.c | 233 +++ lib/pm_workarounds.h | 31 +++ tests/pm_lpsp.c | 25 +- tests/pm_rpm.c | 29 ++- 7 files changed, 279 insertions(+), 57 deletions(-) create mode 100644 lib/pm_workarounds.c create mode 100644 lib/pm_workarounds.h -- 2.6.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
