Re: [Intel-gfx] [PATCH] drm/i915/guc/slpc: Use non-blocking H2G for waitboost

[John Harrison <john.c.harrison@intel.com>]

On 6/7/2022 16:02, John Harrison wrote:
> On 5/16/2022 00:59, Jani Nikula wrote:
>> On Sat, 14 May 2022, Vinay Belgaumkar<vinay.belgaumkar@intel.com>  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@intel.com>
>>> ---
>>>   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.
> But the only constant is the action code. Everything else is 
> parameters and will be different on each call.
> ...

Oops. Just saw your follow up message. No worries!

John.