Hi Lyude,
Thanks for the review.
I’ve updated this patch in a v9 iteration of it, I’ve sent separately from the series this past Monday. The new version has the updated selftest.
Thanks,
Mikita
This patch still needs to be rebased, and the selftests for the PBN
calculation that got added still need to be updated to reflect the changes for
dsc. The code for PBN calculation changed quite a bit upstream since this
series started.
On Tue, 2019-12-03 at 09:35 -0500, mikita.lipski@xxxxxxx wrote:
> From: David Francis <David.Francis@xxxxxxx>
>
> 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
>
> Cc: Lyude Paul <lyude@xxxxxxxxxx>
> Reviewed-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
> Reviewed-by: Harry Wentland <harry.wentland@xxxxxxx>
> Signed-off-by: David Francis <David.Francis@xxxxxxx>
> Signed-off-by: Mikita Lipski <mikita.lipski@xxxxxxx>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
> drivers/gpu/drm/drm_dp_mst_topology.c | 38 ++++++++++++++++++-
> 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 +-
> include/drm/drm_dp_mst_helper.h | 3 +-
> 6 files changed, 43 insertions(+), 7 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..261e2c1828c6 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,12 +4357,47 @@ 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);
> +
> }
> EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>
> +static int test_calc_pbn_mode(void)
> +{
> + int ret;
> + ret = drm_dp_calc_pbn_mode(154000, 30, false);
> + if (ret != 689) {
> + DRM_ERROR("PBN calculation test failed - clock %d, bpp %d,
> expected PBN %d, actual PBN %d.\n",
> + 154000, 30, 689, ret);
> + return -EINVAL;
> + }
> + ret = drm_dp_calc_pbn_mode(234000, 30, false);
> + if (ret != 1047) {
> + DRM_ERROR("PBN calculation test failed - clock %d, bpp %d,
> expected PBN %d, actual PBN %d.\n",
> + 234000, 30, 1047, ret);
> + return -EINVAL;
> + }
> + ret = drm_dp_calc_pbn_mode(297000, 24, false);
> + if (ret != 1063) {
> + DRM_ERROR("PBN calculation test failed - clock %d, bpp %d,
> expected PBN %d, actual PBN %d.\n",
> + 297000, 24, 1063, ret);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +
> /* we want to kick the TX after we've ack the up/down IRQs. */
> static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr)
> {
> 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/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);
--
Cheers,
Lyude Paul
