| Message ID | 20190819155038.1771-7-David.Francis@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Display Stream Compression (DSC) for AMD Navi | expand |
On 8/19/19 11:50 AM, David Francis wrote: > We were using drm helpers to convert a timing into its > bandwidth, its bandwidth into pbn, and its pbn into timeslots > > These helpers > -Did not take DSC timings into account > -Used the link rate and lane count of the link's aux device, > which are not the same as the link's current cap > -Did not take FEC into account (FEC reduces the PBN per timeslot) > > Use the DC helpers (dc_bandwidth_in_kbps_from_timing, > dc_link_bandwidth_kbps) instead > > Cc: Jerry Zuo <Jerry.Zuo@amd.com> > Signed-off-by: David Francis <David.Francis@amd.com> Wouldn't this be a good candidate for shared DRM helpers or to modify the existing ones to support this usecase? Seems like this would be shared across drivers. Nicholas Kazlauskas > --- > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 41 ++++--------------- > 1 file changed, 8 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > index 5f2c315b18ba..b32c0790399a 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > @@ -189,8 +189,8 @@ bool dm_helpers_dp_mst_write_payload_allocation_table( > int slots = 0; > bool ret; > int clock; > - int bpp = 0; > int pbn = 0; > + int pbn_per_timeslot; > > aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context; > > @@ -205,40 +205,15 @@ bool dm_helpers_dp_mst_write_payload_allocation_table( > mst_port = aconnector->port; > > if (enable) { > - clock = stream->timing.pix_clk_100hz / 10; > - > - switch (stream->timing.display_color_depth) { > - > - case COLOR_DEPTH_666: > - bpp = 6; > - break; > - case COLOR_DEPTH_888: > - bpp = 8; > - break; > - case COLOR_DEPTH_101010: > - bpp = 10; > - break; > - case COLOR_DEPTH_121212: > - bpp = 12; > - break; > - case COLOR_DEPTH_141414: > - bpp = 14; > - break; > - case COLOR_DEPTH_161616: > - bpp = 16; > - break; > - default: > - ASSERT(bpp != 0); > - break; > - } > - > - bpp = bpp * 3; > - > - /* TODO need to know link rate */ > + clock = dc_bandwidth_in_kbps_from_timing(&stream->timing); > > - pbn = drm_dp_calc_pbn_mode(clock, bpp); > + /* dc_bandwidth_in_kbps_from_timing already takes bpp into account */ > + pbn = drm_dp_calc_pbn_mode(clock, 1); > > - slots = drm_dp_find_vcpi_slots(mst_mgr, pbn); > + /* Convert kilobits per second / 64 (for 64 timeslots) to pbn (54/64 megabytes per second) */ > + pbn_per_timeslot = dc_link_bandwidth_kbps( > + stream->link, dc_link_get_link_cap(stream->link)) / (8 * 1000 * 54); > + slots = DIV_ROUND_UP(pbn, pbn_per_timeslot); > ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots); > > if (!ret) >
On 8/19/19 11:50 AM, David Francis wrote: >> We were using drm helpers to convert a timing into its >> bandwidth, its bandwidth into pbn, and its pbn into timeslots >> >> These helpers >> -Did not take DSC timings into account >> -Used the link rate and lane count of the link's aux device, >> which are not the same as the link's current cap >> -Did not take FEC into account (FEC reduces the PBN per timeslot) >> >> Use the DC helpers (dc_bandwidth_in_kbps_from_timing, >> dc_link_bandwidth_kbps) instead >> >> Cc: Jerry Zuo <Jerry.Zuo@amd.com> >> Signed-off-by: David Francis <David.Francis@amd.com> > >Wouldn't this be a good candidate for shared DRM helpers or to modify >the existing ones to support this usecase? > >Seems like this would be shared across drivers. > >Nicholas Kazlauskas > These functions use information which is not stored in drm structs but tracked in a driver-specific way - Whether or not FEC is enabled - Whether or not DSC is enabled, and with what config - The current link setting This could be shifted into drm helpers, but it would require functions with signatures like drm_dp_calc_pbn_mode(clock, bpp, dsc_bpp) and drm_dp_find_vcpi_slots(mst_mgr, pbn, fec_enabled, lane_count, link_rate) and each driver would need to convert their own structs into those fields before calling these helpers. Is that worth it? >> --- >> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 41 ++++--------------- >> 1 file changed, 8 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> index 5f2c315b18ba..b32c0790399a 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> @@ -189,8 +189,8 @@ bool dm_helpers_dp_mst_write_payload_allocation_table( >> int slots = 0; >> bool ret; >> int clock; >> - int bpp = 0; >> int pbn = 0; >> + int pbn_per_timeslot; >> >> aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context; >> >> @@ -205,40 +205,15 @@ bool dm_helpers_dp_mst_write_payload_allocation_table( >> mst_port = aconnector->port; >> >> if (enable) { >> - clock = stream->timing.pix_clk_100hz / 10; >> - >> - switch (stream->timing.display_color_depth) { >> - >> - case COLOR_DEPTH_666: >> - bpp = 6; >> - break; >> - case COLOR_DEPTH_888: >> - bpp = 8; >> - break; >> - case COLOR_DEPTH_101010: >> - bpp = 10; >> - break; >> - case COLOR_DEPTH_121212: >> - bpp = 12; >> - break; >> - case COLOR_DEPTH_141414: >> - bpp = 14; >> - break; >> - case COLOR_DEPTH_161616: >> - bpp = 16; >> - break; >> - default: >> - ASSERT(bpp != 0); >> - break; >> - } >> - >> - bpp = bpp * 3; >> - >> - /* TODO need to know link rate */ >> + clock = dc_bandwidth_in_kbps_from_timing(&stream->timing); >> >> - pbn = drm_dp_calc_pbn_mode(clock, bpp); >> + /* dc_bandwidth_in_kbps_from_timing already takes bpp into account */ >> + pbn = drm_dp_calc_pbn_mode(clock, 1); >> >> - slots = drm_dp_find_vcpi_slots(mst_mgr, pbn); >> + /* Convert kilobits per second / 64 (for 64 timeslots) to pbn (54/64 megabytes per second) */ >> + pbn_per_timeslot = dc_link_bandwidth_kbps( >> + stream->link, dc_link_get_link_cap(stream->link)) / (8 * 1000 * 54); >> + slots = DIV_ROUND_UP(pbn, pbn_per_timeslot); >> ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots); >> >> if (!ret)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c index 5f2c315b18ba..b32c0790399a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c @@ -189,8 +189,8 @@ bool dm_helpers_dp_mst_write_payload_allocation_table( int slots = 0; bool ret; int clock; - int bpp = 0; int pbn = 0; + int pbn_per_timeslot; aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context; @@ -205,40 +205,15 @@ bool dm_helpers_dp_mst_write_payload_allocation_table( mst_port = aconnector->port; if (enable) { - clock = stream->timing.pix_clk_100hz / 10; - - switch (stream->timing.display_color_depth) { - - case COLOR_DEPTH_666: - bpp = 6; - break; - case COLOR_DEPTH_888: - bpp = 8; - break; - case COLOR_DEPTH_101010: - bpp = 10; - break; - case COLOR_DEPTH_121212: - bpp = 12; - break; - case COLOR_DEPTH_141414: - bpp = 14; - break; - case COLOR_DEPTH_161616: - bpp = 16; - break; - default: - ASSERT(bpp != 0); - break; - } - - bpp = bpp * 3; - - /* TODO need to know link rate */ + clock = dc_bandwidth_in_kbps_from_timing(&stream->timing); - pbn = drm_dp_calc_pbn_mode(clock, bpp); + /* dc_bandwidth_in_kbps_from_timing already takes bpp into account */ + pbn = drm_dp_calc_pbn_mode(clock, 1); - slots = drm_dp_find_vcpi_slots(mst_mgr, pbn); + /* Convert kilobits per second / 64 (for 64 timeslots) to pbn (54/64 megabytes per second) */ + pbn_per_timeslot = dc_link_bandwidth_kbps( + stream->link, dc_link_get_link_cap(stream->link)) / (8 * 1000 * 54); + slots = DIV_ROUND_UP(pbn, pbn_per_timeslot); ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots); if (!ret)
We were using drm helpers to convert a timing into its bandwidth, its bandwidth into pbn, and its pbn into timeslots These helpers -Did not take DSC timings into account -Used the link rate and lane count of the link's aux device, which are not the same as the link's current cap -Did not take FEC into account (FEC reduces the PBN per timeslot) Use the DC helpers (dc_bandwidth_in_kbps_from_timing, dc_link_bandwidth_kbps) instead Cc: Jerry Zuo <Jerry.Zuo@amd.com> Signed-off-by: David Francis <David.Francis@amd.com> --- .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 41 ++++--------------- 1 file changed, 8 insertions(+), 33 deletions(-)