Re: [Intel-gfx] [PATCH 06/15] drm/i915/guc/slpc: Enable SLPC and add related H2G events

Michal Wajdeczko
2021-07-27T20:19:42Z

On 27.07.2021 22:00, Belgaumkar, Vinay wrote:
>
>
> On 7/27/2021 8:12 AM, Michal Wajdeczko wrote:
>>
>>
>> On 26.07.2021 21:07, Vinay Belgaumkar wrote:
>>> Add methods for interacting with GuC for enabling SLPC. Enable
>>> SLPC after GuC submission has been established. GuC load will
>>> fail if SLPC cannot be successfully initialized. Add various
>>> helper methods to set/unset the parameters for SLPC. They can
>>> be set using H2G calls or directly setting bits in the shared
>>> data structure.
>>>
>>> v2: Address several review comments, add new helpers for
>>> decoding the SLPC min/max frequencies. Use masks instead of hardcoded
>>> constants. (Michal W)
>>>
>>> v3: Split global_state_to_string function, and check for positive
>>> non-zero return value from intel_guc_send() (Michal W)
>>>
>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>>> Signed-off-by: Sundaresan Sujaritha <sujaritha.sundaresan@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c   | 237 ++++++++++++++++++
>>>   .../gpu/drm/i915/gt/uc/intel_guc_slpc_types.h |   2 +
>>>   drivers/gpu/drm/i915/gt/uc/intel_uc.c         |   8 +
>>>   3 files changed, 247 insertions(+)
>>>
>>> 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 bae4e33db0f8..f5808d2acbca 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>>> @@ -45,6 +45,40 @@ void intel_guc_slpc_init_early(struct
>>> intel_guc_slpc *slpc)
>>>       guc->slpc_selected = __guc_slpc_selected(guc);
>>>   }
>>>   +static void slpc_mem_set_param(struct slpc_shared_data *data,
>>> +                u32 id, u32 value)
>>> +{
>>> +    GEM_BUG_ON(id >= SLPC_MAX_OVERRIDE_PARAMETERS);
>>> +    /*
>>> +     * When the flag bit is set, corresponding value will be read
>>> +     * and applied by slpc.
>>
>> s/slpc/SLPC
> ok.
>
>>
>>> +     */
>>> +    data->override_params.bits[id >> 5] |= (1 << (id % 32));
>>> +    data->override_params.values[id] = value;
>>> +}
>>> +
>>> +static void slpc_mem_set_enabled(struct slpc_shared_data *data,
>>> +                u8 enable_id, u8 disable_id)
>>> +{
>>> +    /*
>>> +     * Enabling a param involves setting the enable_id
>>> +     * to 1 and disable_id to 0.
>>> +     */
>>> +    slpc_mem_set_param(data, enable_id, 1);
>>> +    slpc_mem_set_param(data, disable_id, 0);
>>> +}
>>> +
>>> +static void slpc_mem_set_disabled(struct slpc_shared_data *data,
>>> +                u8 enable_id, u8 disable_id)
>>> +{
>>> +    /*
>>> +     * Disabling a param involves setting the enable_id
>>> +     * to 0 and disable_id to 1.
>>> +     */
>>> +    slpc_mem_set_param(data, disable_id, 1);
>>> +    slpc_mem_set_param(data, enable_id, 0);
>>> +}
>>> +
>>>   static int slpc_shared_data_init(struct intel_guc_slpc *slpc)
>>>   {
>>>       struct intel_guc *guc = slpc_to_guc(slpc);
>>> @@ -63,6 +97,129 @@ static int slpc_shared_data_init(struct
>>> intel_guc_slpc *slpc)
>>>       return err;
>>>   }
>>>   +static u32 slpc_get_state(struct intel_guc_slpc *slpc)
>>> +{
>>> +    struct slpc_shared_data *data;
>>> +
>>> +    GEM_BUG_ON(!slpc->vma);
>>> +
>>> +    drm_clflush_virt_range(slpc->vaddr, sizeof(u32));
>>> +    data = slpc->vaddr;
>>> +
>>> +    return data->header.global_state;
>>> +}
>>> +
>>> +static bool slpc_is_running(struct intel_guc_slpc *slpc)
>>> +{
>>> +    return (slpc_get_state(slpc) == SLPC_GLOBAL_STATE_RUNNING);
>>
>> extra ( ) not needed
>
> ok.
>
>>
>>> +}
>>> +
>>> +static int guc_action_slpc_query(struct intel_guc *guc, u32 offset)
>>> +{
>>> +    u32 request[] = {
>>> +        INTEL_GUC_ACTION_SLPC_REQUEST,
>>> +         SLPC_EVENT(SLPC_EVENT_QUERY_TASK_STATE, 2),
>>> +        offset,
>>> +        0,
>>> +    };
>>> +    int ret;
>>> +
>>> +    ret = intel_guc_send(guc, request, ARRAY_SIZE(request));
>>> +
>>> +    return ret > 0 ? -EPROTO : ret;
>>> +}
>>> +
>>> +static int slpc_query_task_state(struct intel_guc_slpc *slpc)
>>> +{
>>> +    struct intel_guc *guc = slpc_to_guc(slpc);
>>> +    struct drm_i915_private *i915 = slpc_to_i915(slpc);
>>> +    u32 shared_data_gtt_offset = intel_guc_ggtt_offset(guc, slpc->vma);
>>
>> just "offset" ? or maybe pass directly in call below ?
>
> ok.
>
>>
>>> +    int ret;
>>> +
>>> +    ret = guc_action_slpc_query(guc, shared_data_gtt_offset);
>>> +    if (ret)
>>> +        drm_err(&i915->drm, "Query task state data returned (%pe)\n",
>>
>> "Failed to query task state (%pe)\n" ?
>
> ok.
>>
>>> +                ERR_PTR(ret));
>>> +
>>> +    drm_clflush_virt_range(slpc->vaddr, SLPC_PAGE_SIZE_BYTES);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static const char *slpc_global_state_to_string(enum
>>> slpc_global_state state)
>>> +{
>>> +    const char *str = NULL;
>>> +
>>> +    switch (state) {
>>> +    case SLPC_GLOBAL_STATE_NOT_RUNNING:
>>> +        str = "not running";
>>> +        break;
>>> +    case SLPC_GLOBAL_STATE_INITIALIZING:
>>> +        str = "initializing";
>>> +        break;
>>> +    case SLPC_GLOBAL_STATE_RESETTING:
>>> +        str = "resetting";
>>> +        break;
>>> +    case SLPC_GLOBAL_STATE_RUNNING:
>>> +        str = "running";
>>> +        break;
>>> +    case SLPC_GLOBAL_STATE_SHUTTING_DOWN:
>>> +        str = "shutting down";
>>> +        break;
>>> +    case SLPC_GLOBAL_STATE_ERROR:
>>> +        str = "error";
>>> +        break;
>>> +    default:
>>> +        str = "unknown";
>>
>> nit: you can do early returns to simplify the code
> ok.
>
>>
>>> +        break;
>>> +    }
>>> +
>>> +    return str;
>>> +}
>>> +
>>> +static const char *slpc_get_state_string(struct intel_guc_slpc *slpc)
>>> +{
>>> +    return slpc_global_state_to_string(slpc_get_state(slpc));
>>> +}
>>> +
>>> +static int guc_action_slpc_reset(struct intel_guc *guc, u32 offset)
>>> +{
>>> +    u32 request[] = {
>>> +        INTEL_GUC_ACTION_SLPC_REQUEST,
>>> +        SLPC_EVENT(SLPC_EVENT_RESET, 2),
>>> +        offset,
>>> +        0,
>>> +    };
>>> +    int ret;
>>> +
>>> +    ret = intel_guc_send(guc, request, ARRAY_SIZE(request));
>>> +
>>> +    return ret > 0 ? -EPROTO : ret;
>>> +}
>>> +
>>> +static int slpc_reset(struct intel_guc_slpc *slpc)
>>> +{
>>> +    struct drm_i915_private *i915 = slpc_to_i915(slpc);
>>> +    struct intel_guc *guc = slpc_to_guc(slpc);
>>> +    u32 offset = intel_guc_ggtt_offset(guc, slpc->vma);
>>> +    int ret;
>>> +
>>> +    ret = guc_action_slpc_reset(guc, offset);
>>> +
>>> +    if (unlikely(ret < 0))
>>> +        return ret;
>>
>> no SLPC error here ?
>
> added.
>
>>
>>> +
>>> +    if (!ret) {
>>> +        if (wait_for(slpc_is_running(slpc), SLPC_RESET_TIMEOUT_MS)) {
>>> +            drm_err(&i915->drm, "SLPC not enabled! State = %s\n",
>>> +                  slpc_get_state_string(slpc));
>>> +            return -EIO;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
>>>   {
>>>       GEM_BUG_ON(slpc->vma);
>>> @@ -70,6 +227,86 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
>>>       return slpc_shared_data_init(slpc);
>>>   }
>>>   +static u32 slpc_decode_min_freq(struct intel_guc_slpc *slpc)
>>> +{
>>> +    struct slpc_shared_data *data = slpc->vaddr;
>>> +
>>> +    GEM_BUG_ON(!slpc->vma);
>>> +
>>> +    return    DIV_ROUND_CLOSEST(
>>> +        REG_FIELD_GET(SLPC_MIN_UNSLICE_FREQ_MASK,
>>> +            data->task_state_data.freq) *
>>> +        GT_FREQUENCY_MULTIPLIER, GEN9_FREQ_SCALER);
>>> +}
>>> +
>>> +static u32 slpc_decode_max_freq(struct intel_guc_slpc *slpc)
>>> +{
>>> +    struct slpc_shared_data *data = slpc->vaddr;
>>> +
>>> +    GEM_BUG_ON(!slpc->vma);
>>> +
>>> +    return    DIV_ROUND_CLOSEST(
>>> +        REG_FIELD_GET(SLPC_MAX_UNSLICE_FREQ_MASK,
>>> +            data->task_state_data.freq) *
>>> +        GT_FREQUENCY_MULTIPLIER, GEN9_FREQ_SCALER);
>>> +}
>>> +
>>> +/*
>>> + * intel_guc_slpc_enable() - Start SLPC
>>> + * @slpc: pointer to intel_guc_slpc.
>>> + *
>>> + * SLPC is enabled by setting up the shared data structure and
>>> + * sending reset event to GuC SLPC. Initial data is setup in
>>> + * intel_guc_slpc_init. Here we send the reset event. We do
>>> + * not currently need a slpc_disable since this is taken care
>>> + * of automatically when a reset/suspend occurs and the GuC
>>> + * CTB is destroyed.
>>> + *
>>> + * Return: 0 on success, non-zero error code on failure.
>>> + */
>>> +int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)
>>> +{
>>> +    struct drm_i915_private *i915 = slpc_to_i915(slpc);
>>> +    struct slpc_shared_data *data;
>>> +    int ret;
>>> +
>>> +    GEM_BUG_ON(!slpc->vma);
>>> +
>>> +    memset(slpc->vaddr, 0, sizeof(struct slpc_shared_data));
>>> +
>>> +    data = slpc->vaddr;
>>
>> vaddr is "struct slpc_shared_data *"
>> do you really need "data" local var?
>>
>>> +    data->header.size = sizeof(struct slpc_shared_data);
>>> +
>>> +    /* Enable only GTPERF task, disable others */
>>> +    slpc_mem_set_enabled(data, SLPC_PARAM_TASK_ENABLE_GTPERF,
>>> +                SLPC_PARAM_TASK_DISABLE_GTPERF);
>>> +
>>> +    slpc_mem_set_disabled(data, SLPC_PARAM_TASK_ENABLE_BALANCER,
>>> +                SLPC_PARAM_TASK_DISABLE_BALANCER);
>>> +
>>> +    slpc_mem_set_disabled(data, SLPC_PARAM_TASK_ENABLE_DCC,
>>> +                SLPC_PARAM_TASK_DISABLE_DCC);
>>
>> btw, all this "data" related calls are good candidate for helper like
>>
>> static void slpc_shared_data_reset(struct slpc_shared_data *data)
>> { ... }
>
> ok.
>
>>
>>> +
>>> +    ret = slpc_reset(slpc);
>>> +    if (unlikely(ret < 0)) {
>>> +        drm_err(&i915->drm, "SLPC Reset event returned (%pe)\n",
>>> +                ERR_PTR(ret));
>>> +        return ret;
>>> +    }
>>> +
>>> +    drm_info(&i915->drm, "GuC SLPC: enabled\n");
>>> +
>>> +    slpc_query_task_state(slpc);
>>
>> as this still may fail, maybe it should be before we claim success and
>> "SLPC: enabled" ?
>
> ok. Added error check for this as well.
>
>>
>>> +
>>> +    /* min and max frequency limits being used by SLPC */
>>> +    drm_info(&i915->drm, "SLPC min freq: %u Mhz, max is %u Mhz\n",
>>> +            slpc_decode_min_freq(slpc),
>>> +            slpc_decode_max_freq(slpc));
>>> +
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   void intel_guc_slpc_fini(struct intel_guc_slpc *slpc)
>>>   {
>>>       if (!slpc->vma)
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
>>> index edcf4c05bd9f..f14f81821a51 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
>>> @@ -6,6 +6,8 @@
>>>   #ifndef _INTEL_GUC_SLPC_TYPES_H_
>>>   #define _INTEL_GUC_SLPC_TYPES_H_
>>>   +#define SLPC_RESET_TIMEOUT_MS 5
>>> +
>>>   struct intel_guc_slpc {
>>>       struct i915_vma *vma;
>>>       struct slpc_shared_data *vaddr;
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> index e6bd9406c7b2..b98c14f8c229 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> @@ -506,6 +506,12 @@ static int __uc_init_hw(struct intel_uc *uc)
>>>            "submission",
>>>            enableddisabled(intel_uc_uses_guc_submission(uc)));
>>>   +    if (intel_uc_uses_guc_slpc(uc)) {
>>> +        ret = intel_guc_slpc_enable(&guc->slpc);
>>> +        if (ret)
>>> +            goto err_submission;
>>
>> hmm, as this may fail, above success message
>>     "GuC submission enabled"
>> will be void
>>
>> what you likely need is to split "slpc_enable" with error messages only
>> that is called before we start reporting successes, and then
>> "slpc_status" part with all drm_info() that could be placed here.
>
> Why? It is totally possible that GuC submission works just fine, but
> SLPC enable fails. In this case, even though we printed "submission
> enabled", we can still fail later with slpc errors.

hmm, in the code above there is "goto err_submission" if SLPC enabling
fails, and that will go to the "We've failed to load the firmware"
section below that disables GuC submission ... no ?

Michal

>
> Thanks,
> Vinay.
>>
>> Michal
>>
>>> +    }
>>> +
>>>       if (intel_uc_uses_huc(uc)) {
>>>           drm_info(&i915->drm, "%s firmware %s version %u.%u %s:%s\n",
>>>                intel_uc_fw_type_repr(INTEL_UC_FW_TYPE_HUC),
>>> @@ -520,6 +526,8 @@ static int __uc_init_hw(struct intel_uc *uc)
>>>       /*
>>>        * We've failed to load the firmware :(
>>>        */
>>> +err_submission:
>>> +    intel_guc_submission_disable(guc);
>>>   err_log_capture:
>>>       __uc_capture_load_err_log(uc);
>>>   err_out:
>>>