Re: [PATCH] drm/i915/slpc: Use platform limits for min/max frequency

["Belgaumkar, Vinay" <vinay.belgaumkar@xxxxxxxxx>]

On 10/13/2022 8:14 AM, Das, Nirmoy wrote:
> On 10/12/2022 8:26 PM, Vinay Belgaumkar wrote:
> > GuC will set the min/max frequencies to theoretical max on
> > ATS-M. This will break kernel ABI, so limit min/max frequency
> > to RP0(platform max) instead.
> >
> > Also modify the SLPC selftest to update the min frequency
> > when we have a server part so that we can iterate between
> > platform min and max.
> >
> > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/gt/selftest_slpc.c       | 40 +++++++++++++------
> >   drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c   | 29 ++++++++++++++
> >   .../gpu/drm/i915/gt/uc/intel_guc_slpc_types.h |  3 ++
> >   3 files changed, 60 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c 
> > b/drivers/gpu/drm/i915/gt/selftest_slpc.c
> > index 4c6e9257e593..1f84362af737 100644
> > --- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
> > +++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
> > @@ -234,6 +234,7 @@ static int run_test(struct intel_gt *gt, int 
> > test_type)
> >       enum intel_engine_id id;
> >       struct igt_spinner spin;
> >       u32 slpc_min_freq, slpc_max_freq;
> > +    u32 saved_min_freq;
> >       int err = 0;
> >         if (!intel_uc_uses_guc_slpc(&gt->uc))
> > @@ -252,20 +253,35 @@ static int run_test(struct intel_gt *gt, int 
> > test_type)
> >           return -EIO;
> >       }
> >   -    /*
> > -     * FIXME: With efficient frequency enabled, GuC can request
> > -     * frequencies higher than the SLPC max. While this is fixed
> > -     * in GuC, we level set these tests with RPn as min.
> > -     */
> > -    err = slpc_set_min_freq(slpc, slpc->min_freq);
> > -    if (err)
> > -        return err;
> > -
> >       if (slpc->min_freq == slpc->rp0_freq) {
> > -        pr_err("Min/Max are fused to the same value\n");
> > -        return -EINVAL;
> > +        /* Servers will have min/max clamped to RP0 */
>
>
> This should be "server parts". Tested the patch with Riana's suggested 
> changes.
>
> Acked-by: Nirmoy Das <nirmoy.das@xxxxxxxxx> with above changes.

Thanks, v2 sent with corrections.

Vinay.

> Nirmoy
>
> > +        if (slpc->min_is_rpmax) {
> > +            err = slpc_set_min_freq(slpc, slpc->min_freq);
> > +            if (err) {
> > +                pr_err("Unable to update min freq on server part");
> > +                return err;
> > +            }
> > +
> > +        } else {
> > +            pr_err("Min/Max are fused to the same value\n");
> > +            return -EINVAL;
> > +        }
> > +    } else {
> > +        /*
> > +         * FIXME: With efficient frequency enabled, GuC can request
> > +         * frequencies higher than the SLPC max. While this is fixed
> > +         * in GuC, we level set these tests with RPn as min.
> > +         */
> > +        err = slpc_set_min_freq(slpc, slpc->min_freq);
> > +        if (err)
> > +            return err;
> >       }
> >   +    saved_min_freq = slpc_min_freq;
> > +
> > +    /* New temp min freq = RPn */
> > +    slpc_min_freq = slpc->min_freq;
> > +
> >       intel_gt_pm_wait_for_idle(gt);
> >       intel_gt_pm_get(gt);
> >       for_each_engine(engine, gt, id) {
> > @@ -347,7 +363,7 @@ static int run_test(struct intel_gt *gt, int 
> > test_type)
> >         /* Restore min/max frequencies */
> >       slpc_set_max_freq(slpc, slpc_max_freq);
> > -    slpc_set_min_freq(slpc, slpc_min_freq);
> > +    slpc_set_min_freq(slpc, saved_min_freq);
> >         if (igt_flush_test(gt->i915))
> >           err = -EIO;
> > 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 fdd895f73f9f..11613d373a49 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> > @@ -263,6 +263,7 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
> >         slpc->max_freq_softlimit = 0;
> >       slpc->min_freq_softlimit = 0;
> > +    slpc->min_is_rpmax = false;
> >         slpc->boost_freq = 0;
> >       atomic_set(&slpc->num_waiters, 0);
> > @@ -588,6 +589,31 @@ static int slpc_set_softlimits(struct 
> > intel_guc_slpc *slpc)
> >       return 0;
> >   }
> >   +static bool is_slpc_min_freq_rpmax(struct intel_guc_slpc *slpc)
> > +{
> > +    int slpc_min_freq;
> > +
> > +    if (intel_guc_slpc_get_min_freq(slpc, &slpc_min_freq))
> > +        return false;
> > +
> > +    if (slpc_min_freq > slpc->rp0_freq)
> > +        return true;
> > +    else
> > +        return false;
> > +}
> > +
> > +static void update_server_min_softlimit(struct intel_guc_slpc *slpc)
> > +{
> > +    /* For server parts, SLPC min will be at RPMax.
> > +     * Use min softlimit to clamp it to RP0 instead.
> > +     */
> > +    if (is_slpc_min_freq_rpmax(slpc) &&
> > +        !slpc->min_freq_softlimit) {
> > +        slpc->min_is_rpmax = true;
> > +        slpc->min_freq_softlimit = slpc->rp0_freq;
> > +    }
> > +}
> > +
> >   static int slpc_use_fused_rp0(struct intel_guc_slpc *slpc)
> >   {
> >       /* Force SLPC to used platform rp0 */
> > @@ -647,6 +673,9 @@ int intel_guc_slpc_enable(struct intel_guc_slpc 
> > *slpc)
> >         slpc_get_rp_values(slpc);
> >   +    /* Handle the case where min=max=RPmax */
> > +    update_server_min_softlimit(slpc);
> > +
> >       /* Set SLPC max limit to RP0 */
> >       ret = slpc_use_fused_rp0(slpc);
> >       if (unlikely(ret)) {
> > 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 73d208123528..a6ef53b04e04 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
> > @@ -19,6 +19,9 @@ struct intel_guc_slpc {
> >       bool supported;
> >       bool selected;
> >   +    /* Indicates this is a server part */
> > +    bool min_is_rpmax;
> > +
> >       /* platform frequency limits */
> >       u32 min_freq;
> >       u32 rp0_freq;