But the only constant is the action code. Everything else is parameters and will be different on each call.On Sat, 14 May 2022, Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx> wrote:SLPC min/max frequency updates require H2G calls. We are seeing timeouts when GuC channel is backed up and it is unable to respond in a timely fashion causing warnings and affecting CI. This is seen when waitboosting happens during a stress test. this patch updates the waitboost path to use a non-blocking H2G call instead, which returns as soon as the message is successfully transmitted. v2: Use drm_notice to report any errors that might occur while sending the waitboost H2G request (Tvrtko) Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx> --- drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 44 +++++++++++++++++---- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c index 1db833da42df..e5e869c96262 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -98,6 +98,30 @@ static u32 slpc_get_state(struct intel_guc_slpc *slpc) return data->header.global_state; } +static int guc_action_slpc_set_param_nb(struct intel_guc *guc, u8 id, u32 value) +{ + u32 request[] = {static const+ GUC_ACTION_HOST2GUC_PC_SLPC_REQUEST, + SLPC_EVENT(SLPC_EVENT_PARAMETER_SET, 2), + id, + value, + }; + int ret; + + ret = intel_guc_send_nb(guc, request, ARRAY_SIZE(request), 0); + + return ret > 0 ? -EPROTO : ret; +} + +static int slpc_set_param_nb(struct intel_guc_slpc *slpc, u8 id, u32 value) +{ + struct intel_guc *guc = slpc_to_guc(slpc); + + GEM_BUG_ON(id >= SLPC_MAX_PARAM); + + return guc_action_slpc_set_param_nb(guc, id, value); +} + static int guc_action_slpc_set_param(struct intel_guc *guc, u8 id, u32 value) { u32 request[] = {Ditto here, and the whole gt/uc directory seems to have tons of these u32 action/request array variables on stack, with the required initialization, that could be in rodata. Please fix all of them. BR, Jani.
You mean something like this?
static const u32 template[] = {
action,
};
u32 *request = kmalloc_array(sizeof(*request), 4);
memcpy(request, template, sizeof(*request) * 1);
request[1] = param0;
request[2] = param1;
request[3] = param2;
ret = send(request);
kfree(request);
return ret;
Not seeing how that would be an improvement. It's a lot more code, a lot less readable, more prone to bugs due to incorrect structure sizes and/or params in the wrong place. The current version is easy to read and therefore to maintain, almost impossible to get wrong, and only puts a few words on the stack. I think the largest request is region of 15 words? I'm not seeing what the problem is.
John.
@@ -208,12 +232,10 @@ static int slpc_force_min_freq(struct intel_guc_slpc *slpc, u32 freq) */ with_intel_runtime_pm(&i915->runtime_pm, wakeref) { - ret = slpc_set_param(slpc, - SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, - freq); - if (ret) - i915_probe_error(i915, "Unable to force min freq to %u: %d", - freq, ret); + /* Non-blocking request will avoid stalls */ + ret = slpc_set_param_nb(slpc, + SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, + freq); } return ret; @@ -222,6 +244,8 @@ static int slpc_force_min_freq(struct intel_guc_slpc *slpc, u32 freq) static void slpc_boost_work(struct work_struct *work) { struct intel_guc_slpc *slpc = container_of(work, typeof(*slpc), boost_work); + struct drm_i915_private *i915 = slpc_to_i915(slpc); + int err; /* * Raise min freq to boost. It's possible that @@ -231,8 +255,12 @@ static void slpc_boost_work(struct work_struct *work) */ mutex_lock(&slpc->lock); if (atomic_read(&slpc->num_waiters)) { - slpc_force_min_freq(slpc, slpc->boost_freq); - slpc->num_boosts++; + err = slpc_force_min_freq(slpc, slpc->boost_freq); + if (!err) + slpc->num_boosts++; + else + drm_notice(&i915->drm, "Failed to send waitboost request (%d)\n", + err); } mutex_unlock(&slpc->lock); }
