Re: [PATCH 01/21] drm/i915/dp: Fix dsc bpp calculations.

[Manasi Navare <manasi.d.navare@intel.com>]

On Mon, Aug 19, 2019 at 12:51:53PM +0200, Maarten Lankhorst wrote:
> There was a integer wraparound when mode_clock became too high,

mode_clock is obtained from crtc_clock from the mode which is an int,
are you saying we also need to change that as well in drm_display_mode struct to handle
higher mode clocks?

> and we didn't correct for the FEC overhead factor when dividing,
> also the calculations would break at HBR3.
> 
> As a result our calculated bpp was way too high, and the link width
> bpp limitation never came into effect.
> 
> Print out the resulting bpp calcululations as a sanity check, just
> in case we ever have to debug it later on again.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: d9218c8f6cf4 ("drm/i915/dp: Add helpers for Compressed BPP and Slice Count for DSC")
> Cc: <stable@vger.kernel.org> # v5.0+
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 16 +++++++++-------
>  drivers/gpu/drm/i915/display/intel_dp.h |  4 ++--
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 5c45a3bb102d..2e9cbc15e41f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4323,10 +4323,10 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  		DP_DPRX_ESI_LEN;
>  }
>  
> -u16 intel_dp_dsc_get_output_bpp(int link_clock, u8 lane_count,
> -				int mode_clock, int mode_hdisplay)
> +u16 intel_dp_dsc_get_output_bpp(u32 link_clock, u8 lane_count,
> +				u32 mode_clock, u32 mode_hdisplay)
>  {
> -	u16 bits_per_pixel, max_bpp_small_joiner_ram;
> +	u32 bits_per_pixel, max_bpp_small_joiner_ram;

Why do you need the bits_per_pixel and max bpp as a u32? u16 should be good to hold
the max value right?

>  	int i;
>  
>  	/*
> @@ -4335,13 +4335,14 @@ u16 intel_dp_dsc_get_output_bpp(int link_clock, u8 lane_count,
>  	 * FECOverhead = 2.4%, for SST -> TimeSlotsPerMTP is 1,
>  	 * for MST -> TimeSlotsPerMTP has to be calculated
>  	 */
> -	bits_per_pixel = (link_clock * lane_count * 8 *
> -			  DP_DSC_FEC_OVERHEAD_FACTOR) /
> -		mode_clock;
> +	bits_per_pixel = div_u64((u64)link_clock * lane_count * 8 *
> +				 DP_DSC_FEC_OVERHEAD_FACTOR, 1000ULL * mode_clock);

Thanks for this catch, I remember having the division by 1000 in the original patch series: https://patchwork.freedesktop.org/patch/241674/?series=47461&rev=2
but may be it got lost in the reviews.

Manasi

> +	DRM_DEBUG_KMS("Max link bpp: %u\n", bits_per_pixel);
>  
>  	/* Small Joiner Check: output bpp <= joiner RAM (bits) / Horiz. width */
>  	max_bpp_small_joiner_ram = DP_DSC_MAX_SMALL_JOINER_RAM_BUFFER /
>  		mode_hdisplay;
> +	DRM_DEBUG_KMS("Max small joiner bpp: %u\n", max_bpp_small_joiner_ram);
>  
>  	/*
>  	 * Greatest allowed DSC BPP = MIN (output BPP from avaialble Link BW
> @@ -4351,7 +4352,8 @@ u16 intel_dp_dsc_get_output_bpp(int link_clock, u8 lane_count,
>  
>  	/* Error out if the max bpp is less than smallest allowed valid bpp */
>  	if (bits_per_pixel < valid_dsc_bpp[0]) {
> -		DRM_DEBUG_KMS("Unsupported BPP %d\n", bits_per_pixel);
> +		DRM_DEBUG_KMS("Unsupported BPP %u, min %u\n",
> +			      bits_per_pixel, valid_dsc_bpp[0]);
>  		return 0;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index 657bbb1f5ed0..007d1981a33b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -102,8 +102,8 @@ bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp);
>  bool intel_dp_source_supports_hbr3(struct intel_dp *intel_dp);
>  bool
>  intel_dp_get_link_status(struct intel_dp *intel_dp, u8 *link_status);
> -u16 intel_dp_dsc_get_output_bpp(int link_clock, u8 lane_count,
> -				int mode_clock, int mode_hdisplay);
> +u16 intel_dp_dsc_get_output_bpp(u32 link_clock, u8 lane_count,
> +				u32 mode_clock, u32 mode_hdisplay);
>  u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp, int mode_clock,
>  				int mode_hdisplay);
>  
> -- 
> 2.20.1
>