> -----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
