| Message ID | 20221130022928.196982-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Remove CONFIG_PM dependency from RC6. | expand |
On 30/11/2022 02:29, Rodrigo Vivi wrote: > RC6 is a sleep state that doesn't depend on the cpu sleep, > or any of the APM or ACPI or anything related to the > CONFIG_PM. > > A long time ago we have removed the module parameter > that allows the RC6 disablement. We want that feature enabled > everywhere. However, for some reason this CONFIG_PM was long > forgotten behind. > > If we end up needing knobs to disable RC6 we should create > individual ones, rather than relying on this master one. Digging in history shows 5ab3633d6907 ("drm/i915: make rc6 in sysfs functions conditional") and then it appears the issue could still be present, since we still use power_group_name which is NULL when !CONFIG_PM. $ ls -l /sys/class/drm/card0/power/ total 0 -rw-r--r-- 1 root root 4096 Nov 30 11:45 async -rw-r--r-- 1 root root 4096 Nov 30 11:45 autosuspend_delay_ms -rw-r--r-- 1 root root 4096 Nov 30 11:45 control -r--r--r-- 1 root root 4096 Nov 30 11:45 rc6_enable -r--r--r-- 1 root root 4096 Nov 30 11:45 rc6_residency_ms -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_active_kids -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_active_time -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_enabled -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_status -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_suspended_time -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_usage Other than rc6 entries I guess come from somewhere else but I haven't looked from where exactly. Regards, Tvrtko > Cc: Paul Cercueil <paul@crapouillou.net> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > index cf71305ad586..77327ede18ad 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > @@ -164,7 +164,6 @@ sysfs_gt_attribute_r_func(struct kobject *kobj, struct attribute *attr, > NULL); \ > INTEL_GT_ATTR_RO(_name) > > -#ifdef CONFIG_PM > static u32 get_residency(struct intel_gt *gt, enum intel_rc6_res_type id) > { > intel_wakeref_t wakeref; > @@ -329,11 +328,6 @@ static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) > gt->info.id, ERR_PTR(ret)); > } > } > -#else > -static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) > -{ > -} > -#endif /* CONFIG_PM */ > > static u32 __act_freq_mhz_show(struct intel_gt *gt) > {
On Wed, 2022-11-30 at 11:47 +0000, Tvrtko Ursulin wrote: > > On 30/11/2022 02:29, Rodrigo Vivi wrote: > > RC6 is a sleep state that doesn't depend on the cpu sleep, > > or any of the APM or ACPI or anything related to the > > CONFIG_PM. > > > > A long time ago we have removed the module parameter > > that allows the RC6 disablement. We want that feature enabled > > everywhere. However, for some reason this CONFIG_PM was long > > forgotten behind. > > > > If we end up needing knobs to disable RC6 we should create > > individual ones, rather than relying on this master one. > > Digging in history shows 5ab3633d6907 ("drm/i915: make rc6 in sysfs > functions conditional") and then it appears the issue could still be > present, since we still use power_group_name which is NULL when > !CONFIG_PM. oh, indeed! So, we should probably go with Paul's proposal: https://lists.freedesktop.org/archives/intel-gfx/2022-November/311569.html > > $ ls -l /sys/class/drm/card0/power/ > total 0 > -rw-r--r-- 1 root root 4096 Nov 30 11:45 async > -rw-r--r-- 1 root root 4096 Nov 30 11:45 autosuspend_delay_ms > -rw-r--r-- 1 root root 4096 Nov 30 11:45 control > -r--r--r-- 1 root root 4096 Nov 30 11:45 rc6_enable > -r--r--r-- 1 root root 4096 Nov 30 11:45 rc6_residency_ms > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_active_kids > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_active_time > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_enabled > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_status > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_suspended_time > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_usage > > Other than rc6 entries I guess come from somewhere else but I haven't > looked from where exactly. Ouch! Everything else here comes from the pci's pm_runtime. Probably our bad decision was to add rc6 to it. But we do need to stick to that. > > Regards, > > Tvrtko > > > Cc: Paul Cercueil <paul@crapouillou.net> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 6 ------ > > 1 file changed, 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > index cf71305ad586..77327ede18ad 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > @@ -164,7 +164,6 @@ sysfs_gt_attribute_r_func(struct kobject *kobj, > > struct attribute *attr, > > > > NULL); \ > > INTEL_GT_ATTR_RO(_name) > > > > -#ifdef CONFIG_PM > > static u32 get_residency(struct intel_gt *gt, enum > > intel_rc6_res_type id) > > { > > intel_wakeref_t wakeref; > > @@ -329,11 +328,6 @@ static void intel_sysfs_rc6_init(struct > > intel_gt *gt, struct kobject *kobj) > > gt->info.id, ERR_PTR(ret)); > > } > > } > > -#else > > -static void intel_sysfs_rc6_init(struct intel_gt *gt, struct > > kobject *kobj) > > -{ > > -} > > -#endif /* CONFIG_PM */ > > > > static u32 __act_freq_mhz_show(struct intel_gt *gt) > > {
Hi Rodrigo, Le mercredi 30 novembre 2022 à 13:37 +0000, Vivi, Rodrigo a écrit : > On Wed, 2022-11-30 at 11:47 +0000, Tvrtko Ursulin wrote: > > > > On 30/11/2022 02:29, Rodrigo Vivi wrote: > > > RC6 is a sleep state that doesn't depend on the cpu sleep, > > > or any of the APM or ACPI or anything related to the > > > CONFIG_PM. > > > > > > A long time ago we have removed the module parameter > > > that allows the RC6 disablement. We want that feature enabled > > > everywhere. However, for some reason this CONFIG_PM was long > > > forgotten behind. > > > > > > If we end up needing knobs to disable RC6 we should create > > > individual ones, rather than relying on this master one. > > > > Digging in history shows 5ab3633d6907 ("drm/i915: make rc6 in sysfs > > functions conditional") and then it appears the issue could still > > be > > present, since we still use power_group_name which is NULL when > > !CONFIG_PM. > > oh, indeed! > So, we should probably go with Paul's proposal: > https://lists.freedesktop.org/archives/intel-gfx/2022-November/311569.html Could you ack it then? Or is there something to change? Cheers, -Paul > > > > $ ls -l /sys/class/drm/card0/power/ > > total 0 > > -rw-r--r-- 1 root root 4096 Nov 30 11:45 async > > -rw-r--r-- 1 root root 4096 Nov 30 11:45 autosuspend_delay_ms > > -rw-r--r-- 1 root root 4096 Nov 30 11:45 control > > -r--r--r-- 1 root root 4096 Nov 30 11:45 rc6_enable > > -r--r--r-- 1 root root 4096 Nov 30 11:45 rc6_residency_ms > > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_active_kids > > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_active_time > > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_enabled > > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_status > > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_suspended_time > > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_usage > > > > Other than rc6 entries I guess come from somewhere else but I > > haven't > > looked from where exactly. > > > Ouch! Everything else here comes from the pci's pm_runtime. > Probably our bad decision was to add rc6 to it. > But we do need to stick to that. > > > > > > Regards, > > > > Tvrtko > > > > > Cc: Paul Cercueil <paul@crapouillou.net> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 6 ------ > > > 1 file changed, 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > > b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > > index cf71305ad586..77327ede18ad 100644 > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > > @@ -164,7 +164,6 @@ sysfs_gt_attribute_r_func(struct kobject > > > *kobj, > > > struct attribute *attr, > > > > > > NULL); \ > > > INTEL_GT_ATTR_RO(_name) > > > > > > -#ifdef CONFIG_PM > > > static u32 get_residency(struct intel_gt *gt, enum > > > intel_rc6_res_type id) > > > { > > > intel_wakeref_t wakeref; > > > @@ -329,11 +328,6 @@ static void intel_sysfs_rc6_init(struct > > > intel_gt *gt, struct kobject *kobj) > > > gt->info.id, ERR_PTR(ret)); > > > } > > > } > > > -#else > > > -static void intel_sysfs_rc6_init(struct intel_gt *gt, struct > > > kobject *kobj) > > > -{ > > > -} > > > -#endif /* CONFIG_PM */ > > > > > > static u32 __act_freq_mhz_show(struct intel_gt *gt) > > > { > >
On Wed, Dec 07, 2022 at 02:35:37PM +0000, Paul Cercueil wrote: > Hi Rodrigo, > > Le mercredi 30 novembre 2022 à 13:37 +0000, Vivi, Rodrigo a écrit : > > On Wed, 2022-11-30 at 11:47 +0000, Tvrtko Ursulin wrote: > > > > > > On 30/11/2022 02:29, Rodrigo Vivi wrote: > > > > RC6 is a sleep state that doesn't depend on the cpu sleep, > > > > or any of the APM or ACPI or anything related to the > > > > CONFIG_PM. > > > > > > > > A long time ago we have removed the module parameter > > > > that allows the RC6 disablement. We want that feature enabled > > > > everywhere. However, for some reason this CONFIG_PM was long > > > > forgotten behind. > > > > > > > > If we end up needing knobs to disable RC6 we should create > > > > individual ones, rather than relying on this master one. > > > > > > Digging in history shows 5ab3633d6907 ("drm/i915: make rc6 in sysfs > > > functions conditional") and then it appears the issue could still > > > be > > > present, since we still use power_group_name which is NULL when > > > !CONFIG_PM. > > > > oh, indeed! > > So, we should probably go with Paul's proposal: > > https://lists.freedesktop.org/archives/intel-gfx/2022-November/311569.html > > Could you ack it then? Or is there something to change? I had just added my rv-b to your patch there. Also, feel free to use my acked-by to merge through drm-misc or any other branch. We will catch this up later to our drm-intel branches with some backmerge. Thanks for the clean up. > > Cheers, > -Paul > > > > > > > $ ls -l /sys/class/drm/card0/power/ > > > total 0 > > > -rw-r--r-- 1 root root 4096 Nov 30 11:45 async > > > -rw-r--r-- 1 root root 4096 Nov 30 11:45 autosuspend_delay_ms > > > -rw-r--r-- 1 root root 4096 Nov 30 11:45 control > > > -r--r--r-- 1 root root 4096 Nov 30 11:45 rc6_enable > > > -r--r--r-- 1 root root 4096 Nov 30 11:45 rc6_residency_ms > > > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_active_kids > > > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_active_time > > > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_enabled > > > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_status > > > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_suspended_time > > > -r--r--r-- 1 root root 4096 Nov 30 11:45 runtime_usage > > > > > > Other than rc6 entries I guess come from somewhere else but I > > > haven't > > > looked from where exactly. > > > > > > Ouch! Everything else here comes from the pci's pm_runtime. > > Probably our bad decision was to add rc6 to it. > > But we do need to stick to that. > > > > > > > > > > Regards, > > > > > > Tvrtko > > > > > > > Cc: Paul Cercueil <paul@crapouillou.net> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 6 ------ > > > > 1 file changed, 6 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > > > b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > > > index cf71305ad586..77327ede18ad 100644 > > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > > > @@ -164,7 +164,6 @@ sysfs_gt_attribute_r_func(struct kobject > > > > *kobj, > > > > struct attribute *attr, > > > > > > > > NULL); \ > > > > INTEL_GT_ATTR_RO(_name) > > > > > > > > -#ifdef CONFIG_PM > > > > static u32 get_residency(struct intel_gt *gt, enum > > > > intel_rc6_res_type id) > > > > { > > > > intel_wakeref_t wakeref; > > > > @@ -329,11 +328,6 @@ static void intel_sysfs_rc6_init(struct > > > > intel_gt *gt, struct kobject *kobj) > > > > gt->info.id, ERR_PTR(ret)); > > > > } > > > > } > > > > -#else > > > > -static void intel_sysfs_rc6_init(struct intel_gt *gt, struct > > > > kobject *kobj) > > > > -{ > > > > -} > > > > -#endif /* CONFIG_PM */ > > > > > > > > static u32 __act_freq_mhz_show(struct intel_gt *gt) > > > > { > > > > >
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c index cf71305ad586..77327ede18ad 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -164,7 +164,6 @@ sysfs_gt_attribute_r_func(struct kobject *kobj, struct attribute *attr, NULL); \ INTEL_GT_ATTR_RO(_name) -#ifdef CONFIG_PM static u32 get_residency(struct intel_gt *gt, enum intel_rc6_res_type id) { intel_wakeref_t wakeref; @@ -329,11 +328,6 @@ static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) gt->info.id, ERR_PTR(ret)); } } -#else -static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) -{ -} -#endif /* CONFIG_PM */ static u32 __act_freq_mhz_show(struct intel_gt *gt) {
RC6 is a sleep state that doesn't depend on the cpu sleep, or any of the APM or ACPI or anything related to the CONFIG_PM. A long time ago we have removed the module parameter that allows the RC6 disablement. We want that feature enabled everywhere. However, for some reason this CONFIG_PM was long forgotten behind. If we end up needing knobs to disable RC6 we should create individual ones, rather than relying on this master one. Cc: Paul Cercueil <paul@crapouillou.net> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 6 ------ 1 file changed, 6 deletions(-)