Re: [Intel-gfx] [PATCH] drm/i915: Remove CONFIG_PM dependency from RC6.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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)
> > > >   {
> > 
> > 
>