| Message ID | 20190513175554.31804-1-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Make devfreq truly optional | expand |
On Mon, May 13, 2019 at 12:56 PM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > Currently, there is some logic to make devfreq optional, > but it fails to cover some cases such as !CONFIG_PM_DEVFREQ. Fails how? compiling? runtime? Or just builds extra code? > Moreover, depending on return codes is not resilient to change, > so let's take a different approach, introducing proper > stubs and only conditionally compiling the devfreq support. The downside here is another build time configuration to test. Isn't devfreq essentially going to be required to run the GPU at higher frequencies and to not melt it? > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/gpu/drm/panfrost/Makefile | 3 ++- > drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 ++----------- > drivers/gpu/drm/panfrost/panfrost_devfreq.h | 19 +++++++++++++++++-- > 3 files changed, 21 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile > index 6de72d13c58f..8b690672f9d9 100644 > --- a/drivers/gpu/drm/panfrost/Makefile > +++ b/drivers/gpu/drm/panfrost/Makefile > @@ -3,10 +3,11 @@ > panfrost-y := \ > panfrost_drv.o \ > panfrost_device.o \ > - panfrost_devfreq.o \ > panfrost_gem.o \ > panfrost_gpu.o \ > panfrost_job.o \ > panfrost_mmu.o > > +panfrost-$(CONFIG_PM_DEVFREQ) += panfrost_devfreq.o > + > obj-$(CONFIG_DRM_PANFROST) += panfrost.o > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > index 238bd1d89d43..29fcffdf2d57 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > @@ -140,8 +140,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) > return 0; > > ret = dev_pm_opp_of_add_table(&pfdev->pdev->dev); > - if (ret == -ENODEV) /* Optional, continue without devfreq */ > - return 0; > + if (ret) > + return ret; > > panfrost_devfreq_reset(pfdev); > > @@ -170,9 +170,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev) > { > int i; > > - if (!pfdev->devfreq.devfreq) > - return; > - > panfrost_devfreq_reset(pfdev); > for (i = 0; i < NUM_JOB_SLOTS; i++) > pfdev->devfreq.slot[i].busy = false; > @@ -182,9 +179,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev) > > void panfrost_devfreq_suspend(struct panfrost_device *pfdev) > { > - if (!pfdev->devfreq.devfreq) > - return; > - > devfreq_suspend_device(pfdev->devfreq.devfreq); > } > > @@ -194,9 +188,6 @@ static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, i > ktime_t now; > ktime_t last; > > - if (!pfdev->devfreq.devfreq) > - return; > - > now = ktime_get(); > last = pfdev->devfreq.slot[slot].time_last_update; > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h > index eb999531ed90..76b56a8de6e3 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h > @@ -4,11 +4,26 @@ > #ifndef __PANFROST_DEVFREQ_H__ > #define __PANFROST_DEVFREQ_H__ > > +#if defined(CONFIG_PM_DEVFREQ) > int panfrost_devfreq_init(struct panfrost_device *pfdev); > - > void panfrost_devfreq_resume(struct panfrost_device *pfdev); > void panfrost_devfreq_suspend(struct panfrost_device *pfdev); > - > void panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot); > +#else > +static inline int panfrost_devfreq_init(struct panfrost_device *pfdev) > +{ > + return 0; > +} > + > +static inline void panfrost_devfreq_resume(struct panfrost_device *pfdev) > +{} > + > +static inline void panfrost_devfreq_suspend(struct panfrost_device *pfdev) > +{} > + > +static inline void > +panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot) > +{} > +#endif > > #endif /* __PANFROST_DEVFREQ_H__ */ > -- > 2.20.1 >
On Tue, 2019-05-14 at 08:38 -0500, Rob Herring wrote: > On Mon, May 13, 2019 at 12:56 PM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > Currently, there is some logic to make devfreq optional, > > but it fails to cover some cases such as !CONFIG_PM_DEVFREQ. > > Fails how? compiling? runtime? Or just builds extra code? > Well, given we currently don't enforce PM_DEVFREQ, the driver fails to probe. Specifically, panfrost_devfreq_init devfreq_recommended_opp -EINVAL > > Moreover, depending on return codes is not resilient to change, > > so let's take a different approach, introducing proper > > stubs and only conditionally compiling the devfreq support. > > The downside here is another build time configuration to test. Isn't > devfreq essentially going to be required to run the GPU at higher > frequencies and to not melt it? > Right. So the alternative is to select or depend on PM_DEVFREQ. Otherwise, it's quite confusing for developers to choose the driver and have it fail to probe because some other option is not selected. Let me know what you think and I'll cook a v2. Thanks, Eze > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/gpu/drm/panfrost/Makefile | 3 ++- > > drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 ++----------- > > drivers/gpu/drm/panfrost/panfrost_devfreq.h | 19 +++++++++++++++++-- > > 3 files changed, 21 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile > > index 6de72d13c58f..8b690672f9d9 100644 > > --- a/drivers/gpu/drm/panfrost/Makefile > > +++ b/drivers/gpu/drm/panfrost/Makefile > > @@ -3,10 +3,11 @@ > > panfrost-y := \ > > panfrost_drv.o \ > > panfrost_device.o \ > > - panfrost_devfreq.o \ > > panfrost_gem.o \ > > panfrost_gpu.o \ > > panfrost_job.o \ > > panfrost_mmu.o > > > > +panfrost-$(CONFIG_PM_DEVFREQ) += panfrost_devfreq.o > > + > > obj-$(CONFIG_DRM_PANFROST) += panfrost.o > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > index 238bd1d89d43..29fcffdf2d57 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > > @@ -140,8 +140,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) > > return 0; > > > > ret = dev_pm_opp_of_add_table(&pfdev->pdev->dev); > > - if (ret == -ENODEV) /* Optional, continue without devfreq */ > > - return 0; > > + if (ret) > > + return ret; > > > > panfrost_devfreq_reset(pfdev); > > > > @@ -170,9 +170,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev) > > { > > int i; > > > > - if (!pfdev->devfreq.devfreq) > > - return; > > - > > panfrost_devfreq_reset(pfdev); > > for (i = 0; i < NUM_JOB_SLOTS; i++) > > pfdev->devfreq.slot[i].busy = false; > > @@ -182,9 +179,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev) > > > > void panfrost_devfreq_suspend(struct panfrost_device *pfdev) > > { > > - if (!pfdev->devfreq.devfreq) > > - return; > > - > > devfreq_suspend_device(pfdev->devfreq.devfreq); > > } > > > > @@ -194,9 +188,6 @@ static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, i > > ktime_t now; > > ktime_t last; > > > > - if (!pfdev->devfreq.devfreq) > > - return; > > - > > now = ktime_get(); > > last = pfdev->devfreq.slot[slot].time_last_update; > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h > > index eb999531ed90..76b56a8de6e3 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h > > @@ -4,11 +4,26 @@ > > #ifndef __PANFROST_DEVFREQ_H__ > > #define __PANFROST_DEVFREQ_H__ > > > > +#if defined(CONFIG_PM_DEVFREQ) > > int panfrost_devfreq_init(struct panfrost_device *pfdev); > > - > > void panfrost_devfreq_resume(struct panfrost_device *pfdev); > > void panfrost_devfreq_suspend(struct panfrost_device *pfdev); > > - > > void panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot); > > +#else > > +static inline int panfrost_devfreq_init(struct panfrost_device *pfdev) > > +{ > > + return 0; > > +} > > + > > +static inline void panfrost_devfreq_resume(struct panfrost_device *pfdev) > > +{} > > + > > +static inline void panfrost_devfreq_suspend(struct panfrost_device *pfdev) > > +{} > > + > > +static inline void > > +panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot) > > +{} > > +#endif > > > > #endif /* __PANFROST_DEVFREQ_H__ */ > > -- > > 2.20.1 > >
On 5/14/19 5:36 PM, Ezequiel Garcia wrote: > On Tue, 2019-05-14 at 08:38 -0500, Rob Herring wrote: >> On Mon, May 13, 2019 at 12:56 PM Ezequiel Garcia <ezequiel@collabora.com> wrote: >>> Currently, there is some logic to make devfreq optional, >>> but it fails to cover some cases such as !CONFIG_PM_DEVFREQ. >> >> Fails how? compiling? runtime? Or just builds extra code? >> > > Well, given we currently don't enforce PM_DEVFREQ, the driver fails > to probe. Specifically, > > panfrost_devfreq_init > devfreq_recommended_opp > -EINVAL > >>> Moreover, depending on return codes is not resilient to change, >>> so let's take a different approach, introducing proper >>> stubs and only conditionally compiling the devfreq support. >> >> The downside here is another build time configuration to test. Isn't >> devfreq essentially going to be required to run the GPU at higher >> frequencies and to not melt it? >> > > Right. So the alternative is to select or depend on PM_DEVFREQ. > > Otherwise, it's quite confusing for developers to choose the driver > and have it fail to probe because some other option is not selected. Yeah, I don't see a good reason to not use devfreq, so I think we should require it. Thanks, Tomeu > Let me know what you think and I'll cook a v2. > > Thanks, > Eze > >>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >>> --- >>> drivers/gpu/drm/panfrost/Makefile | 3 ++- >>> drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 ++----------- >>> drivers/gpu/drm/panfrost/panfrost_devfreq.h | 19 +++++++++++++++++-- >>> 3 files changed, 21 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile >>> index 6de72d13c58f..8b690672f9d9 100644 >>> --- a/drivers/gpu/drm/panfrost/Makefile >>> +++ b/drivers/gpu/drm/panfrost/Makefile >>> @@ -3,10 +3,11 @@ >>> panfrost-y := \ >>> panfrost_drv.o \ >>> panfrost_device.o \ >>> - panfrost_devfreq.o \ >>> panfrost_gem.o \ >>> panfrost_gpu.o \ >>> panfrost_job.o \ >>> panfrost_mmu.o >>> >>> +panfrost-$(CONFIG_PM_DEVFREQ) += panfrost_devfreq.o >>> + >>> obj-$(CONFIG_DRM_PANFROST) += panfrost.o >>> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c >>> index 238bd1d89d43..29fcffdf2d57 100644 >>> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c >>> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c >>> @@ -140,8 +140,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) >>> return 0; >>> >>> ret = dev_pm_opp_of_add_table(&pfdev->pdev->dev); >>> - if (ret == -ENODEV) /* Optional, continue without devfreq */ >>> - return 0; >>> + if (ret) >>> + return ret; >>> >>> panfrost_devfreq_reset(pfdev); >>> >>> @@ -170,9 +170,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev) >>> { >>> int i; >>> >>> - if (!pfdev->devfreq.devfreq) >>> - return; >>> - >>> panfrost_devfreq_reset(pfdev); >>> for (i = 0; i < NUM_JOB_SLOTS; i++) >>> pfdev->devfreq.slot[i].busy = false; >>> @@ -182,9 +179,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev) >>> >>> void panfrost_devfreq_suspend(struct panfrost_device *pfdev) >>> { >>> - if (!pfdev->devfreq.devfreq) >>> - return; >>> - >>> devfreq_suspend_device(pfdev->devfreq.devfreq); >>> } >>> >>> @@ -194,9 +188,6 @@ static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, i >>> ktime_t now; >>> ktime_t last; >>> >>> - if (!pfdev->devfreq.devfreq) >>> - return; >>> - >>> now = ktime_get(); >>> last = pfdev->devfreq.slot[slot].time_last_update; >>> >>> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h >>> index eb999531ed90..76b56a8de6e3 100644 >>> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h >>> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h >>> @@ -4,11 +4,26 @@ >>> #ifndef __PANFROST_DEVFREQ_H__ >>> #define __PANFROST_DEVFREQ_H__ >>> >>> +#if defined(CONFIG_PM_DEVFREQ) >>> int panfrost_devfreq_init(struct panfrost_device *pfdev); >>> - >>> void panfrost_devfreq_resume(struct panfrost_device *pfdev); >>> void panfrost_devfreq_suspend(struct panfrost_device *pfdev); >>> - >>> void panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot); >>> +#else >>> +static inline int panfrost_devfreq_init(struct panfrost_device *pfdev) >>> +{ >>> + return 0; >>> +} >>> + >>> +static inline void panfrost_devfreq_resume(struct panfrost_device *pfdev) >>> +{} >>> + >>> +static inline void panfrost_devfreq_suspend(struct panfrost_device *pfdev) >>> +{} >>> + >>> +static inline void >>> +panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot) >>> +{} >>> +#endif >>> >>> #endif /* __PANFROST_DEVFREQ_H__ */ >>> -- >>> 2.20.1 >>> > > >
diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile index 6de72d13c58f..8b690672f9d9 100644 --- a/drivers/gpu/drm/panfrost/Makefile +++ b/drivers/gpu/drm/panfrost/Makefile @@ -3,10 +3,11 @@ panfrost-y := \ panfrost_drv.o \ panfrost_device.o \ - panfrost_devfreq.o \ panfrost_gem.o \ panfrost_gpu.o \ panfrost_job.o \ panfrost_mmu.o +panfrost-$(CONFIG_PM_DEVFREQ) += panfrost_devfreq.o + obj-$(CONFIG_DRM_PANFROST) += panfrost.o diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index 238bd1d89d43..29fcffdf2d57 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -140,8 +140,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) return 0; ret = dev_pm_opp_of_add_table(&pfdev->pdev->dev); - if (ret == -ENODEV) /* Optional, continue without devfreq */ - return 0; + if (ret) + return ret; panfrost_devfreq_reset(pfdev); @@ -170,9 +170,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev) { int i; - if (!pfdev->devfreq.devfreq) - return; - panfrost_devfreq_reset(pfdev); for (i = 0; i < NUM_JOB_SLOTS; i++) pfdev->devfreq.slot[i].busy = false; @@ -182,9 +179,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev) void panfrost_devfreq_suspend(struct panfrost_device *pfdev) { - if (!pfdev->devfreq.devfreq) - return; - devfreq_suspend_device(pfdev->devfreq.devfreq); } @@ -194,9 +188,6 @@ static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, i ktime_t now; ktime_t last; - if (!pfdev->devfreq.devfreq) - return; - now = ktime_get(); last = pfdev->devfreq.slot[slot].time_last_update; diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h index eb999531ed90..76b56a8de6e3 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h @@ -4,11 +4,26 @@ #ifndef __PANFROST_DEVFREQ_H__ #define __PANFROST_DEVFREQ_H__ +#if defined(CONFIG_PM_DEVFREQ) int panfrost_devfreq_init(struct panfrost_device *pfdev); - void panfrost_devfreq_resume(struct panfrost_device *pfdev); void panfrost_devfreq_suspend(struct panfrost_device *pfdev); - void panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot); +#else +static inline int panfrost_devfreq_init(struct panfrost_device *pfdev) +{ + return 0; +} + +static inline void panfrost_devfreq_resume(struct panfrost_device *pfdev) +{} + +static inline void panfrost_devfreq_suspend(struct panfrost_device *pfdev) +{} + +static inline void +panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot) +{} +#endif #endif /* __PANFROST_DEVFREQ_H__ */
Currently, there is some logic to make devfreq optional, but it fails to cover some cases such as !CONFIG_PM_DEVFREQ. Moreover, depending on return codes is not resilient to change, so let's take a different approach, introducing proper stubs and only conditionally compiling the devfreq support. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/gpu/drm/panfrost/Makefile | 3 ++- drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 ++----------- drivers/gpu/drm/panfrost/panfrost_devfreq.h | 19 +++++++++++++++++-- 3 files changed, 21 insertions(+), 14 deletions(-)