Re: [PATCH 02/13] drm/dp_mst: Add PBN calculation for DSC modes

From: Lyude Paul <lyude@redhat.com>
To: mikita.lipski@amd.com, amd-gfx@lists.freedesktop.org
Cc: David Francis <David.Francis@amd.com>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 02/13] drm/dp_mst: Add PBN calculation for DSC modes
Date: Mon, 04 Nov 2019 17:37:33 -0500	[thread overview]
Message-ID: <a9d40a738bd57723a2aebb2f7e23a95bd191d4b7.camel@redhat.com> (raw)
In-Reply-To: <20191030192431.5798-3-mikita.lipski@amd.com>

hey-sorry this took a little bit to get back to. I reviewed this before, but 
this patch doesn't seem to be rebased against drm-tip otherwise it would
reflect the changes that happened with drm_dp_calc_pbn_mode() upstream:

https://patchwork.freedesktop.org/patch/332935/

can you rebase these patches against drm-tip and also update the
igt_dp_mst_calc_pbn_mode() test here?

https://cgit.freedesktop.org/drm-tip/tree/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c#n14

On Wed, 2019-10-30 at 15:24 -0400, mikita.lipski@amd.com wrote:
> 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
> 
> 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>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c    |  2 +-
>  drivers/gpu/drm/drm_dp_mst_topology.c            | 16 ++++++++++++----
>  drivers/gpu/drm/i915/display/intel_dp_mst.c      |  3 ++-
>  drivers/gpu/drm/nouveau/dispnv50/disp.c          |  3 ++-
>  drivers/gpu/drm/radeon/radeon_dp_mst.c           |  2 +-
>  include/drm/drm_dp_mst_helper.h                  |  3 +--
>  6 files changed, 19 insertions(+), 10 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 d75726013436..1309e9cfdea3 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4656,7 +4656,7 @@ static int dm_encoder_helper_atomic_check(struct
> drm_encoder *encoder,
>  		color_depth = convert_color_depth_from_display_info(connector,
> conn_state);
>  		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 82add736e17d..3e7b7553cf4d 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3534,10 +3534,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)
>  {
>  	u64 kbps;
>  	s64 peak_kbps;
> @@ -3555,11 +3556,18 @@ 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
>  	 */
>  
>  	numerator = 64 * 1006;
>  	denominator = 54 * 8 * 1000 * 1000;
>  
> +	if (dsc)
> +		numerator /= 16;
> +
>  	kbps *= numerator;
>  	peak_kbps = drm_fixp_from_fraction(kbps, denominator);
>  
> @@ -3570,19 +3578,19 @@ EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>  static int test_calc_pbn_mode(void)
>  {
>  	int ret;
> -	ret = drm_dp_calc_pbn_mode(154000, 30);
> +	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);
> +	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);
> +	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);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 2c5ac3dd647f..dfac450841df 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 f1dbc7852414..c45832230ccc 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -778,7 +778,8 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
>  	if (!state->duplicated)
>  		asyh->dp.pbn =
>  			drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock,
> -					     connector->display_info.bpc * 3);
> +					     connector->display_info.bpc * 3,
> +					     false);
>  
>  	if (crtc_state->mode_changed) {
>  		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 2994f07fbad9..c997f88218f2 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -514,7 +514,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 2ba6253ea6d3..9116b2c95239 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -610,8 +610,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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel