| Message ID | 20231030234527.2285179-1-vinay.belgaumkar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Initialize residency registers earlier | expand |
On Mon, 2023-10-30 at 16:45 -0700, Belgaumkar, Vinay wrote: alan:skip > +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c > @@ -608,11 +608,13 @@ void intel_rc6_init(struct intel_rc6 *rc6) > /* Disable runtime-pm until we can save the GPU state with rc6 pctx */ > rpm_get(rc6); > > - if (!rc6_supported(rc6)) > - return; > - > rc6_res_reg_init(rc6); > > + if (!rc6_supported(rc6)) { > + rpm_put(rc6); > + return; > + } > + > if (IS_CHERRYVIEW(i915)) > err = chv_rc6_init(rc6); > else if (IS_VALLEYVIEW(i915)) alan: as far as the bug this patch is addressing (i.e. ensuring that intel_rc6_print_residency has valid rc6.res_reg values for correct dprc debugfs output when rc6 is disabled) and release the rpm, this looks good to me. However, when looking at the other code flows around the intel_rc6_init/fini and intel_rc6_enable/disable, i must point out that the calls to rpm_get and rpm_put from these functions don't seem to be designed with proper mirror-ing. For example during driver startup, intel_rc6_init (which is called by intel_gt_pm_init) calls rpm_get at the start but doesn't call rpm_put before it returns. But back up the callstack in intel_gt_init, after intel_gt_pm_init, a couple of subsystems get intialized before intel_gt_resume is called - which in turn calls intel_rc6_enable which does the rpm_put at its end. However before that get and put, i see several error paths that trigger cleanups (leading eventually to driver load failure), but i think some cases are completely missing the put_rpm that intel_rc6_init took. Additionally, the function names of rc6_init and __get_rc6 inside i915_pmu.c seems to be confusing although static. I wish those were named pmu_rc6_init and __pmu_rc6_init and etc. Anyways, as per offline conversation, we are not trying to solve every bug and design gap in this patch but just one specific bug fix. So as per the agreed condition that we create a separate internal issue to address this "lack of a clean mirrored-function design of rpm_get/put across the rc6 startup sequences", here is my rb: Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c index 7090e4be29cb..58dc0dab9b64 100644 --- a/drivers/gpu/drm/i915/gt/intel_rc6.c +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c @@ -608,11 +608,13 @@ void intel_rc6_init(struct intel_rc6 *rc6) /* Disable runtime-pm until we can save the GPU state with rc6 pctx */ rpm_get(rc6); - if (!rc6_supported(rc6)) - return; - rc6_res_reg_init(rc6); + if (!rc6_supported(rc6)) { + rpm_put(rc6); + return; + } + if (IS_CHERRYVIEW(i915)) err = chv_rc6_init(rc6); else if (IS_VALLEYVIEW(i915))
If we skip RC6 init, residency registers do not get initialized, leading to incorrect drpc debug output. Also release the wakeref since we skip intel_rc6_enable() entirely when rc6_supported is false. Fixes: 78d0b4552c37 ("drm/i915/gt: Use RC6 residency types as arguments to residency functions") Suggested-by: Alan Previn <alan.previn.teres.alexis@intel.com> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> --- drivers/gpu/drm/i915/gt/intel_rc6.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)