| Message ID | 20230123153745.3185032-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Chanwoo Choi |
| Headers | show |
| Series | PM / devfreq: Fix build issues with devfreq disabled | expand |
>Sender : Rob Clark <robdclark@gmail.com> >Date : 2023-01-24 00:37 (GMT+9) >Title : [PATCH] PM / devfreq: Fix build issues with devfreq disabled > >From: Rob Clark <robdclark@chromium.org> > >The existing no-op shims for when PM_DEVFREQ (or an individual governor) >only do half the job. The governor specific config/tuning structs need >to be available to avoid compile errors in drivers using devfreq. > >Fixes: 6563f60f14cb ("drm/msm/gpu: Add devfreq tuning debugfs") >Signed-off-by: Rob Clark <robdclark@chromium.org> Doesn't this imply that DRM_MSM should depend on PM_DEVFREQ ? It appears that gpu/drm/msm/DRM_MSM uses PM_DEVFREQ without actually declaring the dependency on PM_DEVFREQ. You cannot use SIMPLE_ONDEMAND without DEVFREQ. Cheers, MyungJoo >--- >Assuming this doesn't conflict with anything else landing via another >tree, an a-b to land this via drm/msm-next would let us un-break builds. >(And also start removing "select DEVFREQ_GOV_SIMPLE_ONDEMAND"s added in >various places to try to work around this issue.) > > include/linux/devfreq.h | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > >diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >index 4dc7cda4fd46..7fd704bb8f3d 100644 >--- a/include/linux/devfreq.h >+++ b/include/linux/devfreq.h >@@ -273,8 +273,8 @@ void devm_devfreq_unregister_notifier(struct device *dev, > struct devfreq *devfreq_get_devfreq_by_node(struct device_node *node); > struct devfreq *devfreq_get_devfreq_by_phandle(struct device *dev, > const char *phandle_name, int index); >+#endif /* CONFIG_PM_DEVFREQ */ > >-#if IS_ENABLED(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND) > /** > * struct devfreq_simple_ondemand_data - ``void *data`` fed to struct devfreq > * and devfreq_add_device >@@ -292,9 +292,7 @@ struct devfreq_simple_ondemand_data { > unsigned int upthreshold; > unsigned int downdifferential; > }; >-#endif > >-#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE) > enum devfreq_parent_dev_type { > DEVFREQ_PARENT_DEV, > CPUFREQ_PARENT_DEV, >@@ -337,9 +335,8 @@ struct devfreq_passive_data { > struct notifier_block nb; > struct list_head cpu_data_list; > }; >-#endif > >-#else /* !CONFIG_PM_DEVFREQ */ >+#if !defined(CONFIG_PM_DEVFREQ) > static inline struct devfreq *devfreq_add_device(struct device *dev, > struct devfreq_dev_profile *profile, > const char *governor_name, >-- >2.38.1 >
On Tue, Jan 24, 2023 at 8:04 PM MyungJoo Ham <myungjoo.ham@samsung.com> wrote: > > >Sender : Rob Clark <robdclark@gmail.com> > >Date : 2023-01-24 00:37 (GMT+9) > >Title : [PATCH] PM / devfreq: Fix build issues with devfreq disabled > > > >From: Rob Clark <robdclark@chromium.org> > > > >The existing no-op shims for when PM_DEVFREQ (or an individual governor) > >only do half the job. The governor specific config/tuning structs need > >to be available to avoid compile errors in drivers using devfreq. > > > >Fixes: 6563f60f14cb ("drm/msm/gpu: Add devfreq tuning debugfs") > >Signed-off-by: Rob Clark <robdclark@chromium.org> > > Doesn't this imply that DRM_MSM should depend on PM_DEVFREQ ? > > It appears that gpu/drm/msm/DRM_MSM uses PM_DEVFREQ without actually > declaring the dependency on PM_DEVFREQ. > You cannot use SIMPLE_ONDEMAND without DEVFREQ. Possibly that would resolve some issues, and that might have been my mistake in assuming that depending on SIMPLE_ONDEMAND implied a dependency on DEFREQ (which seems like a reasonable assumption, IMHO) But AFAICT some kernel configs that could otherwise use DRM_MSM don't support PM_DEVFREQ.. either way, lets solve this properly and remove needless dependencies on devfreq. BR, -R > Cheers, > MyungJoo > > >--- > >Assuming this doesn't conflict with anything else landing via another > >tree, an a-b to land this via drm/msm-next would let us un-break builds. > >(And also start removing "select DEVFREQ_GOV_SIMPLE_ONDEMAND"s added in > >various places to try to work around this issue.) > > > > include/linux/devfreq.h | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > >diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > >index 4dc7cda4fd46..7fd704bb8f3d 100644 > >--- a/include/linux/devfreq.h > >+++ b/include/linux/devfreq.h > >@@ -273,8 +273,8 @@ void devm_devfreq_unregister_notifier(struct device *dev, > > struct devfreq *devfreq_get_devfreq_by_node(struct device_node *node); > > struct devfreq *devfreq_get_devfreq_by_phandle(struct device *dev, > > const char *phandle_name, int index); > >+#endif /* CONFIG_PM_DEVFREQ */ > > > >-#if IS_ENABLED(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND) > > /** > > * struct devfreq_simple_ondemand_data - ``void *data`` fed to struct devfreq > > * and devfreq_add_device > >@@ -292,9 +292,7 @@ struct devfreq_simple_ondemand_data { > > unsigned int upthreshold; > > unsigned int downdifferential; > > }; > >-#endif > > > >-#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE) > > enum devfreq_parent_dev_type { > > DEVFREQ_PARENT_DEV, > > CPUFREQ_PARENT_DEV, > >@@ -337,9 +335,8 @@ struct devfreq_passive_data { > > struct notifier_block nb; > > struct list_head cpu_data_list; > > }; > >-#endif > > > >-#else /* !CONFIG_PM_DEVFREQ */ > >+#if !defined(CONFIG_PM_DEVFREQ) > > static inline struct devfreq *devfreq_add_device(struct device *dev, > > struct devfreq_dev_profile *profile, > > const char *governor_name, > >-- > >2.38.1 > > >
>On Tue, Jan 24, 2023 at 8:04 PM MyungJoo Ham <myungjoo.ham@samsung.com> wrote: >> >> >Sender : Rob Clark <robdclark@gmail.com> >> >Date : 2023-01-24 00:37 (GMT+9) >> >Title : [PATCH] PM / devfreq: Fix build issues with devfreq disabled >> > >> >From: Rob Clark <robdclark@chromium.org> >> > >> >The existing no-op shims for when PM_DEVFREQ (or an individual governor) >> >only do half the job. The governor specific config/tuning structs need >> >to be available to avoid compile errors in drivers using devfreq. >> > >> >Fixes: 6563f60f14cb ("drm/msm/gpu: Add devfreq tuning debugfs") >> >Signed-off-by: Rob Clark <robdclark@chromium.org> >> >> Doesn't this imply that DRM_MSM should depend on PM_DEVFREQ ? >> >> It appears that gpu/drm/msm/DRM_MSM uses PM_DEVFREQ without actually >> declaring the dependency on PM_DEVFREQ. >> You cannot use SIMPLE_ONDEMAND without DEVFREQ. > >Possibly that would resolve some issues, and that might have been my >mistake in assuming that depending on SIMPLE_ONDEMAND implied a >dependency on DEFREQ (which seems like a reasonable assumption, IMHO) > >But AFAICT some kernel configs that could otherwise use DRM_MSM don't >support PM_DEVFREQ.. either way, lets solve this properly and remove >needless dependencies on devfreq. > >BR, >-R Ok. You are enabling struct and enum only and that looks harmless. PTAL, Chanwoo. Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com> Cheers, MyungJoo.
On Tue, Jan 24, 2023 at 9:46 PM MyungJoo Ham <myungjoo.ham@samsung.com> wrote: > > >On Tue, Jan 24, 2023 at 8:04 PM MyungJoo Ham <myungjoo.ham@samsung.com> wrote: > >> > >> >Sender : Rob Clark <robdclark@gmail.com> > >> >Date : 2023-01-24 00:37 (GMT+9) > >> >Title : [PATCH] PM / devfreq: Fix build issues with devfreq disabled > >> > > >> >From: Rob Clark <robdclark@chromium.org> > >> > > >> >The existing no-op shims for when PM_DEVFREQ (or an individual governor) > >> >only do half the job. The governor specific config/tuning structs need > >> >to be available to avoid compile errors in drivers using devfreq. > >> > > >> >Fixes: 6563f60f14cb ("drm/msm/gpu: Add devfreq tuning debugfs") > >> >Signed-off-by: Rob Clark <robdclark@chromium.org> > >> > >> Doesn't this imply that DRM_MSM should depend on PM_DEVFREQ ? > >> > >> It appears that gpu/drm/msm/DRM_MSM uses PM_DEVFREQ without actually > >> declaring the dependency on PM_DEVFREQ. > >> You cannot use SIMPLE_ONDEMAND without DEVFREQ. > > > >Possibly that would resolve some issues, and that might have been my > >mistake in assuming that depending on SIMPLE_ONDEMAND implied a > >dependency on DEFREQ (which seems like a reasonable assumption, IMHO) > > > >But AFAICT some kernel configs that could otherwise use DRM_MSM don't > >support PM_DEVFREQ.. either way, lets solve this properly and remove > >needless dependencies on devfreq. > > > >BR, > >-R > > Ok. You are enabling struct and enum only and that looks harmless. > > PTAL, Chanwoo. > > Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com> Thanks, if possible, an ack to land this via msm-next would avoid build break headaches with COMPILE_TEST=y and other more obscure setups BR, -R
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index 4dc7cda4fd46..7fd704bb8f3d 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -273,8 +273,8 @@ void devm_devfreq_unregister_notifier(struct device *dev, struct devfreq *devfreq_get_devfreq_by_node(struct device_node *node); struct devfreq *devfreq_get_devfreq_by_phandle(struct device *dev, const char *phandle_name, int index); +#endif /* CONFIG_PM_DEVFREQ */ -#if IS_ENABLED(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND) /** * struct devfreq_simple_ondemand_data - ``void *data`` fed to struct devfreq * and devfreq_add_device @@ -292,9 +292,7 @@ struct devfreq_simple_ondemand_data { unsigned int upthreshold; unsigned int downdifferential; }; -#endif -#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE) enum devfreq_parent_dev_type { DEVFREQ_PARENT_DEV, CPUFREQ_PARENT_DEV, @@ -337,9 +335,8 @@ struct devfreq_passive_data { struct notifier_block nb; struct list_head cpu_data_list; }; -#endif -#else /* !CONFIG_PM_DEVFREQ */ +#if !defined(CONFIG_PM_DEVFREQ) static inline struct devfreq *devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile, const char *governor_name,