* [PATCH v9] drm/dp_mst: Add PBN calculation for DSC modes @ 2019-12-03 16:28 ` mikita.lipski 0 siblings, 0 replies; 6+ messages in thread From: mikita.lipski @ 2019-12-03 16:28 UTC (permalink / raw) To: amd-gfx; +Cc: David Francis, Mikita Lipski, dri-devel From: David Francis <David.Francis@amd.com> With DSC, bpp can be fractional in multiples of 1/16. Change drm_dp_calc_pbn_mode to reflect this, adding a new parameter bool dsc. When this parameter is true, treat the bpp parameter as having units not of bits per pixel, but 1/16 of a bit per pixel v2: Don't add separate function for this v3: Keep the calculation in a single equation v4: Update the tests in test-drm_dp_mst_helper.c Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> Reviewed-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Harry Wentland <harry.wentland@amd.com> Signed-off-by: David Francis <David.Francis@amd.com> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- drivers/gpu/drm/drm_dp_mst_topology.c | 12 +++++++++++- drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 ++- drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- drivers/gpu/drm/radeon/radeon_dp_mst.c | 2 +- drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c | 10 ++++++---- include/drm/drm_dp_mst_helper.h | 3 +-- 7 files changed, 23 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 455c51c38720..9fc03fc1017d 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4967,7 +4967,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder, is_y420); bpp = convert_dc_color_depth_into_bpc(color_depth) * 3; clock = adjusted_mode->clock; - dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp); + dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp, false); } dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state, mst_mgr, diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index ae5809a1f19a..828b5eae529c 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4342,10 +4342,11 @@ EXPORT_SYMBOL(drm_dp_check_act_status); * drm_dp_calc_pbn_mode() - Calculate the PBN for a mode. * @clock: dot clock for the mode * @bpp: bpp for the mode. + * @dsc: DSC mode. If true, bpp has units of 1/16 of a bit per pixel * * This uses the formula in the spec to calculate the PBN value for a mode. */ -int drm_dp_calc_pbn_mode(int clock, int bpp) +int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc) { /* * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006 @@ -4356,7 +4357,16 @@ int drm_dp_calc_pbn_mode(int clock, int bpp) * peak_kbps *= (1006/1000) * peak_kbps *= (64/54) * peak_kbps *= 8 convert to bytes + * + * If the bpp is in units of 1/16, further divide by 16. Put this + * factor in the numerator rather than the denominator to avoid + * integer overflow */ + + if(dsc) + return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 / 16), + 8 * 54 * 1000 * 1000); + return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006), 8 * 54 * 1000 * 1000); } diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 03d1cba0b696..92be17711287 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -61,7 +61,8 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, crtc_state->pipe_bpp = bpp; crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, - crtc_state->pipe_bpp); + crtc_state->pipe_bpp, + false); slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr, port, crtc_state->pbn); diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 549486f1d937..1c9e23d5a6fd 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -782,7 +782,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, const int bpp = connector->display_info.bpc * 3; const int clock = crtc_state->adjusted_mode.clock; - asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp); + asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp, false); } slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c index ee28f5b3785e..28eef9282874 100644 --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c @@ -518,7 +518,7 @@ static bool radeon_mst_mode_fixup(struct drm_encoder *encoder, mst_enc = radeon_encoder->enc_priv; - mst_enc->pbn = drm_dp_calc_pbn_mode(adjusted_mode->clock, bpp); + mst_enc->pbn = drm_dp_calc_pbn_mode(adjusted_mode->clock, bpp, false); mst_enc->primary->active_device = mst_enc->primary->devices & mst_enc->connector->devices; DRM_DEBUG_KMS("setting active device to %08x from %08x %08x for encoder %d\n", diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c index af2b2de65316..73fc1c485283 100644 --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c @@ -18,15 +18,17 @@ int igt_dp_mst_calc_pbn_mode(void *ignored) int rate; int bpp; int expected; + bool dsc; } test_params[] = { - { 154000, 30, 689 }, - { 234000, 30, 1047 }, - { 297000, 24, 1063 }, + { 154000, 30, 689, false }, + { 234000, 30, 1047, false }, + { 297000, 24, 1063, false }, }; for (i = 0; i < ARRAY_SIZE(test_params); i++) { pbn = drm_dp_calc_pbn_mode(test_params[i].rate, - test_params[i].bpp); + test_params[i].bpp, + test_params[i].dsc); FAIL(pbn != test_params[i].expected, "Expected PBN %d for clock %d bpp %d, got %d\n", test_params[i].expected, test_params[i].rate, diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index d5fc90b30487..68656913cfe5 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -719,8 +719,7 @@ bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr, struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port); -int drm_dp_calc_pbn_mode(int clock, int bpp); - +int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc); bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, int pbn, int slots); -- 2.17.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v9] drm/dp_mst: Add PBN calculation for DSC modes @ 2019-12-03 16:28 ` mikita.lipski 0 siblings, 0 replies; 6+ messages in thread From: mikita.lipski @ 2019-12-03 16:28 UTC (permalink / raw) To: amd-gfx; +Cc: David Francis, Mikita Lipski, dri-devel From: David Francis <David.Francis@amd.com> With DSC, bpp can be fractional in multiples of 1/16. Change drm_dp_calc_pbn_mode to reflect this, adding a new parameter bool dsc. When this parameter is true, treat the bpp parameter as having units not of bits per pixel, but 1/16 of a bit per pixel v2: Don't add separate function for this v3: Keep the calculation in a single equation v4: Update the tests in test-drm_dp_mst_helper.c Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> Reviewed-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Harry Wentland <harry.wentland@amd.com> Signed-off-by: David Francis <David.Francis@amd.com> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- drivers/gpu/drm/drm_dp_mst_topology.c | 12 +++++++++++- drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 ++- drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- drivers/gpu/drm/radeon/radeon_dp_mst.c | 2 +- drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c | 10 ++++++---- include/drm/drm_dp_mst_helper.h | 3 +-- 7 files changed, 23 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 455c51c38720..9fc03fc1017d 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4967,7 +4967,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder, is_y420); bpp = convert_dc_color_depth_into_bpc(color_depth) * 3; clock = adjusted_mode->clock; - dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp); + dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp, false); } dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state, mst_mgr, diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index ae5809a1f19a..828b5eae529c 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4342,10 +4342,11 @@ EXPORT_SYMBOL(drm_dp_check_act_status); * drm_dp_calc_pbn_mode() - Calculate the PBN for a mode. * @clock: dot clock for the mode * @bpp: bpp for the mode. + * @dsc: DSC mode. If true, bpp has units of 1/16 of a bit per pixel * * This uses the formula in the spec to calculate the PBN value for a mode. */ -int drm_dp_calc_pbn_mode(int clock, int bpp) +int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc) { /* * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006 @@ -4356,7 +4357,16 @@ int drm_dp_calc_pbn_mode(int clock, int bpp) * peak_kbps *= (1006/1000) * peak_kbps *= (64/54) * peak_kbps *= 8 convert to bytes + * + * If the bpp is in units of 1/16, further divide by 16. Put this + * factor in the numerator rather than the denominator to avoid + * integer overflow */ + + if(dsc) + return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 / 16), + 8 * 54 * 1000 * 1000); + return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006), 8 * 54 * 1000 * 1000); } diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 03d1cba0b696..92be17711287 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -61,7 +61,8 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, crtc_state->pipe_bpp = bpp; crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, - crtc_state->pipe_bpp); + crtc_state->pipe_bpp, + false); slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr, port, crtc_state->pbn); diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 549486f1d937..1c9e23d5a6fd 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -782,7 +782,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, const int bpp = connector->display_info.bpc * 3; const int clock = crtc_state->adjusted_mode.clock; - asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp); + asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp, false); } slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c index ee28f5b3785e..28eef9282874 100644 --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c @@ -518,7 +518,7 @@ static bool radeon_mst_mode_fixup(struct drm_encoder *encoder, mst_enc = radeon_encoder->enc_priv; - mst_enc->pbn = drm_dp_calc_pbn_mode(adjusted_mode->clock, bpp); + mst_enc->pbn = drm_dp_calc_pbn_mode(adjusted_mode->clock, bpp, false); mst_enc->primary->active_device = mst_enc->primary->devices & mst_enc->connector->devices; DRM_DEBUG_KMS("setting active device to %08x from %08x %08x for encoder %d\n", diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c index af2b2de65316..73fc1c485283 100644 --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c @@ -18,15 +18,17 @@ int igt_dp_mst_calc_pbn_mode(void *ignored) int rate; int bpp; int expected; + bool dsc; } test_params[] = { - { 154000, 30, 689 }, - { 234000, 30, 1047 }, - { 297000, 24, 1063 }, + { 154000, 30, 689, false }, + { 234000, 30, 1047, false }, + { 297000, 24, 1063, false }, }; for (i = 0; i < ARRAY_SIZE(test_params); i++) { pbn = drm_dp_calc_pbn_mode(test_params[i].rate, - test_params[i].bpp); + test_params[i].bpp, + test_params[i].dsc); FAIL(pbn != test_params[i].expected, "Expected PBN %d for clock %d bpp %d, got %d\n", test_params[i].expected, test_params[i].rate, diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index d5fc90b30487..68656913cfe5 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -719,8 +719,7 @@ bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr, struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port); -int drm_dp_calc_pbn_mode(int clock, int bpp); - +int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc); bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, int pbn, int slots); -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v9] drm/dp_mst: Add PBN calculation for DSC modes 2019-12-03 16:28 ` mikita.lipski @ 2019-12-04 16:45 ` Jani Nikula -1 siblings, 0 replies; 6+ messages in thread From: Jani Nikula @ 2019-12-04 16:45 UTC (permalink / raw) To: mikita.lipski, amd-gfx; +Cc: David Francis, Mikita Lipski, dri-devel On Tue, 03 Dec 2019, <mikita.lipski@amd.com> wrote: > From: David Francis <David.Francis@amd.com> > > With DSC, bpp can be fractional in multiples of 1/16. Can be? I worry a bit that "bpp" can either be integer or fixed point depending on other variables. I admit I haven't followed up on this too much, but how widespread it is? It seems like something that is bound to be broken in subtle ways, when it won't even cross people's minds that bpp is fractional. BR, Jani. > > Change drm_dp_calc_pbn_mode to reflect this, adding a new > parameter bool dsc. When this parameter is true, treat the > bpp parameter as having units not of bits per pixel, but > 1/16 of a bit per pixel > > v2: Don't add separate function for this > v3: Keep the calculation in a single equation > v4: Update the tests in test-drm_dp_mst_helper.c > > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> > Reviewed-by: Lyude Paul <lyude@redhat.com> > Reviewed-by: Harry Wentland <harry.wentland@amd.com> > Signed-off-by: David Francis <David.Francis@amd.com> > Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- > drivers/gpu/drm/drm_dp_mst_topology.c | 12 +++++++++++- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 ++- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- > drivers/gpu/drm/radeon/radeon_dp_mst.c | 2 +- > drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c | 10 ++++++---- > include/drm/drm_dp_mst_helper.h | 3 +-- > 7 files changed, 23 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 455c51c38720..9fc03fc1017d 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -4967,7 +4967,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder, > is_y420); > bpp = convert_dc_color_depth_into_bpc(color_depth) * 3; > clock = adjusted_mode->clock; > - dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp); > + dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp, false); > } > dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state, > mst_mgr, > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index ae5809a1f19a..828b5eae529c 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -4342,10 +4342,11 @@ EXPORT_SYMBOL(drm_dp_check_act_status); > * drm_dp_calc_pbn_mode() - Calculate the PBN for a mode. > * @clock: dot clock for the mode > * @bpp: bpp for the mode. > + * @dsc: DSC mode. If true, bpp has units of 1/16 of a bit per pixel > * > * This uses the formula in the spec to calculate the PBN value for a mode. > */ > -int drm_dp_calc_pbn_mode(int clock, int bpp) > +int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc) > { > /* > * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006 > @@ -4356,7 +4357,16 @@ int drm_dp_calc_pbn_mode(int clock, int bpp) > * peak_kbps *= (1006/1000) > * peak_kbps *= (64/54) > * peak_kbps *= 8 convert to bytes > + * > + * If the bpp is in units of 1/16, further divide by 16. Put this > + * factor in the numerator rather than the denominator to avoid > + * integer overflow > */ > + > + if(dsc) > + return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 / 16), > + 8 * 54 * 1000 * 1000); > + > return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006), > 8 * 54 * 1000 * 1000); > } > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 03d1cba0b696..92be17711287 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -61,7 +61,8 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > crtc_state->pipe_bpp = bpp; > > crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, > - crtc_state->pipe_bpp); > + crtc_state->pipe_bpp, > + false); > > slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr, > port, crtc_state->pbn); > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index 549486f1d937..1c9e23d5a6fd 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > @@ -782,7 +782,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, > const int bpp = connector->display_info.bpc * 3; > const int clock = crtc_state->adjusted_mode.clock; > > - asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp); > + asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp, false); > } > > slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, > diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c > index ee28f5b3785e..28eef9282874 100644 > --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c > +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c > @@ -518,7 +518,7 @@ static bool radeon_mst_mode_fixup(struct drm_encoder *encoder, > > mst_enc = radeon_encoder->enc_priv; > > - mst_enc->pbn = drm_dp_calc_pbn_mode(adjusted_mode->clock, bpp); > + mst_enc->pbn = drm_dp_calc_pbn_mode(adjusted_mode->clock, bpp, false); > > mst_enc->primary->active_device = mst_enc->primary->devices & mst_enc->connector->devices; > DRM_DEBUG_KMS("setting active device to %08x from %08x %08x for encoder %d\n", > diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c > index af2b2de65316..73fc1c485283 100644 > --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c > +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c > @@ -18,15 +18,17 @@ int igt_dp_mst_calc_pbn_mode(void *ignored) > int rate; > int bpp; > int expected; > + bool dsc; > } test_params[] = { > - { 154000, 30, 689 }, > - { 234000, 30, 1047 }, > - { 297000, 24, 1063 }, > + { 154000, 30, 689, false }, > + { 234000, 30, 1047, false }, > + { 297000, 24, 1063, false }, > }; > > for (i = 0; i < ARRAY_SIZE(test_params); i++) { > pbn = drm_dp_calc_pbn_mode(test_params[i].rate, > - test_params[i].bpp); > + test_params[i].bpp, > + test_params[i].dsc); > FAIL(pbn != test_params[i].expected, > "Expected PBN %d for clock %d bpp %d, got %d\n", > test_params[i].expected, test_params[i].rate, > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > index d5fc90b30487..68656913cfe5 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -719,8 +719,7 @@ bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr, > struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port); > > > -int drm_dp_calc_pbn_mode(int clock, int bpp); > - > +int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc); > > bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, > struct drm_dp_mst_port *port, int pbn, int slots); -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v9] drm/dp_mst: Add PBN calculation for DSC modes @ 2019-12-04 16:45 ` Jani Nikula 0 siblings, 0 replies; 6+ messages in thread From: Jani Nikula @ 2019-12-04 16:45 UTC (permalink / raw) To: mikita.lipski, amd-gfx; +Cc: David Francis, Mikita Lipski, dri-devel On Tue, 03 Dec 2019, <mikita.lipski@amd.com> wrote: > From: David Francis <David.Francis@amd.com> > > With DSC, bpp can be fractional in multiples of 1/16. Can be? I worry a bit that "bpp" can either be integer or fixed point depending on other variables. I admit I haven't followed up on this too much, but how widespread it is? It seems like something that is bound to be broken in subtle ways, when it won't even cross people's minds that bpp is fractional. BR, Jani. > > Change drm_dp_calc_pbn_mode to reflect this, adding a new > parameter bool dsc. When this parameter is true, treat the > bpp parameter as having units not of bits per pixel, but > 1/16 of a bit per pixel > > v2: Don't add separate function for this > v3: Keep the calculation in a single equation > v4: Update the tests in test-drm_dp_mst_helper.c > > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> > Reviewed-by: Lyude Paul <lyude@redhat.com> > Reviewed-by: Harry Wentland <harry.wentland@amd.com> > Signed-off-by: David Francis <David.Francis@amd.com> > Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- > drivers/gpu/drm/drm_dp_mst_topology.c | 12 +++++++++++- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 ++- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- > drivers/gpu/drm/radeon/radeon_dp_mst.c | 2 +- > drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c | 10 ++++++---- > include/drm/drm_dp_mst_helper.h | 3 +-- > 7 files changed, 23 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 455c51c38720..9fc03fc1017d 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -4967,7 +4967,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder, > is_y420); > bpp = convert_dc_color_depth_into_bpc(color_depth) * 3; > clock = adjusted_mode->clock; > - dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp); > + dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp, false); > } > dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state, > mst_mgr, > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index ae5809a1f19a..828b5eae529c 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -4342,10 +4342,11 @@ EXPORT_SYMBOL(drm_dp_check_act_status); > * drm_dp_calc_pbn_mode() - Calculate the PBN for a mode. > * @clock: dot clock for the mode > * @bpp: bpp for the mode. > + * @dsc: DSC mode. If true, bpp has units of 1/16 of a bit per pixel > * > * This uses the formula in the spec to calculate the PBN value for a mode. > */ > -int drm_dp_calc_pbn_mode(int clock, int bpp) > +int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc) > { > /* > * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006 > @@ -4356,7 +4357,16 @@ int drm_dp_calc_pbn_mode(int clock, int bpp) > * peak_kbps *= (1006/1000) > * peak_kbps *= (64/54) > * peak_kbps *= 8 convert to bytes > + * > + * If the bpp is in units of 1/16, further divide by 16. Put this > + * factor in the numerator rather than the denominator to avoid > + * integer overflow > */ > + > + if(dsc) > + return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 / 16), > + 8 * 54 * 1000 * 1000); > + > return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006), > 8 * 54 * 1000 * 1000); > } > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 03d1cba0b696..92be17711287 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -61,7 +61,8 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > crtc_state->pipe_bpp = bpp; > > crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, > - crtc_state->pipe_bpp); > + crtc_state->pipe_bpp, > + false); > > slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr, > port, crtc_state->pbn); > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index 549486f1d937..1c9e23d5a6fd 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > @@ -782,7 +782,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, > const int bpp = connector->display_info.bpc * 3; > const int clock = crtc_state->adjusted_mode.clock; > > - asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp); > + asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp, false); > } > > slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, > diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c > index ee28f5b3785e..28eef9282874 100644 > --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c > +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c > @@ -518,7 +518,7 @@ static bool radeon_mst_mode_fixup(struct drm_encoder *encoder, > > mst_enc = radeon_encoder->enc_priv; > > - mst_enc->pbn = drm_dp_calc_pbn_mode(adjusted_mode->clock, bpp); > + mst_enc->pbn = drm_dp_calc_pbn_mode(adjusted_mode->clock, bpp, false); > > mst_enc->primary->active_device = mst_enc->primary->devices & mst_enc->connector->devices; > DRM_DEBUG_KMS("setting active device to %08x from %08x %08x for encoder %d\n", > diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c > index af2b2de65316..73fc1c485283 100644 > --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c > +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c > @@ -18,15 +18,17 @@ int igt_dp_mst_calc_pbn_mode(void *ignored) > int rate; > int bpp; > int expected; > + bool dsc; > } test_params[] = { > - { 154000, 30, 689 }, > - { 234000, 30, 1047 }, > - { 297000, 24, 1063 }, > + { 154000, 30, 689, false }, > + { 234000, 30, 1047, false }, > + { 297000, 24, 1063, false }, > }; > > for (i = 0; i < ARRAY_SIZE(test_params); i++) { > pbn = drm_dp_calc_pbn_mode(test_params[i].rate, > - test_params[i].bpp); > + test_params[i].bpp, > + test_params[i].dsc); > FAIL(pbn != test_params[i].expected, > "Expected PBN %d for clock %d bpp %d, got %d\n", > test_params[i].expected, test_params[i].rate, > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > index d5fc90b30487..68656913cfe5 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -719,8 +719,7 @@ bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr, > struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port); > > > -int drm_dp_calc_pbn_mode(int clock, int bpp); > - > +int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc); > > bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, > struct drm_dp_mst_port *port, int pbn, int slots); -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v9] drm/dp_mst: Add PBN calculation for DSC modes 2019-12-04 16:45 ` Jani Nikula @ 2019-12-04 20:38 ` Mikita Lipski -1 siblings, 0 replies; 6+ messages in thread From: Mikita Lipski @ 2019-12-04 20:38 UTC (permalink / raw) To: Jani Nikula, mikita.lipski, amd-gfx; +Cc: David Francis, dri-devel On 12/4/19 11:45 AM, Jani Nikula wrote: > On Tue, 03 Dec 2019, <mikita.lipski@amd.com> wrote: >> From: David Francis <David.Francis@amd.com> >> >> With DSC, bpp can be fractional in multiples of 1/16. > > Can be? > > I worry a bit that "bpp" can either be integer or fixed point depending > on other variables. I admit I haven't followed up on this too much, but > how widespread it is? It seems like something that is bound to be broken > in subtle ways, when it won't even cross people's minds that bpp is > fractional. > Hi Jani, Target rate is expected to be a multiple of bits per pixel of incremental divider (which is 16), so incoming bpp variable is an integer and not a fixed point. It is expected, as it is described in the DP1.4a spec. It is also true that if driver is passing non DSC bit rate with dsc flag set to true it will cause issue on MST as there likely won't be enough bandwidth allocated. But you have pointed to an error, I shouldn't divide (64 * 1006) by 16 since it comes out to 18645,45 and after it is floored to an integer it will cause pbn to be lower than needed. Instead the numerator should rather be like this: mul_u32_u32(clock * bpp / 16, 64 * 1006) Mikita > BR, > Jani. > > >> >> Change drm_dp_calc_pbn_mode to reflect this, adding a new >> parameter bool dsc. When this parameter is true, treat the >> bpp parameter as having units not of bits per pixel, but >> 1/16 of a bit per pixel >> >> v2: Don't add separate function for this >> v3: Keep the calculation in a single equation >> v4: Update the tests in test-drm_dp_mst_helper.c >> >> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> >> Reviewed-by: Lyude Paul <lyude@redhat.com> >> Reviewed-by: Harry Wentland <harry.wentland@amd.com> >> Signed-off-by: David Francis <David.Francis@amd.com> >> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> >> --- >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- >> drivers/gpu/drm/drm_dp_mst_topology.c | 12 +++++++++++- >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 ++- >> drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- >> drivers/gpu/drm/radeon/radeon_dp_mst.c | 2 +- >> drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c | 10 ++++++---- >> include/drm/drm_dp_mst_helper.h | 3 +-- >> 7 files changed, 23 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> index 455c51c38720 ..9fc03fc1017d 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -4967,7 +4967,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder, >> is_y420); >> bpp = convert_dc_color_depth_into_bpc(color_depth) * 3; >> clock = adjusted_mode->clock; >> - dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp); >> + dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp, false); >> } >> dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state, >> mst_mgr, >> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c >> index ae5809a1f19a..828b5eae529c 100644 >> --- a/drivers/gpu/drm/drm_dp_mst_topology.c >> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c >> @@ -4342,10 +4342,11 @@ EXPORT_SYMBOL(drm_dp_check_act_status); >> * drm_dp_calc_pbn_mode() - Calculate the PBN for a mode. >> * @clock: dot clock for the mode >> * @bpp: bpp for the mode. >> + * @dsc: DSC mode. If true, bpp has units of 1/16 of a bit per pixel >> * >> * This uses the formula in the spec to calculate the PBN value for a mode. >> */ >> -int drm_dp_calc_pbn_mode(int clock, int bpp) >> +int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc) >> { >> /* >> * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006 >> @@ -4356,7 +4357,16 @@ int drm_dp_calc_pbn_mode(int clock, int bpp) >> * peak_kbps *= (1006/1000) >> * peak_kbps *= (64/54) >> * peak_kbps *= 8 convert to bytes >> + * >> + * If the bpp is in units of 1/16, further divide by 16. Put this >> + * factor in the numerator rather than the denominator to avoid >> + * integer overflow >> */ >> + >> + if(dsc) >> + return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 / 16), >> + 8 * 54 * 1000 * 1000); >> + >> return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006), >> 8 * 54 * 1000 * 1000); >> } >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> index 03d1cba0b696..92be17711287 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> @@ -61,7 +61,8 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, >> crtc_state->pipe_bpp = bpp; >> >> crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, >> - crtc_state->pipe_bpp); >> + crtc_state->pipe_bpp, >> + false); >> >> slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr, >> port, crtc_state->pbn); >> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c >> index 549486f1d937..1c9e23d5a6fd 100644 >> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c >> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c >> @@ -782,7 +782,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, >> const int bpp = connector->display_info.bpc * 3; >> const int clock = crtc_state->adjusted_mode.clock; >> >> - asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp); >> + asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp, false); >> } >> >> slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, >> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c >> index ee28f5b3785e..28eef9282874 100644 >> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c >> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c >> @@ -518,7 +518,7 @@ static bool radeon_mst_mode_fixup(struct drm_encoder *encoder, >> >> mst_enc = radeon_encoder->enc_priv; >> >> - mst_enc->pbn = drm_dp_calc_pbn_mode(adjusted_mode->clock, bpp); >> + mst_enc->pbn = drm_dp_calc_pbn_mode(adjusted_mode->clock, bpp, false); >> >> mst_enc->primary->active_device = mst_enc->primary->devices & mst_enc->connector->devices; >> DRM_DEBUG_KMS("setting active device to %08x from %08x %08x for encoder %d\n", >> diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c >> index af2b2de65316..73fc1c485283 100644 >> --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c >> +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c >> @@ -18,15 +18,17 @@ int igt_dp_mst_calc_pbn_mode(void *ignored) >> int rate; >> int bpp; >> int expected; >> + bool dsc; >> } test_params[] = { >> - { 154000, 30, 689 }, >> - { 234000, 30, 1047 }, >> - { 297000, 24, 1063 }, >> + { 154000, 30, 689, false }, >> + { 234000, 30, 1047, false }, >> + { 297000, 24, 1063, false }, >> }; >> >> for (i = 0; i < ARRAY_SIZE(test_params); i++) { >> pbn = drm_dp_calc_pbn_mode(test_params[i].rate, >> - test_params[i].bpp); >> + test_params[i].bpp, >> + test_params[i].dsc); >> FAIL(pbn != test_params[i].expected, >> "Expected PBN %d for clock %d bpp %d, got %d\n", >> test_params[i].expected, test_params[i].rate, >> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h >> index d5fc90b30487..68656913cfe5 100644 >> --- a/include/drm/drm_dp_mst_helper.h >> +++ b/include/drm/drm_dp_mst_helper.h >> @@ -719,8 +719,7 @@ bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr, >> struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port); >> >> >> -int drm_dp_calc_pbn_mode(int clock, int bpp); >> - >> +int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc); >> >> bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, >> struct drm_dp_mst_port *port, int pbn, int slots); > -- Thanks, Mikita Lipski Software Engineer, AMD mikita.lipski@amd.com _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v9] drm/dp_mst: Add PBN calculation for DSC modes @ 2019-12-04 20:38 ` Mikita Lipski 0 siblings, 0 replies; 6+ messages in thread From: Mikita Lipski @ 2019-12-04 20:38 UTC (permalink / raw) To: Jani Nikula, mikita.lipski, amd-gfx; +Cc: David Francis, dri-devel On 12/4/19 11:45 AM, Jani Nikula wrote: > On Tue, 03 Dec 2019, <mikita.lipski@amd.com> wrote: >> From: David Francis <David.Francis@amd.com> >> >> With DSC, bpp can be fractional in multiples of 1/16. > > Can be? > > I worry a bit that "bpp" can either be integer or fixed point depending > on other variables. I admit I haven't followed up on this too much, but > how widespread it is? It seems like something that is bound to be broken > in subtle ways, when it won't even cross people's minds that bpp is > fractional. > Hi Jani, Target rate is expected to be a multiple of bits per pixel of incremental divider (which is 16), so incoming bpp variable is an integer and not a fixed point. It is expected, as it is described in the DP1.4a spec. It is also true that if driver is passing non DSC bit rate with dsc flag set to true it will cause issue on MST as there likely won't be enough bandwidth allocated. But you have pointed to an error, I shouldn't divide (64 * 1006) by 16 since it comes out to 18645,45 and after it is floored to an integer it will cause pbn to be lower than needed. Instead the numerator should rather be like this: mul_u32_u32(clock * bpp / 16, 64 * 1006) Mikita > BR, > Jani. > > >> >> Change drm_dp_calc_pbn_mode to reflect this, adding a new >> parameter bool dsc. When this parameter is true, treat the >> bpp parameter as having units not of bits per pixel, but >> 1/16 of a bit per pixel >> >> v2: Don't add separate function for this >> v3: Keep the calculation in a single equation >> v4: Update the tests in test-drm_dp_mst_helper.c >> >> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> >> Reviewed-by: Lyude Paul <lyude@redhat.com> >> Reviewed-by: Harry Wentland <harry.wentland@amd.com> >> Signed-off-by: David Francis <David.Francis@amd.com> >> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> >> --- >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- >> drivers/gpu/drm/drm_dp_mst_topology.c | 12 +++++++++++- >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 ++- >> drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- >> drivers/gpu/drm/radeon/radeon_dp_mst.c | 2 +- >> drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c | 10 ++++++---- >> include/drm/drm_dp_mst_helper.h | 3 +-- >> 7 files changed, 23 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> index 455c51c38720 ..9fc03fc1017d 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -4967,7 +4967,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder, >> is_y420); >> bpp = convert_dc_color_depth_into_bpc(color_depth) * 3; >> clock = adjusted_mode->clock; >> - dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp); >> + dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp, false); >> } >> dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state, >> mst_mgr, >> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c >> index ae5809a1f19a..828b5eae529c 100644 >> --- a/drivers/gpu/drm/drm_dp_mst_topology.c >> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c >> @@ -4342,10 +4342,11 @@ EXPORT_SYMBOL(drm_dp_check_act_status); >> * drm_dp_calc_pbn_mode() - Calculate the PBN for a mode. >> * @clock: dot clock for the mode >> * @bpp: bpp for the mode. >> + * @dsc: DSC mode. If true, bpp has units of 1/16 of a bit per pixel >> * >> * This uses the formula in the spec to calculate the PBN value for a mode. >> */ >> -int drm_dp_calc_pbn_mode(int clock, int bpp) >> +int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc) >> { >> /* >> * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006 >> @@ -4356,7 +4357,16 @@ int drm_dp_calc_pbn_mode(int clock, int bpp) >> * peak_kbps *= (1006/1000) >> * peak_kbps *= (64/54) >> * peak_kbps *= 8 convert to bytes >> + * >> + * If the bpp is in units of 1/16, further divide by 16. Put this >> + * factor in the numerator rather than the denominator to avoid >> + * integer overflow >> */ >> + >> + if(dsc) >> + return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 / 16), >> + 8 * 54 * 1000 * 1000); >> + >> return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006), >> 8 * 54 * 1000 * 1000); >> } >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> index 03d1cba0b696..92be17711287 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> @@ -61,7 +61,8 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, >> crtc_state->pipe_bpp = bpp; >> >> crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, >> - crtc_state->pipe_bpp); >> + crtc_state->pipe_bpp, >> + false); >> >> slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr, >> port, crtc_state->pbn); >> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c >> index 549486f1d937..1c9e23d5a6fd 100644 >> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c >> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c >> @@ -782,7 +782,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, >> const int bpp = connector->display_info.bpc * 3; >> const int clock = crtc_state->adjusted_mode.clock; >> >> - asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp); >> + asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp, false); >> } >> >> slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, >> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c >> index ee28f5b3785e..28eef9282874 100644 >> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c >> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c >> @@ -518,7 +518,7 @@ static bool radeon_mst_mode_fixup(struct drm_encoder *encoder, >> >> mst_enc = radeon_encoder->enc_priv; >> >> - mst_enc->pbn = drm_dp_calc_pbn_mode(adjusted_mode->clock, bpp); >> + mst_enc->pbn = drm_dp_calc_pbn_mode(adjusted_mode->clock, bpp, false); >> >> mst_enc->primary->active_device = mst_enc->primary->devices & mst_enc->connector->devices; >> DRM_DEBUG_KMS("setting active device to %08x from %08x %08x for encoder %d\n", >> diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c >> index af2b2de65316..73fc1c485283 100644 >> --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c >> +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c >> @@ -18,15 +18,17 @@ int igt_dp_mst_calc_pbn_mode(void *ignored) >> int rate; >> int bpp; >> int expected; >> + bool dsc; >> } test_params[] = { >> - { 154000, 30, 689 }, >> - { 234000, 30, 1047 }, >> - { 297000, 24, 1063 }, >> + { 154000, 30, 689, false }, >> + { 234000, 30, 1047, false }, >> + { 297000, 24, 1063, false }, >> }; >> >> for (i = 0; i < ARRAY_SIZE(test_params); i++) { >> pbn = drm_dp_calc_pbn_mode(test_params[i].rate, >> - test_params[i].bpp); >> + test_params[i].bpp, >> + test_params[i].dsc); >> FAIL(pbn != test_params[i].expected, >> "Expected PBN %d for clock %d bpp %d, got %d\n", >> test_params[i].expected, test_params[i].rate, >> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h >> index d5fc90b30487..68656913cfe5 100644 >> --- a/include/drm/drm_dp_mst_helper.h >> +++ b/include/drm/drm_dp_mst_helper.h >> @@ -719,8 +719,7 @@ bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr, >> struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port); >> >> >> -int drm_dp_calc_pbn_mode(int clock, int bpp); >> - >> +int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc); >> >> bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, >> struct drm_dp_mst_port *port, int pbn, int slots); > -- Thanks, Mikita Lipski Software Engineer, AMD mikita.lipski@amd.com _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-04 20:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-12-03 16:28 [PATCH v9] drm/dp_mst: Add PBN calculation for DSC modes mikita.lipski 2019-12-03 16:28 ` mikita.lipski 2019-12-04 16:45 ` Jani Nikula 2019-12-04 16:45 ` Jani Nikula 2019-12-04 20:38 ` Mikita Lipski 2019-12-04 20:38 ` Mikita Lipski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.