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]);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