[05/12] drm/amd/display: Fix bunch of warnings in DC

Submitted by Harry Wentland on Dec. 8, 2016, 1:26 a.m.

Details

Message ID 20161208012624.8914-6-harry.wentland@amd.com
State New
Headers show
Series "DC Patches for Dec 7, 2016" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Harry Wentland Dec. 8, 2016, 1:26 a.m.
Some of those are potential bugs

Change-Id: I53b2c663e18b57013e1b891fc2ecf1fb2d7d3a08
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Tony Cheng <Tony.Cheng@amd.com>
Acked-by: Harry Wentland <Harry.Wentland@amd.com>
---
 .../gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c |  2 +-
 drivers/gpu/drm/amd/display/dc/core/dc_resource.c  |  5 +--
 .../gpu/drm/amd/display/dc/dce/dce_clock_source.c  |  5 ---
 drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c    | 43 +---------------------
 drivers/gpu/drm/amd/display/dc/dce/dce_transform.c |  2 -
 .../drm/amd/display/dc/dce110/dce110_transform_v.c |  3 +-
 6 files changed, 6 insertions(+), 54 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
index 0b2bb3992f1a..3b0710ef4716 100644
--- a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
+++ b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
@@ -1709,7 +1709,7 @@  static void calculate_bandwidth(
 			else {
 				data->blackout_recovery_time = bw_max2(data->blackout_recovery_time, bw_add(bw_mul(bw_int_to_fixed(2), vbios->mcifwrmc_urgent_latency), data->mcifwr_burst_time[data->y_clk_level][data->sclk_level]));
 				if (bw_ltn(data->adjusted_data_buffer_size[k], bw_mul(bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k]), (bw_add(vbios->blackout_duration, bw_add(bw_mul(bw_int_to_fixed(2), vbios->mcifwrmc_urgent_latency), data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])))))) {
-					data->blackout_recovery_time = bw_max2(data->blackout_recovery_time, bw_div((bw_add(bw_mul(bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k]), vbios->blackout_duration), bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_int_to_fixed(2), vbios->mcifwrmc_urgent_latency), data->dmif_burst_time[i][j]), data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])), data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])), data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]), data->adjusted_data_buffer_size[k]))), (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk, bw_int_to_fixed(data->bytes_per_pixel[k])), data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]), bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k])))));
+					data->blackout_recovery_time = bw_max2(data->blackout_recovery_time, bw_div((bw_add(bw_mul(bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k]), vbios->blackout_duration), bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_int_to_fixed(2), vbios->mcifwrmc_urgent_latency), data->dmif_burst_time[data->y_clk_level][data->sclk_level]), data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])), data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])), data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]), data->adjusted_data_buffer_size[k]))), (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk, bw_int_to_fixed(data->bytes_per_pixel[k])), data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]), bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k])))));
 				}
 			}
 		}
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
index bd53d27e5414..f552b0468186 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
@@ -1834,11 +1834,10 @@  void resource_build_info_frame(struct pipe_ctx *pipe_ctx)
 		set_vendor_info_packet(
 			pipe_ctx->stream, &info_frame.vendor_info_packet);
 		set_spd_info_packet(pipe_ctx->stream, &info_frame.spd_packet);
-	}
-
-	else if (dc_is_dp_signal(signal))
+	} else if (dc_is_dp_signal(signal)) {
 		set_vsc_info_packet(pipe_ctx->stream, &info_frame.vsc_packet);
 		set_spd_info_packet(pipe_ctx->stream, &info_frame.spd_packet);
+	}
 
 	translate_info_frame(&info_frame,
 			&pipe_ctx->encoder_info_frame);
diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
index 80ac5d9efa71..3d1c32122d69 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
@@ -465,7 +465,6 @@  static uint32_t dce110_get_pix_clk_dividers_helper (
 		struct pll_settings *pll_settings,
 		struct pixel_clk_params *pix_clk_params)
 {
-	uint32_t addr = 0;
 	uint32_t value = 0;
 	uint32_t field = 0;
 	uint32_t pll_calc_error = MAX_PLL_CALC_ERROR;
@@ -731,8 +730,6 @@  static void dce110_program_pixel_clk_resync(
 		enum signal_type signal_type,
 		enum dc_color_depth colordepth)
 {
-	uint32_t value = 0;
-
 	REG_UPDATE(RESYNC_CNTL,
 			DCCG_DEEP_COLOR_CNTL1, 0);
 	/*
@@ -772,8 +769,6 @@  static void dce112_program_pixel_clk_resync(
 		enum dc_color_depth colordepth,
 		bool enable_ycbcr420)
 {
-	uint32_t value = 0;
-
 	REG_UPDATE(PIXCLK_RESYNC_CNTL,
 			PHYPLLA_DCCG_DEEP_COLOR_CNTL, 0);
 	/*
diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c b/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
index c2bd8dc7b4ad..262612061c68 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
@@ -148,29 +148,6 @@  static int dce_divider_range_calc_divider(
 
 }
 
-static int dce_divider_range_calc_did(
-	struct dce_divider_range *div_range,
-	int div)
-{
-	int did;
-	/* Check before dividing.*/
-	if (div_range->div_range_step == 0) {
-		div_range->div_range_step = 1;
-		/*div_range_step cannot be zero*/
-		BREAK_TO_DEBUGGER();
-	}
-	/* Is this divider within our range?*/
-	if ((div < div_range->div_range_start)
-		|| (div >= div_range->div_range_end))
-		return INVALID_DID;
-/* did = (divider - range_start + (range_step-1)) / range_step) + did_min*/
-	did = div - div_range->div_range_start;
-	did += div_range->div_range_step - 1;
-	did /= div_range->div_range_step;
-	did += div_range->did_min;
-	return did;
-}
-
 static int dce_divider_range_get_divider(
 	struct dce_divider_range *div_range,
 	int ranges_num,
@@ -189,25 +166,7 @@  static int dce_divider_range_get_divider(
 	return div;
 }
 
-static int dce_divider_range_get_did(
-	struct dce_divider_range *div_range,
-	int ranges_num,
-	int divider)
-{
-	int did = INVALID_DID;
-	int i;
-
-	for (i = 0; i < ranges_num; i++) {
-		/*  CalcDid returns InvalidDid if a divider ID isn't found*/
-		did = dce_divider_range_calc_did(&div_range[i], divider);
-		/* Found a valid return did*/
-		if (did != INVALID_DID)
-			break;
-	}
-	return did;
-}
-
-static uint32_t dce_clocks_get_dp_ref_freq(struct display_clock *clk)
+static int dce_clocks_get_dp_ref_freq(struct display_clock *clk)
 {
 	struct dce_disp_clk *clk_dce = TO_DCE_CLOCKS(clk);
 	int dprefclk_wdivider;
diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c b/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
index f47b6617f662..bbf4d97cb980 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
@@ -83,8 +83,6 @@  static bool setup_scaling_configuration(
 	struct dce_transform *xfm_dce,
 	const struct scaler_data *data)
 {
-	struct dc_context *ctx = xfm_dce->base.ctx;
-
 	if (data->taps.h_taps + data->taps.v_taps <= 2) {
 		/* Set bypass */
 		REG_UPDATE_2(SCL_MODE, SCL_MODE, 0, SCL_PSCL_EN, 0);
diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
index 7d8cf7a58f46..feb5f3c29804 100644
--- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
+++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
@@ -621,7 +621,8 @@  static void dce110_xfmv_set_pixel_storage_depth(
 	const struct bit_depth_reduction_params *bit_depth_params)
 {
 	struct dce_transform *xfm_dce = TO_DCE_TRANSFORM(xfm);
-	int pixel_depth, expan_mode;
+	int pixel_depth = 0;
+	int expan_mode = 0;
 	uint32_t reg_data = 0;
 
 	switch (depth) {

Comments

[Please CC me on all replies, I'm not subscribed to the list.]

Harry Wentland wrote on 08.12.2016 02:26:
> Some of those are potential bugs

Are they related? If not: maybe split the changes up?

> Change-Id: I53b2c663e18b57013e1b891fc2ecf1fb2d7d3a08
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> Reviewed-by: Tony Cheng <Tony.Cheng@amd.com>

It'd be nice to see these reviews on list... (applies to all patches here)

> Acked-by: Harry Wentland <Harry.Wentland@amd.com>
> ---
>  .../gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c |  2 +-
>  drivers/gpu/drm/amd/display/dc/core/dc_resource.c  |  5 +--
>  .../gpu/drm/amd/display/dc/dce/dce_clock_source.c  |  5 ---
>  drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c    | 43 +---------------------
>  drivers/gpu/drm/amd/display/dc/dce/dce_transform.c |  2 -
>  .../drm/amd/display/dc/dce110/dce110_transform_v.c |  3 +-
>  6 files changed, 6 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
> index 0b2bb3992f1a..3b0710ef4716 100644
> --- a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
> +++ b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
> @@ -1709,7 +1709,7 @@ static void calculate_bandwidth(
>  			else {
>  				data->blackout_recovery_time = bw_max2(data->blackout_recovery_time, bw_add(bw_mul(bw_int_to_fixed(2), vbios->mcifwrmc_urgent_latency), data->mcifwr_burst_time[data->y_clk_level][data->sclk_level]));
>  				if (bw_ltn(data->adjusted_data_buffer_size[k], bw_mul(bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k]), (bw_add(vbios->blackout_duration, bw_add(bw_mul(bw_int_to_fixed(2), vbios->mcifwrmc_urgent_latency), data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])))))) {
> -					data->blackout_recovery_time = bw_max2(data->blackout_recovery_time, bw_div((bw_add(bw_mul(bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k]), vbios->blackout_duration), bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_int_to_fixed(2), vbios->mcifwrmc_urgent_latency), data->dmif_burst_time[i][j]), data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])), data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])), data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]), data->adjusted_data_buffer_size[k]))), (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk, bw_int_to_fixed(data->bytes_per_pixel[k])), data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]), bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k])))));
> +					data->blackout_recovery_time = bw_max2(data->blackout_recovery_time, bw_div((bw_add(bw_mul(bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k]), vbios->blackout_duration), bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_int_to_fixed(2), vbios->mcifwrmc_urgent_latency), data->dmif_burst_time[data->y_clk_level][data->sclk_level]), data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])), data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])), data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]), data->adjusted_data_buffer_size[k]))), (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk, bw_int_to_fixed(data->bytes_per_pixel[k])), data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]), bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k])))));

Uhg, can this be wrapped/split up somehow? Seeing the changes is nigh impossible
and I suspect it might even be impossible for you to check internally.

A nice comment above giving the formula and maybe some reasoning would also be
nice, that way somebody from the outside has at least a chance to check if the
code does what it's supposed to do.

Cheers,
Kai


>  				}
>  			}
>  		}
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
> index bd53d27e5414..f552b0468186 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
> @@ -1834,11 +1834,10 @@ void resource_build_info_frame(struct pipe_ctx *pipe_ctx)
>  		set_vendor_info_packet(
>  			pipe_ctx->stream, &info_frame.vendor_info_packet);
>  		set_spd_info_packet(pipe_ctx->stream, &info_frame.spd_packet);
> -	}
> -
> -	else if (dc_is_dp_signal(signal))
> +	} else if (dc_is_dp_signal(signal)) {
>  		set_vsc_info_packet(pipe_ctx->stream, &info_frame.vsc_packet);
>  		set_spd_info_packet(pipe_ctx->stream, &info_frame.spd_packet);
> +	}
>  
>  	translate_info_frame(&info_frame,
>  			&pipe_ctx->encoder_info_frame);
> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
> index 80ac5d9efa71..3d1c32122d69 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
> @@ -465,7 +465,6 @@ static uint32_t dce110_get_pix_clk_dividers_helper (
>  		struct pll_settings *pll_settings,
>  		struct pixel_clk_params *pix_clk_params)
>  {
> -	uint32_t addr = 0;
>  	uint32_t value = 0;
>  	uint32_t field = 0;
>  	uint32_t pll_calc_error = MAX_PLL_CALC_ERROR;
> @@ -731,8 +730,6 @@ static void dce110_program_pixel_clk_resync(
>  		enum signal_type signal_type,
>  		enum dc_color_depth colordepth)
>  {
> -	uint32_t value = 0;
> -
>  	REG_UPDATE(RESYNC_CNTL,
>  			DCCG_DEEP_COLOR_CNTL1, 0);
>  	/*
> @@ -772,8 +769,6 @@ static void dce112_program_pixel_clk_resync(
>  		enum dc_color_depth colordepth,
>  		bool enable_ycbcr420)
>  {
> -	uint32_t value = 0;
> -
>  	REG_UPDATE(PIXCLK_RESYNC_CNTL,
>  			PHYPLLA_DCCG_DEEP_COLOR_CNTL, 0);
>  	/*
> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c b/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
> index c2bd8dc7b4ad..262612061c68 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
> @@ -148,29 +148,6 @@ static int dce_divider_range_calc_divider(
>  
>  }
>  
> -static int dce_divider_range_calc_did(
> -	struct dce_divider_range *div_range,
> -	int div)
> -{
> -	int did;
> -	/* Check before dividing.*/
> -	if (div_range->div_range_step == 0) {
> -		div_range->div_range_step = 1;
> -		/*div_range_step cannot be zero*/
> -		BREAK_TO_DEBUGGER();
> -	}
> -	/* Is this divider within our range?*/
> -	if ((div < div_range->div_range_start)
> -		|| (div >= div_range->div_range_end))
> -		return INVALID_DID;
> -/* did = (divider - range_start + (range_step-1)) / range_step) + did_min*/
> -	did = div - div_range->div_range_start;
> -	did += div_range->div_range_step - 1;
> -	did /= div_range->div_range_step;
> -	did += div_range->did_min;
> -	return did;
> -}
> -
>  static int dce_divider_range_get_divider(
>  	struct dce_divider_range *div_range,
>  	int ranges_num,
> @@ -189,25 +166,7 @@ static int dce_divider_range_get_divider(
>  	return div;
>  }
>  
> -static int dce_divider_range_get_did(
> -	struct dce_divider_range *div_range,
> -	int ranges_num,
> -	int divider)
> -{
> -	int did = INVALID_DID;
> -	int i;
> -
> -	for (i = 0; i < ranges_num; i++) {
> -		/*  CalcDid returns InvalidDid if a divider ID isn't found*/
> -		did = dce_divider_range_calc_did(&div_range[i], divider);
> -		/* Found a valid return did*/
> -		if (did != INVALID_DID)
> -			break;
> -	}
> -	return did;
> -}
> -
> -static uint32_t dce_clocks_get_dp_ref_freq(struct display_clock *clk)
> +static int dce_clocks_get_dp_ref_freq(struct display_clock *clk)
>  {
>  	struct dce_disp_clk *clk_dce = TO_DCE_CLOCKS(clk);
>  	int dprefclk_wdivider;
> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c b/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
> index f47b6617f662..bbf4d97cb980 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
> @@ -83,8 +83,6 @@ static bool setup_scaling_configuration(
>  	struct dce_transform *xfm_dce,
>  	const struct scaler_data *data)
>  {
> -	struct dc_context *ctx = xfm_dce->base.ctx;
> -
>  	if (data->taps.h_taps + data->taps.v_taps <= 2) {
>  		/* Set bypass */
>  		REG_UPDATE_2(SCL_MODE, SCL_MODE, 0, SCL_PSCL_EN, 0);
> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
> index 7d8cf7a58f46..feb5f3c29804 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
> @@ -621,7 +621,8 @@ static void dce110_xfmv_set_pixel_storage_depth(
>  	const struct bit_depth_reduction_params *bit_depth_params)
>  {
>  	struct dce_transform *xfm_dce = TO_DCE_TRANSFORM(xfm);
> -	int pixel_depth, expan_mode;
> +	int pixel_depth = 0;
> +	int expan_mode = 0;
>  	uint32_t reg_data = 0;
>  
>  	switch (depth) {
>
On 2016-12-08 11:22 AM, Kai Wasserbäch wrote:
> [Please CC me on all replies, I'm not subscribed to the list.]
>
> Harry Wentland wrote on 08.12.2016 02:26:
>> Some of those are potential bugs
> Are they related? If not: maybe split the changes up?
>
>> Change-Id: I53b2c663e18b57013e1b891fc2ecf1fb2d7d3a08
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Reviewed-by: Tony Cheng <Tony.Cheng@amd.com>
> It'd be nice to see these reviews on list... (applies to all patches here)
These patches currently start out in our internal tree with a whole lot 
of stuff that we can't open up, like new IP, so unfortunately can't be 
done in a public forum. I pull the r-b from our internal review system 
for reference when I prepare the patches for public review.
>> Acked-by: Harry Wentland <Harry.Wentland@amd.com>
>> ---
>>   .../gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c |  2 +-
>>   drivers/gpu/drm/amd/display/dc/core/dc_resource.c  |  5 +--
>>   .../gpu/drm/amd/display/dc/dce/dce_clock_source.c  |  5 ---
>>   drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c    | 43 +---------------------
>>   drivers/gpu/drm/amd/display/dc/dce/dce_transform.c |  2 -
>>   .../drm/amd/display/dc/dce110/dce110_transform_v.c |  3 +-
>>   6 files changed, 6 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>> index 0b2bb3992f1a..3b0710ef4716 100644
>> --- a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>> +++ b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>> @@ -1709,7 +1709,7 @@ static void calculate_bandwidth(
>>   			else {
>>   				data->blackout_recovery_time = bw_max2(data->blackout_recovery_time, bw_add(bw_mul(bw_int_to_fixed(2), vbios->mcifwrmc_urgent_latency), data->mcifwr_burst_time[data->y_clk_level][data->sclk_level]));
>>   				if (bw_ltn(data->adjusted_data_buffer_size[k], bw_mul(bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k]), (bw_add(vbios->blackout_duration, bw_add(bw_mul(bw_int_to_fixed(2), vbios->mcifwrmc_urgent_latency), data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])))))) {
>> -					data->blackout_recovery_time = bw_max2(data->blackout_recovery_time, bw_div((bw_add(bw_mul(bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k]), vbios->blackout_duration), bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_int_to_fixed(2), vbios->mcifwrmc_urgent_latency), data->dmif_burst_time[i][j]), data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])), data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])), data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]), data->adjusted_data_buffer_size[k]))), (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk, bw_int_to_fixed(data->bytes_per_pixel[k])), data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]), bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k])))));
>> +					data->blackout_recovery_time = bw_max2(data->blackout_recovery_time, bw_div((bw_add(bw_mul(bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k]), vbios->blackout_duration), bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_int_to_fixed(2), vbios->mcifwrmc_urgent_latency), data->dmif_burst_time[data->y_clk_level][data->sclk_level]), data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])), data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])), data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]), data->adjusted_data_buffer_size[k]))), (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk, bw_int_to_fixed(data->bytes_per_pixel[k])), data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]), bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]), data->bytes_per_request[k])))));
> Uhg, can this be wrapped/split up somehow? Seeing the changes is nigh impossible
> and I suspect it might even be impossible for you to check internally.
I believe this comes straight from some complicated code HW gives us and 
we use directly. I'll see if we can somehow make this more readable or 
at least add a comment, as you mention below.
> A nice comment above giving the formula and maybe some reasoning would also be
> nice, that way somebody from the outside has at least a chance to check if the
> code does what it's supposed to do.
>
> Cheers,
> Kai
>
>
>>   				}
>>   			}
>>   		}
>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>> index bd53d27e5414..f552b0468186 100644
>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>> @@ -1834,11 +1834,10 @@ void resource_build_info_frame(struct pipe_ctx *pipe_ctx)
>>   		set_vendor_info_packet(
>>   			pipe_ctx->stream, &info_frame.vendor_info_packet);
>>   		set_spd_info_packet(pipe_ctx->stream, &info_frame.spd_packet);
>> -	}
>> -
>> -	else if (dc_is_dp_signal(signal))
>> +	} else if (dc_is_dp_signal(signal)) {
>>   		set_vsc_info_packet(pipe_ctx->stream, &info_frame.vsc_packet);
>>   		set_spd_info_packet(pipe_ctx->stream, &info_frame.spd_packet);
>> +	}
>>   
>>   	translate_info_frame(&info_frame,
>>   			&pipe_ctx->encoder_info_frame);
>> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
>> index 80ac5d9efa71..3d1c32122d69 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
>> @@ -465,7 +465,6 @@ static uint32_t dce110_get_pix_clk_dividers_helper (
>>   		struct pll_settings *pll_settings,
>>   		struct pixel_clk_params *pix_clk_params)
>>   {
>> -	uint32_t addr = 0;
>>   	uint32_t value = 0;
>>   	uint32_t field = 0;
>>   	uint32_t pll_calc_error = MAX_PLL_CALC_ERROR;
>> @@ -731,8 +730,6 @@ static void dce110_program_pixel_clk_resync(
>>   		enum signal_type signal_type,
>>   		enum dc_color_depth colordepth)
>>   {
>> -	uint32_t value = 0;
>> -
>>   	REG_UPDATE(RESYNC_CNTL,
>>   			DCCG_DEEP_COLOR_CNTL1, 0);
>>   	/*
>> @@ -772,8 +769,6 @@ static void dce112_program_pixel_clk_resync(
>>   		enum dc_color_depth colordepth,
>>   		bool enable_ycbcr420)
>>   {
>> -	uint32_t value = 0;
>> -
>>   	REG_UPDATE(PIXCLK_RESYNC_CNTL,
>>   			PHYPLLA_DCCG_DEEP_COLOR_CNTL, 0);
>>   	/*
>> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c b/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
>> index c2bd8dc7b4ad..262612061c68 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
>> @@ -148,29 +148,6 @@ static int dce_divider_range_calc_divider(
>>   
>>   }
>>   
>> -static int dce_divider_range_calc_did(
>> -	struct dce_divider_range *div_range,
>> -	int div)
>> -{
>> -	int did;
>> -	/* Check before dividing.*/
>> -	if (div_range->div_range_step == 0) {
>> -		div_range->div_range_step = 1;
>> -		/*div_range_step cannot be zero*/
>> -		BREAK_TO_DEBUGGER();
>> -	}
>> -	/* Is this divider within our range?*/
>> -	if ((div < div_range->div_range_start)
>> -		|| (div >= div_range->div_range_end))
>> -		return INVALID_DID;
>> -/* did = (divider - range_start + (range_step-1)) / range_step) + did_min*/
>> -	did = div - div_range->div_range_start;
>> -	did += div_range->div_range_step - 1;
>> -	did /= div_range->div_range_step;
>> -	did += div_range->did_min;
>> -	return did;
>> -}
>> -
>>   static int dce_divider_range_get_divider(
>>   	struct dce_divider_range *div_range,
>>   	int ranges_num,
>> @@ -189,25 +166,7 @@ static int dce_divider_range_get_divider(
>>   	return div;
>>   }
>>   
>> -static int dce_divider_range_get_did(
>> -	struct dce_divider_range *div_range,
>> -	int ranges_num,
>> -	int divider)
>> -{
>> -	int did = INVALID_DID;
>> -	int i;
>> -
>> -	for (i = 0; i < ranges_num; i++) {
>> -		/*  CalcDid returns InvalidDid if a divider ID isn't found*/
>> -		did = dce_divider_range_calc_did(&div_range[i], divider);
>> -		/* Found a valid return did*/
>> -		if (did != INVALID_DID)
>> -			break;
>> -	}
>> -	return did;
>> -}
>> -
>> -static uint32_t dce_clocks_get_dp_ref_freq(struct display_clock *clk)
>> +static int dce_clocks_get_dp_ref_freq(struct display_clock *clk)
>>   {
>>   	struct dce_disp_clk *clk_dce = TO_DCE_CLOCKS(clk);
>>   	int dprefclk_wdivider;
>> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c b/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
>> index f47b6617f662..bbf4d97cb980 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
>> @@ -83,8 +83,6 @@ static bool setup_scaling_configuration(
>>   	struct dce_transform *xfm_dce,
>>   	const struct scaler_data *data)
>>   {
>> -	struct dc_context *ctx = xfm_dce->base.ctx;
>> -
>>   	if (data->taps.h_taps + data->taps.v_taps <= 2) {
>>   		/* Set bypass */
>>   		REG_UPDATE_2(SCL_MODE, SCL_MODE, 0, SCL_PSCL_EN, 0);
>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
>> index 7d8cf7a58f46..feb5f3c29804 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
>> @@ -621,7 +621,8 @@ static void dce110_xfmv_set_pixel_storage_depth(
>>   	const struct bit_depth_reduction_params *bit_depth_params)
>>   {
>>   	struct dce_transform *xfm_dce = TO_DCE_TRANSFORM(xfm);
>> -	int pixel_depth, expan_mode;
>> +	int pixel_depth = 0;
>> +	int expan_mode = 0;
>>   	uint32_t reg_data = 0;
>>   
>>   	switch (depth) {
>>
Harry Wentland wrote on 08.12.2016 17:36:
> 
> 
> On 2016-12-08 11:22 AM, Kai Wasserbäch wrote:
>> [Please CC me on all replies, I'm not subscribed to the list.]
>>
>> Harry Wentland wrote on 08.12.2016 02:26:
>>> Some of those are potential bugs
>> Are they related? If not: maybe split the changes up?
>>
>>> Change-Id: I53b2c663e18b57013e1b891fc2ecf1fb2d7d3a08
>>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>>> Reviewed-by: Tony Cheng <Tony.Cheng@amd.com>
>> It'd be nice to see these reviews on list... (applies to all patches here)
> These patches currently start out in our internal tree with a whole lot of stuff
> that we can't open up, like new IP, so unfortunately can't be done in a public
> forum. I pull the r-b from our internal review system for reference when I
> prepare the patches for public review.

Hm, not nice, but not my place to tell you to change this. That's Dave's and
Linus' job. ;-)

>>> Acked-by: Harry Wentland <Harry.Wentland@amd.com>
>>> ---
>>>   .../gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c |  2 +-
>>>   drivers/gpu/drm/amd/display/dc/core/dc_resource.c  |  5 +--
>>>   .../gpu/drm/amd/display/dc/dce/dce_clock_source.c  |  5 ---
>>>   drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c    | 43 +---------------------
>>>   drivers/gpu/drm/amd/display/dc/dce/dce_transform.c |  2 -
>>>   .../drm/amd/display/dc/dce110/dce110_transform_v.c |  3 +-
>>>   6 files changed, 6 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>>> b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>>> index 0b2bb3992f1a..3b0710ef4716 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>>> @@ -1709,7 +1709,7 @@ static void calculate_bandwidth(
>>>               else {
>>>                   data->blackout_recovery_time =
>>> bw_max2(data->blackout_recovery_time, bw_add(bw_mul(bw_int_to_fixed(2),
>>> vbios->mcifwrmc_urgent_latency),
>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level]));
>>>                   if (bw_ltn(data->adjusted_data_buffer_size[k],
>>> bw_mul(bw_div(bw_mul(data->display_bandwidth[k],
>>> data->useful_bytes_per_request[k]), data->bytes_per_request[k]),
>>> (bw_add(vbios->blackout_duration, bw_add(bw_mul(bw_int_to_fixed(2),
>>> vbios->mcifwrmc_urgent_latency),
>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])))))) {
>>> -                    data->blackout_recovery_time =
>>> bw_max2(data->blackout_recovery_time,
>>> bw_div((bw_add(bw_mul(bw_div(bw_mul(data->display_bandwidth[k],
>>> data->useful_bytes_per_request[k]), data->bytes_per_request[k]),
>>> vbios->blackout_duration),
>>> bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_int_to_fixed(2),
>>> vbios->mcifwrmc_urgent_latency), data->dmif_burst_time[i][j]),
>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])),
>>> data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])),
>>> data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]),
>>> data->adjusted_data_buffer_size[k]))),
>>> (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk,
>>> bw_int_to_fixed(data->bytes_per_pixel[k])),
>>> data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]),
>>> bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]),
>>> data->bytes_per_request[k])))));
>>> +                    data->blackout_recovery_time =
>>> bw_max2(data->blackout_recovery_time,
>>> bw_div((bw_add(bw_mul(bw_div(bw_mul(data->display_bandwidth[k],
>>> data->useful_bytes_per_request[k]), data->bytes_per_request[k]),
>>> vbios->blackout_duration),
>>> bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_int_to_fixed(2),
>>> vbios->mcifwrmc_urgent_latency),
>>> data->dmif_burst_time[data->y_clk_level][data->sclk_level]),
>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])),
>>> data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])),
>>> data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]),
>>> data->adjusted_data_buffer_size[k]))),
>>> (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk,
>>> bw_int_to_fixed(data->bytes_per_pixel[k])),
>>> data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]),
>>> bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]),
>>> data->bytes_per_request[k])))));
>> Uhg, can this be wrapped/split up somehow? Seeing the changes is nigh impossible
>> and I suspect it might even be impossible for you to check internally.
> I believe this comes straight from some complicated code HW gives us and we use
> directly. I'll see if we can somehow make this more readable or at least add a
> comment, as you mention below.

That would be really appreciated. I mean, I'm in no way able to review
DAL/Display thoroughly (huge code base I'm not familiar with and where I might
easily miss interactions), but this is just not reviewable/verifiable at all for
any person outside your inner circle.

Thanks,
Kai

>> A nice comment above giving the formula and maybe some reasoning would also be
>> nice, that way somebody from the outside has at least a chance to check if the
>> code does what it's supposed to do.
>>
>> Cheers,
>> Kai
>>
>>
>>>                   }
>>>               }
>>>           }
>>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>>> b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>>> index bd53d27e5414..f552b0468186 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>>> @@ -1834,11 +1834,10 @@ void resource_build_info_frame(struct pipe_ctx
>>> *pipe_ctx)
>>>           set_vendor_info_packet(
>>>               pipe_ctx->stream, &info_frame.vendor_info_packet);
>>>           set_spd_info_packet(pipe_ctx->stream, &info_frame.spd_packet);
>>> -    }
>>> -
>>> -    else if (dc_is_dp_signal(signal))
>>> +    } else if (dc_is_dp_signal(signal)) {
>>>           set_vsc_info_packet(pipe_ctx->stream, &info_frame.vsc_packet);
>>>           set_spd_info_packet(pipe_ctx->stream, &info_frame.spd_packet);
>>> +    }
>>>         translate_info_frame(&info_frame,
>>>               &pipe_ctx->encoder_info_frame);
>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
>>> b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
>>> index 80ac5d9efa71..3d1c32122d69 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
>>> @@ -465,7 +465,6 @@ static uint32_t dce110_get_pix_clk_dividers_helper (
>>>           struct pll_settings *pll_settings,
>>>           struct pixel_clk_params *pix_clk_params)
>>>   {
>>> -    uint32_t addr = 0;
>>>       uint32_t value = 0;
>>>       uint32_t field = 0;
>>>       uint32_t pll_calc_error = MAX_PLL_CALC_ERROR;
>>> @@ -731,8 +730,6 @@ static void dce110_program_pixel_clk_resync(
>>>           enum signal_type signal_type,
>>>           enum dc_color_depth colordepth)
>>>   {
>>> -    uint32_t value = 0;
>>> -
>>>       REG_UPDATE(RESYNC_CNTL,
>>>               DCCG_DEEP_COLOR_CNTL1, 0);
>>>       /*
>>> @@ -772,8 +769,6 @@ static void dce112_program_pixel_clk_resync(
>>>           enum dc_color_depth colordepth,
>>>           bool enable_ycbcr420)
>>>   {
>>> -    uint32_t value = 0;
>>> -
>>>       REG_UPDATE(PIXCLK_RESYNC_CNTL,
>>>               PHYPLLA_DCCG_DEEP_COLOR_CNTL, 0);
>>>       /*
>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
>>> b/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
>>> index c2bd8dc7b4ad..262612061c68 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
>>> @@ -148,29 +148,6 @@ static int dce_divider_range_calc_divider(
>>>     }
>>>   -static int dce_divider_range_calc_did(
>>> -    struct dce_divider_range *div_range,
>>> -    int div)
>>> -{
>>> -    int did;
>>> -    /* Check before dividing.*/
>>> -    if (div_range->div_range_step == 0) {
>>> -        div_range->div_range_step = 1;
>>> -        /*div_range_step cannot be zero*/
>>> -        BREAK_TO_DEBUGGER();
>>> -    }
>>> -    /* Is this divider within our range?*/
>>> -    if ((div < div_range->div_range_start)
>>> -        || (div >= div_range->div_range_end))
>>> -        return INVALID_DID;
>>> -/* did = (divider - range_start + (range_step-1)) / range_step) + did_min*/
>>> -    did = div - div_range->div_range_start;
>>> -    did += div_range->div_range_step - 1;
>>> -    did /= div_range->div_range_step;
>>> -    did += div_range->did_min;
>>> -    return did;
>>> -}
>>> -
>>>   static int dce_divider_range_get_divider(
>>>       struct dce_divider_range *div_range,
>>>       int ranges_num,
>>> @@ -189,25 +166,7 @@ static int dce_divider_range_get_divider(
>>>       return div;
>>>   }
>>>   -static int dce_divider_range_get_did(
>>> -    struct dce_divider_range *div_range,
>>> -    int ranges_num,
>>> -    int divider)
>>> -{
>>> -    int did = INVALID_DID;
>>> -    int i;
>>> -
>>> -    for (i = 0; i < ranges_num; i++) {
>>> -        /*  CalcDid returns InvalidDid if a divider ID isn't found*/
>>> -        did = dce_divider_range_calc_did(&div_range[i], divider);
>>> -        /* Found a valid return did*/
>>> -        if (did != INVALID_DID)
>>> -            break;
>>> -    }
>>> -    return did;
>>> -}
>>> -
>>> -static uint32_t dce_clocks_get_dp_ref_freq(struct display_clock *clk)
>>> +static int dce_clocks_get_dp_ref_freq(struct display_clock *clk)
>>>   {
>>>       struct dce_disp_clk *clk_dce = TO_DCE_CLOCKS(clk);
>>>       int dprefclk_wdivider;
>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
>>> b/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
>>> index f47b6617f662..bbf4d97cb980 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
>>> @@ -83,8 +83,6 @@ static bool setup_scaling_configuration(
>>>       struct dce_transform *xfm_dce,
>>>       const struct scaler_data *data)
>>>   {
>>> -    struct dc_context *ctx = xfm_dce->base.ctx;
>>> -
>>>       if (data->taps.h_taps + data->taps.v_taps <= 2) {
>>>           /* Set bypass */
>>>           REG_UPDATE_2(SCL_MODE, SCL_MODE, 0, SCL_PSCL_EN, 0);
>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
>>> b/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
>>> index 7d8cf7a58f46..feb5f3c29804 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
>>> @@ -621,7 +621,8 @@ static void dce110_xfmv_set_pixel_storage_depth(
>>>       const struct bit_depth_reduction_params *bit_depth_params)
>>>   {
>>>       struct dce_transform *xfm_dce = TO_DCE_TRANSFORM(xfm);
>>> -    int pixel_depth, expan_mode;
>>> +    int pixel_depth = 0;
>>> +    int expan_mode = 0;
>>>       uint32_t reg_data = 0;
>>>         switch (depth) {
>>>
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 2016-12-08 11:58 AM, Kai Wasserbäch wrote:
> Harry Wentland wrote on 08.12.2016 17:36:
>>
>> On 2016-12-08 11:22 AM, Kai Wasserbäch wrote:
>>> [Please CC me on all replies, I'm not subscribed to the list.]
>>>
>>> Harry Wentland wrote on 08.12.2016 02:26:
>>>> Some of those are potential bugs
>>> Are they related? If not: maybe split the changes up?
>>>
>>>> Change-Id: I53b2c663e18b57013e1b891fc2ecf1fb2d7d3a08
>>>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>>>> Reviewed-by: Tony Cheng <Tony.Cheng@amd.com>
>>> It'd be nice to see these reviews on list... (applies to all patches here)
>> These patches currently start out in our internal tree with a whole lot of stuff
>> that we can't open up, like new IP, so unfortunately can't be done in a public
>> forum. I pull the r-b from our internal review system for reference when I
>> prepare the patches for public review.
> Hm, not nice, but not my place to tell you to change this. That's Dave's and
> Linus' job. ;-)
>
>>>> Acked-by: Harry Wentland <Harry.Wentland@amd.com>
>>>> ---
>>>>    .../gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c |  2 +-
>>>>    drivers/gpu/drm/amd/display/dc/core/dc_resource.c  |  5 +--
>>>>    .../gpu/drm/amd/display/dc/dce/dce_clock_source.c  |  5 ---
>>>>    drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c    | 43 +---------------------
>>>>    drivers/gpu/drm/amd/display/dc/dce/dce_transform.c |  2 -
>>>>    .../drm/amd/display/dc/dce110/dce110_transform_v.c |  3 +-
>>>>    6 files changed, 6 insertions(+), 54 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>>>> b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>>>> index 0b2bb3992f1a..3b0710ef4716 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>>>> @@ -1709,7 +1709,7 @@ static void calculate_bandwidth(
>>>>                else {
>>>>                    data->blackout_recovery_time =
>>>> bw_max2(data->blackout_recovery_time, bw_add(bw_mul(bw_int_to_fixed(2),
>>>> vbios->mcifwrmc_urgent_latency),
>>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level]));
>>>>                    if (bw_ltn(data->adjusted_data_buffer_size[k],
>>>> bw_mul(bw_div(bw_mul(data->display_bandwidth[k],
>>>> data->useful_bytes_per_request[k]), data->bytes_per_request[k]),
>>>> (bw_add(vbios->blackout_duration, bw_add(bw_mul(bw_int_to_fixed(2),
>>>> vbios->mcifwrmc_urgent_latency),
>>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])))))) {
>>>> -                    data->blackout_recovery_time =
>>>> bw_max2(data->blackout_recovery_time,
>>>> bw_div((bw_add(bw_mul(bw_div(bw_mul(data->display_bandwidth[k],
>>>> data->useful_bytes_per_request[k]), data->bytes_per_request[k]),
>>>> vbios->blackout_duration),
>>>> bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_int_to_fixed(2),
>>>> vbios->mcifwrmc_urgent_latency), data->dmif_burst_time[i][j]),
>>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])),
>>>> data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])),
>>>> data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]),
>>>> data->adjusted_data_buffer_size[k]))),
>>>> (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk,
>>>> bw_int_to_fixed(data->bytes_per_pixel[k])),
>>>> data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]),
>>>> bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]),
>>>> data->bytes_per_request[k])))));
>>>> +                    data->blackout_recovery_time =
>>>> bw_max2(data->blackout_recovery_time,
>>>> bw_div((bw_add(bw_mul(bw_div(bw_mul(data->display_bandwidth[k],
>>>> data->useful_bytes_per_request[k]), data->bytes_per_request[k]),
>>>> vbios->blackout_duration),
>>>> bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_int_to_fixed(2),
>>>> vbios->mcifwrmc_urgent_latency),
>>>> data->dmif_burst_time[data->y_clk_level][data->sclk_level]),
>>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])),
>>>> data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])),
>>>> data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]),
>>>> data->adjusted_data_buffer_size[k]))),
>>>> (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk,
>>>> bw_int_to_fixed(data->bytes_per_pixel[k])),
>>>> data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]),
>>>> bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]),
>>>> data->bytes_per_request[k])))));
>>> Uhg, can this be wrapped/split up somehow? Seeing the changes is nigh impossible
>>> and I suspect it might even be impossible for you to check internally.
>> I believe this comes straight from some complicated code HW gives us and we use
>> directly. I'll see if we can somehow make this more readable or at least add a
>> comment, as you mention below.
> That would be really appreciated. I mean, I'm in no way able to review
> DAL/Display thoroughly (huge code base I'm not familiar with and where I might
> easily miss interactions), but this is just not reviewable/verifiable at all for
> any person outside your inner circle.
I followed up with Tony, who's been working with HW guys to get them to 
share this code with us. Unfortunately there isn't much we can do in the 
way of changing this code.

Harry
> Thanks,
> Kai
>
>>> A nice comment above giving the formula and maybe some reasoning would also be
>>> nice, that way somebody from the outside has at least a chance to check if the
>>> code does what it's supposed to do.
>>>
>>> Cheers,
>>> Kai
>>>
>>>
>>>>                    }
>>>>                }
>>>>            }
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>>>> b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>>>> index bd53d27e5414..f552b0468186 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>>>> @@ -1834,11 +1834,10 @@ void resource_build_info_frame(struct pipe_ctx
>>>> *pipe_ctx)
>>>>            set_vendor_info_packet(
>>>>                pipe_ctx->stream, &info_frame.vendor_info_packet);
>>>>            set_spd_info_packet(pipe_ctx->stream, &info_frame.spd_packet);
>>>> -    }
>>>> -
>>>> -    else if (dc_is_dp_signal(signal))
>>>> +    } else if (dc_is_dp_signal(signal)) {
>>>>            set_vsc_info_packet(pipe_ctx->stream, &info_frame.vsc_packet);
>>>>            set_spd_info_packet(pipe_ctx->stream, &info_frame.spd_packet);
>>>> +    }
>>>>          translate_info_frame(&info_frame,
>>>>                &pipe_ctx->encoder_info_frame);
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
>>>> b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
>>>> index 80ac5d9efa71..3d1c32122d69 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
>>>> @@ -465,7 +465,6 @@ static uint32_t dce110_get_pix_clk_dividers_helper (
>>>>            struct pll_settings *pll_settings,
>>>>            struct pixel_clk_params *pix_clk_params)
>>>>    {
>>>> -    uint32_t addr = 0;
>>>>        uint32_t value = 0;
>>>>        uint32_t field = 0;
>>>>        uint32_t pll_calc_error = MAX_PLL_CALC_ERROR;
>>>> @@ -731,8 +730,6 @@ static void dce110_program_pixel_clk_resync(
>>>>            enum signal_type signal_type,
>>>>            enum dc_color_depth colordepth)
>>>>    {
>>>> -    uint32_t value = 0;
>>>> -
>>>>        REG_UPDATE(RESYNC_CNTL,
>>>>                DCCG_DEEP_COLOR_CNTL1, 0);
>>>>        /*
>>>> @@ -772,8 +769,6 @@ static void dce112_program_pixel_clk_resync(
>>>>            enum dc_color_depth colordepth,
>>>>            bool enable_ycbcr420)
>>>>    {
>>>> -    uint32_t value = 0;
>>>> -
>>>>        REG_UPDATE(PIXCLK_RESYNC_CNTL,
>>>>                PHYPLLA_DCCG_DEEP_COLOR_CNTL, 0);
>>>>        /*
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
>>>> b/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
>>>> index c2bd8dc7b4ad..262612061c68 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c
>>>> @@ -148,29 +148,6 @@ static int dce_divider_range_calc_divider(
>>>>      }
>>>>    -static int dce_divider_range_calc_did(
>>>> -    struct dce_divider_range *div_range,
>>>> -    int div)
>>>> -{
>>>> -    int did;
>>>> -    /* Check before dividing.*/
>>>> -    if (div_range->div_range_step == 0) {
>>>> -        div_range->div_range_step = 1;
>>>> -        /*div_range_step cannot be zero*/
>>>> -        BREAK_TO_DEBUGGER();
>>>> -    }
>>>> -    /* Is this divider within our range?*/
>>>> -    if ((div < div_range->div_range_start)
>>>> -        || (div >= div_range->div_range_end))
>>>> -        return INVALID_DID;
>>>> -/* did = (divider - range_start + (range_step-1)) / range_step) + did_min*/
>>>> -    did = div - div_range->div_range_start;
>>>> -    did += div_range->div_range_step - 1;
>>>> -    did /= div_range->div_range_step;
>>>> -    did += div_range->did_min;
>>>> -    return did;
>>>> -}
>>>> -
>>>>    static int dce_divider_range_get_divider(
>>>>        struct dce_divider_range *div_range,
>>>>        int ranges_num,
>>>> @@ -189,25 +166,7 @@ static int dce_divider_range_get_divider(
>>>>        return div;
>>>>    }
>>>>    -static int dce_divider_range_get_did(
>>>> -    struct dce_divider_range *div_range,
>>>> -    int ranges_num,
>>>> -    int divider)
>>>> -{
>>>> -    int did = INVALID_DID;
>>>> -    int i;
>>>> -
>>>> -    for (i = 0; i < ranges_num; i++) {
>>>> -        /*  CalcDid returns InvalidDid if a divider ID isn't found*/
>>>> -        did = dce_divider_range_calc_did(&div_range[i], divider);
>>>> -        /* Found a valid return did*/
>>>> -        if (did != INVALID_DID)
>>>> -            break;
>>>> -    }
>>>> -    return did;
>>>> -}
>>>> -
>>>> -static uint32_t dce_clocks_get_dp_ref_freq(struct display_clock *clk)
>>>> +static int dce_clocks_get_dp_ref_freq(struct display_clock *clk)
>>>>    {
>>>>        struct dce_disp_clk *clk_dce = TO_DCE_CLOCKS(clk);
>>>>        int dprefclk_wdivider;
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
>>>> b/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
>>>> index f47b6617f662..bbf4d97cb980 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c
>>>> @@ -83,8 +83,6 @@ static bool setup_scaling_configuration(
>>>>        struct dce_transform *xfm_dce,
>>>>        const struct scaler_data *data)
>>>>    {
>>>> -    struct dc_context *ctx = xfm_dce->base.ctx;
>>>> -
>>>>        if (data->taps.h_taps + data->taps.v_taps <= 2) {
>>>>            /* Set bypass */
>>>>            REG_UPDATE_2(SCL_MODE, SCL_MODE, 0, SCL_PSCL_EN, 0);
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
>>>> b/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
>>>> index 7d8cf7a58f46..feb5f3c29804 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_transform_v.c
>>>> @@ -621,7 +621,8 @@ static void dce110_xfmv_set_pixel_storage_depth(
>>>>        const struct bit_depth_reduction_params *bit_depth_params)
>>>>    {
>>>>        struct dce_transform *xfm_dce = TO_DCE_TRANSFORM(xfm);
>>>> -    int pixel_depth, expan_mode;
>>>> +    int pixel_depth = 0;
>>>> +    int expan_mode = 0;
>>>>        uint32_t reg_data = 0;
>>>>          switch (depth) {
>>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Harry Wentland wrote on 08.12.2016 19:58:
> On 2016-12-08 11:58 AM, Kai Wasserbäch wrote:
>> Harry Wentland wrote on 08.12.2016 17:36:
>>> On 2016-12-08 11:22 AM, Kai Wasserbäch wrote:
>>>> [Please CC me on all replies, I'm not subscribed to the list.]
>>>>
>>>> Harry Wentland wrote on 08.12.2016 02:26:
>>>>> Some of those are potential bugs
>>>> Are they related? If not: maybe split the changes up?
>>>>
>>>>> Change-Id: I53b2c663e18b57013e1b891fc2ecf1fb2d7d3a08
>>>>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>>>>> Reviewed-by: Tony Cheng <Tony.Cheng@amd.com>
>>>> It'd be nice to see these reviews on list... (applies to all patches here)
>>> These patches currently start out in our internal tree with a whole lot of stuff
>>> that we can't open up, like new IP, so unfortunately can't be done in a public
>>> forum. I pull the r-b from our internal review system for reference when I
>>> prepare the patches for public review.
>> Hm, not nice, but not my place to tell you to change this. That's Dave's and
>> Linus' job. ;-)
>>
>>>>> Acked-by: Harry Wentland <Harry.Wentland@amd.com>
>>>>> ---
>>>>>    .../gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c |  2 +-
>>>>>    drivers/gpu/drm/amd/display/dc/core/dc_resource.c  |  5 +--
>>>>>    .../gpu/drm/amd/display/dc/dce/dce_clock_source.c  |  5 ---
>>>>>    drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c    | 43
>>>>> +---------------------
>>>>>    drivers/gpu/drm/amd/display/dc/dce/dce_transform.c |  2 -
>>>>>    .../drm/amd/display/dc/dce110/dce110_transform_v.c |  3 +-
>>>>>    6 files changed, 6 insertions(+), 54 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>>>>> b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>>>>> index 0b2bb3992f1a..3b0710ef4716 100644
>>>>> --- a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>>>>> +++ b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c
>>>>> @@ -1709,7 +1709,7 @@ static void calculate_bandwidth(
>>>>>                else {
>>>>>                    data->blackout_recovery_time =
>>>>> bw_max2(data->blackout_recovery_time, bw_add(bw_mul(bw_int_to_fixed(2),
>>>>> vbios->mcifwrmc_urgent_latency),
>>>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level]));
>>>>>                    if (bw_ltn(data->adjusted_data_buffer_size[k],
>>>>> bw_mul(bw_div(bw_mul(data->display_bandwidth[k],
>>>>> data->useful_bytes_per_request[k]), data->bytes_per_request[k]),
>>>>> (bw_add(vbios->blackout_duration, bw_add(bw_mul(bw_int_to_fixed(2),
>>>>> vbios->mcifwrmc_urgent_latency),
>>>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])))))) {
>>>>> -                    data->blackout_recovery_time =
>>>>> bw_max2(data->blackout_recovery_time,
>>>>> bw_div((bw_add(bw_mul(bw_div(bw_mul(data->display_bandwidth[k],
>>>>> data->useful_bytes_per_request[k]), data->bytes_per_request[k]),
>>>>> vbios->blackout_duration),
>>>>> bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_int_to_fixed(2),
>>>>> vbios->mcifwrmc_urgent_latency), data->dmif_burst_time[i][j]),
>>>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])),
>>>>> data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])),
>>>>> data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]),
>>>>> data->adjusted_data_buffer_size[k]))),
>>>>> (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk,
>>>>> bw_int_to_fixed(data->bytes_per_pixel[k])),
>>>>> data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]),
>>>>> bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]),
>>>>> data->bytes_per_request[k])))));
>>>>> +                    data->blackout_recovery_time =
>>>>> bw_max2(data->blackout_recovery_time,
>>>>> bw_div((bw_add(bw_mul(bw_div(bw_mul(data->display_bandwidth[k],
>>>>> data->useful_bytes_per_request[k]), data->bytes_per_request[k]),
>>>>> vbios->blackout_duration),
>>>>> bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_int_to_fixed(2),
>>>>> vbios->mcifwrmc_urgent_latency),
>>>>> data->dmif_burst_time[data->y_clk_level][data->sclk_level]),
>>>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])),
>>>>> data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])),
>>>>> data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]),
>>>>> data->adjusted_data_buffer_size[k]))),
>>>>> (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk,
>>>>> bw_int_to_fixed(data->bytes_per_pixel[k])),
>>>>> data->lines_interleaved_in_mem_access[k]), data->latency_hiding_lines[k]),
>>>>> bw_div(bw_mul(data->display_bandwidth[k], data->useful_bytes_per_request[k]),
>>>>> data->bytes_per_request[k])))));
>>>> Uhg, can this be wrapped/split up somehow? Seeing the changes is nigh
>>>> impossible
>>>> and I suspect it might even be impossible for you to check internally.
>>> I believe this comes straight from some complicated code HW gives us and we use
>>> directly. I'll see if we can somehow make this more readable or at least add a
>>> comment, as you mention below.
>> That would be really appreciated. I mean, I'm in no way able to review
>> DAL/Display thoroughly (huge code base I'm not familiar with and where I might
>> easily miss interactions), but this is just not reviewable/verifiable at all for
>> any person outside your inner circle.
> I followed up with Tony, who's been working with HW guys to get them to share
> this code with us. Unfortunately there isn't much we can do in the way of
> changing this code.

