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

Harry Wentland harry.wentland at amd.com
Thu Dec 8 16:36:56 UTC 2016



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 at amd.com>
>> Reviewed-by: Tony Cheng <Tony.Cheng at 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 at 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) {
>>



More information about the amd-gfx mailing list