| Message ID | 1527745019-25155-1-git-send-email-akhilpo@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
> Currently, DEVFREQ reevaluates the device state periodically and/or > based on the OPP list changes. Private API has to be exposed to allow > the device driver to alert/notify the governor to reevaluate when a new > set of data is available. This makes the governor more coupled to a > particular device driver. We can improve here by exposing a DEVFREQ API > to allow the device drivers to send generic alerts to the governor. > > Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> > --- > drivers/devfreq/devfreq.c | 21 +++++++++++++++++++++ > drivers/devfreq/governor.h | 1 + > include/linux/devfreq.h | 5 +++++ > 3 files changed, 27 insertions(+) > Hello Akhil, It appears that this will have the same effect with "[PATCH 08/11] PM / devfreq: Make update_devfreq() public" from Matthias Kaehlcke, doesn't it? Cheers, MyungJoo
On 5/31/2018 11:47 AM, MyungJoo Ham wrote: >> Currently, DEVFREQ reevaluates the device state periodically and/or >> based on the OPP list changes. Private API has to be exposed to allow >> the device driver to alert/notify the governor to reevaluate when a new >> set of data is available. This makes the governor more coupled to a >> particular device driver. We can improve here by exposing a DEVFREQ API >> to allow the device drivers to send generic alerts to the governor. >> >> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> >> --- >> drivers/devfreq/devfreq.c | 21 +++++++++++++++++++++ >> drivers/devfreq/governor.h | 1 + >> include/linux/devfreq.h | 5 +++++ >> 3 files changed, 27 insertions(+) >> > Hello Akhil, > > It appears that this will have the same effect with > "[PATCH 08/11] PM / devfreq: Make update_devfreq() public" from Matthias Kaehlcke, doesn't it? > > > Cheers, > MyungJoo > Hi MyngJoo, The patch you mentioned is a step in the right direction. But this patch allows: 1. the governor to decide whether to reevaluate or not. I feel it would be a better architecture (better Separation of Concern) if that decision is left to the governor alone. 2. the devices to share multiple types of alerts. A governor may use these alerts for internal bookkeeping/algorithm and decide to reevaluate policy when it is necessary. Since we are opening up a new devfreq API for devices, isn't it better to go for a generic one? Regards, Akhil.
Hi Akhil, Actually, this provides custom events for governors, with different behaviors per each governor, while Matthias's give you the effects not targeted for a specific governor. Could you please show me why (the usage cases) you need this additional event for governors? (You need to implement an event handler in each governor for this approach). Cheers, MyungJoo On Fri, Jun 1, 2018 at 8:43 PM, Akhil P Oommen <akhilpo@codeaurora.org> wrote: > > > On 5/31/2018 11:47 AM, MyungJoo Ham wrote: >>> >>> Currently, DEVFREQ reevaluates the device state periodically and/or >>> based on the OPP list changes. Private API has to be exposed to allow >>> the device driver to alert/notify the governor to reevaluate when a new >>> set of data is available. This makes the governor more coupled to a >>> particular device driver. We can improve here by exposing a DEVFREQ API >>> to allow the device drivers to send generic alerts to the governor. >>> >>> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> >>> --- >>> drivers/devfreq/devfreq.c | 21 +++++++++++++++++++++ >>> drivers/devfreq/governor.h | 1 + >>> include/linux/devfreq.h | 5 +++++ >>> 3 files changed, 27 insertions(+) >>> >> Hello Akhil, >> >> It appears that this will have the same effect with >> "[PATCH 08/11] PM / devfreq: Make update_devfreq() public" from Matthias >> Kaehlcke, doesn't it? >> >> >> Cheers, >> MyungJoo >> > Hi MyngJoo, > > The patch you mentioned is a step in the right direction. But this patch > allows: > 1. the governor to decide whether to reevaluate or not. I feel it would be a > better architecture (better Separation of Concern) if that decision is left > to the governor alone. > 2. the devices to share multiple types of alerts. A governor may use these > alerts for internal bookkeeping/algorithm and decide to reevaluate policy > when it is necessary. Since we are opening up a new devfreq API for devices, > isn't it better to go for a generic one? > > Regards, > Akhil.
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index fe2af6a..24a8046 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -1532,3 +1532,24 @@ void devm_devfreq_unregister_notifier(struct device *dev, devm_devfreq_dev_match, devfreq)); } EXPORT_SYMBOL(devm_devfreq_unregister_notifier); + +/** + * devfreq_alert_governor() - Alert governor about an event + * @devfreq: The devfreq object. + * @type: Optional alert type. + * + * This function lets the device alert the governor about an event. + * A governor may not implement or choose to completely ignore this alert. + */ +void devfreq_alert_governor(struct devfreq *devfreq, unsigned int type) +{ + /* Don't let someone change the governor until we are done here. */ + mutex_lock(&devfreq_list_lock); + + if (devfreq) + devfreq->governor->event_handler(devfreq, + DEVFREQ_GOV_ALERT, &type); + + mutex_unlock(&devfreq_list_lock); +} +EXPORT_SYMBOL(devfreq_alert_governor); diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h index cfc50a6..e5da3442 100644 --- a/drivers/devfreq/governor.h +++ b/drivers/devfreq/governor.h @@ -24,6 +24,7 @@ #define DEVFREQ_GOV_INTERVAL 0x3 #define DEVFREQ_GOV_SUSPEND 0x4 #define DEVFREQ_GOV_RESUME 0x5 +#define DEVFREQ_GOV_ALERT 0x6 /** * struct devfreq_governor - Devfreq policy governor diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index 3aae5b3..740c228 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -193,6 +193,7 @@ extern struct devfreq *devm_devfreq_add_device(struct device *dev, void *data); extern void devm_devfreq_remove_device(struct device *dev, struct devfreq *devfreq); +extern void devfreq_alert_governor(struct devfreq *devfreq, unsigned int type); /* Supposed to be called by PM callbacks */ extern int devfreq_suspend_device(struct devfreq *devfreq); @@ -306,6 +307,10 @@ static inline void devm_devfreq_remove_device(struct device *dev, { } +static void devfreq_alert_governor(struct devfreq *devfreq, unsigned int type) +{ +} + static inline int devfreq_suspend_device(struct devfreq *devfreq) { return 0;
Currently, DEVFREQ reevaluates the device state periodically and/or based on the OPP list changes. Private API has to be exposed to allow the device driver to alert/notify the governor to reevaluate when a new set of data is available. This makes the governor more coupled to a particular device driver. We can improve here by exposing a DEVFREQ API to allow the device drivers to send generic alerts to the governor. Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> --- drivers/devfreq/devfreq.c | 21 +++++++++++++++++++++ drivers/devfreq/governor.h | 1 + include/linux/devfreq.h | 5 +++++ 3 files changed, 27 insertions(+)