[igt-dev] [PATCH i-g-t] tests/i915/i915_pm_dc: Add new dc9 test
Gupta, Anshuman
anshuman.gupta at intel.com
Mon Sep 5 05:58:35 UTC 2022
> -----Original Message-----
> From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of Sharma,
> Swati2
> Sent: Monday, September 5, 2022 12:20 AM
> To: Deak, Imre <imre.deak at intel.com>
> Cc: igt-dev at lists.freedesktop.org
> Subject: Re: [igt-dev] [PATCH i-g-t] tests/i915/i915_pm_dc: Add new dc9 test
>
> Hi Imre,
>
> Thanks for the review.
> Have answered your review comments in
> https://patchwork.freedesktop.org/series/108079/#rev2
> Please review rev#2
Here we are testing runtime suspend time for DC9.
I have few question regarding the test mythology.
1. Is there any power differences, When all display pipes are off (i.e all connectors are dpms off) vs DC9 ?
We are already having a lot runtime pm test in i915_pm_rpm ? why do we need a test which just check the runtime suspend ?
We are already checking d3hot and d3cold in i915_pm_rpm ?
Thanks,
Anshuman Gupta.
>
> On 24-Aug-22 7:08 PM, Imre Deak wrote:
> > On Tue, Aug 23, 2022 at 09:08:29PM +0530, Swati Sharma wrote:
> >> Added new test to validate dc9 on both igfx and dgfx.
> >> runtime_suspended_time value increases when system enters DC9.
> >> Existing test will be restricted to igfx only.
> >> Also, changed debugfs entry to check if platform is dgfx or igfx.
> >>
> >> Signed-off-by: Swati Sharma <swati2.sharma at intel.com>
> >> ---
> >> lib/igt_pm.c | 25 ++++++++++++++++++++++---
> >> lib/igt_pm.h | 1 +
> >> tests/i915/i915_pm_dc.c | 39 ++++++++++++++++++++++++++++++++++-----
> >> 3 files changed, 57 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/lib/igt_pm.c b/lib/igt_pm.c index 6ebbad330..6c99eaff9
> >> 100644
> >> --- a/lib/igt_pm.c
> >> +++ b/lib/igt_pm.c
> >> @@ -947,19 +947,23 @@ static int igt_pm_get_power_attr_fd(struct
> pci_device *pci_dev, const char *attr
> >> snprintf(name, PATH_MAX,
> "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/power/%s",
> >> pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func,
> >> attr);
> >>
> >> - fd = open(name, O_RDWR);
> >> + if (!strcmp(attr,"runtime_suspended_time"))
> >> + fd = open(name, O_RDONLY);
> >> + else
> >> + fd = open(name, O_RDWR);
> >> +
> >
> > There is a few other places opening an attribute with RDONLY, so maybe
> > add an igt_pm_get_power_attr_fd_rdonly() helper for that?
> >
> >> igt_assert_f(fd >= 0, "Can't open %s\n", attr);
> >>
> >> return fd;
> >> }
> >>
> >> -static bool igt_pm_read_power_attr(int fd, char *attr, int len, bool
> >> autosuspend_delay)
> >> +static bool igt_pm_read_power_attr(int fd, char *attr, int len, bool
> >> +power_attr)
> >> {
> >> int size;
> >>
> >> size = read(fd, attr, len - 1);
> >>
> >> - if (autosuspend_delay) {
> >> + if (power_attr) {
> >
> > How did this fail for runtime_suspended_time? It doesn't return -EIO
> > as opposed to autosuspend_delay, so maybe fd was invalid?
> >
> >> if (size < 0 && errno == EIO)
> >> return false;
> >> } else {
> >> @@ -1202,3 +1206,18 @@ void igt_pm_print_pci_card_runtime_status(void)
> >>
> igt_pm_print_pci_dev_runtime_status(__pci_dev_pwrattr[i].pci_dev);
> >> }
> >> }
> >> +
> >> +int igt_pm_get_runtime_suspended_time(struct pci_device *pci_dev) {
> >> + char time_str[64];
> >> + int time, time_fd;
> >> +
> >> + time_fd = igt_pm_get_power_attr_fd(pci_dev,
> "runtime_suspended_time");
> >> + if (igt_pm_read_power_attr(time_fd, time_str, 64, true))
> >> + igt_assert(sscanf(time_str, "%d", &time) > 0);
> >
> > If reading the attribute failed for any reason then we'd return random
> > data from this function. Being able to open the attribute and reading
> > it is a requirement for the given test.
> >
> >> +
> >> + igt_info("runtime suspend time for PCI '%04x:%02x:%02x.%01x' =
> %d\n",
> >> + pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func,
> >> +time);
> >> +
> >> + return time;
> >> +}
> >> diff --git a/lib/igt_pm.h b/lib/igt_pm.h index f28b6ebfd..ca1db7440
> >> 100644
> >> --- a/lib/igt_pm.h
> >> +++ b/lib/igt_pm.h
> >> @@ -79,5 +79,6 @@ void igt_pm_enable_pci_card_runtime_pm(struct
> pci_device *root,
> >> void igt_pm_setup_pci_card_runtime_pm(struct pci_device *pci_dev);
> >> void igt_pm_restore_pci_card_runtime_pm(void);
> >> void igt_pm_print_pci_card_runtime_status(void);
> >> +int igt_pm_get_runtime_suspended_time(struct pci_device *pci_dev);
> >>
> >> #endif /* IGT_PM_H */
> >> diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c index
> >> f664a2a2c..69afa3e6f 100644
> >> --- a/tests/i915/i915_pm_dc.c
> >> +++ b/tests/i915/i915_pm_dc.c
> >> @@ -31,8 +31,11 @@
> >> #include "igt_kmod.h"
> >> #include "igt_psr.h"
> >> #include "igt_sysfs.h"
> >> +#include "igt_device.h"
> >> +#include "igt_device_scan.h"
> >> #include "limits.h"
> >> #include "time.h"
> >> +#include "igt_pm.h"
> >>
> >> /* DC State Flags */
> >> #define CHECK_DC5 (1 << 0)
> >> @@ -426,7 +429,7 @@ static bool check_is_dgfx(data_t *data)
> >> {
> >> char buf[4096];
> >>
> >> - igt_debugfs_simple_read(data->debugfs_fd, "i915_gpu_info",
> >> + igt_debugfs_simple_read(data->debugfs_fd, "i915_capabilities",
> >> buf, sizeof(buf));
> >
> > Should this be a separate patch explaining the rational of the change?
> >
> >> return strstr(buf, "is_dgfx: yes");
> >> }
> >> @@ -453,6 +456,23 @@ static void setup_dc9_dpms(data_t *data, int
> dc_target)
> >> dpms_on(data);
> >> }
> >>
> >> +static void check_dc9_runtime_suspend_time(data_t *data) {
> >> + struct pci_device *i915;
> >> + int before, after;
> >> +
> >> + i915 = igt_device_get_pci_device(data->drm_fd);
> >> +
> >> + before = igt_pm_get_runtime_suspended_time(i915);
> >> + dpms_on(data);
> >> + cleanup_dc_dpms(data);
> >> + dpms_off(data);
> >> + sleep(1);
> >> + after = igt_pm_get_runtime_suspended_time(i915);
> >> + igt_assert_lt(before, after);
> >> + dpms_on(data);
> >> +}
> >
> > I think it's better to run test_dc9_dpms() on dGFX as well, only
> > change
> > dc9_wait_entry() which polls for both runtime_suspend_time to increase
> > (on both iGFX and dGFX) and for the DC5/6 counter to reset (on iGFX
> > only).
> >
> >> +
> >> static void test_dc9_dpms(data_t *data)
> >> {
> >> int dc_target;
> >> @@ -539,11 +559,20 @@ int main(int argc, char *argv[])
> >> test_dc_state_dpms(&data, CHECK_DC6);
> >> }
> >>
> >> - igt_describe("This test validates display engine entry to DC9 state");
> >> + igt_describe("This test validates display engine entry to DC9 state"
> >> + "for both igfx and dgfx");
> >> igt_subtest("dc9-dpms") {
> >> - if (!(check_is_dgfx(&data)))
> >> -
> igt_require_f(igt_pm_pc8_plus_residencies_enabled(data.msr_fd),
> >> - "PC8+ residencies not supported\n");
> >> + check_dc9_runtime_suspend_time(&data);
> >> + }
> >> +
> >> + igt_fixture
> >> + igt_require(!(check_is_dgfx(&data)));
> >> +
> >> + igt_describe("This test validates display engine entry to DC9 state"
> >> + "only for igfx");
> >> + igt_subtest("dc9-dpms-igfx") {
> >> +
> igt_require_f(igt_pm_pc8_plus_residencies_enabled(data.msr_fd),
> >> + "PC8+ residencies not supported\n");
> >> test_dc9_dpms(&data);
> >> }
> >>
> >> --
> >> 2.25.1
> >>
>
> --
> ~Swati Sharma
More information about the igt-dev
mailing list