Re: [Intel-gfx] [PATCH 1/3] drm/i915/guc/slpc: Define and initialize boost frequency

Belgaumkar, Vinay
2021-11-02T00:47:35Z

On 11/1/2021 1:26 PM, Dixit, Ashutosh wrote:
> On Sun, 31 Oct 2021 21:39:35 -0700, Belgaumkar, Vinay wrote:
>>
>> Define helpers and struct members required to record boost info.
>> Boost frequency is initialized to RP0 at SLPC init. Also define num_waiters
>> which can track the pending boost requests.
>>
>> Boost will be done by scheduling a worker thread. This will allow
>> us to make H2G calls inside an interrupt context. Initialize the
>
> "to not make H2G calls from interrupt context" is probably better.
>
>> +static int slpc_force_min_freq(struct intel_guc_slpc *slpc, u32 freq)
>> +{
>> + struct drm_i915_private *i915 = slpc_to_i915(slpc);
>> + intel_wakeref_t wakeref;
>> + int ret = 0;
>> +
>> + lockdep_assert_held(&slpc->lock);
>> +
>> + /**
>
> nit: this I believe should just be
>
> /*

ok.

>
> /** I believe shows up in kerneldoc so shouldn't be used unless we want
> something in kerneldoc.
>
>> + * This function is a little different as compared to
>> + * intel_guc_slpc_set_min_freq(). Softlimit will not be updated
>> + * here since this is used to temporarily change min freq,
>> + * for example, during a waitboost. Caller is responsible for
>> + * checking bounds.
>> + */
>> +
>> + with_intel_runtime_pm(&i915->runtime_pm, wakeref) {
>> + ret = slpc_set_param(slpc,
>> + SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
>> + freq);
>> + if (ret)
>> + drm_err(&i915->drm, "Unable to force min freq to %u: %d",
>
> Probably drm_err_ratelimited since it's called at run time not only at
> init? Not sure if drm_err_once suffizes, probably not.

Keeping it drm_err as discussed offline.

>
>> + freq, ret);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void slpc_boost_work(struct work_struct *work)
>> +{
>> + struct intel_guc_slpc *slpc = container_of(work, typeof(*slpc), boost_work);
>> +
>> + /* Raise min freq to boost. It's possible that
>> + * this is greater than current max. But it will
>> + * certainly be limited by RP0. An error setting
>> + * the min param is not fatal.
>> + */
>
> nit: do we follow the following format for multi-line comments,
> Documentation/process/coding-style.rst mentions this:
>
> /*
> * Line 1
> * Line 2
> */

Ok.

Thanks,
Vinay.
>