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 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.
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 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.
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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
--
Kai Wasserbäch (Kai Wasserbaech)
E-Mail: kai at dev.carbon-project.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20161208/2cd85a2e/attachment.sig>
