[PATCH v2] drm/msm/dp: add opp_table corner voting support base on dp_ink_clk rate
khsieh at codeaurora.org
khsieh at codeaurora.org
Sat Oct 10 21:31:30 UTC 2020
On 2020-10-06 00:31, Rajendra Nayak wrote:
> On 10/4/2020 3:56 AM, Kuogee Hsieh wrote:
>> Set link rate by using OPP set rate api so that CX level will be set
>> accordingly based on the link rate.
>>
>> Changes in v2:
>> -- remove dev from dp_ctrl_put() parameters
>> -- address review comments
>
> This needs to go below '---' and should not be part of the
> change log.
>
>>
>> Signed-off-by: Kuogee Hsieh <khsieh at codeaurora.org>
>> ---
>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 26 +++++++++++++++++
>> drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
>> drivers/gpu/drm/msm/dp/dp_power.c | 44
>> ++++++++++++++++++++++++++---
>> drivers/gpu/drm/msm/dp/dp_power.h | 2 +-
>> 4 files changed, 68 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 2e3e1917351f..6eb9cdad1421 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -10,6 +10,7 @@
>> #include <linux/delay.h>
>> #include <linux/phy/phy.h>
>> #include <linux/phy/phy-dp.h>
>> +#include <linux/pm_opp.h>
>> #include <drm/drm_fixed.h>
>> #include <drm/drm_dp_helper.h>
>> #include <drm/drm_print.h>
>> @@ -76,6 +77,8 @@ struct dp_ctrl_private {
>> struct dp_parser *parser;
>> struct dp_catalog *catalog;
>> + struct opp_table *opp_table;
>> +
>> struct completion idle_comp;
>> struct completion video_comp;
>> };
>> @@ -1836,6 +1839,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev,
>> struct dp_link *link,
>> struct dp_parser *parser)
>> {
>> struct dp_ctrl_private *ctrl;
>> + int ret;
>> if (!dev || !panel || !aux ||
>> !link || !catalog) {
>> @@ -1849,6 +1853,19 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev,
>> struct dp_link *link,
>> return ERR_PTR(-ENOMEM);
>> }
>> + ctrl->opp_table = dev_pm_opp_set_clkname(dev, "ctrl_link");
>> + if (IS_ERR(ctrl->opp_table)) {
>> + dev_err(dev, "invalid DP OPP table in device tree\n");
>
> You do this regardless of an OPP table in DT, so for starters the error
> message is wrong. Secondly this can return you a -EPROBE_DEFER if the
> clock driver isn't ready yet.
> So the ideal thing to do here, is return a PTR_ERR(ctrl->opp_table)
>
>> + ctrl->opp_table = NULL;
>> + } else {
>> + /* OPP table is optional */
>> + ret = dev_pm_opp_of_add_table(dev);
>> + if (ret && ret != -ENODEV) {
>> + dev_pm_opp_put_clkname(ctrl->opp_table);
>> + ctrl->opp_table = NULL;
>> + }
>> + }
>> +
>> init_completion(&ctrl->idle_comp);
>> init_completion(&ctrl->video_comp);
>> @@ -1866,4 +1883,13 @@ struct dp_ctrl *dp_ctrl_get(struct device
>> *dev, struct dp_link *link,
>> void dp_ctrl_put(struct dp_ctrl *dp_ctrl)
>> {
>> + struct dp_ctrl_private *ctrl;
>> +
>> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> +
>> + if (ctrl->opp_table) {
>> + dev_pm_opp_of_remove_table(ctrl->dev);
>> + dev_pm_opp_put_clkname(ctrl->opp_table);
>> + ctrl->opp_table = NULL;
>> + }
>> }
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index e175aa3fd3a9..269f83550b46 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -698,7 +698,7 @@ static int dp_init_sub_modules(struct
>> dp_display_private *dp)
>> goto error;
>> }
>> - dp->power = dp_power_get(dp->parser);
>> + dp->power = dp_power_get(dev, dp->parser);
>> if (IS_ERR(dp->power)) {
>> rc = PTR_ERR(dp->power);
>> DRM_ERROR("failed to initialize power, rc = %d\n", rc);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c
>> b/drivers/gpu/drm/msm/dp/dp_power.c
>> index 17c1fc6a2d44..9c4ea00a5f2a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_power.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
>> @@ -8,12 +8,14 @@
>> #include <linux/clk.h>
>> #include <linux/clk-provider.h>
>> #include <linux/regulator/consumer.h>
>> +#include <linux/pm_opp.h>
>> #include "dp_power.h"
>> #include "msm_drv.h"
>> struct dp_power_private {
>> struct dp_parser *parser;
>> struct platform_device *pdev;
>> + struct device *dev;
>> struct clk *link_clk_src;
>> struct clk *pixel_provider;
>> struct clk *link_provider;
>> @@ -148,18 +150,51 @@ static int dp_power_clk_deinit(struct
>> dp_power_private *power)
>> return 0;
>> }
>> +static int dp_power_clk_set_link_rate(struct dp_power_private
>> *power,
>> + struct dss_clk *clk_arry, int num_clk, int enable)
>> +{
>> + u32 rate;
>> + int i, rc = 0;
>> +
>> + for (i = 0; i < num_clk; i++) {
>> + if (clk_arry[i].clk) {
>> + if (clk_arry[i].type == DSS_CLK_PCLK) {
>> + if (enable)
>> + rate = clk_arry[i].rate;
>> + else
>> + rate = 0;
>> +
>> + rc = dev_pm_opp_set_rate(power->dev, rate);
>
> I am not sure how this is expected to work when you have multiple link
> clocks,
> since you can only associate one of them with the OPP table which ends
> up
> getting scaled when you do a dev_pm_opp_set_rate()
> Do you really have platforms which will have multiple link clocks?
this clk_arry[] contains two entries, dp_link_clk and dp_link_intf_clk.
only dp_link_clk with DSS_CLK_PCLK type, hence only dp_link_clk use
dev_pm_opp_set_rate()
to set link rate.
>
>> + if (rc)
>> + break;
>> + }
>> +
>> + }
>> + }
>> + return rc;
>> +}
>> +
>> static int dp_power_clk_set_rate(struct dp_power_private *power,
>> enum dp_pm_type module, bool enable)
>> {
>> int rc = 0;
>> struct dss_module_power *mp = &power->parser->mp[module];
>> - if (enable) {
>> - rc = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk);
>> + if (module == DP_CTRL_PM) {
>> + rc = dp_power_clk_set_link_rate(power, mp->clk_config, mp->num_clk,
>> enable);
>> if (rc) {
>> - DRM_ERROR("failed to set clks rate.\n");
>> + DRM_ERROR("failed to set link clks rate\n");
>> return rc;
>> }
>> + } else {
>> +
>
> extra blank line
>
>> + if (enable) {
>> + rc = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk);
>> + if (rc) {
>> + DRM_ERROR("failed to set clks rate\n");
>> + return rc;
>> + }
>> + }
>> }
>> rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, enable);
>> @@ -349,7 +384,7 @@ int dp_power_deinit(struct dp_power *dp_power)
>> return 0;
>> }
>> -struct dp_power *dp_power_get(struct dp_parser *parser)
>> +struct dp_power *dp_power_get(struct device *dev, struct dp_parser
>> *parser)
>> {
>> struct dp_power_private *power;
>> struct dp_power *dp_power;
>> @@ -365,6 +400,7 @@ struct dp_power *dp_power_get(struct dp_parser
>> *parser)
>> power->parser = parser;
>> power->pdev = parser->pdev;
>> + power->dev = dev;
>> dp_power = &power->dp_power;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_power.h
>> b/drivers/gpu/drm/msm/dp/dp_power.h
>> index 76743d755833..7d0327bbc0d5 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_power.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_power.h
>> @@ -102,6 +102,6 @@ void dp_power_client_deinit(struct dp_power
>> *power);
>> * methods to be called by the client to configure the power related
>> * modueles.
>> */
>> -struct dp_power *dp_power_get(struct dp_parser *parser);
>> +struct dp_power *dp_power_get(struct device *dev, struct dp_parser
>> *parser);
>> #endif /* _DP_POWER_H_ */
>>
>> base-commit: d1ea914925856d397b0b3241428f20b945e31434
>
> ??
More information about the dri-devel
mailing list