On Fri, 2014-01-17 at 17:32 +0800, Wei Ni wrote:
> On 01/17/2014 04:22 PM, Zhang Rui wrote:
> > On Mon, 2014-01-13 at 19:17 +0800, Wei Ni wrote:
> >> In the of-thermal driver, it register a thermal zone device
> >> without governor, since the governers are not static, we
> >> should be able to update to a new one in run-time, so I add
> >> a new fucntion thermal_update_governor() to do it.
> >>
> >> In the future, the governor will be more complex, it may want
> >> to tune governor-specific parameters/configuration for
> >> different thermal zone at different run-time, so I add
> >> start/stop for the governor and add governor_data as governor's
> >> parameters, so that the thermal platform driver can change
> >> the governor with start/stop, and tune the governor parameters
> >> in run-time.
> >
> > are you going to introduce a new governor with .start/.stop implemented
> > or are you going to introduce .start/.stop callbacks for the current
> > governors?
>
> We developed a new governor, based on PID logic. It will need some
> parameters, such as proportional, integral and derivative. These values
> will be initialized in different values for different thermal zone.
> When the thermal driver use the pid governor, the thermal framework will
> call the pid governor's .start(thz), and in the .start(), it will
> allocate structures for those parameters, which are in the thermal zone,
> something like:
> int pid_thermal_gov_start(struct thermal_zone_device *tz)
> {
> ...
> gov = kzalloc(sizeof(struct pid_thermal_governor), GFP_KERNEL);
> gov->parameter_1 = tz->governor_params->parameter_1;
> gov->parameter_2 = tz->governor_params->parameter_2;
> ...
> tz->governor_data = gov;
>
> /* and also will create ABI for the pid governor in this thermal zone */
> }
> So when run .throttle(), it can get those parameters from the
> tz->governor_data to run.
> And in the .stop, it will free those structures.
>
> I'm preparing the patches for the pid governor, and need to be reviewed
> internal first, and then I will try to upstream them :)
>
sounds interesting.
but the problem is still that you're doing two things in this patch,
one is to introduce runtime-governor tuning support,
another is to introduce two new callbacks for thermal governor.
For me, I'd like to take a patch that does the first thing ONLY, right
now, and I'd also like to see a patch that does the second thing,
together with the new governor patches.
thanks,
rui
> >
> > I'd like to see if there is any real request for this.
> >>
> >> Signed-off-by: Wei Ni <wni@xxxxxxxxxx>
> >> ---
> >> Documentation/thermal/sysfs-api.txt | 5 +++
> >> drivers/thermal/thermal_core.c | 80 +++++++++++++++++++++++++++++++----
> >> include/linux/thermal.h | 12 ++++++
> >> 3 files changed, 88 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> >> index 6a70b55c..7efd8e6 100644
> >> --- a/Documentation/thermal/sysfs-api.txt
> >> +++ b/Documentation/thermal/sysfs-api.txt
> >> @@ -405,3 +405,8 @@ platform data is provided, this uses the step_wise throttling policy.
> >> This function serves as an arbitrator to set the state of a cooling
> >> device. It sets the cooling device to the deepest cooling state if
> >> possible.
> >> +
> >> +5.5:thermal_update_governor:
> >> +This function update the thermal zone's governor to a new one and update
> >> +the governor data of the new governor for that thermal zone. Returns 0 if
> >> +success, an erro code otherwise.
> >> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> >> index aab1df8..d922baf 100644
> >> --- a/drivers/thermal/thermal_core.c
> >> +++ b/drivers/thermal/thermal_core.c
> >> @@ -93,10 +93,17 @@ int thermal_register_governor(struct thermal_governor *governor)
> >> name = pos->tzp->governor_name;
> >> else
> >> name = DEFAULT_THERMAL_GOVERNOR;
> >> - if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH))
> >> + if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH)) {
> >> + if (governor->start) {
> >> + err = governor->start(pos);
> >> + if (err < 0)
> >> + goto exit;
> >> + }
> >> pos->governor = governor;
> >> + }
> >> }
> >>
> >> +exit:
> >> mutex_unlock(&thermal_list_lock);
> >> mutex_unlock(&thermal_governor_lock);
> >>
> >> @@ -119,8 +126,11 @@ void thermal_unregister_governor(struct thermal_governor *governor)
> >>
> >> list_for_each_entry(pos, &thermal_tz_list, node) {
> >> if (!strnicmp(pos->governor->name, governor->name,
> >> - THERMAL_NAME_LENGTH))
> >> + THERMAL_NAME_LENGTH)) {
> >> + if (pos->governor->stop)
> >> + pos->governor->stop(pos);
> >> pos->governor = NULL;
> >> + }
> >> }
> >>
> >> mutex_unlock(&thermal_list_lock);
> >> @@ -130,6 +140,51 @@ exit:
> >> return;
> >> }
> >>
> >> +/**
> >> + * thermal_update_governor() - update the thermal zone device's governor
> >> + * to a new one.
> >> + * @tzd: pointer of thermal zone device, which need to update governor.
> >> + * @name: new governor name.
> >> + * @gov_data: governor data for the new governor if needed.
> >> + *
> >> + * Return: On success returns 0, an error code otherwise
> >> + */
> >> +int thermal_update_governor(struct thermal_zone_device *tzd,
> >> + const char *name, void *gov_data)
> >> +{
> >> + struct thermal_governor *old_gov, *new_gov;
> >> + int ret = 0;
> >> +
> >> + mutex_lock(&thermal_governor_lock);
> >> +
> >> + new_gov = __find_governor(name);
> >> + if (!new_gov) {
> >> + ret = -EINVAL;
> >> + goto exit;
> >> + }
> >> +
> >> + old_gov = tzd->governor;
> >> +
> >> + if (old_gov && old_gov->stop)
> >> + old_gov->stop(tzd);
> >> +
> >> + if (new_gov->start) {
> >> + ret = new_gov->start(tzd);
> >> + if (ret < 0) {
> >> + if (old_gov && old_gov->start)
> >> + old_gov->start(tzd);
> >> + goto exit;
> >> + }
> >> + }
> >> +
> >> + tzd->governor = new_gov;
> >> + tzd->governor_data = gov_data;
> >> +
> >> +exit:
> >> + mutex_unlock(&thermal_governor_lock);
> >> + return ret;
> >> +}
> >> +
> >> static int get_idr(struct idr *idr, struct mutex *lock, int *id)
> >> {
> >> int ret;
> >> @@ -734,22 +789,17 @@ policy_store(struct device *dev, struct device_attribute *attr,
> >> {
> >> int ret = -EINVAL;
> >> struct thermal_zone_device *tz = to_thermal_zone(dev);
> >> - struct thermal_governor *gov;
> >> char name[THERMAL_NAME_LENGTH];
> >>
> >> snprintf(name, sizeof(name), "%s", buf);
> >>
> >> - mutex_lock(&thermal_governor_lock);
> >> -
> >> - gov = __find_governor(strim(name));
> >> - if (!gov)
> >> + ret = thermal_update_governor(tz, strim(name), NULL);
> >> + if (ret < 0)
> >> goto exit;
> >>
> >> - tz->governor = gov;
> >> ret = count;
> >>
> >> exit:
> >> - mutex_unlock(&thermal_governor_lock);
> >> return ret;
> >> }
> >>
> >> @@ -1559,6 +1609,12 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
> >> else
> >> tz->governor = __find_governor(DEFAULT_THERMAL_GOVERNOR);
> >>
> >> + if (tz->governor->start) {
> >
> > Note that tz->governor may be NULL at this time.
> > although this seems like a bug to me.
>
> Yes, I have noticed this bug.
> BTW, Eduardo have submitted a new patch to set the governor to default
> governor [PATCH 1/1] thermal: fix default governor assignment .
>
> Anyway, in here, I should fix it as:
> if (tz->governor && tz->governor->start)
>
>
> Thanks.
> Wei.
>
> >
> >> + result = tz->governor->start(tz);
> >> + if (result < 0)
> >> + tz->governor = NULL;
> >> + }
> >> +
> > why not invoke thermal_update_governor() directly here?
>
> Oh, yes, we can invoke it here.
>
> >
> > thanks,
> > rui
> >> mutex_unlock(&thermal_governor_lock);
> >>
> >> if (!tz->tzp || !tz->tzp->no_hwmon) {
> >> @@ -1585,6 +1641,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
> >> return tz;
> >>
> >> unregister:
> >> + if (tz->governor && tz->governor->stop)
> >> + tz->governor->stop(tz);
> >> + tz->governor = NULL;
> >> release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
> >> device_unregister(&tz->device);
> >> return ERR_PTR(result);
> >> @@ -1648,6 +1707,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
> >> device_remove_file(&tz->device, &dev_attr_policy);
> >> device_remove_file(&tz->device, &dev_attr_available_policies);
> >> remove_trip_attrs(tz);
> >> +
> >> + if (tz->governor && tz->governor->stop)
> >> + tz->governor->stop(tz);
> >> tz->governor = NULL;
> >>
> >
> >> thermal_remove_hwmon_sysfs(tz);
> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >> index f7e11c7..a473736 100644
> >> --- a/include/linux/thermal.h
> >> +++ b/include/linux/thermal.h
> >> @@ -177,6 +177,7 @@ struct thermal_zone_device {
> >> struct thermal_zone_device_ops *ops;
> >> const struct thermal_zone_params *tzp;
> >> struct thermal_governor *governor;
> >> + void *governor_data;
> >> struct list_head thermal_instances;
> >> struct idr idr;
> >> struct mutex lock; /* protect thermal_instances list */
> >> @@ -187,6 +188,15 @@ struct thermal_zone_device {
> >> /* Structure that holds thermal governor information */
> >> struct thermal_governor {
> >> char name[THERMAL_NAME_LENGTH];
> >> +
> >> + /*
> >> + * The start and stop operations will be called when thermal zone is
> >> + * registered and when change governor via sysfs or driver in running
> >> + * time.
> >> + */
> >> + int (*start)(struct thermal_zone_device *tz);
> >> + void (*stop)(struct thermal_zone_device *tz);
> >> +
> >> int (*throttle)(struct thermal_zone_device *tz, int trip);
> >> struct list_head governor_list;
> >> };
> >> @@ -293,6 +303,8 @@ struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
> >> struct thermal_cooling_device *, int);
> >> void thermal_cdev_update(struct thermal_cooling_device *);
> >> void thermal_notify_framework(struct thermal_zone_device *, int);
> >> +int thermal_update_governor(struct thermal_zone_device *, const char *,
> >> + void *);
> >>
> >> #ifdef CONFIG_NET
> >> extern int thermal_generate_netlink_event(struct thermal_zone_device *tz,
> >
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
