Re: [i-g-t,1/2] tests/i915/pm_rc6_residency: Use sysfs to achieve peak frequency

[Riana Tauro <riana.tauro@intel.com>]

Hi Anirban

On 5/26/2025 6:35 PM, Anirban, Sk wrote:
> Hi Riana,
> 
> On 22-05-2025 09:04, Riana Tauro wrote:
>> Hi Anirban
>>
>> On 5/14/2025 12:07 AM, sk.anirban@intel.com wrote:
>>> From: Sk Anirban <sk.anirban@intel.com>
>>>
>>> Leverage IGT spinner to trigger frequency scaling and
>>> set RPS minimum frequency to RP0 for achieving peak frequency.
>>> This method replaces the existing waitboost mechanism used
>>> to reach maximum frequency.
>>>
>>> v2:
>>>   - Disable power consumption check for dgfx
>>>
>>> v3:
>>>   - Implement igt exit handler
>>>
>>> v4:
>>>   - Cosmetic changes (Riana)
>>>
>>> v5:
>>>   - Limit the power consumption test to GT0 only
>>>
>>> v6:
>>>   - Fix frequency restoration (Riana)
>>>
>>> Signed-off-by: Sk Anirban <sk.anirban@intel.com>
>>> ---
>>>   tests/intel/i915_pm_rc6_residency.c | 136 +++++++++++++++++++---------
>>>   1 file changed, 91 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/tests/intel/i915_pm_rc6_residency.c b/tests/intel/ 
>>> i915_pm_rc6_residency.c
>>> index c9128481d..b400460c7 100644
>>> --- a/tests/intel/i915_pm_rc6_residency.c
>>> +++ b/tests/intel/i915_pm_rc6_residency.c
>>> @@ -264,17 +264,6 @@ static bool __pmu_wait_for_rc6(int fd)
>>>       return false;
>>>   }
>>>   -static uint32_t batch_create(int fd)
>>> -{
>>> -    const uint32_t bbe = MI_BATCH_BUFFER_END;
>>> -    uint32_t handle;
>>> -
>>> -    handle = gem_create(fd, 4096);
>>> -    gem_write(fd, handle, 0, &bbe, sizeof(bbe));
>>> -
>>> -    return handle;
>>> -}
>>> -
>>>   static int open_pmu(int i915, uint64_t config)
>>>   {
>>>       int fd;
>>> @@ -286,45 +275,88 @@ static int open_pmu(int i915, uint64_t config)
>>>       return fd;
>>>   }
>>>   -#define WAITBOOST 0x1
>>> +#define FREQUENT_BOOST 0x1
>>>   #define ONCE 0x2
>>>     static void sighandler(int sig)
>>>   {
>>>   }
>>>   -static void bg_load(int i915, uint32_t ctx_id, uint64_t 
>>> engine_flags, unsigned int flags, unsigned long *ctl)
>>> +static uint32_t get_freq(int dirfd, uint8_t id)
>>> +{
>>> +    uint32_t val;
>>> +
>>> +    igt_assert(igt_sysfs_rps_scanf(dirfd, id, "%u", &val) == 1);
>>> +
>>> +    return val;
>>> +}
>>> +
>>> +static int set_freq(int dirfd, uint8_t id, uint32_t val)
>>> +{
>>> +    return igt_sysfs_rps_printf(dirfd, id, "%u", val);
>>> +}
>>> +
>>> +uint32_t stash_min;
>>> +int i915 = -1;
>>> +
>>> +static void restore_freq(int sig)
>>> +{
>>> +    int dirfd;
>>> +    int gt = 0;
>>> +
>>> +    i915 = drm_open_driver(DRIVER_INTEL);
>>
>> Why open again. The reason for making it static was re-use
>> Is i915 not open when exit handler is run?
> The exit handler is getting called after the driver closure so re- 
> opening it to the restore frequency.

you can keep it local like before or remove close in fixture

With the above change
Reviewed-by: Riana Tauro <riana.tauro@intel.com>>>
>> Rest of the code looks good.
>>
>> Also respond on the BAT failures as the test is same and are legacy 
>> issues
> Yeah, testing rev again so that ci team can re-report the BAT failures.
>>
>> Thanks
>> Riana
> Thanks,
> Anirban
>>
>>> +    dirfd = igt_sysfs_gt_open(i915, 0);
>>> +    igt_sysfs_gt_open(i915, gt);
>>> +
>>> +    igt_assert_lt(0, set_freq(dirfd, RPS_MIN_FREQ_MHZ, stash_min));
>>> +
>>> +    drm_close_driver(i915);
>>> +    close(dirfd);
>>> +}
>>> +
>>> +static void bg_load(const intel_ctx_t *ctx, int dirfd, uint64_t 
>>> engine_flags,
>>> +            unsigned int flags, unsigned long *ctl, unsigned int gt)
>>>   {
>>>       const bool has_execlists = intel_gen(intel_get_drm_devid(i915)) 
>>> >= 8;
>>> -    struct drm_i915_gem_exec_object2 obj = {
>>> -        .handle = batch_create(i915),
>>> -    };
>>> -    struct drm_i915_gem_execbuffer2 execbuf = {
>>> -        .buffers_ptr = to_user_pointer(&obj),
>>> -        .buffer_count = 1,
>>> -        .flags = engine_flags,
>>> -        .rsvd1 = ctx_id,
>>> -    };
>>>       struct sigaction act = {
>>>           .sa_handler = sighandler
>>>       };
>>> +    int64_t timeout = 1;
>>> +    uint64_t ahnd;
>>> +    int rp0;
>>>   +    ahnd = get_reloc_ahnd(i915, ctx->id);
>>> +    rp0 = get_freq(dirfd, RPS_RP0_FREQ_MHZ);
>>>       sigaction(SIGINT, &act, NULL);
>>>       do {
>>>           uint64_t submit, wait, elapsed;
>>>           struct timespec tv = {};
>>> +        igt_spin_t *spin;
>>>             igt_nsec_elapsed(&tv);
>>> -
>>> -        gem_execbuf(i915, &execbuf);
>>> +        spin = igt_spin_new(i915,
>>> +                    .ahnd = ahnd,
>>> +                    .ctx = ctx,
>>> +                    .engine = engine_flags);
>>>           submit = igt_nsec_elapsed(&tv);
>>> -        if (flags & WAITBOOST) {
>>> -            gem_sync(i915, obj.handle);
>>> +        if (flags & FREQUENT_BOOST) {
>>> +            /* Set MIN freq to RP0 to achieve the peak freq */
>>> +            igt_assert_lt(0, set_freq(dirfd, RPS_MIN_FREQ_MHZ, rp0));
>>> +            igt_assert(gem_bo_busy(i915, spin->handle));
>>> +            gem_wait(i915, spin->handle, &timeout);
>>> +
>>> +            /* Restore the MIN freq back to default */
>>> +            igt_assert_lt(0, set_freq(dirfd, RPS_MIN_FREQ_MHZ, 
>>> stash_min));
>>> +            igt_spin_end(spin);
>>> +            igt_spin_free(i915, spin);
>>> +            gem_quiescent_gpu(i915);
>>>               if (flags & ONCE)
>>> -                flags &= ~WAITBOOST;
>>> +                flags &= ~FREQUENT_BOOST;
>>>           } else  {
>>> -            while (gem_bo_busy(i915, obj.handle))
>>> -                usleep(0);
>>> +            igt_assert(gem_bo_busy(i915, spin->handle));
>>> +            igt_spin_end(spin);
>>> +            igt_spin_free(i915, spin);
>>> +            gem_quiescent_gpu(i915);
>>>           }
>>>           wait = igt_nsec_elapsed(&tv);
>>>   @@ -350,6 +382,7 @@ static void bg_load(int i915, uint32_t ctx_id, 
>>> uint64_t engine_flags, unsigned i
>>>           /* aim for ~1% busy */
>>>           usleep(min_t(elapsed, elapsed / 10, 50 * 1000));
>>>       } while (!READ_ONCE(*ctl));
>>> +    put_ahnd(ahnd);
>>>   }
>>>     static void kill_children(int sig)
>>> @@ -361,9 +394,9 @@ static void kill_children(int sig)
>>>       signal(sig, old);
>>>   }
>>>   -static void rc6_idle(int i915, uint32_t ctx_id, uint64_t flags, 
>>> unsigned int gt)
>>> +static void rc6_idle(const intel_ctx_t *ctx, uint64_t flags, 
>>> unsigned int gt)
>>>   {
>>> -    const int64_t duration_ns = SLEEP_DURATION * (int64_t)NSEC_PER_SEC;
>>> +    const int64_t duration_ns = 2 * SLEEP_DURATION * 
>>> (int64_t)NSEC_PER_SEC;
>>>       const int tolerance = 20; /* Some RC6 is better than none! */
>>>       const unsigned int gen = intel_gen(intel_get_drm_devid(i915));
>>>       struct {
>>> @@ -371,10 +404,11 @@ static void rc6_idle(int i915, uint32_t ctx_id, 
>>> uint64_t flags, unsigned int gt)
>>>           unsigned int flags;
>>>           double power;
>>>       } phases[] = {
>>> +        { "once", FREQUENT_BOOST | ONCE },
>>>           { "normal", 0 },
>>> -        { "boost", WAITBOOST },
>>> -        { "once", WAITBOOST | ONCE },
>>> +        { "boost", FREQUENT_BOOST }
>>>       };
>>> +    int dirfd = igt_sysfs_gt_open(i915, gt);
>>>       struct power_sample sample[2];
>>>       unsigned long slept, cycles;
>>>       unsigned long *done;
>>> @@ -407,12 +441,21 @@ static void rc6_idle(int i915, uint32_t ctx_id, 
>>> uint64_t flags, unsigned int gt)
>>>         assert_within_epsilon_debug(rc6, ts[1] - ts[0], 5, drpc);
>>>   +    if (gt) {
>>> +        close(fd);
>>> +        close(dirfd);
>>> +        igt_power_close(&gpu);
>>> +        return;
>>> +    }
>>> +
>>>       done = mmap(0, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0);
>>>   +    stash_min = get_freq(dirfd, RPS_MIN_FREQ_MHZ);
>>> +
>>>       for (int p = 0; p < ARRAY_SIZE(phases); p++) {
>>>           memset(done, 0, 2 * sizeof(*done));
>>>           igt_fork(child, 1) /* Setup up a very light load */
>>> -            bg_load(i915, ctx_id, flags, phases[p].flags, done);
>>> +            bg_load(ctx, dirfd, flags, phases[p].flags, done, gt);
>>>             igt_power_get_energy(&gpu, &sample[0]);
>>>           cycles = -READ_ONCE(done[1]);
>>> @@ -451,19 +494,20 @@ static void rc6_idle(int i915, uint32_t ctx_id, 
>>> uint64_t flags, unsigned int gt)
>>>         munmap(done, 4096);
>>>       close(fd);
>>> +    close(dirfd);
>>>         igt_power_close(&gpu);
>>>   -    if (phases[1].power - phases[0].power > 10) {
>>> -        igt_assert_f(2 * phases[2].power - phases[0].power <= 
>>> phases[1].power,
>>> +    if (phases[2].power - phases[1].power > 20 && ! 
>>> gem_has_lmem(i915)) {
>>> +        igt_assert_f(2 * phases[0].power - phases[1].power <= 
>>> phases[2].power,
>>>                    "Exceeded energy expectations for single busy wait 
>>> load\n"
>>>                    "Used %.1fmW, min %.1fmW, max %.1fmW, expected 
>>> less than %.1fmW\n",
>>> -                 phases[2].power, phases[0].power, phases[1].power,
>>> -                 phases[0].power + (phases[1].power - 
>>> phases[0].power) / 2);
>>> +                 phases[0].power, phases[1].power, phases[2].power,
>>> +                 phases[1].power + (phases[2].power - 
>>> phases[1].power) / 2);
>>>       }
>>>   }
>>>   -static void rc6_fence(int i915, unsigned int gt)
>>> +static void rc6_fence(unsigned int gt)
>>>   {
>>>       const int64_t duration_ns = SLEEP_DURATION * 
>>> (int64_t)NSEC_PER_SEC;
>>>       const int tolerance = 20; /* Some RC6 is better than none! */
>>> @@ -557,7 +601,7 @@ static void rc6_fence(int i915, unsigned int gt)
>>>       close(fd);
>>>   }
>>>   -static unsigned int rc6_enabled_mask(int i915, int dirfd)
>>> +static unsigned int rc6_enabled_mask(int dirfd)
>>>   {
>>>       igt_require(has_rc6_residency(dirfd, RC6_RESIDENCY_MS));
>>>   @@ -570,7 +614,6 @@ static unsigned int rc6_enabled_mask(int i915, 
>>> int dirfd)
>>>     igt_main
>>>   {
>>> -    int i915 = -1;
>>>       unsigned int dirfd, gt;
>>>       const intel_ctx_t *ctx;
>>>   @@ -582,19 +625,22 @@ igt_main
>>>       igt_subtest_with_dynamic("rc6-idle") {
>>>           const struct intel_execution_engine2 *e;
>>>   +        igt_install_exit_handler(restore_freq);
>>>           igt_require_gem(i915);
>>>           gem_quiescent_gpu(i915);
>>> +        intel_allocator_multiprocess_start();
>>>             i915_for_each_gt(i915, dirfd, gt) {
>>>               ctx = intel_ctx_create_for_gt(i915, gt);
>>>               for_each_ctx_engine(i915, ctx, e) {
>>>                   if (e->instance == 0) {
>>>                       igt_dynamic_f("gt%u-%s", gt, e->name)
>>> -                        rc6_idle(i915, ctx->id, e->flags, gt);
>>> +                        rc6_idle(ctx, e->flags, gt);
>>>                   }
>>>               }
>>>               intel_ctx_destroy(i915, ctx);
>>>           }
>>> +        intel_allocator_multiprocess_stop();
>>>       }
>>>         igt_subtest_with_dynamic("rc6-fence") {
>>> @@ -603,7 +649,7 @@ igt_main
>>>             i915_for_each_gt(i915, dirfd, gt)
>>>               igt_dynamic_f("gt%u", gt)
>>> -                rc6_fence(i915, gt);
>>> +                rc6_fence(gt);
>>>       }
>>>         igt_subtest_group {
>>> @@ -621,7 +667,7 @@ igt_main
>>>                   igt_dynamic_f("gt%u", gt) {
>>>                       struct residencies res;
>>>   -                    rc6_enabled = rc6_enabled_mask(i915, dirfd);
>>> +                    rc6_enabled = rc6_enabled_mask(dirfd);
>>>                       igt_require(rc6_enabled & RC6_ENABLED);
>>>                         measure_residencies(devid, dirfd, 
>>> rc6_enabled, &res);
>>> @@ -635,7 +681,7 @@ igt_main
>>>                 igt_require(IS_VALLEYVIEW(devid) || 
>>> IS_CHERRYVIEW(devid));
>>>   -            rc6_enabled = rc6_enabled_mask(i915, sysfs);
>>> +            rc6_enabled = rc6_enabled_mask(sysfs);
>>>               igt_require(rc6_enabled & RC6_ENABLED);
>>>                 measure_residencies(devid, sysfs, rc6_enabled, &res);
>>
>