Re: [DPU PATCH v2 12/12] drm/msm/dpu: add error handling in dpu_core_perf_crtc_update

[Sean Paul <seanpaul@chromium.org>]

On Fri, May 11, 2018 at 08:19:38PM +0530, Rajesh Yadav wrote:
> dpu_core_perf_crtc_update() is responsible for aggregating
> the data bus bandwidth and dpu core clock rate requirements
> and request the same for all active crtcs.
> Currently, there is no error handling support in this function
> so there is no way caller can know if the perf request fails.
> This change adds error handling code in dpu_core_perf_crtc_update().
> The caller side error handling is not added in this patch.
> 
> Signed-off-by: Rajesh Yadav <ryadav@codeaurora.org>

Cool! Thanks for doing this :-)

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 37 ++++++++++++++++++---------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 ++-
>  2 files changed, 27 insertions(+), 13 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 d3a1ed9..85c0229 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -248,7 +248,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>  	return 0;
>  }
>  
> -static void _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
> +static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>  		struct drm_crtc *crtc, u32 bus_id)
>  {
>  	u64 bw_sum_of_intfs = 0, bus_ab_quota, bus_ib_quota;
> @@ -257,6 +257,7 @@ static void _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>  					= dpu_crtc_get_client_type(crtc);
>  	struct drm_crtc *tmp_crtc;
>  	struct dpu_crtc_state *dpu_cstate;
> +	int ret = 0;
>  
>  	drm_for_each_crtc(tmp_crtc, crtc->dev) {
>  		if (_dpu_core_perf_crtc_is_power_on(tmp_crtc) &&
> @@ -286,25 +287,28 @@ static void _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>  
>  	switch (curr_client_type) {
>  	case NRT_CLIENT:
> -		dpu_power_data_bus_set_quota(&kms->phandle, kms->core_client,
> +		ret = dpu_power_data_bus_set_quota(
> +				&kms->phandle, kms->core_client,
>  				DPU_POWER_HANDLE_DATA_BUS_CLIENT_NRT,
>  				bus_id, bus_ab_quota, bus_ib_quota);
>  		DPU_DEBUG("client:%s bus_id=%d ab=%llu ib=%llu\n", "nrt",
> -				bus_id, bus_ab_quota, bus_ib_quota);
> +			  bus_id, bus_ab_quota, bus_ib_quota);
>  		break;
>  
>  	case RT_CLIENT:
> -		dpu_power_data_bus_set_quota(&kms->phandle, kms->core_client,
> +		ret = dpu_power_data_bus_set_quota(
> +				&kms->phandle, kms->core_client,
>  				DPU_POWER_HANDLE_DATA_BUS_CLIENT_RT,
>  				bus_id, bus_ab_quota, bus_ib_quota);
>  		DPU_DEBUG("client:%s bus_id=%d ab=%llu ib=%llu\n", "rt",
> -				bus_id, bus_ab_quota, bus_ib_quota);
> +			  bus_id, bus_ab_quota, bus_ib_quota);
>  		break;
>  
>  	default:
>  		DPU_ERROR("invalid client type:%d\n", curr_client_type);
>  		break;
>  	}
> +	return ret;
>  }
>  
>  /**
> @@ -399,7 +403,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>  	return clk_rate;
>  }
>  
> -void dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> +int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>  		int params_changed, bool stop_req)
>  {
>  	struct dpu_core_perf_params *new, *old;
> @@ -410,16 +414,17 @@ void dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>  	int i;
>  	struct msm_drm_private *priv;
>  	struct dpu_kms *kms;
> +	int ret;
>  
>  	if (!crtc) {
>  		DPU_ERROR("invalid crtc\n");
> -		return;
> +		return -EINVAL;
>  	}
>  
>  	kms = _dpu_crtc_get_kms(crtc);
>  	if (!kms || !kms->catalog) {
>  		DPU_ERROR("invalid kms\n");
> -		return;
> +		return -EINVAL;
>  	}
>  	priv = kms->dev->dev_private;
>  
> @@ -482,8 +487,14 @@ void dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>  				update_bus, update_clk);
>  
>  	for (i = 0; i < DPU_POWER_HANDLE_DBUS_ID_MAX; i++) {
> -		if (update_bus & BIT(i))
> -			_dpu_core_perf_crtc_update_bus(kms, crtc, i);
> +		if (update_bus & BIT(i)) {
> +			ret = _dpu_core_perf_crtc_update_bus(kms, crtc, i);
> +			if (ret) {
> +				DPU_ERROR("crtc-%d: failed to update bw vote for bus-%d\n",
> +					  crtc->base.id, i);
> +				return ret;
> +			}
> +		}
>  	}
>  
>  	/*
> @@ -495,15 +506,17 @@ void dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>  
>  		DPU_EVT32(kms->dev, stop_req, clk_rate);
>  
> -		if (_dpu_core_perf_set_core_clk_rate(kms, clk_rate)) {
> +		ret = _dpu_core_perf_set_core_clk_rate(kms, clk_rate);
> +		if (ret) {
>  			DPU_ERROR("failed to set %s clock rate %llu\n",
>  					kms->perf.core_clk->clk_name, clk_rate);
> -			return;
> +			return ret;
>  		}
>  
>  		kms->perf.core_clk_rate = clk_rate;
>  		DPU_DEBUG("update clk rate = %lld HZ\n", clk_rate);
>  	}
> +	return 0;
>  }
>  
>  #ifdef CONFIG_DEBUG_FS
> 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 cde48df..440d6a2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -91,8 +91,9 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>   * @crtc: Pointer to crtc
>   * @params_changed: true if crtc parameters are modified
>   * @stop_req: true if this is a stop request
> + * return: zero if success, or error code otherwise
>   */
> -void dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> +int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>  		int params_changed, bool stop_req);
>  
>  /**
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

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