Re: [Intel-gfx] [PATCH 01/10] drm/i915/dp: Fix dsc bpp calculations.

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

On Mon, Aug 26, 2019 at 01:33:05PM +0200, Maarten Lankhorst wrote:
> Op 23-08-2019 om 12:17 schreef Jani Nikula:
> > On Wed, 21 Aug 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> >> On Wed, Aug 21, 2019 at 03:32:12PM +0200, Maarten Lankhorst wrote:
> >>> There was a integer wraparound when mode_clock became too high,
> >>> and we didn't correct for the FEC overhead factor when dividing,
> >>> also the calculations would break at HBR3.
> >> But the mode_clock is obtained from the adusted_mode->crtc_clock which is
> >> defined as an int in drm_mode_config struct, does that need to change also?
> >>
> >>> 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 921ad0a2f7ba..614a25911f07 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;
> >> But bits_per_pixel is a 16 bit value for DSC PPS, why does this need to be u32?
> > I would use int for *all* of the parameters and local vars. Don't try to
> > save space or limit the range by unsigned or fixed size. Only use the
> > fixed size types for when the size really matters, i.e. serialization
> > and lots of data. IMO almost everything else is trouble in C.
> 
> Hmm, looking at the source drm_mode_modeinfo it's intended to be unsigned.
> 
> We will definitely need a 64-bits because 1000 * 1 Ghz clock overflows u32.

Okay so for the denominator when you do 1000 * mode_Clock, thats when the mode_clock will overflow
so you need it a u64?

But the bits_per_pixel i think can be int like Jani said correct? Since that is only a 16 bit value after the div
but we can make it generic int.

Manasi
> 
> I will see if I can make this to work with mul_u32_u32 for the nominator and denominator.
> 
> 
> > BR,
> > Jani.
> >
> >
> >>>  	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 catching this, this had the 1000 in the denominator in my original patches :https://patchwork.freedesktop.org/series/47514/#rev3
> >> And  then the nex rev lost it somehow, so thanks for catching this 
> >>
> >> 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
> >>>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
>