Hi,
This serie cleans and adds regulator support to Panfrost devfreq.
This is mostly based on comment for the freshly introduced lima
devfreq.
We need to add regulator support because on Allwinner the GPU OPP
table defines both frequencies and voltages.
First patches [01-08] should not change the actual behavior
and introduce a proper panfrost_devfreq struct.
Fatches after are WIP and add regulator support.
However I got several issues first we need to avoid getting regulator
if devfreq get by itself the regulator, but as of today the OPP
framework only get and don't enable the regulator...
An HACK for now is to add regulator-always-on in the device-tree.
Then when I enable devfreq I got several faults like.
I'm totally noob on GPU sched/fault and couldn't be helpfull with this.
I got this running glmark2 on T720 (Allwinner H6) with Mesa 20.0.5.
# glmark2-es2-drm
=======================================================
glmark2 2017.07
=======================================================
OpenGL Information
GL_VENDOR: Panfrost
GL_RENDERER: Mali T720 (Panfrost)
GL_VERSION: OpenGL ES 2.0 Mesa 20.0.5
=======================================================
[ 93.550063] panfrost 1800000.gpu: GPU Fault 0x00000088 (UNKNOWN) at 0x0000000080117100
[ 94.045401] panfrost 1800000.gpu: gpu sched timeout, js=0, config=0x3700, status=0x8, head=0x21d6c00, tail=0x21d6c00, sched_job=00000000e3c2132f
[ 328.871070] panfrost 1800000.gpu: Unhandled Page fault in AS0 at VA 0x0000000000000000
[ 328.871070] Reason: TODO
[ 328.871070] raw fault status: 0xAA0003C2
[ 328.871070] decoded fault status: SLAVE FAULT
[ 328.871070] exception type 0xC2: TRANSLATION_FAULT_LEVEL2
[ 328.871070] access type 0x3: WRITE
[ 328.871070] source id 0xAA00
[ 329.373327] panfrost 1800000.gpu: gpu sched timeout, js=1, config=0x3700, status=0x8, head=0xa1a4900, tail=0xa1a4900, sched_job=000000007ac31097
[ 329.386527] panfrost 1800000.gpu: js fault, js=0, status=DATA_INVALID_FAULT, head=0xa1a4c00, tail=0xa1a4c00
[ 329.396293] panfrost 1800000.gpu: gpu sched timeout, js=0, config=0x3700, status=0x58, head=0xa1a4c00, tail=0xa1a4c00, sched_job=0000000004c90381
[ 329.411521] panfrost 1800000.gpu: Unhandled Page fault in AS0 at VA 0x0000000000000000
[ 329.411521] Reason: TODO
[ 329.411521] raw fault status: 0xAA0003C2
[ 329.411521] decoded fault status: SLAVE FAULT
[ 329.411521] exception type 0xC2: TRANSLATION_FAULT_LEVEL2
[ 329.411521] access type 0x3: WRITE
[ 329.411521] source id 0xAA00
Thanks for your reviews, help on this serie,
Clement
Clément Péron (15):
drm/panfrost: avoid static declaration
drm/panfrost: clean headers in devfreq
drm/panfrost: don't use pfdevfreq.busy_count to know if hw is idle
drm/panfrost: introduce panfrost_devfreq struct
drm/panfrost: use spinlock instead of atomic
drm/panfrost: properly handle error in probe
drm/panfrost: use device_property_present to check for OPP
drm/panfrost: move devfreq_init()/fini() in device
drm/panfrost: dynamically alloc regulators
drm/panfrost: add regulators to devfreq
drm/panfrost: set devfreq clock name
arm64: defconfig: Enable devfreq cooling device
arm64: dts: allwinner: h6: Add cooling map for GPU
[DO NOT MERGE] arm64: dts: allwinner: h6: Add GPU OPP table
[DO NOT MERGE] arm64: dts: allwinner: force GPU regulator to be always
.../dts/allwinner/sun50i-h6-beelink-gs1.dts | 1 +
arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 102 ++++++++++
arch/arm64/configs/defconfig | 1 +
drivers/gpu/drm/panfrost/panfrost_devfreq.c | 190 ++++++++++++------
drivers/gpu/drm/panfrost/panfrost_devfreq.h | 32 ++-
drivers/gpu/drm/panfrost/panfrost_device.c | 56 ++++--
drivers/gpu/drm/panfrost/panfrost_device.h | 14 +-
drivers/gpu/drm/panfrost/panfrost_drv.c | 15 +-
drivers/gpu/drm/panfrost/panfrost_job.c | 10 +-
9 files changed, 310 insertions(+), 111 deletions(-)
--
2.20.1
Instead of expecting an error from dev_pm_opp_of_add_table()
do a simple device_property_present() check.
Signed-off-by: Clément Péron <[email protected]>
---
drivers/gpu/drm/panfrost/panfrost_devfreq.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index d9007f44b772..fce21c682414 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -96,15 +96,19 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
struct thermal_cooling_device *cooling;
struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
- ret = dev_pm_opp_of_add_table(dev);
- if (ret == -ENODEV) /* Optional, continue without devfreq */
+ if (!device_property_present(dev, "operating-points-v2"))
+ /* Optional, continue without devfreq */
return 0;
- else if (ret)
- return ret;
- pfdevfreq->opp_of_table_added = true;
spin_lock_init(&pfdevfreq->lock);
+ ret = dev_pm_opp_of_add_table(dev);
+ if (ret) {
+ DRM_DEV_ERROR(dev, "Couldn't add OPP table\n");
+ goto err_fini;
+ }
+ pfdevfreq->opp_of_table_added = true;
+
panfrost_devfreq_reset(pfdevfreq);
cur_freq = clk_get_rate(pfdev->clock);
--
2.20.1
Introduce a boolean to know if opp table has been added.
With this, we can call panfrost_devfreq_fini() in case of error
and release what has been initialised.
Signed-off-by: Clément Péron <[email protected]>
---
drivers/gpu/drm/panfrost/panfrost_devfreq.c | 25 ++++++++++++++++-----
drivers/gpu/drm/panfrost/panfrost_devfreq.h | 1 +
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 78753cfb59fb..d9007f44b772 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -101,6 +101,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
return 0;
else if (ret)
return ret;
+ pfdevfreq->opp_of_table_added = true;
spin_lock_init(&pfdevfreq->lock);
@@ -109,8 +110,10 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
cur_freq = clk_get_rate(pfdev->clock);
opp = devfreq_recommended_opp(dev, &cur_freq, 0);
- if (IS_ERR(opp))
- return PTR_ERR(opp);
+ if (IS_ERR(opp)) {
+ ret = PTR_ERR(opp);
+ goto err_fini;
+ }
panfrost_devfreq_profile.initial_freq = cur_freq;
dev_pm_opp_put(opp);
@@ -119,8 +122,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
if (IS_ERR(devfreq)) {
DRM_DEV_ERROR(dev, "Couldn't initialize GPU devfreq\n");
- dev_pm_opp_of_remove_table(dev);
- return PTR_ERR(devfreq);
+ ret = PTR_ERR(devfreq);
+ goto err_fini;
}
pfdevfreq->devfreq = devfreq;
@@ -131,15 +134,25 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
pfdevfreq->cooling = cooling;
return 0;
+
+err_fini:
+ panfrost_devfreq_fini(pfdev);
+ return ret;
}
void panfrost_devfreq_fini(struct panfrost_device *pfdev)
{
struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
- if (pfdevfreq->cooling)
+ if (pfdevfreq->cooling) {
devfreq_cooling_unregister(pfdevfreq->cooling);
- dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
+ pfdevfreq->cooling = NULL;
+ }
+
+ if (pfdevfreq->opp_of_table_added) {
+ dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
+ pfdevfreq->opp_of_table_added = false;
+ }
}
void panfrost_devfreq_resume(struct panfrost_device *pfdev)
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
index e6629900a618..add203cb00c2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
@@ -15,6 +15,7 @@ struct panfrost_device;
struct panfrost_devfreq {
struct devfreq *devfreq;
struct thermal_cooling_device *cooling;
+ bool opp_of_table_added;
ktime_t busy_time;
ktime_t idle_time;
--
2.20.1
On 5/10/20 6:55 PM, Clément Péron wrote:
> Hi,
>
> This serie cleans and adds regulator support to Panfrost devfreq.
> This is mostly based on comment for the freshly introduced lima
> devfreq.
>
> We need to add regulator support because on Allwinner the GPU OPP
> table defines both frequencies and voltages.
>
> First patches [01-08] should not change the actual behavior
> and introduce a proper panfrost_devfreq struct.
>
> Fatches after are WIP and add regulator support.
>
> However I got several issues first we need to avoid getting regulator
> if devfreq get by itself the regulator, but as of today the OPP
> framework only get and don't enable the regulator...
> An HACK for now is to add regulator-always-on in the device-tree.
>
> Then when I enable devfreq I got several faults like.
> I'm totally noob on GPU sched/fault and couldn't be helpfull with this.
Do you know at which frequencies do the faults happen? From what I can
see, it's just the GPU behaving erratically, and the CPU reading random
values from the GPU registers. Given the subject of this series, I guess
the GPU isn't getting enough power.
There could be a problem with the OPP table, might be a good idea to see
what levels are problematic and try with a more conservative table.
Besides that, there could be a problem with clock frequency changes, or
voltage changes. It may take some time for the final state to be stable,
depending how the regulation happens.
Thanks,
Tomeu
> I got this running glmark2 on T720 (Allwinner H6) with Mesa 20.0.5.
> # glmark2-es2-drm
> =======================================================
> glmark2 2017.07
> =======================================================
> OpenGL Information
> GL_VENDOR: Panfrost
> GL_RENDERER: Mali T720 (Panfrost)
> GL_VERSION: OpenGL ES 2.0 Mesa 20.0.5
> =======================================================
>
> [ 93.550063] panfrost 1800000.gpu: GPU Fault 0x00000088 (UNKNOWN) at 0x0000000080117100
> [ 94.045401] panfrost 1800000.gpu: gpu sched timeout, js=0, config=0x3700, status=0x8, head=0x21d6c00, tail=0x21d6c00, sched_job=00000000e3c2132f
>
> [ 328.871070] panfrost 1800000.gpu: Unhandled Page fault in AS0 at VA 0x0000000000000000
> [ 328.871070] Reason: TODO
> [ 328.871070] raw fault status: 0xAA0003C2
> [ 328.871070] decoded fault status: SLAVE FAULT
> [ 328.871070] exception type 0xC2: TRANSLATION_FAULT_LEVEL2
> [ 328.871070] access type 0x3: WRITE
> [ 328.871070] source id 0xAA00
> [ 329.373327] panfrost 1800000.gpu: gpu sched timeout, js=1, config=0x3700, status=0x8, head=0xa1a4900, tail=0xa1a4900, sched_job=000000007ac31097
> [ 329.386527] panfrost 1800000.gpu: js fault, js=0, status=DATA_INVALID_FAULT, head=0xa1a4c00, tail=0xa1a4c00
> [ 329.396293] panfrost 1800000.gpu: gpu sched timeout, js=0, config=0x3700, status=0x58, head=0xa1a4c00, tail=0xa1a4c00, sched_job=0000000004c90381
> [ 329.411521] panfrost 1800000.gpu: Unhandled Page fault in AS0 at VA 0x0000000000000000
> [ 329.411521] Reason: TODO
> [ 329.411521] raw fault status: 0xAA0003C2
> [ 329.411521] decoded fault status: SLAVE FAULT
> [ 329.411521] exception type 0xC2: TRANSLATION_FAULT_LEVEL2
> [ 329.411521] access type 0x3: WRITE
> [ 329.411521] source id 0xAA00
>
> Thanks for your reviews, help on this serie,
> Clement
>
> Clément Péron (15):
> drm/panfrost: avoid static declaration
> drm/panfrost: clean headers in devfreq
> drm/panfrost: don't use pfdevfreq.busy_count to know if hw is idle
> drm/panfrost: introduce panfrost_devfreq struct
> drm/panfrost: use spinlock instead of atomic
> drm/panfrost: properly handle error in probe
> drm/panfrost: use device_property_present to check for OPP
> drm/panfrost: move devfreq_init()/fini() in device
> drm/panfrost: dynamically alloc regulators
> drm/panfrost: add regulators to devfreq
> drm/panfrost: set devfreq clock name
> arm64: defconfig: Enable devfreq cooling device
> arm64: dts: allwinner: h6: Add cooling map for GPU
> [DO NOT MERGE] arm64: dts: allwinner: h6: Add GPU OPP table
> [DO NOT MERGE] arm64: dts: allwinner: force GPU regulator to be always
>
> .../dts/allwinner/sun50i-h6-beelink-gs1.dts | 1 +
> arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 102 ++++++++++
> arch/arm64/configs/defconfig | 1 +
> drivers/gpu/drm/panfrost/panfrost_devfreq.c | 190 ++++++++++++------
> drivers/gpu/drm/panfrost/panfrost_devfreq.h | 32 ++-
> drivers/gpu/drm/panfrost/panfrost_device.c | 56 ++++--
> drivers/gpu/drm/panfrost/panfrost_device.h | 14 +-
> drivers/gpu/drm/panfrost/panfrost_drv.c | 15 +-
> drivers/gpu/drm/panfrost/panfrost_job.c | 10 +-
> 9 files changed, 310 insertions(+), 111 deletions(-)
>
On 10/05/2020 17:55, Clément Péron wrote:
> Introduce a boolean to know if opp table has been added.
>
> With this, we can call panfrost_devfreq_fini() in case of error
> and release what has been initialised.
>
> Signed-off-by: Clément Péron <[email protected]>
LGTM:
Reviewed-by: Steven Price <[email protected]>
> ---
> drivers/gpu/drm/panfrost/panfrost_devfreq.c | 25 ++++++++++++++++-----
> drivers/gpu/drm/panfrost/panfrost_devfreq.h | 1 +
> 2 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 78753cfb59fb..d9007f44b772 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -101,6 +101,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> return 0;
> else if (ret)
> return ret;
> + pfdevfreq->opp_of_table_added = true;
>
> spin_lock_init(&pfdevfreq->lock);
>
> @@ -109,8 +110,10 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> cur_freq = clk_get_rate(pfdev->clock);
>
> opp = devfreq_recommended_opp(dev, &cur_freq, 0);
> - if (IS_ERR(opp))
> - return PTR_ERR(opp);
> + if (IS_ERR(opp)) {
> + ret = PTR_ERR(opp);
> + goto err_fini;
> + }
>
> panfrost_devfreq_profile.initial_freq = cur_freq;
> dev_pm_opp_put(opp);
> @@ -119,8 +122,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
> if (IS_ERR(devfreq)) {
> DRM_DEV_ERROR(dev, "Couldn't initialize GPU devfreq\n");
> - dev_pm_opp_of_remove_table(dev);
> - return PTR_ERR(devfreq);
> + ret = PTR_ERR(devfreq);
> + goto err_fini;
> }
> pfdevfreq->devfreq = devfreq;
>
> @@ -131,15 +134,25 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> pfdevfreq->cooling = cooling;
>
> return 0;
> +
> +err_fini:
> + panfrost_devfreq_fini(pfdev);
> + return ret;
> }
>
> void panfrost_devfreq_fini(struct panfrost_device *pfdev)
> {
> struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
>
> - if (pfdevfreq->cooling)
> + if (pfdevfreq->cooling) {
> devfreq_cooling_unregister(pfdevfreq->cooling);
> - dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
> + pfdevfreq->cooling = NULL;
> + }
> +
> + if (pfdevfreq->opp_of_table_added) {
> + dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
> + pfdevfreq->opp_of_table_added = false;
> + }
> }
>
> void panfrost_devfreq_resume(struct panfrost_device *pfdev)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> index e6629900a618..add203cb00c2 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> @@ -15,6 +15,7 @@ struct panfrost_device;
> struct panfrost_devfreq {
> struct devfreq *devfreq;
> struct thermal_cooling_device *cooling;
> + bool opp_of_table_added;
>
> ktime_t busy_time;
> ktime_t idle_time;
>
On 10/05/2020 17:55, Clément Péron wrote:
> Instead of expecting an error from dev_pm_opp_of_add_table()
> do a simple device_property_present() check.
>
> Signed-off-by: Clément Péron <[email protected]>
I'm not sure I understand why this is better. We seem to have more code
to do roughly the same thing just with the hard-coded
"operating-points-v2" name (if there's ever a 'v3' we'll then have to
update this).
Is the desire just to get an error on probe if the table is malformed?
Have you hit this situation? If so this sounds like something which
would be better fixed in the generic OPP code rather than Panfrost itself.
Steve
> ---
> drivers/gpu/drm/panfrost/panfrost_devfreq.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index d9007f44b772..fce21c682414 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -96,15 +96,19 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> struct thermal_cooling_device *cooling;
> struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
>
> - ret = dev_pm_opp_of_add_table(dev);
> - if (ret == -ENODEV) /* Optional, continue without devfreq */
> + if (!device_property_present(dev, "operating-points-v2"))
> + /* Optional, continue without devfreq */
> return 0;
> - else if (ret)
> - return ret;
> - pfdevfreq->opp_of_table_added = true;
>
> spin_lock_init(&pfdevfreq->lock);
>
> + ret = dev_pm_opp_of_add_table(dev);
> + if (ret) {
> + DRM_DEV_ERROR(dev, "Couldn't add OPP table\n");
> + goto err_fini;
> + }
> + pfdevfreq->opp_of_table_added = true;
> +
> panfrost_devfreq_reset(pfdevfreq);
>
> cur_freq = clk_get_rate(pfdev->clock);
>
Hi Steven,
On Thu, 28 May 2020 at 15:22, Steven Price <[email protected]> wrote:
>
> On 10/05/2020 17:55, Clément Péron wrote:
> > Instead of expecting an error from dev_pm_opp_of_add_table()
> > do a simple device_property_present() check.
> >
> > Signed-off-by: Clément Péron <[email protected]>
>
> I'm not sure I understand why this is better. We seem to have more code
> to do roughly the same thing just with the hard-coded
> "operating-points-v2" name (if there's ever a 'v3' we'll then have to
> update this).
>
> Is the desire just to get an error on probe if the table is malformed?
> Have you hit this situation? If so this sounds like something which
> would be better fixed in the generic OPP code rather than Panfrost itself.
The idea was to avoid calling devfreq if there is no opp table.
But I think you're right we don't have to check for malformed
device-tree in the driver.
I will drop this patch,
Regards,
Clement
>
> Steve
>
> > ---
> > drivers/gpu/drm/panfrost/panfrost_devfreq.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > index d9007f44b772..fce21c682414 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > @@ -96,15 +96,19 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> > struct thermal_cooling_device *cooling;
> > struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
> >
> > - ret = dev_pm_opp_of_add_table(dev);
> > - if (ret == -ENODEV) /* Optional, continue without devfreq */
> > + if (!device_property_present(dev, "operating-points-v2"))
> > + /* Optional, continue without devfreq */
> > return 0;
> > - else if (ret)
> > - return ret;
> > - pfdevfreq->opp_of_table_added = true;
> >
> > spin_lock_init(&pfdevfreq->lock);
> >
> > + ret = dev_pm_opp_of_add_table(dev);
> > + if (ret) {
> > + DRM_DEV_ERROR(dev, "Couldn't add OPP table\n");
> > + goto err_fini;
> > + }
> > + pfdevfreq->opp_of_table_added = true;
> > +
> > panfrost_devfreq_reset(pfdevfreq);
> >
> > cur_freq = clk_get_rate(pfdev->clock);
> >
>