Re: [PATCH V3 07/10] OPP: Add dev_pm_opp_{set|put}_genpd_virt_dev() helper

[Ulf Hansson <ulf.hansson@linaro.org>]

On 25 October 2018 at 07:52, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Multiple generic power domains for a consumer device are supported with
> the help of virtual devices, which are created for each consumer device
> - genpd pair. These are the device structures which are attached to the
> power domain and are required by the OPP core to set the performance
> state of the genpd.
>
> The helpers added by this commit are required to be called once for each
> of these virtual devices. These are required only if multiple domains
> are available for a device, otherwise the actual device structure will
> be used instead by the OPP core.
>
> The new helpers also support the complex cases where the consumer device
> wouldn't always require all the domains. For example, a camera may
> require only one power domain during normal operations but two during
> high resolution operations. The consumer driver can call
> dev_pm_opp_put_genpd_virt_dev(high_resolution_genpd_virt_dev) if it is
> currently operating in the normal mode and doesn't have any performance
> requirements from the genpd which manages high resolution power
> requirements. The consumer driver can later call
> dev_pm_opp_set_genpd_virt_dev(high_resolution_genpd_virt_dev) once it
> switches back to the high resolution mode.
>
> The new helpers differ from other OPP set/put helpers as the new ones
> can be called with OPPs initialized for the table as we may need to call
> them on the fly because of the complex case explained above. For this
> reason it is possible that the genpd virt_dev structure may be used in
> parallel while the new helpers are running and a new mutex is added to
> protect against that. We didn't use the existing opp_table->lock mutex
> as that is widely used in the OPP core and we will need this lock in the
> dev_pm_opp_set_rate() helper while changing OPP and we need to make sure
> there is not much contention while doing that as that's the hotpath.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/opp/core.c     | 88 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/opp/of.c       | 16 +++++++-
>  drivers/opp/opp.h      |  4 ++
>  include/linux/pm_opp.h |  8 ++++
>  4 files changed, 115 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 02a69a62dac8..cef2ccda355d 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -823,6 +823,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>                 return NULL;
>
>         mutex_init(&opp_table->lock);
> +       mutex_init(&opp_table->genpd_virt_dev_lock);
>         INIT_LIST_HEAD(&opp_table->dev_list);
>
>         opp_dev = _add_opp_dev(dev, opp_table);
> @@ -920,6 +921,7 @@ static void _opp_table_kref_release(struct kref *kref)
>                 _remove_opp_dev(opp_dev, opp_table);
>         }
>
> +       mutex_destroy(&opp_table->genpd_virt_dev_lock);
>         mutex_destroy(&opp_table->lock);
>         list_del(&opp_table->node);
>         kfree(opp_table);
> @@ -1602,6 +1604,92 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_set_opp_helper);
>
> +/**
> + * dev_pm_opp_set_genpd_virt_dev - Set virtual genpd device for an index
> + * @dev: Consumer device for which the genpd device is getting set.
> + * @virt_dev: virtual genpd device.
> + * @index: index.
> + *
> + * Multiple generic power domains for a device are supported with the help of
> + * virtual genpd devices, which are created for each consumer device - genpd
> + * pair. These are the device structures which are attached to the power domain
> + * and are required by the OPP core to set the performance state of the genpd.
> + *
> + * This helper will normally be called by the consumer driver of the device
> + * "dev", as only that has details of the genpd devices.
> + *
> + * This helper needs to be called once for each of those virtual devices, but
> + * only if multiple domains are available for a device. Otherwise the original
> + * device structure will be used instead by the OPP core.
> + */
> +struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev,
> +                                               struct device *virt_dev,
> +                                               int index)
> +{
> +       struct opp_table *opp_table;
> +
> +       opp_table = dev_pm_opp_get_opp_table(dev);
> +       if (!opp_table)
> +               return ERR_PTR(-ENOMEM);
> +
> +       mutex_lock(&opp_table->genpd_virt_dev_lock);
> +
> +       if (unlikely(!opp_table->genpd_virt_devs ||
> +                    index >= opp_table->required_opp_count ||
> +                    opp_table->genpd_virt_devs[index])) {
> +
> +               dev_err(dev, "Invalid request to set required device\n");
> +               dev_pm_opp_put_opp_table(opp_table);
> +               mutex_unlock(&opp_table->genpd_virt_dev_lock);
> +
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       opp_table->genpd_virt_devs[index] = virt_dev;
> +       mutex_unlock(&opp_table->genpd_virt_dev_lock);
> +
> +       return opp_table;
> +}
> +
> +/**
> + * dev_pm_opp_put_genpd_virt_dev() - Releases resources blocked for genpd device.
> + * @opp_table: OPP table returned by dev_pm_opp_set_genpd_virt_dev().
> + * @virt_dev: virtual genpd device.
> + *
> + * This releases the resource previously acquired with a call to
> + * dev_pm_opp_set_genpd_virt_dev(). The consumer driver shall call this helper
> + * if it doesn't want OPP core to update performance state of a power domain
> + * anymore.
> + */
> +void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table,
> +                                  struct device *virt_dev)
> +{
> +       int i;
> +
> +       /*
> +        * Acquire genpd_virt_dev_lock to make sure virt_dev isn't getting
> +        * used in parallel.
> +        */
> +       mutex_lock(&opp_table->genpd_virt_dev_lock);
> +
> +       for (i = 0; i < opp_table->required_opp_count; i++) {
> +               if (opp_table->genpd_virt_devs[i] != virt_dev)
> +                       continue;
> +
> +               opp_table->genpd_virt_devs[i] = NULL;
> +               dev_pm_opp_put_opp_table(opp_table);
> +
> +               /* Drop the vote */
> +               dev_pm_genpd_set_performance_state(virt_dev, 0);
> +               break;
> +       }
> +
> +       mutex_unlock(&opp_table->genpd_virt_dev_lock);
> +
> +       if (unlikely(i == opp_table->required_opp_count))
> +               dev_err(virt_dev, "Failed to find required device entry\n");
> +}
> +
>  /**
>   * dev_pm_opp_add()  - Add an OPP table from a table definitions
>   * @dev:       device for which we do this operation
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index ffaeefef98ce..71aef28953c2 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -134,6 +134,7 @@ static struct opp_table *_find_table_of_opp_np(struct device_node *opp_np)
>  static void _opp_table_free_required_tables(struct opp_table *opp_table)
>  {
>         struct opp_table **required_opp_tables = opp_table->required_opp_tables;
> +       struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
>         int i;
>
>         if (!required_opp_tables)
> @@ -147,8 +148,10 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
>         }
>
>         kfree(required_opp_tables);
> +       kfree(genpd_virt_devs);
>
>         opp_table->required_opp_count = 0;
> +       opp_table->genpd_virt_devs = NULL;
>         opp_table->required_opp_tables = NULL;
>  }
>
> @@ -161,6 +164,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>                                              struct device_node *opp_np)
>  {
>         struct opp_table **required_opp_tables;
> +       struct device **genpd_virt_devs = NULL;
>         struct device_node *required_np, *np;
>         int count, i;
>
> @@ -175,11 +179,21 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>         if (!count)
>                 goto put_np;
>
> +       if (count > 1) {
> +               genpd_virt_devs = kcalloc(count, sizeof(*genpd_virt_devs),
> +                                       GFP_KERNEL);
> +               if (!genpd_virt_devs)
> +                       goto put_np;
> +       }
> +
>         required_opp_tables = kcalloc(count, sizeof(*required_opp_tables),
>                                       GFP_KERNEL);
> -       if (!required_opp_tables)
> +       if (!required_opp_tables) {
> +               kfree(genpd_virt_devs);
>                 goto put_np;
> +       }
>
> +       opp_table->genpd_virt_devs = genpd_virt_devs;
>         opp_table->required_opp_tables = required_opp_tables;
>         opp_table->required_opp_count = count;
>
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 24b340ad18d1..8aec38792cae 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -135,6 +135,8 @@ enum opp_table_access {
>   * @parsed_static_opps: True if OPPs are initialized from DT.
>   * @shared_opp: OPP is shared between multiple devices.
>   * @suspend_opp: Pointer to OPP to be used during device suspend.
> + * @genpd_virt_dev_lock: Mutex protecting the genpd virtual device pointers.
> + * @genpd_virt_devs: List of virtual devices for multiple genpd support.
>   * @required_opp_tables: List of device OPP tables that are required by OPPs in
>   *             this table.
>   * @required_opp_count: Number of required devices.
> @@ -177,6 +179,8 @@ struct opp_table {
>         enum opp_table_access shared_opp;
>         struct dev_pm_opp *suspend_opp;
>
> +       struct mutex genpd_virt_dev_lock;
> +       struct device **genpd_virt_devs;
>         struct opp_table **required_opp_tables;
>         unsigned int required_opp_count;
>
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 5d399eeef172..8fed222c089b 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -126,6 +126,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char * name);
>  void dev_pm_opp_put_clkname(struct opp_table *opp_table);
>  struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
>  void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
> +struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev, struct device *virt_dev, int index);
> +void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table, struct device *virt_dev);
>  int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
>  int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
>  int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
> @@ -272,6 +274,12 @@ static inline struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const
>
>  static inline void dev_pm_opp_put_clkname(struct opp_table *opp_table) {}
>
> +static inline struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev, struct device *virt_dev, int index)
> +{
> +       return ERR_PTR(-ENOTSUPP);
> +}
> +
> +static inline void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table, struct device *virt_dev) {}
>  static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  {
>         return -ENOTSUPP;
> --
> 2.19.1.568.g152ad8e3369a
>