Re: [Intel-gfx] [PATCH] drm/i915: Remove CONFIG_PM dependency from RC6.
On Wed, Dec 07, 2022 at 02:35:37PM +, Paul Cercueil wrote:
> Hi Rodrigo,
>
> Le mercredi 30 novembre 2022 à 13:37 +, Vivi, Rodrigo a écrit :
> > On Wed, 2022-11-30 at 11:47 +, 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
> > > > Signed-off-by: Rodrigo Vivi
> > > > ---
> > > > 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)
> > > > {
> >
> >
>
Re: [Intel-gfx] [PATCH] drm/i915: Remove CONFIG_PM dependency from RC6.
Hi Rodrigo,
Le mercredi 30 novembre 2022 à 13:37 +, Vivi, Rodrigo a écrit :
> On Wed, 2022-11-30 at 11:47 +, 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
> > > Signed-off-by: Rodrigo Vivi
> > > ---
> > > 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)
> > > {
>
>
Re: [Intel-gfx] [PATCH] drm/i915: Remove CONFIG_PM dependency from RC6.
On Wed, 2022-11-30 at 11:47 +, 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
> > Signed-off-by: Rodrigo Vivi
> > ---
> > 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)
> > {
Re: [Intel-gfx] [PATCH] drm/i915: Remove CONFIG_PM dependency from RC6.
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
Signed-off-by: Rodrigo Vivi
---
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)
{
[Intel-gfx] [PATCH] drm/i915: Remove CONFIG_PM dependency from RC6.
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
Signed-off-by: Rodrigo Vivi
---
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)
{
--
2.38.1
