| Message ID | 20230721180349.1737284-1-vinay.belgaumkar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc/slpc: Restore efficient freq earlier | expand |
On Fri, Jul 21, 2023 at 11:03:49AM -0700, Vinay Belgaumkar wrote: > This should be done before the soft min/max frequencies are restored. > When we disable the "Ignore efficient frequency" flag, GuC does not > actually bring the requested freq down to RPn. > > Specifically, this scenario- > > - ignore efficient freq set to true > - reduce min to RPn (from efficient) > - suspend > - resume (includes GuC load, restore soft min/max, restore efficient freq) > - validate min freq has been resored to RPn > > This will fail if we didn't first restore(disable, in this case) efficient > freq flag before setting the soft min frequency. that's strange. so guc is returning the rpe when we request the min freq during the soft config? we could alternatively change the soft config to actually get the min and not be tricked by this. But also the patch below doesn't hurt. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> (Although I'm still curious and want to understand exactly why the soft min gets messed up when we don't tell guc to ignore the efficient freq beforehand. Please help me to understand.) > > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8736 > Fixes: 55f9720dbf23 ("drm/i915/guc/slpc: Provide sysfs for efficient freq") > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 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 ee9f83af7cf6..f16dff7c3185 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > @@ -743,6 +743,9 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) > > intel_guc_pm_intrmsk_enable(slpc_to_gt(slpc)); > > + /* Set cached value of ignore efficient freq */ > + intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); > + > slpc_get_rp_values(slpc); > > /* Handle the case where min=max=RPmax */ > @@ -765,9 +768,6 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) > /* Set cached media freq ratio mode */ > intel_guc_slpc_set_media_ratio_mode(slpc, slpc->media_ratio_mode); > > - /* Set cached value of ignore efficient freq */ > - intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); > - > return 0; > } > > -- > 2.38.1 >
On 7/21/2023 1:41 PM, Rodrigo Vivi wrote: > On Fri, Jul 21, 2023 at 11:03:49AM -0700, Vinay Belgaumkar wrote: >> This should be done before the soft min/max frequencies are restored. >> When we disable the "Ignore efficient frequency" flag, GuC does not >> actually bring the requested freq down to RPn. >> >> Specifically, this scenario- >> >> - ignore efficient freq set to true >> - reduce min to RPn (from efficient) >> - suspend >> - resume (includes GuC load, restore soft min/max, restore efficient freq) >> - validate min freq has been resored to RPn >> >> This will fail if we didn't first restore(disable, in this case) efficient >> freq flag before setting the soft min frequency. > that's strange. so guc is returning the rpe when we request the min freq > during the soft config? > > we could alternatively change the soft config to actually get the min > and not be tricked by this. > > But also the patch below doesn't hurt. > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > (Although I'm still curious and want to understand exactly why > the soft min gets messed up when we don't tell guc to ignore the > efficient freq beforehand. Please help me to understand.) The soft min does not get messed up, but GuC keeps requesting RPe even after disabling efficient freq. (unless we manually set min freq to RPn AFTER disabling efficient). Thanks, Vinay. > > >> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8736 >> Fixes: 55f9720dbf23 ("drm/i915/guc/slpc: Provide sysfs for efficient freq") >> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 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 ee9f83af7cf6..f16dff7c3185 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c >> @@ -743,6 +743,9 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) >> >> intel_guc_pm_intrmsk_enable(slpc_to_gt(slpc)); >> >> + /* Set cached value of ignore efficient freq */ >> + intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); >> + >> slpc_get_rp_values(slpc); >> >> /* Handle the case where min=max=RPmax */ >> @@ -765,9 +768,6 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) >> /* Set cached media freq ratio mode */ >> intel_guc_slpc_set_media_ratio_mode(slpc, slpc->media_ratio_mode); >> >> - /* Set cached value of ignore efficient freq */ >> - intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); >> - >> return 0; >> } >> >> -- >> 2.38.1 >>
On Fri, Jul 21, 2023 at 01:44:34PM -0700, Belgaumkar, Vinay wrote: > > On 7/21/2023 1:41 PM, Rodrigo Vivi wrote: > > On Fri, Jul 21, 2023 at 11:03:49AM -0700, Vinay Belgaumkar wrote: > > > This should be done before the soft min/max frequencies are restored. > > > When we disable the "Ignore efficient frequency" flag, GuC does not > > > actually bring the requested freq down to RPn. > > > > > > Specifically, this scenario- > > > > > > - ignore efficient freq set to true > > > - reduce min to RPn (from efficient) > > > - suspend > > > - resume (includes GuC load, restore soft min/max, restore efficient freq) > > > - validate min freq has been resored to RPn > > > > > > This will fail if we didn't first restore(disable, in this case) efficient > > > freq flag before setting the soft min frequency. > > that's strange. so guc is returning the rpe when we request the min freq > > during the soft config? > > > > we could alternatively change the soft config to actually get the min > > and not be tricked by this. > > > > But also the patch below doesn't hurt. > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > (Although I'm still curious and want to understand exactly why > > the soft min gets messed up when we don't tell guc to ignore the > > efficient freq beforehand. Please help me to understand.) > > The soft min does not get messed up, but GuC keeps requesting RPe even after > disabling efficient freq. (unless we manually set min freq to RPn AFTER > disabling efficient). so it looks to me that the right solution would be to ensure that everytime that we disable the efficient freq we make sure to also set the mim freq to RPn, no?! > > Thanks, > > Vinay. > > > > > > > > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8736 > > > Fixes: 55f9720dbf23 ("drm/i915/guc/slpc: Provide sysfs for efficient freq") > > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > --- > > > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 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 ee9f83af7cf6..f16dff7c3185 100644 > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > > > @@ -743,6 +743,9 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) > > > intel_guc_pm_intrmsk_enable(slpc_to_gt(slpc)); > > > + /* Set cached value of ignore efficient freq */ > > > + intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); > > > + > > > slpc_get_rp_values(slpc); > > > /* Handle the case where min=max=RPmax */ > > > @@ -765,9 +768,6 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) > > > /* Set cached media freq ratio mode */ > > > intel_guc_slpc_set_media_ratio_mode(slpc, slpc->media_ratio_mode); > > > - /* Set cached value of ignore efficient freq */ > > > - intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); > > > - > > > return 0; > > > } > > > -- > > > 2.38.1 > > >
On 7/21/2023 2:23 PM, Rodrigo Vivi wrote: > On Fri, Jul 21, 2023 at 01:44:34PM -0700, Belgaumkar, Vinay wrote: >> On 7/21/2023 1:41 PM, Rodrigo Vivi wrote: >>> On Fri, Jul 21, 2023 at 11:03:49AM -0700, Vinay Belgaumkar wrote: >>>> This should be done before the soft min/max frequencies are restored. >>>> When we disable the "Ignore efficient frequency" flag, GuC does not >>>> actually bring the requested freq down to RPn. >>>> >>>> Specifically, this scenario- >>>> >>>> - ignore efficient freq set to true >>>> - reduce min to RPn (from efficient) >>>> - suspend >>>> - resume (includes GuC load, restore soft min/max, restore efficient freq) >>>> - validate min freq has been resored to RPn >>>> >>>> This will fail if we didn't first restore(disable, in this case) efficient >>>> freq flag before setting the soft min frequency. >>> that's strange. so guc is returning the rpe when we request the min freq >>> during the soft config? >>> >>> we could alternatively change the soft config to actually get the min >>> and not be tricked by this. >>> >>> But also the patch below doesn't hurt. >>> >>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> (Although I'm still curious and want to understand exactly why >>> the soft min gets messed up when we don't tell guc to ignore the >>> efficient freq beforehand. Please help me to understand.) >> The soft min does not get messed up, but GuC keeps requesting RPe even after >> disabling efficient freq. (unless we manually set min freq to RPn AFTER >> disabling efficient). > so it looks to me that the right solution would be to ensure that everytime > that we disable the efficient freq we make sure to also set the mim freq to RPn, > no?! Hmm, may not be applicable every time. What if someone disables efficient frequency while running a workload or with frequency fixed to 800, for example? Thanks, Vinay. > >> Thanks, >> >> Vinay. >> >>> >>>> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8736 >>>> Fixes: 55f9720dbf23 ("drm/i915/guc/slpc: Provide sysfs for efficient freq") >>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 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 ee9f83af7cf6..f16dff7c3185 100644 >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c >>>> @@ -743,6 +743,9 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) >>>> intel_guc_pm_intrmsk_enable(slpc_to_gt(slpc)); >>>> + /* Set cached value of ignore efficient freq */ >>>> + intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); >>>> + >>>> slpc_get_rp_values(slpc); >>>> /* Handle the case where min=max=RPmax */ >>>> @@ -765,9 +768,6 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) >>>> /* Set cached media freq ratio mode */ >>>> intel_guc_slpc_set_media_ratio_mode(slpc, slpc->media_ratio_mode); >>>> - /* Set cached value of ignore efficient freq */ >>>> - intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); >>>> - >>>> return 0; >>>> } >>>> -- >>>> 2.38.1 >>>>
On 7/21/2023 3:08 PM, Belgaumkar, Vinay wrote: > > On 7/21/2023 2:23 PM, Rodrigo Vivi wrote: >> On Fri, Jul 21, 2023 at 01:44:34PM -0700, Belgaumkar, Vinay wrote: >>> On 7/21/2023 1:41 PM, Rodrigo Vivi wrote: >>>> On Fri, Jul 21, 2023 at 11:03:49AM -0700, Vinay Belgaumkar wrote: >>>>> This should be done before the soft min/max frequencies are restored. >>>>> When we disable the "Ignore efficient frequency" flag, GuC does not >>>>> actually bring the requested freq down to RPn. >>>>> >>>>> Specifically, this scenario- >>>>> >>>>> - ignore efficient freq set to true >>>>> - reduce min to RPn (from efficient) >>>>> - suspend >>>>> - resume (includes GuC load, restore soft min/max, restore >>>>> efficient freq) >>>>> - validate min freq has been resored to RPn >>>>> >>>>> This will fail if we didn't first restore(disable, in this case) >>>>> efficient >>>>> freq flag before setting the soft min frequency. >>>> that's strange. so guc is returning the rpe when we request the min >>>> freq >>>> during the soft config? >>>> >>>> we could alternatively change the soft config to actually get the min >>>> and not be tricked by this. >>>> >>>> But also the patch below doesn't hurt. >>>> >>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>> (Although I'm still curious and want to understand exactly why >>>> the soft min gets messed up when we don't tell guc to ignore the >>>> efficient freq beforehand. Please help me to understand.) >>> The soft min does not get messed up, but GuC keeps requesting RPe >>> even after >>> disabling efficient freq. (unless we manually set min freq to RPn AFTER >>> disabling efficient). >> so it looks to me that the right solution would be to ensure that >> everytime >> that we disable the efficient freq we make sure to also set the mim >> freq to RPn, >> no?! > > Hmm, may not be applicable every time. What if someone disables > efficient frequency while running a workload or with frequency fixed > to 800, for example? I'll take that back, it should not matter. GuC will not change it's request just because we switched min lower. I will resend the patch with the min setting as well. Thanks, Vinay. > > Thanks, > > Vinay. > >> >>> Thanks, >>> >>> Vinay. >>> >>>> >>>>> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8736 >>>>> Fixes: 55f9720dbf23 ("drm/i915/guc/slpc: Provide sysfs for >>>>> efficient freq") >>>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 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 ee9f83af7cf6..f16dff7c3185 100644 >>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c >>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c >>>>> @@ -743,6 +743,9 @@ int intel_guc_slpc_enable(struct >>>>> intel_guc_slpc *slpc) >>>>> intel_guc_pm_intrmsk_enable(slpc_to_gt(slpc)); >>>>> + /* Set cached value of ignore efficient freq */ >>>>> + intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); >>>>> + >>>>> slpc_get_rp_values(slpc); >>>>> /* Handle the case where min=max=RPmax */ >>>>> @@ -765,9 +768,6 @@ int intel_guc_slpc_enable(struct >>>>> intel_guc_slpc *slpc) >>>>> /* Set cached media freq ratio mode */ >>>>> intel_guc_slpc_set_media_ratio_mode(slpc, >>>>> slpc->media_ratio_mode); >>>>> - /* Set cached value of ignore efficient freq */ >>>>> - intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); >>>>> - >>>>> return 0; >>>>> } >>>>> -- >>>>> 2.38.1 >>>>>
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 ee9f83af7cf6..f16dff7c3185 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -743,6 +743,9 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) intel_guc_pm_intrmsk_enable(slpc_to_gt(slpc)); + /* Set cached value of ignore efficient freq */ + intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); + slpc_get_rp_values(slpc); /* Handle the case where min=max=RPmax */ @@ -765,9 +768,6 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) /* Set cached media freq ratio mode */ intel_guc_slpc_set_media_ratio_mode(slpc, slpc->media_ratio_mode); - /* Set cached value of ignore efficient freq */ - intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); - return 0; }
This should be done before the soft min/max frequencies are restored. When we disable the "Ignore efficient frequency" flag, GuC does not actually bring the requested freq down to RPn. Specifically, this scenario- - ignore efficient freq set to true - reduce min to RPn (from efficient) - suspend - resume (includes GuC load, restore soft min/max, restore efficient freq) - validate min freq has been resored to RPn This will fail if we didn't first restore(disable, in this case) efficient freq flag before setting the soft min frequency. Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8736 Fixes: 55f9720dbf23 ("drm/i915/guc/slpc: Provide sysfs for efficient freq") Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)