That's rather unfortunate, that not even reformatting into an easier to read
format is possible (though I do not really understand why, beyond "it takes
work"); maybe the comment showing the formula in an easier to read form can be
done then?

Cheers,
Kai

P.S.: Did you see my comments on patch 2? That contained the null pointer deref
(if I didn't misread something).
> -----Original Message-----

> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf

> Of Kai Wasserbäch

> Sent: Thursday, December 08, 2016 2:52 PM

> To: Wentland, Harry; amd-gfx@lists.freedesktop.org; Cheng, Tony

> Subject: Re: [PATCH 05/12] drm/amd/display: Fix bunch of warnings in DC

> 

> Harry Wentland wrote on 08.12.2016 19:58:

> > On 2016-12-08 11:58 AM, Kai Wasserbäch wrote:

> >> Harry Wentland wrote on 08.12.2016 17:36:

> >>> On 2016-12-08 11:22 AM, Kai Wasserbäch wrote:

> >>>> [Please CC me on all replies, I'm not subscribed to the list.]

> >>>>

> >>>> Harry Wentland wrote on 08.12.2016 02:26:

> >>>>> Some of those are potential bugs

> >>>> Are they related? If not: maybe split the changes up?

> >>>>

> >>>>> Change-Id: I53b2c663e18b57013e1b891fc2ecf1fb2d7d3a08

> >>>>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>

> >>>>> Reviewed-by: Tony Cheng <Tony.Cheng@amd.com>

> >>>> It'd be nice to see these reviews on list... (applies to all patches here)

> >>> These patches currently start out in our internal tree with a whole lot of

> stuff

> >>> that we can't open up, like new IP, so unfortunately can't be done in a

> public

> >>> forum. I pull the r-b from our internal review system for reference when

> I

> >>> prepare the patches for public review.

> >> Hm, not nice, but not my place to tell you to change this. That's Dave's and

> >> Linus' job. ;-)

> >>

> >>>>> Acked-by: Harry Wentland <Harry.Wentland@amd.com>

> >>>>> ---

> >>>>>    .../gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c |  2 +-

> >>>>>    drivers/gpu/drm/amd/display/dc/core/dc_resource.c  |  5 +--

> >>>>>    .../gpu/drm/amd/display/dc/dce/dce_clock_source.c  |  5 ---

> >>>>>    drivers/gpu/drm/amd/display/dc/dce/dce_clocks.c    | 43

> >>>>> +---------------------

> >>>>>    drivers/gpu/drm/amd/display/dc/dce/dce_transform.c |  2 -

> >>>>>    .../drm/amd/display/dc/dce110/dce110_transform_v.c |  3 +-

> >>>>>    6 files changed, 6 insertions(+), 54 deletions(-)

> >>>>>

> >>>>> diff --git a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c

> >>>>> b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c

> >>>>> index 0b2bb3992f1a..3b0710ef4716 100644

> >>>>> --- a/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c

> >>>>> +++ b/drivers/gpu/drm/amd/display/dc/calcs/bandwidth_calcs.c

> >>>>> @@ -1709,7 +1709,7 @@ static void calculate_bandwidth(

> >>>>>                else {

> >>>>>                    data->blackout_recovery_time =

> >>>>> bw_max2(data->blackout_recovery_time,

> bw_add(bw_mul(bw_int_to_fixed(2),

> >>>>> vbios->mcifwrmc_urgent_latency),

> >>>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level]));

> >>>>>                    if (bw_ltn(data->adjusted_data_buffer_size[k],

> >>>>> bw_mul(bw_div(bw_mul(data->display_bandwidth[k],

> >>>>> data->useful_bytes_per_request[k]), data->bytes_per_request[k]),

> >>>>> (bw_add(vbios->blackout_duration,

> bw_add(bw_mul(bw_int_to_fixed(2),

> >>>>> vbios->mcifwrmc_urgent_latency),

> >>>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])))))) {

> >>>>> -                    data->blackout_recovery_time =

> >>>>> bw_max2(data->blackout_recovery_time,

> >>>>> bw_div((bw_add(bw_mul(bw_div(bw_mul(data-

> >display_bandwidth[k],

> >>>>> data->useful_bytes_per_request[k]), data->bytes_per_request[k]),

> >>>>> vbios->blackout_duration),

> >>>>>

> bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_i

> nt_to_fixed(2),

> >>>>> vbios->mcifwrmc_urgent_latency), data->dmif_burst_time[i][j]),

> >>>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])),

> >>>>> data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])),

