Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx> Thanks! On Tue, 2023-05-02 at 17:38 +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > The current code does '(bpp << 4) / 16' in the MST PBN > calculation, but that is just the same as 'bpp' so the > DSC codepath achieves absolutely nothing. Fix it up so that > the fractional part of the bpp value is actually used instead > of truncated away. 64*1006 has enough zero lsbs that we can > just shift that down in the dividend and thus still manage > to stick to a 32bit divisor. > > And while touching this, let's just make the whole thing more > straightforward by making the passed in bpp value .4 binary > fixed point always, instead of having to pass in different > things based on whether DSC is enabled or not. > > Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx> > Cc: Lyude Paul <lyude@xxxxxxxxxx> > Cc: Harry Wentland <harry.wentland@xxxxxxx> > Cc: David Francis <David.Francis@xxxxxxx> > Cc: Mikita Lipski <mikita.lipski@xxxxxxx> > Cc: Alex Deucher <alexander.deucher@xxxxxxx> > Fixes: dc48529fb14e ("drm/dp_mst: Add PBN calculation for DSC modes") > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +- > drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 +++++-------------- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 5 ++--- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 3 +-- > .../gpu/drm/tests/drm_dp_mst_helper_test.c | 2 +- > include/drm/display/drm_dp_mst_helper.h | 2 +- > 7 files changed, 12 insertions(+), 24 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 6cacb76f389e..7d58f08a5444 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -6763,7 +6763,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder, > max_bpc); > 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, false); > + dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp << 4); > } > > dm_new_connector_state->vcpi_slots = > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > index 994ba426ca66..eb4b666e50e8 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > @@ -1515,7 +1515,7 @@ enum dc_status dm_dp_mst_is_port_support_mode( > } else { > /* check if mode could be supported within full_pbn */ > bpp = convert_dc_color_depth_into_bpc(stream->timing.display_color_depth) * 3; > - pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp, false); > + pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp << 4); > > if (pbn > aconnector->mst_output_port->full_pbn) > return DC_FAIL_BANDWIDTH_VALIDATE; > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c > index 38dab76ae69e..cd4c4f22c903 100644 > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c > @@ -4619,13 +4619,12 @@ 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 > + * @clock: dot clock > + * @bpp: bpp as .4 binary fixed point > * > * 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, bool dsc) > +int drm_dp_calc_pbn_mode(int clock, int bpp) > { > /* > * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006 > @@ -4636,18 +4635,9 @@ int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc) > * 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 / 16), 64 * 1006), > - 8 * 54 * 1000 * 1000); > - > - return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006), > - 8 * 54 * 1000 * 1000); > + return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 >> 4), > + 1000 * 8 * 54 * 1000); > } > EXPORT_SYMBOL(drm_dp_calc_pbn_mode); > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 2c49d9ab86a2..44c15d6faac4 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -109,8 +109,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder, > continue; > > crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, > - dsc ? bpp << 4 : bpp, > - dsc); > + bpp << 4); > > slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr, > connector->port, > @@ -936,7 +935,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector, > return ret; > > if (mode_rate > max_rate || mode->clock > max_dotclk || > - drm_dp_calc_pbn_mode(mode->clock, min_bpp, false) > port->full_pbn) { > + drm_dp_calc_pbn_mode(mode->clock, min_bpp << 4) > port->full_pbn) { > *status = MODE_CLOCK_HIGH; > return 0; > } > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index 5bb777ff1313..d896cbb8cf3d 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > @@ -961,8 +961,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, > const int clock = crtc_state->adjusted_mode.clock; > > asyh->or.bpc = connector->display_info.bpc; > - asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3, > - false); > + asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3 << 4); > } > > mst_state = drm_atomic_get_mst_topology_state(state, &mstm->mgr); > diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c > index 545beea33e8c..39fc449148e1 100644 > --- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c > +++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c > @@ -56,7 +56,7 @@ static void drm_test_dp_mst_calc_pbn_mode(struct kunit *test) > { > const struct drm_dp_mst_calc_pbn_mode_test *params = test->param_value; > > - KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp, params->dsc), > + KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp << 4), > params->expected); > } > > diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h > index 32c764fb9cb5..c254500b4507 100644 > --- a/include/drm/display/drm_dp_mst_helper.h > +++ b/include/drm/display/drm_dp_mst_helper.h > @@ -829,7 +829,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_ > int drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr, > int link_rate, int link_lane_count); > > -int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc); > +int drm_dp_calc_pbn_mode(int clock, int bpp); > > void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_t link_encoding_cap); > -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat