[Public]
> -----Original Message-----
> From: Wentland, Harry <Harry.Wentland@xxxxxxx>
> Sent: Wednesday, November 8, 2023 9:40 AM
> To: Imre Deak <imre.deak@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; Manasi Navare
> <manasi.d.navare@xxxxxxxxx>; Lyude Paul <lyude@xxxxxxxxxx>; Francis,
> David <David.Francis@xxxxxxx>; Mikita Lipski <mikita.lipski@xxxxxxx>;
> Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> Subject: Re: [PATCH v4 02/30] drm/dp_mst: Fix fractional DSC bpp handling
>
>
>
> On 2023-10-30 11:58, Imre Deak 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.
> >
> > v2:
> > - Fix DSC kunit test cases.
> >
> > 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")
> > Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx> (v1)
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > [Imre: Fix kunit test cases]
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
>
> Acked-by: Harry Wentland <harry.wentland@xxxxxxx>
Acked-by: Alex Deucher <alexander.deucher@xxxxxxx>
>
> Harry
>
> > ---
> > .../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 | 6 +++---
> > include/drm/display/drm_dp_mst_helper.h | 2 +-
> > 7 files changed, 14 insertions(+), 26 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 9a712791f309f..ada3773869ff0 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -6918,7 +6918,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 d3b13d362edac..9a58e1a4c5f49 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
> > @@ -1642,7 +1642,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 0e0d0e76de065..772b00ebd57bd 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4718,13 +4718,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 @@
> > -4735,18 +4734,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 851b312bd8449..5bf45a2a85b0e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -106,8 +106,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,
> > @@ -975,7 +974,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 d2be40337b92e..153717e1df1a2 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -982,8 +982,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 545beea33e8c7..e3c818dfc0e6d 100644
> > --- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > +++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > @@ -42,13 +42,13 @@ static const struct
> drm_dp_mst_calc_pbn_mode_test drm_dp_mst_calc_pbn_mode_cases
> > .clock = 332880,
> > .bpp = 24,
> > .dsc = true,
> > - .expected = 50
> > + .expected = 1191
> > },
> > {
> > .clock = 324540,
> > .bpp = 24,
> > .dsc = true,
> > - .expected = 49
> > + .expected = 1161
> > },
> > };
> >
> > @@ -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 4429d3b1745b6..655862b3d2a49 100644
> > --- a/include/drm/display/drm_dp_mst_helper.h
> > +++ b/include/drm/display/drm_dp_mst_helper.h
> > @@ -842,7 +842,7 @@ struct edid *drm_dp_mst_get_edid(struct
> > drm_connector *connector, 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);
> >
