| Message ID | 20230609220252.410740-1-vinay.belgaumkar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc/slpc: Apply min softlimit correctly | expand |
On Fri, 09 Jun 2023 15:02:52 -0700, Vinay Belgaumkar wrote: > Hi Vinay, > We were skipping when min_softlimit was equal to RPn. We need to apply > it rergardless as efficient frequency will push the SLPC min to RPe. regardless > This will break scenarios where user sets a min softlimit < RPe before > reset and then performs a GT reset. Can you explain the reason for the patch clearly in terms of variables in the code, what variable has what value and what is the bug. I am not following from the above description. Thanks. -- Ashutosh > > Fixes: 95ccf312a1e4 ("drm/i915/guc/slpc: Allow SLPC to use efficient frequency") > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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 01b75529311c..ee9f83af7cf6 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > @@ -606,7 +606,7 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc) > if (unlikely(ret)) > return ret; > slpc_to_gt(slpc)->defaults.min_freq = slpc->min_freq_softlimit; > - } else if (slpc->min_freq_softlimit != slpc->min_freq) { > + } else { > return intel_guc_slpc_set_min_freq(slpc, > slpc->min_freq_softlimit); > } > -- > 2.38.1 >
On 6/13/2023 7:25 PM, Dixit, Ashutosh wrote: > On Fri, 09 Jun 2023 15:02:52 -0700, Vinay Belgaumkar wrote: > Hi Vinay, > >> We were skipping when min_softlimit was equal to RPn. We need to apply >> it rergardless as efficient frequency will push the SLPC min to RPe. > regardless > >> This will break scenarios where user sets a min softlimit < RPe before >> reset and then performs a GT reset. > Can you explain the reason for the patch clearly in terms of variables in > the code, what variable has what value and what is the bug. I am not > following from the above description. Hi Ashutosh, Scenario being fixed here is exactly the one in i915_pm_freq_api reset/suspend subtests (currently in review). Test sets min freq to RPn and then performs a reset. It then checks if cur_freq is RPn. Here's the sequence that shows the problem- RPLS:/home/gta# modprobe i915 RPLS:/home/gta# echo 1 > /sys/class/drm/card0/gt/gt0/slpc_ignore_eff_freq RPLS:/home/gta# echo 300 > /sys/class/drm/card0/gt_min_freq_mhz (RPn) RPLS:/home/gta# cat /sys/class/drm/card0/gt_cur_freq_mhz --> cur == RPn as expected 300 RPLS:/home/gta# echo 1 > /sys/kernel/debug/dri/0/gt0/reset --> reset RPLS:/home/gta# cat /sys/class/drm/card0/gt_min_freq_mhz --> shows the internal cached variable correctly 300 RPLS:/home/gta# cat /sys/class/drm/card0/gt_cur_freq_mhz --> actual freq being requested by SLPC (it's not RPn!!) 700 We need to sync up driver min freq value and SLPC min after a reset/suspend. Currently, we skip if the user had manually set min to RPn (this was an optimization we had before we enabled efficient freq usage). Thanks, Vinay. > > Thanks. > -- > Ashutosh > > >> Fixes: 95ccf312a1e4 ("drm/i915/guc/slpc: Allow SLPC to use efficient frequency") >> >> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> 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 01b75529311c..ee9f83af7cf6 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c >> @@ -606,7 +606,7 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc) >> if (unlikely(ret)) >> return ret; >> slpc_to_gt(slpc)->defaults.min_freq = slpc->min_freq_softlimit; >> - } else if (slpc->min_freq_softlimit != slpc->min_freq) { >> + } else { >> return intel_guc_slpc_set_min_freq(slpc, >> slpc->min_freq_softlimit); >> } >> -- >> 2.38.1 >>
On Fri, 09 Jun 2023 15:02:52 -0700, Vinay Belgaumkar wrote: > Hi Vinay, > We were skipping when min_softlimit was equal to RPn. We need to apply > it rergardless as efficient frequency will push the SLPC min to RPe. > This will break scenarios where user sets a min softlimit < RPe before > reset and then performs a GT reset. > > Fixes: 95ccf312a1e4 ("drm/i915/guc/slpc: Allow SLPC to use efficient frequency") > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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 01b75529311c..ee9f83af7cf6 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > @@ -606,7 +606,7 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc) > if (unlikely(ret)) > return ret; > slpc_to_gt(slpc)->defaults.min_freq = slpc->min_freq_softlimit; > - } else if (slpc->min_freq_softlimit != slpc->min_freq) { > + } else { > return intel_guc_slpc_set_min_freq(slpc, > slpc->min_freq_softlimit); IMO the current code is unnecessarily complicated and confusing and similar changes (with a little tweaking) should be made for max_freq too. But at least this is a step in the right direction so: Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > } > -- > 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 01b75529311c..ee9f83af7cf6 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -606,7 +606,7 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc) if (unlikely(ret)) return ret; slpc_to_gt(slpc)->defaults.min_freq = slpc->min_freq_softlimit; - } else if (slpc->min_freq_softlimit != slpc->min_freq) { + } else { return intel_guc_slpc_set_min_freq(slpc, slpc->min_freq_softlimit); }
We were skipping when min_softlimit was equal to RPn. We need to apply it rergardless as efficient frequency will push the SLPC min to RPe. This will break scenarios where user sets a min softlimit < RPe before reset and then performs a GT reset. Fixes: 95ccf312a1e4 ("drm/i915/guc/slpc: Allow SLPC to use efficient frequency") Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)