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

Thu Dec 8 19:59:31 UTC 2016

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf
> Of Kai Wasserbäch
> Sent: Thursday, December 08, 2016 2:52 PM
> To: Wentland, Harry; amd-gfx at 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 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_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