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

Kai Wasserbäch kai at dev.carbon-project.org
Thu Dec 8 16:58:44 UTC 2016


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>


More information about the amd-gfx mailing list