Re: [DPU PATCH 07/11] drm/msm/dpu: remove clock management code from dpu_power_handle

[Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]

On Thu, May 10, 2018 at 01:59:41PM +0530, Rajesh Yadav wrote:
> MDSS and dpu drivers manage their respective clocks via
> runtime_pm. Remove custom clock management code from
> dpu_power_handle.
> 
> Also dpu core clock management code is restricted to
> dpu_core_perf module.
> 
> Signed-off-by: Rajesh Yadav <ryadav@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c      |  44 ++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h      |   8 +-
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |   5 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            |  32 +++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h            |   9 +
>  drivers/gpu/drm/msm/dpu_power_handle.c             | 195 +--------------------
>  drivers/gpu/drm/msm/dpu_power_handle.h             |  40 -----
>  7 files changed, 69 insertions(+), 264 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index 981f77f..d1364fa 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -365,6 +365,20 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc)
>  	}
>  }
>  
> +static int _dpu_core_perf_set_core_clk_rate(struct dpu_kms *kms, u64 rate)
> +{
> +	struct dss_clk *core_clk = kms->perf.core_clk;
> +	int rc = -EINVAL;

No need to initialize this.

> +
> +	if (core_clk->max_rate && (rate > core_clk->max_rate))
> +		rate = core_clk->max_rate;
> +
> +	core_clk->rate = rate;
> +	rc = msm_dss_clk_set_rate(core_clk, 1);
> +
> +	return rc;

return msm_dss_clk_set_rate(core_clk, 1);

and get rid of rc entirely

> +}
> +
>  static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>  {
>  	u64 clk_rate = kms->perf.perf_tune.min_core_clk;
> @@ -376,7 +390,8 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>  			dpu_cstate = to_dpu_crtc_state(crtc->state);
>  			clk_rate = max(dpu_cstate->new_perf.core_clk_rate,
>  							clk_rate);
> -			clk_rate = clk_round_rate(kms->perf.core_clk, clk_rate);
> +			clk_rate = clk_round_rate(kms->perf.core_clk->clk,
> +					clk_rate);
>  		}
>  	}
>  
> @@ -484,15 +499,11 @@ void dpu_core_perf_crtc_update(struct drm_crtc *crtc,

This function should really propagate the errors. I realize they're eventually
dropped in the frame event worker, but we should still return an error here in
case the worker ever wants to use it (and it should be the one to decide to drop
it if that's the best decision). Although this wasn't introduced with your
patch, could you please add a small patch to the v2 to add this?

>  
>  		DPU_EVT32(kms->dev, stop_req, clk_rate);
>  
> -		/* Temp change to avoid crash in clk_set_rate API. */
> -#ifdef QCOM_DPU_SET_CLK
> -		if (dpu_power_clk_set_rate(&priv->phandle,
> -					   kms->perf.clk_name, clk_rate)) {
> +		if (_dpu_core_perf_set_core_clk_rate(kms, clk_rate)) {
>  			DPU_ERROR("failed to set %s clock rate %llu\n",
> -					kms->perf.clk_name, clk_rate);
> +					kms->perf.core_clk->clk_name, clk_rate);
>  			return;
>  		}
> -#endif
>  
>  		kms->perf.core_clk_rate = clk_rate;
>  		DPU_DEBUG("update clk rate = %lld HZ\n", clk_rate);
> @@ -656,7 +667,6 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf)
>  	dpu_core_perf_debugfs_destroy(perf);
>  	perf->max_core_clk_rate = 0;
>  	perf->core_clk = NULL;
> -	perf->clk_name = NULL;
>  	perf->phandle = NULL;
>  	perf->catalog = NULL;
>  	perf->dev = NULL;
> @@ -667,9 +677,9 @@ int dpu_core_perf_init(struct dpu_core_perf *perf,
>  		struct dpu_mdss_cfg *catalog,
>  		struct dpu_power_handle *phandle,
>  		struct dpu_power_client *pclient,
> -		char *clk_name)
> +		struct dss_clk *core_clk)
>  {
> -	if (!perf || !dev || !catalog || !phandle || !pclient || !clk_name) {
> +	if (!perf || !dev || !catalog || !phandle || !pclient || !core_clk) {

These blind NULL checks drive me a little crazy. How many of these can 
_actually_ be NULL? When we encounter these, we should evaluate and trim them so
as not to clutter up every function with superfluous checks.

>  		DPU_ERROR("invalid parameters\n");
>  		return -EINVAL;
>  	}
> @@ -678,23 +688,13 @@ int dpu_core_perf_init(struct dpu_core_perf *perf,
>  	perf->catalog = catalog;
>  	perf->phandle = phandle;
>  	perf->pclient = pclient;
> -	perf->clk_name = clk_name;
> +	perf->core_clk = core_clk;
>  
> -	perf->core_clk = dpu_power_clk_get_clk(phandle, clk_name);
> -	if (!perf->core_clk) {
> -		DPU_ERROR("invalid core clk\n");
> -		goto err;
> -	}
> -
> -	perf->max_core_clk_rate = dpu_power_clk_get_max_rate(phandle, clk_name);
> +	perf->max_core_clk_rate = core_clk->max_rate;
>  	if (!perf->max_core_clk_rate) {
>  		DPU_DEBUG("optional max core clk rate, use default\n");
>  		perf->max_core_clk_rate = DPU_PERF_DEFAULT_MAX_CORE_CLK_RATE;
>  	}
>  
>  	return 0;
> -
> -err:
> -	dpu_core_perf_destroy(perf);
> -	return -ENODEV;
>  }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index 1965ff5..9c1a719 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -54,7 +54,6 @@ struct dpu_core_perf_tune {
>   * @catalog: Pointer to catalog configuration
>   * @phandle: Pointer to power handler
>   * @pclient: Pointer to power client
> - * @clk_name: core clock name
>   * @core_clk: Pointer to core clock structure
>   * @core_clk_rate: current core clock rate
>   * @max_core_clk_rate: maximum allowable core clock rate
> @@ -70,8 +69,7 @@ struct dpu_core_perf {
>  	struct dpu_mdss_cfg *catalog;
>  	struct dpu_power_handle *phandle;
>  	struct dpu_power_client *pclient;
> -	char *clk_name;
> -	struct clk *core_clk;
> +	struct dss_clk *core_clk;
>  	u64 core_clk_rate;
>  	u64 max_core_clk_rate;
>  	struct dpu_core_perf_tune perf_tune;
> @@ -118,14 +116,14 @@ void dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>   * @catalog: Pointer to catalog
>   * @phandle: Pointer to power handle
>   * @pclient: Pointer to power client
> - * @clk_name: core clock name
> + * @core_clk: pointer to core clock
>   */
>  int dpu_core_perf_init(struct dpu_core_perf *perf,
>  		struct drm_device *dev,
>  		struct dpu_mdss_cfg *catalog,
>  		struct dpu_power_handle *phandle,
>  		struct dpu_power_client *pclient,
> -		char *clk_name);
> +		struct dss_clk *core_clk);
>  
>  /**
>   * dpu_core_perf_debugfs_init - initialize debugfs for core performance context
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 072939c..a9f946b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -428,8 +428,9 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
>  	 * vsync_count is ratio of MDP VSYNC clock frequency to LCD panel
>  	 * frequency divided by the no. of rows (lines) in the LCDpanel.
>  	 */
> -	vsync_hz = dpu_power_clk_get_rate(&priv->phandle, "vsync_clk");
> -	if (!vsync_hz || !mode->vtotal || !mode->vrefresh) {
> +	vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync_clk");
> +	if ((vsync_hz == -EINVAL) || !vsync_hz || !mode->vtotal ||

Instead of checking vsync_hz twice, just do vsync_hz <= 0.

I'm also a little concerned as to how vtotal could be 0...

> +		!mode->vrefresh) {
>  		DPU_DEBUG_CMDENC(cmd_enc,
>  			"invalid params - vsync_hz %u vtot %u vrefresh %u\n",
>  			vsync_hz, mode->vtotal, mode->vrefresh);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 6dd0d8e..f6511c9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1502,6 +1502,35 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
>  	return ret;
>  }
>  
> +static struct dss_clk *_dpu_kms_get_clk(struct dpu_kms *dpu_kms,
> +		char *clock_name)
> +{
> +	struct dss_module_power *mp = &dpu_kms->mp;
> +	int i;
> +	struct dss_clk *clk = NULL;
> +
> +	for (i = 0; i < mp->num_clk; i++) {
> +		if (!strcmp(mp->clk_config[i].clk_name, clock_name)) {
> +			clk = &mp->clk_config[i];
> +			break;

                        return &mp->clk_config[i];

This allows you to eliminate clk variable.

> +		}
> +	}
> +
> +	return clk;

return NULL;

> +}
> +
> +u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name)
> +{
> +	u64 rate = -EINVAL;
> +	struct dss_clk *clk;
> +
> +	clk = _dpu_kms_get_clk(dpu_kms, clock_name);
> +	if (clk)
> +		rate = clk_get_rate(clk->clk);
> +
> +	return rate;

        if (!clk)
                return -EINVAL;

        return clk_get_rate(clk->clk);


and remove rate variable

> +}
> +
>  static void dpu_kms_handle_power_event(u32 event_type, void *usr)
>  {
>  	struct dpu_kms *dpu_kms = usr;
> @@ -1699,7 +1728,8 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>  #endif
>  
>  	rc = dpu_core_perf_init(&dpu_kms->perf, dev, dpu_kms->catalog,
> -			&priv->phandle, priv->pclient, "core_clk");
> +			&priv->phandle, priv->pclient,
> +			_dpu_kms_get_clk(dpu_kms, "core_clk"));
>  	if (rc) {
>  		DPU_ERROR("failed to init perf %d\n", rc);
>  		goto perf_err;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index 3c69921..a8255fe 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -537,4 +537,13 @@ struct dpu_kms_fbo *dpu_kms_fbo_alloc(struct drm_device *dev,
>  
>  void dpu_kms_encoder_enable(struct drm_encoder *encoder);
>  
> +/**
> + * dpu_kms_get_clk_rate() - get the clock rate
> + * @dpu_kms:  poiner to dpu_kms structure
> + * @clock_name: clock name to get the rate
> + *
> + * Return: current clock rate
> + */
> +u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
> +
>  #endif /* __dpu_kms_H__ */
> diff --git a/drivers/gpu/drm/msm/dpu_power_handle.c b/drivers/gpu/drm/msm/dpu_power_handle.c
> index e9e344a..17bae4b 100644
> --- a/drivers/gpu/drm/msm/dpu_power_handle.c
> +++ b/drivers/gpu/drm/msm/dpu_power_handle.c
> @@ -13,7 +13,6 @@
>  
>  #define pr_fmt(fmt)	"[drm:%s:%d]: " fmt, __func__, __LINE__
>  
> -#include <linux/clk.h>
>  #include <linux/kernel.h>
>  #include <linux/of.h>
>  #include <linux/string.h>
> @@ -246,62 +245,6 @@ static int dpu_power_parse_dt_supply(struct platform_device *pdev,
>  	return rc;
>  }
>  
> -static int dpu_power_parse_dt_clock(struct platform_device *pdev,
> -					struct dss_module_power *mp)
> -{
> -	u32 i = 0, rc = 0;
> -	const char *clock_name;
> -	u32 clock_rate = 0;
> -	u32 clock_max_rate = 0;
> -	int num_clk = 0;
> -
> -	if (!pdev || !mp) {
> -		pr_err("invalid input param pdev:%pK mp:%pK\n", pdev, mp);
> -		return -EINVAL;
> -	}
> -
> -	mp->num_clk = 0;
> -	num_clk = of_property_count_strings(pdev->dev.of_node,
> -							"clock-names");
> -	if (num_clk <= 0) {
> -		pr_debug("clocks are not defined\n");
> -		goto clk_err;
> -	}
> -
> -	mp->num_clk = num_clk;
> -	mp->clk_config = devm_kzalloc(&pdev->dev,
> -			sizeof(struct dss_clk) * num_clk, GFP_KERNEL);
> -	if (!mp->clk_config) {
> -		rc = -ENOMEM;
> -		mp->num_clk = 0;
> -		goto clk_err;
> -	}
> -
> -	for (i = 0; i < num_clk; i++) {
> -		of_property_read_string_index(pdev->dev.of_node, "clock-names",
> -							i, &clock_name);
> -		strlcpy(mp->clk_config[i].clk_name, clock_name,
> -				sizeof(mp->clk_config[i].clk_name));
> -
> -		of_property_read_u32_index(pdev->dev.of_node, "clock-rate",
> -							i, &clock_rate);
> -		mp->clk_config[i].rate = clock_rate;
> -
> -		if (!clock_rate)
> -			mp->clk_config[i].type = DSS_CLK_AHB;
> -		else
> -			mp->clk_config[i].type = DSS_CLK_PCLK;
> -
> -		clock_max_rate = 0;
> -		of_property_read_u32_index(pdev->dev.of_node, "clock-max-rate",
> -							i, &clock_max_rate);
> -		mp->clk_config[i].max_rate = clock_max_rate;
> -	}
> -
> -clk_err:
> -	return rc;
> -}
> -
>  #ifdef CONFIG_QCOM_BUS_SCALING
>  
>  #define MAX_AXI_PORT_COUNT 3
> @@ -681,16 +624,10 @@ int dpu_power_resource_init(struct platform_device *pdev,
>  	mp = &phandle->mp;
>  	phandle->dev = &pdev->dev;
>  
> -	rc = dpu_power_parse_dt_clock(pdev, mp);
> -	if (rc) {
> -		pr_err("device clock parsing failed\n");
> -		goto end;
> -	}
> -
>  	rc = dpu_power_parse_dt_supply(pdev, mp);
>  	if (rc) {
>  		pr_err("device vreg supply parsing failed\n");
> -		goto parse_vreg_err;
> +		goto end;

Just return directly and remove the end label.

>  	}
>  
>  	rc = msm_dss_config_vreg(&pdev->dev,
> @@ -700,18 +637,6 @@ int dpu_power_resource_init(struct platform_device *pdev,
>  		goto vreg_err;
>  	}
>  
> -	rc = msm_dss_get_clk(&pdev->dev, mp->clk_config, mp->num_clk);
> -	if (rc) {
> -		pr_err("clock get failed rc=%d\n", rc);
> -		goto clk_err;
> -	}
> -
> -	rc = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk);
> -	if (rc) {
> -		pr_err("clock set rate failed rc=%d\n", rc);
> -		goto bus_err;
> -	}
> -
>  	rc = dpu_power_reg_bus_parse(pdev, phandle);
>  	if (rc) {
>  		pr_err("register bus parse failed rc=%d\n", rc);
> @@ -742,17 +667,11 @@ int dpu_power_resource_init(struct platform_device *pdev,
>  		dpu_power_data_bus_unregister(&phandle->data_bus_handle[i]);
>  	dpu_power_reg_bus_unregister(phandle->reg_bus_hdl);
>  bus_err:
> -	msm_dss_put_clk(mp->clk_config, mp->num_clk);
> -clk_err:
>  	msm_dss_config_vreg(&pdev->dev, mp->vreg_config, mp->num_vreg, 0);
>  vreg_err:
>  	if (mp->vreg_config)
>  		devm_kfree(&pdev->dev, mp->vreg_config);
>  	mp->num_vreg = 0;
> -parse_vreg_err:
> -	if (mp->clk_config)
> -		devm_kfree(&pdev->dev, mp->clk_config);
> -	mp->num_clk = 0;
>  end:
>  	return rc;
>  }

<snip />

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno