[Intel-gfx] [PATCH] drm/i915/guc/slpc: Add a new SLPC selftest
Belgaumkar, Vinay
vinay.belgaumkar at intel.com
Thu Jun 23 23:21:46 UTC 2022
On 6/22/2022 1:32 PM, Dixit, Ashutosh wrote:
> On Fri, 10 Jun 2022 16:47:12 -0700, Vinay Belgaumkar wrote:
>> This test will validate we can achieve actual frequency of RP0. Pcode
>> grants frequencies based on what GuC is requesting. However, thermal
>> throttling can limit what is being granted. Add a test to request for
>> max, but don't fail the test if RP0 is not granted due to throttle
>> reasons.
>>
>> Also optimize the selftest by using a common run_test function to avoid
>> code duplication.
> The refactoring does change the order of operations (changing the freq vs
> spawning the spinner) but should be fine I think.
Yes, we now start the spinner outside the for loop, so that freq changes
occur quickly. This ensures we don't mess with SLPC algorithm's history
by frequently restarting the WL in the for loop.
>
>> Rename the "clamp" tests to vary_max_freq and vary_min_freq.
> Either is ok, but maybe "clamp" names were ok I think since they verify req
> freq is clamped at min/max.
True, though clamp usually is associated with limiting, whereas we
actually increase the min.
>
>> v2: Fix compile warning
>>
>> Fixes 8ee2c227822e ("drm/i915/guc/slpc: Add SLPC selftest")
>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/selftest_slpc.c | 323 ++++++++++++------------
>> 1 file changed, 158 insertions(+), 165 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c b/drivers/gpu/drm/i915/gt/selftest_slpc.c
>> index b768cea5943d..099129aae9a5 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
>> @@ -8,6 +8,11 @@
>> #define delay_for_h2g() usleep_range(H2G_DELAY, H2G_DELAY + 10000)
>> #define FREQUENCY_REQ_UNIT DIV_ROUND_CLOSEST(GT_FREQUENCY_MULTIPLIER, \
>> GEN9_FREQ_SCALER)
>> +enum test_type {
>> + VARY_MIN,
>> + VARY_MAX,
>> + MAX_GRANTED
>> +};
>>
>> static int slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 freq)
>> {
>> @@ -36,147 +41,120 @@ static int slpc_set_max_freq(struct intel_guc_slpc *slpc, u32 freq)
>> return ret;
>> }
>>
>> -static int live_slpc_clamp_min(void *arg)
>> +static int vary_max_freq(struct intel_guc_slpc *slpc, struct intel_rps *rps,
>> + u32 *max_act_freq)
> Please run checkpatch, indentation seems off.
I had run it. Not sure why this wasn't caught.
>
>> {
>> - struct drm_i915_private *i915 = arg;
>> - struct intel_gt *gt = to_gt(i915);
>> - struct intel_guc_slpc *slpc = >->uc.guc.slpc;
>> - struct intel_rps *rps = >->rps;
>> - struct intel_engine_cs *engine;
>> - enum intel_engine_id id;
>> - struct igt_spinner spin;
>> + u32 step, max_freq, req_freq;
>> + u32 act_freq;
>> u32 slpc_min_freq, slpc_max_freq;
>> int err = 0;
>>
>> - if (!intel_uc_uses_guc_slpc(>->uc))
>> - return 0;
>> -
>> - if (igt_spinner_init(&spin, gt))
>> - return -ENOMEM;
>> + slpc_min_freq = slpc->min_freq;
>> + slpc_max_freq = slpc->rp0_freq;
> nit but we don't really need such variables since we don't change their
> values, we should just use slpc->min_freq, slpc->rp0_freq directly. I'd
> change this in all places in this patch.
I will remove it from the sub-functions, but will need to keep the one
in the main run_test(). We should query SLPC's min and max and then
restore that at the end of the test. It is possible that SLPC's min is
different from platform min for certain skus.
>
>> - if (intel_guc_slpc_get_max_freq(slpc, &slpc_max_freq)) {
>> - pr_err("Could not get SLPC max freq\n");
>> - return -EIO;
>> - }
>> -
>> - if (intel_guc_slpc_get_min_freq(slpc, &slpc_min_freq)) {
>> - pr_err("Could not get SLPC min freq\n");
>> - return -EIO;
> Why do we need these two function calls? Can't we just use slpc->rp0_freq
> and slpc->min_freq as we are doing in the vary_min/max_freq() functions
> above?
Same as above.
>
> Also, as mentioned below I think here we should just do:
>
> slpc_set_max_freq(slpc, slpc->rp0_freq);
> slpc_set_min_freq(slpc, slpc->min_freq);
>
> to restore freq to a known state before starting the test (just in case a
> previous test changed the values).
Any test that changes the frequencies should restore them as well.
>
>> - }
>> -
>> - if (slpc_min_freq == slpc_max_freq) {
>> - pr_err("Min/Max are fused to the same value\n");
>> - return -EINVAL;
> What if they are actually equal? I think basically the max/min freq test
> loops will just not be entered (so effectively the tests will just
> skip). The granted freq test will be fine. So I think we can just delete
> this if statement?
>
> (It is showing deleted above in the patch but is in the new code somewhere
> too).
Actually, we should set it to min/rp0 if this is the case. That change
will be in a separate patch. This is needed for certain cases.
>
>> - }
>> -
>> - intel_gt_pm_wait_for_idle(gt);
>> - intel_gt_pm_get(gt);
>> - for_each_engine(engine, gt, id) {
>> - struct i915_request *rq;
>> - u32 step, min_freq, req_freq;
>> - u32 act_freq, max_act_freq;
>> -
>> - if (!intel_engine_can_store_dword(engine))
>> - continue;
>> + /* Go from max to min in 5 steps */
>> + step = (slpc_max_freq - slpc_min_freq) / NUM_STEPS;
>> + *max_act_freq = slpc_min_freq;
>> + for (max_freq = slpc_max_freq; max_freq > slpc_min_freq;
>> + max_freq -= step) {
>> + err = slpc_set_max_freq(slpc, max_freq);
>> + if (err)
>> + break;
>>
>> - /* Go from min to max in 5 steps */
>> - step = (slpc_max_freq - slpc_min_freq) / NUM_STEPS;
>> - max_act_freq = slpc_min_freq;
>> - for (min_freq = slpc_min_freq; min_freq < slpc_max_freq;
>> - min_freq += step) {
>> - err = slpc_set_min_freq(slpc, min_freq);
>> - if (err)
>> - break;
>> -
>> - st_engine_heartbeat_disable(engine);
>> -
>> - rq = igt_spinner_create_request(&spin,
>> - engine->kernel_context,
>> - MI_NOOP);
>> - if (IS_ERR(rq)) {
>> - err = PTR_ERR(rq);
>> - st_engine_heartbeat_enable(engine);
>> - break;
>> - }
>> + req_freq = intel_rps_read_punit_req_frequency(rps);
>>
>> - i915_request_add(rq);
>> + /* GuC requests freq in multiples of 50/3 MHz */
>> + if (req_freq > (max_freq + FREQUENCY_REQ_UNIT)) {
>> + pr_err("SWReq is %d, should be at most %d\n", req_freq,
>> + max_freq + FREQUENCY_REQ_UNIT);
>> + err = -EINVAL;
> Probably a nit but check can be (so should we be checking both high and low
> limits?):
> if (req_freq > (max_freq + FREQUENCY_REQ_UNIT) ||
> req_freq < (slpc_min_freq - FREQUENCY_REQ_UNIT))
>
> Though if we do this we'd need to change the pr_err() or have two separate
> if statements. Not sure if it's worth it but thought I'll mention it.
We are looking to validate it does not cross the upper limit.
>
>> +static int vary_min_freq(struct intel_guc_slpc *slpc, struct intel_rps *rps,
>> + u32 *max_act_freq)
>> +{
>> + u32 step, min_freq, req_freq;
>> + u32 act_freq;
>> + u32 slpc_min_freq, slpc_max_freq;
>> + int err = 0;
>>
>> - act_freq = intel_rps_read_actual_frequency(rps);
>> - if (act_freq > max_act_freq)
>> - max_act_freq = act_freq;
>> + slpc_min_freq = slpc->min_freq;
>> + slpc_max_freq = slpc->rp0_freq;
>>
>> - igt_spinner_end(&spin);
>> - st_engine_heartbeat_enable(engine);
>> - }
>> + /* Go from min to max in 5 steps */
>> + step = (slpc_max_freq - slpc_min_freq) / NUM_STEPS;
>> + *max_act_freq = slpc_min_freq;
>> + for (min_freq = slpc_min_freq; min_freq < slpc_max_freq;
>> + min_freq += step) {
>> + err = slpc_set_min_freq(slpc, min_freq);
>> + if (err)
>> + break;
>>
>> - pr_info("Max actual frequency for %s was %d\n",
>> - engine->name, max_act_freq);
>> + req_freq = intel_rps_read_punit_req_frequency(rps);
>>
>> - /* Actual frequency should rise above min */
>> - if (max_act_freq == slpc_min_freq) {
> Nit again. This check is somewhere in the new code but I think a better
> check is
>
> if (max_act_freq <= slpc_min_freq)
>
> just in case the act freq for whatever reason falls below
> slpc_min_freq. Even if we know this is impossible (bugs make the impossible
> possible).
sure.
>
>> - pr_err("Actual freq did not rise above min\n");
>> + /* GuC requests freq in multiples of 50/3 MHz */
>> + if (req_freq < (min_freq - FREQUENCY_REQ_UNIT)) {
>> + pr_err("SWReq is %d, should be at least %d\n", req_freq,
>> + min_freq - FREQUENCY_REQ_UNIT);
>> err = -EINVAL;
> Again nit as above, but check can be:
> if (req_freq < (min_freq - FREQUENCY_REQ_UNIT) ||
> req_freq > (slpc_max_freq + FREQUENCY_REQ_UNIT)) {
It can be higher, we want to validate lower range.
>
>> }
>>
>> + act_freq = intel_rps_read_actual_frequency(rps);
>> + if (act_freq > *max_act_freq)
>> + *max_act_freq = act_freq;
>> +
>> if (err)
>> break;
>> }
>>
>> - /* Restore min/max frequencies */
>> - slpc_set_max_freq(slpc, slpc_max_freq);
>> - slpc_set_min_freq(slpc, slpc_min_freq);
>> + return err;
>> +}
>>
>> - if (igt_flush_test(gt->i915))
>> - err = -EIO;
>> +static int max_granted_freq(struct intel_guc_slpc *slpc, struct intel_rps *rps, u32 *max_act_freq)
>> +{
>> + struct intel_gt *gt = rps_to_gt(rps);
>> + u32 perf_limit_reasons;
>> + int err = 0;
>>
>> - intel_gt_pm_put(gt);
>> - igt_spinner_fini(&spin);
>> - intel_gt_pm_wait_for_idle(gt);
>> + err = slpc_set_min_freq(slpc, slpc->rp0_freq);
>> + if (err)
>> + return err;
>> +
>> + *max_act_freq = intel_rps_read_actual_frequency(rps);
>> + if (!(*max_act_freq == slpc->rp0_freq)) {
>> + /* Check if there was some throttling by pcode */
>> + perf_limit_reasons = intel_uncore_read(gt->uncore, GT0_PERF_LIMIT_REASONS);
>> +
>> + /* If not, this is an error */
>> + if (perf_limit_reasons && GT0_PERF_LIMIT_REASONS_MASK) {
>> + pr_err("Pcode did not grant max freq\n");
>> + err = -EINVAL;
> Looks incorrect, probably something like:
> if (!(perf_limit_reasons & GT0_PERF_LIMIT_REASONS_MASK))
Hmm, good catch. We should flag error iff there is no throttling and act
freq does not go to max.
>
>> + }
>> + }
>>
>> return err;
>> }
>>
>> -static int live_slpc_clamp_max(void *arg)
>> +static int run_test(struct intel_gt *gt, int test_type)
>> {
>> - struct drm_i915_private *i915 = arg;
>> - struct intel_gt *gt = to_gt(i915);
>> - struct intel_guc_slpc *slpc;
>> - struct intel_rps *rps;
>> + struct intel_guc_slpc *slpc = >->uc.guc.slpc;
>> + struct intel_rps *rps = >->rps;
>> struct intel_engine_cs *engine;
>> enum intel_engine_id id;
>> struct igt_spinner spin;
>> - int err = 0;
>> u32 slpc_min_freq, slpc_max_freq;
>> -
>> - slpc = >->uc.guc.slpc;
>> - rps = >->rps;
>> + int err = 0;
>>
>> if (!intel_uc_uses_guc_slpc(>->uc))
>> return 0;
>> @@ -203,69 +181,56 @@ static int live_slpc_clamp_max(void *arg)
>> intel_gt_pm_get(gt);
>> for_each_engine(engine, gt, id) {
>> struct i915_request *rq;
>> - u32 max_freq, req_freq;
>> - u32 act_freq, max_act_freq;
>> - u32 step;
>> + u32 max_act_freq;
>>
>> if (!intel_engine_can_store_dword(engine))
>> continue;
>>
>> - /* Go from max to min in 5 steps */
>> - step = (slpc_max_freq - slpc_min_freq) / NUM_STEPS;
>> - max_act_freq = slpc_min_freq;
>> - for (max_freq = slpc_max_freq; max_freq > slpc_min_freq;
>> - max_freq -= step) {
>> - err = slpc_set_max_freq(slpc, max_freq);
>> - if (err)
>> - break;
>> -
>> - st_engine_heartbeat_disable(engine);
>> -
>> - rq = igt_spinner_create_request(&spin,
>> - engine->kernel_context,
>> - MI_NOOP);
>> - if (IS_ERR(rq)) {
>> - st_engine_heartbeat_enable(engine);
>> - err = PTR_ERR(rq);
>> - break;
>> - }
>> + st_engine_heartbeat_disable(engine);
>>
>> - i915_request_add(rq);
>> + rq = igt_spinner_create_request(&spin,
>> + engine->kernel_context,
>> + MI_NOOP);
>> + if (IS_ERR(rq)) {
>> + err = PTR_ERR(rq);
>> + st_engine_heartbeat_enable(engine);
>> + break;
>> + }
>>
>> - if (!igt_wait_for_spinner(&spin, rq)) {
>> - pr_err("%s: SLPC spinner did not start\n",
>> - engine->name);
>> - igt_spinner_end(&spin);
>> - st_engine_heartbeat_enable(engine);
>> - intel_gt_set_wedged(engine->gt);
>> - err = -EIO;
>> - break;
>> - }
>> + i915_request_add(rq);
>> +
>> + if (!igt_wait_for_spinner(&spin, rq)) {
>> + pr_err("%s: Spinner did not start\n",
>> + engine->name);
>> + igt_spinner_end(&spin);
>> + st_engine_heartbeat_enable(engine);
>> + intel_gt_set_wedged(engine->gt);
>> + err = -EIO;
>> + break;
>> + }
>>
>> - delay_for_h2g();
>> + switch (test_type) {
>>
>> - /* Verify that SWREQ indeed was set to specific value */
>> - req_freq = intel_rps_read_punit_req_frequency(rps);
>> + case VARY_MIN:
>> + err = vary_min_freq(slpc, rps, &max_act_freq);
>> + break;
>> +
>> + case VARY_MAX:
>> + err = vary_max_freq(slpc, rps, &max_act_freq);
>> + break;
>>
>> - /* GuC requests freq in multiples of 50/3 MHz */
>> - if (req_freq > (max_freq + FREQUENCY_REQ_UNIT)) {
>> - pr_err("SWReq is %d, should be at most %d\n", req_freq,
>> - max_freq + FREQUENCY_REQ_UNIT);
>> + case MAX_GRANTED:
>> + /* Media engines have a different RP0 */
>> + if ((engine->class == VIDEO_DECODE_CLASS) ||
>> + (engine->class == VIDEO_ENHANCEMENT_CLASS)) {
>> igt_spinner_end(&spin);
>> st_engine_heartbeat_enable(engine);
>> - err = -EINVAL;
>> - break;
>> + err = 0;
>> + continue;
> I think it's preferable to move this media engine code out of the main loop
> into max_granted_freq() function if possible (maybe by faking max_act_freq
> if needed)?
All the engine related info is here. I will need to pass it to the
max_granted_freq() function. Also, faking the act_freq probably
overkill. I can add a fixme here instead to update when we have a
reliable way to obtain media RP0 instead.
>
>> }
>>
>> - act_freq = intel_rps_read_actual_frequency(rps);
>> - if (act_freq > max_act_freq)
>> - max_act_freq = act_freq;
>> -
>> - st_engine_heartbeat_enable(engine);
>> - igt_spinner_end(&spin);
>> -
>> - if (err)
>> - break;
>> + err = max_granted_freq(slpc, rps, &max_act_freq);
>> + break;
>> }
>>
>> pr_info("Max actual frequency for %s was %d\n",
>> @@ -277,31 +242,59 @@ static int live_slpc_clamp_max(void *arg)
>> err = -EINVAL;
>> }
>>
>> - if (igt_flush_test(gt->i915)) {
>> - err = -EIO;
>> - break;
>> - }
>> + igt_spinner_end(&spin);
>> + st_engine_heartbeat_enable(engine);
>>
>> if (err)
>> break;
>> }
>>
>> - /* Restore min/max freq */
>> + /* Restore min/max frequencies */
>> slpc_set_max_freq(slpc, slpc_max_freq);
>> slpc_set_min_freq(slpc, slpc_min_freq);
> As mentioned above maybe we should restore at the beginning of the test too
> (before the for_each_engine() loop) to start from a known state?
>
> Anyway here maybe get rid of the variables and:
This is restoring whatever frequencies SLPC was running with initially.
Regarding resetting the frequencies to min for every engine loop
iteration, we are already iterating from min->max inside the for loop,
so will be duplication.
Thanks for the detailed review,
Vinay.
>
> slpc_set_max_freq(slpc, slpc->rp0_freq);
> slpc_set_min_freq(slpc, slpc->min_freq);
>
> Thanks.
> --
> Ashutosh
More information about the Intel-gfx
mailing list