> >>>>> data->lines_interleaved_in_mem_access[k]), data-

> >latency_hiding_lines[k]),

> >>>>> data->adjusted_data_buffer_size[k]))),

> >>>>> (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk,

> >>>>> bw_int_to_fixed(data->bytes_per_pixel[k])),

> >>>>> data->lines_interleaved_in_mem_access[k]), data-

> >latency_hiding_lines[k]),

> >>>>> bw_div(bw_mul(data->display_bandwidth[k], data-

> >useful_bytes_per_request[k]),

> >>>>> data->bytes_per_request[k])))));

> >>>>> +                    data->blackout_recovery_time =

> >>>>> bw_max2(data->blackout_recovery_time,

> >>>>> bw_div((bw_add(bw_mul(bw_div(bw_mul(data-

> >display_bandwidth[k],

> >>>>> data->useful_bytes_per_request[k]), data->bytes_per_request[k]),

> >>>>> vbios->blackout_duration),

> >>>>>

> bw_sub(bw_div(bw_mul(bw_mul(bw_mul((bw_add(bw_add(bw_mul(bw_i

> nt_to_fixed(2),

> >>>>> vbios->mcifwrmc_urgent_latency),

> >>>>> data->dmif_burst_time[data->y_clk_level][data->sclk_level]),

> >>>>> data->mcifwr_burst_time[data->y_clk_level][data->sclk_level])),

> >>>>> data->dispclk), bw_int_to_fixed(data->bytes_per_pixel[k])),

> >>>>> data->lines_interleaved_in_mem_access[k]), data-

> >latency_hiding_lines[k]),

> >>>>> data->adjusted_data_buffer_size[k]))),

> >>>>> (bw_sub(bw_div(bw_mul(bw_mul(data->dispclk,

> >>>>> bw_int_to_fixed(data->bytes_per_pixel[k])),

> >>>>> data->lines_interleaved_in_mem_access[k]), data-

> >latency_hiding_lines[k]),

> >>>>> bw_div(bw_mul(data->display_bandwidth[k], data-

> >useful_bytes_per_request[k]),

> >>>>> data->bytes_per_request[k])))));

> >>>> Uhg, can this be wrapped/split up somehow? Seeing the changes is

> nigh

> >>>> impossible

> >>>> and I suspect it might even be impossible for you to check internally.

> >>> I believe this comes straight from some complicated code HW gives us

> and we use

> >>> directly. I'll see if we can somehow make this more readable or at least

> add a

> >>> comment, as you mention below.

> >> That would be really appreciated. I mean, I'm in no way able to review

> >> DAL/Display thoroughly (huge code base I'm not familiar with and where I

> might

> >> easily miss interactions), but this is just not reviewable/verifiable at all for

> >> any person outside your inner circle.

> > I followed up with Tony, who's been working with HW guys to get them to

> share

> > this code with us. Unfortunately there isn't much we can do in the way of

> > changing this code.

> 

> That's rather unfortunate, that not even reformatting into an easier to read

> format is possible (though I do not really understand why, beyond "it takes

> work"); maybe the comment showing the formula in an easier to read form

> can be

> done then?


IIRC, this code is machine generated by the hw team.  We've been down this road with them before.  Unfortunately, it hasn't been a high enough priority for them to get around to it.  We don't really have insight into it at the moment to know how much work it would be.

Alex