Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator
Hi, On Tue, 6 Sept 2022 at 10:06, Biju Das wrote: > > Hi Clement, > > > Subject: Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the > > recommended one to configure and enable regulator > > > > Hi Biju, > > > > On Tue, 6 Sept 2022 at 08:42, Biju Das > > wrote: > > > > > > Hi Clement, > > > > > > > > > > > Hi, > > > > > > > > On Mon, 5 Sept 2022 at 20:17, Biju Das > > > > wrote: > > > > > > > > > > Hi, > > > > > > > > > > Thanks for the patch. > > > > > > > > > > > Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the > > > > > > recommended one to configure and enable regulator > > > > > > > > > > > > devm_pm_opp_set_regulators() doesn't enable regulator, which > > > > > > make regulator framework switching it off during > > regulator_late_cleanup(). > > > > > > > > > > In that case, why not regulator_get()for Dynamic regulator(non > > > > > fixed regulator)?? > > > > > > > > Sorry I don't understand, what do you mean? > > > > > > Normally we need to turn on regulator and clock only when needed. > > > I am not sure with your new code, will make it always on and drains > > > the power unnecessarily and does it set lower opp or higher opp at the > > > start?? > > > > The code doesn't make it always on, it makes it how it should be at the > > recommended OPP which is the "start point". > > > > If the recommended OPP says to switch off the regulator then it will. > > > > > > > > Compared to the fixed regulator, you have voltage regulator to control > > > that is the difference between my environment and Your environment. > > > > > > I am not sure any other SoC is using voltage regulator?? > > > If yes, thenthere should be some bug or some difference in HW which is > > > giving different behaviour?? > > > > > > If you are the first one using voltage regulator with mali gpu, Then > > > Your implementation may be correct, as you have proper HW to check. > > > > The issue is that my regulator is not marked as "always-on", if no OPP is > > called before regulator_late_cleanup() then nobody sets the > > regulator_enable() and the regulator is switched off, which makes my > > board hang. > > Cool, From your testing looks like no one tested this feature with > mali GPU on mainline?? Or no one without always-on. Clement > > Cheers, > Biju > > > > > > Like Viresh recommends I will send an update with more details in the > > commit log. > > > > Regards, > > Clement > > > > > > > > > > > > > > > > > > > > > > > > > > > > Call dev_pm_opp_set_opp() with the recommend OPP in > > > > > > panfrost_devfreq_init() to enable the regulator and avoid any > > > > > > switch off by regulator_late_cleanup(). > > > > > > > > > > > > Suggested-by: Viresh Kumar > > > > > > Signed-off-by: Clément Péron > > > > > > --- > > > > > > drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > > > > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > > > > index 5110cd9b2425..67b242407156 100644 > > > > > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > > > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > > > > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct > > > > > > panfrost_device > > > > > > *pfdev) > > > > > > return PTR_ERR(opp); > > > > > > > > > > > > panfrost_devfreq_profile.initial_freq = cur_freq; > > > > > > + > > > > > > + /* Setup and enable regulator */ > > > > > > + ret = dev_pm_opp_set_opp(dev, opp); > > > > > > + if (ret) { > > > > > > + DRM_DEV_ERROR(dev, "Couldn't set recommended > > OPP\n"); > > > > > > + return ret; > > > > > > + } > > &
RE: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator
Hi Clement, > Subject: Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the > recommended one to configure and enable regulator > > Hi Biju, > > On Tue, 6 Sept 2022 at 08:42, Biju Das > wrote: > > > > Hi Clement, > > > > > > > > Hi, > > > > > > On Mon, 5 Sept 2022 at 20:17, Biju Das > > > wrote: > > > > > > > > Hi, > > > > > > > > Thanks for the patch. > > > > > > > > > Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the > > > > > recommended one to configure and enable regulator > > > > > > > > > > devm_pm_opp_set_regulators() doesn't enable regulator, which > > > > > make regulator framework switching it off during > regulator_late_cleanup(). > > > > > > > > In that case, why not regulator_get()for Dynamic regulator(non > > > > fixed regulator)?? > > > > > > Sorry I don't understand, what do you mean? > > > > Normally we need to turn on regulator and clock only when needed. > > I am not sure with your new code, will make it always on and drains > > the power unnecessarily and does it set lower opp or higher opp at the > > start?? > > The code doesn't make it always on, it makes it how it should be at the > recommended OPP which is the "start point". > > If the recommended OPP says to switch off the regulator then it will. > > > > > Compared to the fixed regulator, you have voltage regulator to control > > that is the difference between my environment and Your environment. > > > > I am not sure any other SoC is using voltage regulator?? > > If yes, thenthere should be some bug or some difference in HW which is > > giving different behaviour?? > > > > If you are the first one using voltage regulator with mali gpu, Then > > Your implementation may be correct, as you have proper HW to check. > > The issue is that my regulator is not marked as "always-on", if no OPP is > called before regulator_late_cleanup() then nobody sets the > regulator_enable() and the regulator is switched off, which makes my > board hang. Cool, From your testing looks like no one tested this feature with mali GPU on mainline?? Cheers, Biju > > Like Viresh recommends I will send an update with more details in the > commit log. > > Regards, > Clement > > > > > > > > > > > > > > > > > > > > > Call dev_pm_opp_set_opp() with the recommend OPP in > > > > > panfrost_devfreq_init() to enable the regulator and avoid any > > > > > switch off by regulator_late_cleanup(). > > > > > > > > > > Suggested-by: Viresh Kumar > > > > > Signed-off-by: Clément Péron > > > > > --- > > > > > drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > > > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > > > index 5110cd9b2425..67b242407156 100644 > > > > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > > > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct > > > > > panfrost_device > > > > > *pfdev) > > > > > return PTR_ERR(opp); > > > > > > > > > > panfrost_devfreq_profile.initial_freq = cur_freq; > > > > > + > > > > > + /* Setup and enable regulator */ > > > > > + ret = dev_pm_opp_set_opp(dev, opp); > > > > > + if (ret) { > > > > > + DRM_DEV_ERROR(dev, "Couldn't set recommended > OPP\n"); > > > > > + return ret; > > > > > + } > > > > > > > > > > > > FYI, > > > > On RZ/G2L mali gpu, we have fixed regulator and I was able to do > > > > GPU OPP transition without any issues previously. > > > > > > rzg2l-smarc-som.dtsi uses regulator reg_1p1v; which is marked as > > > regulator-always-on; that's why > > > regulator_late_cleanup() doesn't switch it off. > > > > Yes that is correct. It is fixed regulator and always on. > > We control only frequency. > > > > Cheers, > > Biju > > > > > >
Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator
Hi Biju, On Tue, 6 Sept 2022 at 08:42, Biju Das wrote: > > Hi Clement, > > > > > Hi, > > > > On Mon, 5 Sept 2022 at 20:17, Biju Das > > wrote: > > > > > > Hi, > > > > > > Thanks for the patch. > > > > > > > Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the > > > > recommended one to configure and enable regulator > > > > > > > > devm_pm_opp_set_regulators() doesn't enable regulator, which make > > > > regulator framework switching it off during regulator_late_cleanup(). > > > > > > In that case, why not regulator_get()for Dynamic regulator(non fixed > > > regulator)?? > > > > Sorry I don't understand, what do you mean? > > Normally we need to turn on regulator and clock only when needed. > I am not sure with your new code, will make it always on and > drains the power unnecessarily and does it set lower opp or higher > opp at the start?? The code doesn't make it always on, it makes it how it should be at the recommended OPP which is the "start point". If the recommended OPP says to switch off the regulator then it will. > > Compared to the fixed regulator, you have voltage regulator to > control that is the difference between my environment and > Your environment. > > I am not sure any other SoC is using voltage regulator?? > If yes, thenthere should be some bug or some difference in HW > which is giving different behaviour?? > > If you are the first one using voltage regulator with mali gpu, > Then Your implementation may be correct, as you have proper > HW to check. The issue is that my regulator is not marked as "always-on", if no OPP is called before regulator_late_cleanup() then nobody sets the regulator_enable() and the regulator is switched off, which makes my board hang. Like Viresh recommends I will send an update with more details in the commit log. Regards, Clement > > > > > > > > > > > > > > Call dev_pm_opp_set_opp() with the recommend OPP in > > > > panfrost_devfreq_init() to enable the regulator and avoid any switch > > > > off by regulator_late_cleanup(). > > > > > > > > Suggested-by: Viresh Kumar > > > > Signed-off-by: Clément Péron > > > > --- > > > > drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > > index 5110cd9b2425..67b242407156 100644 > > > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct > > > > panfrost_device > > > > *pfdev) > > > > return PTR_ERR(opp); > > > > > > > > panfrost_devfreq_profile.initial_freq = cur_freq; > > > > + > > > > + /* Setup and enable regulator */ > > > > + ret = dev_pm_opp_set_opp(dev, opp); > > > > + if (ret) { > > > > + DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n"); > > > > + return ret; > > > > + } > > > > > > > > > FYI, > > > On RZ/G2L mali gpu, we have fixed regulator and I was able to do GPU > > > OPP transition without any issues previously. > > > > rzg2l-smarc-som.dtsi uses regulator reg_1p1v; which is marked as > > regulator-always-on; that's why > > regulator_late_cleanup() doesn't switch it off. > > Yes that is correct. It is fixed regulator and always on. > We control only frequency. > > Cheers, > Biju > > > > > > > > > root@smarc-rzg2l:~# cat /sys/class/devfreq/1184.gpu/trans_stat > > > From : To > > >: 5000 6250 1 12500 2 > > 25000 4 5 time(ms) > > > * 5000: 0 0 0 0 0 > > 0 0 1 144 > > >6250: 0 0 0 0 0 > > 0 0 0 0 > > > 1: 0 0 0 0 0 > > 0 0 9 524 > > > 12500: 0 0 9 0 0 > > 0 0 3 2544 > > > 2: 0 0 011 0 > > 0 046 3304 > > > 25000: 1 0 0 033 > > 0 0 0 7496 > > > 4: 0 0 0 016 > > 19 0 0 2024 > > > 5: 1 0 0 1 8 > > 1535 0 4032 > > > Total transition : 208 > > > > > > Cheers, > > > Biju > > >
RE: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator
Hi Clement, > > Hi, > > On Mon, 5 Sept 2022 at 20:17, Biju Das > wrote: > > > > Hi, > > > > Thanks for the patch. > > > > > Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the > > > recommended one to configure and enable regulator > > > > > > devm_pm_opp_set_regulators() doesn't enable regulator, which make > > > regulator framework switching it off during regulator_late_cleanup(). > > > > In that case, why not regulator_get()for Dynamic regulator(non fixed > > regulator)?? > > Sorry I don't understand, what do you mean? Normally we need to turn on regulator and clock only when needed. I am not sure with your new code, will make it always on and drains the power unnecessarily and does it set lower opp or higher opp at the start?? Compared to the fixed regulator, you have voltage regulator to control that is the difference between my environment and Your environment. I am not sure any other SoC is using voltage regulator?? If yes, thenthere should be some bug or some difference in HW which is giving different behaviour?? If you are the first one using voltage regulator with mali gpu, Then Your implementation may be correct, as you have proper HW to check. > > > > > > > > > Call dev_pm_opp_set_opp() with the recommend OPP in > > > panfrost_devfreq_init() to enable the regulator and avoid any switch > > > off by regulator_late_cleanup(). > > > > > > Suggested-by: Viresh Kumar > > > Signed-off-by: Clément Péron > > > --- > > > drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > index 5110cd9b2425..67b242407156 100644 > > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct > > > panfrost_device > > > *pfdev) > > > return PTR_ERR(opp); > > > > > > panfrost_devfreq_profile.initial_freq = cur_freq; > > > + > > > + /* Setup and enable regulator */ > > > + ret = dev_pm_opp_set_opp(dev, opp); > > > + if (ret) { > > > + DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n"); > > > + return ret; > > > + } > > > > > > FYI, > > On RZ/G2L mali gpu, we have fixed regulator and I was able to do GPU > > OPP transition without any issues previously. > > rzg2l-smarc-som.dtsi uses regulator reg_1p1v; which is marked as > regulator-always-on; that's why > regulator_late_cleanup() doesn't switch it off. Yes that is correct. It is fixed regulator and always on. We control only frequency. Cheers, Biju > > > > > root@smarc-rzg2l:~# cat /sys/class/devfreq/1184.gpu/trans_stat > > From : To > >: 5000 6250 1 12500 2 > 25000 4 5 time(ms) > > * 5000: 0 0 0 0 0 > 0 0 1 144 > >6250: 0 0 0 0 0 > 0 0 0 0 > > 1: 0 0 0 0 0 > 0 0 9 524 > > 12500: 0 0 9 0 0 > 0 0 3 2544 > > 2: 0 0 011 0 > 0 046 3304 > > 25000: 1 0 0 033 > 0 0 0 7496 > > 4: 0 0 0 016 > 19 0 0 2024 > > 5: 1 0 0 1 8 > 1535 0 4032 > > Total transition : 208 > > > > Cheers, > > Biju > >
Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator
Hi, On Mon, 5 Sept 2022 at 20:17, Biju Das wrote: > > Hi, > > Thanks for the patch. > > > Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended > > one to configure and enable regulator > > > > devm_pm_opp_set_regulators() doesn't enable regulator, which make > > regulator framework switching it off during regulator_late_cleanup(). > > In that case, why not regulator_get()for > Dynamic regulator(non fixed regulator)?? Sorry I don't understand, what do you mean? > > > > > Call dev_pm_opp_set_opp() with the recommend OPP in > > panfrost_devfreq_init() to enable the regulator and avoid any switch off > > by regulator_late_cleanup(). > > > > Suggested-by: Viresh Kumar > > Signed-off-by: Clément Péron > > --- > > drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > index 5110cd9b2425..67b242407156 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct panfrost_device > > *pfdev) > > return PTR_ERR(opp); > > > > panfrost_devfreq_profile.initial_freq = cur_freq; > > + > > + /* Setup and enable regulator */ > > + ret = dev_pm_opp_set_opp(dev, opp); > > + if (ret) { > > + DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n"); > > + return ret; > > + } > > > FYI, > On RZ/G2L mali gpu, we have fixed regulator and > I was able to do GPU OPP transition without any issues previously. rzg2l-smarc-som.dtsi uses regulator reg_1p1v; which is marked as regulator-always-on; that's why regulator_late_cleanup() doesn't switch it off. Regards, Clement > > root@smarc-rzg2l:~# cat /sys/class/devfreq/1184.gpu/trans_stat > From : To >: 5000 6250 1 12500 2 25000 > 4 5 time(ms) > * 5000: 0 0 0 0 0 0 >0 1 144 >6250: 0 0 0 0 0 0 >0 0 0 > 1: 0 0 0 0 0 0 >0 9 524 > 12500: 0 0 9 0 0 0 >0 3 2544 > 2: 0 0 011 0 0 >046 3304 > 25000: 1 0 0 033 0 >0 0 7496 > 4: 0 0 0 01619 >0 0 2024 > 5: 1 0 0 1 815 > 35 0 4032 > Total transition : 208 > > Cheers, > Biju >
[PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator
devm_pm_opp_set_regulators() doesn't enable regulator, which make regulator framework switching it off during regulator_late_cleanup(). Call dev_pm_opp_set_opp() with the recommend OPP in panfrost_devfreq_init() to enable the regulator and avoid any switch off by regulator_late_cleanup(). Suggested-by: Viresh Kumar Signed-off-by: Clément Péron --- drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index 5110cd9b2425..67b242407156 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) return PTR_ERR(opp); panfrost_devfreq_profile.initial_freq = cur_freq; + + /* Setup and enable regulator */ + ret = dev_pm_opp_set_opp(dev, opp); + if (ret) { + DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n"); + return ret; + } + dev_pm_opp_put(opp); /* -- 2.34.1
Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator
Your subject is 87 columns long, better to squeeze it a bit. On 05-09-22, 19:16, Clément Péron wrote: > devm_pm_opp_set_regulators() doesn't enable regulator, which make > regulator framework switching it off during regulator_late_cleanup(). This isn't the normal behavior as it works for everyone at the moment. You need to explain what special you are doing here, because of which you are reaching such a situation. i.e. you are disabling some code that uses GPU ? Please specify exact code so others can reproduce it as well. > Call dev_pm_opp_set_opp() with the recommend OPP in > panfrost_devfreq_init() to enable the regulator and avoid any switch off > by regulator_late_cleanup(). The regulator is already enabled I think at this point by the bootloader. What you are doing here is syncing the state of the hardware with the software, which would disallow disabling of the resource unnecessarily. > Suggested-by: Viresh Kumar > Signed-off-by: Clément Péron > --- > drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > index 5110cd9b2425..67b242407156 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) > return PTR_ERR(opp); > > panfrost_devfreq_profile.initial_freq = cur_freq; > + > + /* Setup and enable regulator */ Similarly here, explain why this is required to be done. > + ret = dev_pm_opp_set_opp(dev, opp); > + if (ret) { > + DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n"); > + return ret; > + } > + > dev_pm_opp_put(opp); Do this before checking if (ret), so the resource can be freed all the time. > > /* > -- > 2.34.1 -- viresh
RE: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator
Hi, Thanks for the patch. > Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended > one to configure and enable regulator > > devm_pm_opp_set_regulators() doesn't enable regulator, which make > regulator framework switching it off during regulator_late_cleanup(). In that case, why not regulator_get()for Dynamic regulator(non fixed regulator)?? > > Call dev_pm_opp_set_opp() with the recommend OPP in > panfrost_devfreq_init() to enable the regulator and avoid any switch off > by regulator_late_cleanup(). > > Suggested-by: Viresh Kumar > Signed-off-by: Clément Péron > --- > drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > index 5110cd9b2425..67b242407156 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct panfrost_device > *pfdev) > return PTR_ERR(opp); > > panfrost_devfreq_profile.initial_freq = cur_freq; > + > + /* Setup and enable regulator */ > + ret = dev_pm_opp_set_opp(dev, opp); > + if (ret) { > + DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n"); > + return ret; > + } FYI, On RZ/G2L mali gpu, we have fixed regulator and I was able to do GPU OPP transition without any issues previously. root@smarc-rzg2l:~# cat /sys/class/devfreq/1184.gpu/trans_stat From : To : 5000 6250 1 12500 2 25000 4 5 time(ms) * 5000: 0 0 0 0 0 0 0 1 144 6250: 0 0 0 0 0 0 0 0 0 1: 0 0 0 0 0 0 0 9 524 12500: 0 0 9 0 0 0 0 3 2544 2: 0 0 011 0 0 046 3304 25000: 1 0 0 033 0 0 0 7496 4: 0 0 0 01619 0 0 2024 5: 1 0 0 1 815 35 0 4032 Total transition : 208 Cheers, Biju