diff mbox series

[v4,02/30] drm/dp_mst: Fix fractional DSC bpp handling

Message ID 20231030155843.2251023-3-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Improve BW management on MST links | expand

Commit Message

Imre Deak Oct. 30, 2023, 3:58 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The current code does '(bpp << 4) / 16' in the MST PBN
calculation, but that is just the same as 'bpp' so the
DSC codepath achieves absolutely nothing. Fix it up so that
the fractional part of the bpp value is actually used instead
of truncated away. 64*1006 has enough zero lsbs that we can
just shift that down in the dividend and thus still manage
to stick to a 32bit divisor.

And while touching this, let's just make the whole thing more
straightforward by making the passed in bpp value .4 binary
fixed point always, instead of having to pass in different
things based on whether DSC is enabled or not.

v2:
- Fix DSC kunit test cases.

Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: David Francis <David.Francis@amd.com>
Cc: Mikita Lipski <mikita.lipski@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Fixes: dc48529fb14e ("drm/dp_mst: Add PBN calculation for DSC modes")
Reviewed-by: Lyude Paul <lyude@redhat.com> (v1)
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
[Imre: Fix kunit test cases]
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  2 +-
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 +++++--------------
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  5 ++---
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  3 +--
 .../gpu/drm/tests/drm_dp_mst_helper_test.c    |  6 +++---
 include/drm/display/drm_dp_mst_helper.h       |  2 +-
 7 files changed, 14 insertions(+), 26 deletions(-)

Comments

Imre Deak Oct. 31, 2023, 7:52 p.m. UTC | #1
On Mon, Oct 30, 2023 at 05:58:15PM +0200, Imre Deak wrote:
Hi Lyude, AMD folks et al,

could you ack patches 2-9 in this patchset if they are ok and it's ok to
merge them via the i915 tree?

Thanks,
Imre

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The current code does '(bpp << 4) / 16' in the MST PBN
> calculation, but that is just the same as 'bpp' so the
> DSC codepath achieves absolutely nothing. Fix it up so that
> the fractional part of the bpp value is actually used instead
> of truncated away. 64*1006 has enough zero lsbs that we can
> just shift that down in the dividend and thus still manage
> to stick to a 32bit divisor.
> 
> And while touching this, let's just make the whole thing more
> straightforward by making the passed in bpp value .4 binary
> fixed point always, instead of having to pass in different
> things based on whether DSC is enabled or not.
> 
> v2:
> - Fix DSC kunit test cases.
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: David Francis <David.Francis@amd.com>
> Cc: Mikita Lipski <mikita.lipski@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Fixes: dc48529fb14e ("drm/dp_mst: Add PBN calculation for DSC modes")
> Reviewed-by: Lyude Paul <lyude@redhat.com> (v1)
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> [Imre: Fix kunit test cases]
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +-
>  .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  2 +-
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 +++++--------------
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  5 ++---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  3 +--
>  .../gpu/drm/tests/drm_dp_mst_helper_test.c    |  6 +++---
>  include/drm/display/drm_dp_mst_helper.h       |  2 +-
>  7 files changed, 14 insertions(+), 26 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 9a712791f309f..ada3773869ff0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6918,7 +6918,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
>  								    max_bpc);
>  		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, false);
> +		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp << 4);
>  	}
>  
>  	dm_new_connector_state->vcpi_slots =
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index d3b13d362edac..9a58e1a4c5f49 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -1642,7 +1642,7 @@ enum dc_status dm_dp_mst_is_port_support_mode(
>  	} else {
>  		/* check if mode could be supported within full_pbn */
>  		bpp = convert_dc_color_depth_into_bpc(stream->timing.display_color_depth) * 3;
> -		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp, false);
> +		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp << 4);
>  
>  		if (pbn > aconnector->mst_output_port->full_pbn)
>  			return DC_FAIL_BANDWIDTH_VALIDATE;
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 0e0d0e76de065..772b00ebd57bd 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4718,13 +4718,12 @@ 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
> + * @clock: dot clock
> + * @bpp: bpp as .4 binary fixed point
>   *
>   * 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, bool dsc)
> +int drm_dp_calc_pbn_mode(int clock, int bpp)
>  {
>  	/*
>  	 * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
> @@ -4735,18 +4734,9 @@ int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc)
>  	 * 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 / 16), 64 * 1006),
> -					8 * 54 * 1000 * 1000);
> -
> -	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
> -				8 * 54 * 1000 * 1000);
> +	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 >> 4),
> +				1000 * 8 * 54 * 1000);
>  }
>  EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 851b312bd8449..5bf45a2a85b0e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -106,8 +106,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  			continue;
>  
>  		crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,
> -						       dsc ? bpp << 4 : bpp,
> -						       dsc);
> +						       bpp << 4);
>  
>  		slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
>  						      connector->port,
> @@ -975,7 +974,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  		return ret;
>  
>  	if (mode_rate > max_rate || mode->clock > max_dotclk ||
> -	    drm_dp_calc_pbn_mode(mode->clock, min_bpp, false) > port->full_pbn) {
> +	    drm_dp_calc_pbn_mode(mode->clock, min_bpp << 4) > port->full_pbn) {
>  		*status = MODE_CLOCK_HIGH;
>  		return 0;
>  	}
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index d2be40337b92e..153717e1df1a2 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -982,8 +982,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
>  		const int clock = crtc_state->adjusted_mode.clock;
>  
>  		asyh->or.bpc = connector->display_info.bpc;
> -		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3,
> -						    false);
> +		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3 << 4);
>  	}
>  
>  	mst_state = drm_atomic_get_mst_topology_state(state, &mstm->mgr);
> diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> index 545beea33e8c7..e3c818dfc0e6d 100644
> --- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> @@ -42,13 +42,13 @@ static const struct drm_dp_mst_calc_pbn_mode_test drm_dp_mst_calc_pbn_mode_cases
>  		.clock = 332880,
>  		.bpp = 24,
>  		.dsc = true,
> -		.expected = 50
> +		.expected = 1191
>  	},
>  	{
>  		.clock = 324540,
>  		.bpp = 24,
>  		.dsc = true,
> -		.expected = 49
> +		.expected = 1161
>  	},
>  };
>  
> @@ -56,7 +56,7 @@ static void drm_test_dp_mst_calc_pbn_mode(struct kunit *test)
>  {
>  	const struct drm_dp_mst_calc_pbn_mode_test *params = test->param_value;
>  
> -	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp, params->dsc),
> +	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp << 4),
>  			params->expected);
>  }
>  
> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> index 4429d3b1745b6..655862b3d2a49 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -842,7 +842,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector,
>  int drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
>  			     int link_rate, int link_lane_count);
>  
> -int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc);
> +int drm_dp_calc_pbn_mode(int clock, int bpp);
>  
>  void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_t link_encoding_cap);
>  
> -- 
> 2.39.2
>
Jani Nikula Nov. 1, 2023, 12:59 p.m. UTC | #2
On Tue, 31 Oct 2023, Imre Deak <imre.deak@intel.com> wrote:
> On Mon, Oct 30, 2023 at 05:58:15PM +0200, Imre Deak wrote:
> Hi Lyude, AMD folks et al,
>
> could you ack patches 2-9 in this patchset if they are ok and it's ok to
> merge them via the i915 tree?

Need acks from drm-misc maintainers too!

Cc: Maxime, Thomas, Maarten


>
> Thanks,
> Imre
>
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> 
>> The current code does '(bpp << 4) / 16' in the MST PBN
>> calculation, but that is just the same as 'bpp' so the
>> DSC codepath achieves absolutely nothing. Fix it up so that
>> the fractional part of the bpp value is actually used instead
>> of truncated away. 64*1006 has enough zero lsbs that we can
>> just shift that down in the dividend and thus still manage
>> to stick to a 32bit divisor.
>> 
>> And while touching this, let's just make the whole thing more
>> straightforward by making the passed in bpp value .4 binary
>> fixed point always, instead of having to pass in different
>> things based on whether DSC is enabled or not.
>> 
>> v2:
>> - Fix DSC kunit test cases.
>> 
>> Cc: Manasi Navare <manasi.d.navare@intel.com>
>> Cc: Lyude Paul <lyude@redhat.com>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Cc: David Francis <David.Francis@amd.com>
>> Cc: Mikita Lipski <mikita.lipski@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Fixes: dc48529fb14e ("drm/dp_mst: Add PBN calculation for DSC modes")
>> Reviewed-by: Lyude Paul <lyude@redhat.com> (v1)
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> [Imre: Fix kunit test cases]
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>> ---
>>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +-
>>  .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  2 +-
>>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 +++++--------------
>>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  5 ++---
>>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  3 +--
>>  .../gpu/drm/tests/drm_dp_mst_helper_test.c    |  6 +++---
>>  include/drm/display/drm_dp_mst_helper.h       |  2 +-
>>  7 files changed, 14 insertions(+), 26 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 9a712791f309f..ada3773869ff0 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -6918,7 +6918,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
>>  								    max_bpc);
>>  		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, false);
>> +		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp << 4);
>>  	}
>>  
>>  	dm_new_connector_state->vcpi_slots =
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> index d3b13d362edac..9a58e1a4c5f49 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> @@ -1642,7 +1642,7 @@ enum dc_status dm_dp_mst_is_port_support_mode(
>>  	} else {
>>  		/* check if mode could be supported within full_pbn */
>>  		bpp = convert_dc_color_depth_into_bpc(stream->timing.display_color_depth) * 3;
>> -		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp, false);
>> +		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp << 4);
>>  
>>  		if (pbn > aconnector->mst_output_port->full_pbn)
>>  			return DC_FAIL_BANDWIDTH_VALIDATE;
>> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> index 0e0d0e76de065..772b00ebd57bd 100644
>> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> @@ -4718,13 +4718,12 @@ 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
>> + * @clock: dot clock
>> + * @bpp: bpp as .4 binary fixed point
>>   *
>>   * 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, bool dsc)
>> +int drm_dp_calc_pbn_mode(int clock, int bpp)
>>  {
>>  	/*
>>  	 * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
>> @@ -4735,18 +4734,9 @@ int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc)
>>  	 * 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 / 16), 64 * 1006),
>> -					8 * 54 * 1000 * 1000);
>> -
>> -	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
>> -				8 * 54 * 1000 * 1000);
>> +	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 >> 4),
>> +				1000 * 8 * 54 * 1000);
>>  }
>>  EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>>  
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> index 851b312bd8449..5bf45a2a85b0e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> @@ -106,8 +106,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>>  			continue;
>>  
>>  		crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,
>> -						       dsc ? bpp << 4 : bpp,
>> -						       dsc);
>> +						       bpp << 4);
>>  
>>  		slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
>>  						      connector->port,
>> @@ -975,7 +974,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>>  		return ret;
>>  
>>  	if (mode_rate > max_rate || mode->clock > max_dotclk ||
>> -	    drm_dp_calc_pbn_mode(mode->clock, min_bpp, false) > port->full_pbn) {
>> +	    drm_dp_calc_pbn_mode(mode->clock, min_bpp << 4) > port->full_pbn) {
>>  		*status = MODE_CLOCK_HIGH;
>>  		return 0;
>>  	}
>> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> index d2be40337b92e..153717e1df1a2 100644
>> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> @@ -982,8 +982,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
>>  		const int clock = crtc_state->adjusted_mode.clock;
>>  
>>  		asyh->or.bpc = connector->display_info.bpc;
>> -		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3,
>> -						    false);
>> +		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3 << 4);
>>  	}
>>  
>>  	mst_state = drm_atomic_get_mst_topology_state(state, &mstm->mgr);
>> diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
>> index 545beea33e8c7..e3c818dfc0e6d 100644
>> --- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
>> +++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
>> @@ -42,13 +42,13 @@ static const struct drm_dp_mst_calc_pbn_mode_test drm_dp_mst_calc_pbn_mode_cases
>>  		.clock = 332880,
>>  		.bpp = 24,
>>  		.dsc = true,
>> -		.expected = 50
>> +		.expected = 1191
>>  	},
>>  	{
>>  		.clock = 324540,
>>  		.bpp = 24,
>>  		.dsc = true,
>> -		.expected = 49
>> +		.expected = 1161
>>  	},
>>  };
>>  
>> @@ -56,7 +56,7 @@ static void drm_test_dp_mst_calc_pbn_mode(struct kunit *test)
>>  {
>>  	const struct drm_dp_mst_calc_pbn_mode_test *params = test->param_value;
>>  
>> -	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp, params->dsc),
>> +	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp << 4),
>>  			params->expected);
>>  }
>>  
>> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
>> index 4429d3b1745b6..655862b3d2a49 100644
>> --- a/include/drm/display/drm_dp_mst_helper.h
>> +++ b/include/drm/display/drm_dp_mst_helper.h
>> @@ -842,7 +842,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector,
>>  int drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
>>  			     int link_rate, int link_lane_count);
>>  
>> -int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc);
>> +int drm_dp_calc_pbn_mode(int clock, int bpp);
>>  
>>  void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_t link_encoding_cap);
>>  
>> -- 
>> 2.39.2
>>
Maxime Ripard Nov. 6, 2023, 8:16 a.m. UTC | #3
On Wed, Nov 01, 2023 at 02:59:50PM +0200, Jani Nikula wrote:
> On Tue, 31 Oct 2023, Imre Deak <imre.deak@intel.com> wrote:
> > On Mon, Oct 30, 2023 at 05:58:15PM +0200, Imre Deak wrote:
> > Hi Lyude, AMD folks et al,
> >
> > could you ack patches 2-9 in this patchset if they are ok and it's ok to
> > merge them via the i915 tree?
> 
> Need acks from drm-misc maintainers too!
> 
> Cc: Maxime, Thomas, Maarten

Acked-by: Maxime Ripard <mripard@kernel.org>

Maxime
Lyude Paul Nov. 7, 2023, 10:45 p.m. UTC | #4
Reviewed-by: Lyude Paul <lyude@redhat.com>

On Mon, 2023-10-30 at 17:58 +0200, Imre Deak wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The current code does '(bpp << 4) / 16' in the MST PBN
> calculation, but that is just the same as 'bpp' so the
> DSC codepath achieves absolutely nothing. Fix it up so that
> the fractional part of the bpp value is actually used instead
> of truncated away. 64*1006 has enough zero lsbs that we can
> just shift that down in the dividend and thus still manage
> to stick to a 32bit divisor.
> 
> And while touching this, let's just make the whole thing more
> straightforward by making the passed in bpp value .4 binary
> fixed point always, instead of having to pass in different
> things based on whether DSC is enabled or not.
> 
> v2:
> - Fix DSC kunit test cases.
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: David Francis <David.Francis@amd.com>
> Cc: Mikita Lipski <mikita.lipski@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Fixes: dc48529fb14e ("drm/dp_mst: Add PBN calculation for DSC modes")
> Reviewed-by: Lyude Paul <lyude@redhat.com> (v1)
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> [Imre: Fix kunit test cases]
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +-
>  .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  2 +-
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 +++++--------------
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  5 ++---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  3 +--
>  .../gpu/drm/tests/drm_dp_mst_helper_test.c    |  6 +++---
>  include/drm/display/drm_dp_mst_helper.h       |  2 +-
>  7 files changed, 14 insertions(+), 26 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 9a712791f309f..ada3773869ff0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6918,7 +6918,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
>  								    max_bpc);
>  		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, false);
> +		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp << 4);
>  	}
>  
>  	dm_new_connector_state->vcpi_slots =
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index d3b13d362edac..9a58e1a4c5f49 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -1642,7 +1642,7 @@ enum dc_status dm_dp_mst_is_port_support_mode(
>  	} else {
>  		/* check if mode could be supported within full_pbn */
>  		bpp = convert_dc_color_depth_into_bpc(stream->timing.display_color_depth) * 3;
> -		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp, false);
> +		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp << 4);
>  
>  		if (pbn > aconnector->mst_output_port->full_pbn)
>  			return DC_FAIL_BANDWIDTH_VALIDATE;
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 0e0d0e76de065..772b00ebd57bd 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4718,13 +4718,12 @@ 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
> + * @clock: dot clock
> + * @bpp: bpp as .4 binary fixed point
>   *
>   * 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, bool dsc)
> +int drm_dp_calc_pbn_mode(int clock, int bpp)
>  {
>  	/*
>  	 * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
> @@ -4735,18 +4734,9 @@ int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc)
>  	 * 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 / 16), 64 * 1006),
> -					8 * 54 * 1000 * 1000);
> -
> -	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
> -				8 * 54 * 1000 * 1000);
> +	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 >> 4),
> +				1000 * 8 * 54 * 1000);
>  }
>  EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 851b312bd8449..5bf45a2a85b0e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -106,8 +106,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  			continue;
>  
>  		crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,
> -						       dsc ? bpp << 4 : bpp,
> -						       dsc);
> +						       bpp << 4);
>  
>  		slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
>  						      connector->port,
> @@ -975,7 +974,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  		return ret;
>  
>  	if (mode_rate > max_rate || mode->clock > max_dotclk ||
> -	    drm_dp_calc_pbn_mode(mode->clock, min_bpp, false) > port->full_pbn) {
> +	    drm_dp_calc_pbn_mode(mode->clock, min_bpp << 4) > port->full_pbn) {
>  		*status = MODE_CLOCK_HIGH;
>  		return 0;
>  	}
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index d2be40337b92e..153717e1df1a2 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -982,8 +982,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
>  		const int clock = crtc_state->adjusted_mode.clock;
>  
>  		asyh->or.bpc = connector->display_info.bpc;
> -		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3,
> -						    false);
> +		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3 << 4);
>  	}
>  
>  	mst_state = drm_atomic_get_mst_topology_state(state, &mstm->mgr);
> diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> index 545beea33e8c7..e3c818dfc0e6d 100644
> --- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> @@ -42,13 +42,13 @@ static const struct drm_dp_mst_calc_pbn_mode_test drm_dp_mst_calc_pbn_mode_cases
>  		.clock = 332880,
>  		.bpp = 24,
>  		.dsc = true,
> -		.expected = 50
> +		.expected = 1191
>  	},
>  	{
>  		.clock = 324540,
>  		.bpp = 24,
>  		.dsc = true,
> -		.expected = 49
> +		.expected = 1161
>  	},
>  };
>  
> @@ -56,7 +56,7 @@ static void drm_test_dp_mst_calc_pbn_mode(struct kunit *test)
>  {
>  	const struct drm_dp_mst_calc_pbn_mode_test *params = test->param_value;
>  
> -	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp, params->dsc),
> +	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp << 4),
>  			params->expected);
>  }
>  
> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> index 4429d3b1745b6..655862b3d2a49 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -842,7 +842,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector,
>  int drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
>  			     int link_rate, int link_lane_count);
>  
> -int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc);
> +int drm_dp_calc_pbn_mode(int clock, int bpp);
>  
>  void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_t link_encoding_cap);
>
Harry Wentland Nov. 8, 2023, 2:40 p.m. UTC | #5
On 2023-10-30 11:58, Imre Deak wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The current code does '(bpp << 4) / 16' in the MST PBN
> calculation, but that is just the same as 'bpp' so the
> DSC codepath achieves absolutely nothing. Fix it up so that
> the fractional part of the bpp value is actually used instead
> of truncated away. 64*1006 has enough zero lsbs that we can
> just shift that down in the dividend and thus still manage
> to stick to a 32bit divisor.
> 
> And while touching this, let's just make the whole thing more
> straightforward by making the passed in bpp value .4 binary
> fixed point always, instead of having to pass in different
> things based on whether DSC is enabled or not.
> 
> v2:
> - Fix DSC kunit test cases.
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: David Francis <David.Francis@amd.com>
> Cc: Mikita Lipski <mikita.lipski@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Fixes: dc48529fb14e ("drm/dp_mst: Add PBN calculation for DSC modes")
> Reviewed-by: Lyude Paul <lyude@redhat.com> (v1)
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> [Imre: Fix kunit test cases]
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Acked-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +-
>  .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  2 +-
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 +++++--------------
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  5 ++---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  3 +--
>  .../gpu/drm/tests/drm_dp_mst_helper_test.c    |  6 +++---
>  include/drm/display/drm_dp_mst_helper.h       |  2 +-
>  7 files changed, 14 insertions(+), 26 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 9a712791f309f..ada3773869ff0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6918,7 +6918,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
>  								    max_bpc);
>  		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, false);
> +		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp << 4);
>  	}
>  
>  	dm_new_connector_state->vcpi_slots =
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index d3b13d362edac..9a58e1a4c5f49 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -1642,7 +1642,7 @@ enum dc_status dm_dp_mst_is_port_support_mode(
>  	} else {
>  		/* check if mode could be supported within full_pbn */
>  		bpp = convert_dc_color_depth_into_bpc(stream->timing.display_color_depth) * 3;
> -		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp, false);
> +		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp << 4);
>  
>  		if (pbn > aconnector->mst_output_port->full_pbn)
>  			return DC_FAIL_BANDWIDTH_VALIDATE;
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 0e0d0e76de065..772b00ebd57bd 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4718,13 +4718,12 @@ 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
> + * @clock: dot clock
> + * @bpp: bpp as .4 binary fixed point
>   *
>   * 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, bool dsc)
> +int drm_dp_calc_pbn_mode(int clock, int bpp)
>  {
>  	/*
>  	 * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
> @@ -4735,18 +4734,9 @@ int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc)
>  	 * 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 / 16), 64 * 1006),
> -					8 * 54 * 1000 * 1000);
> -
> -	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
> -				8 * 54 * 1000 * 1000);
> +	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 >> 4),
> +				1000 * 8 * 54 * 1000);
>  }
>  EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 851b312bd8449..5bf45a2a85b0e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -106,8 +106,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  			continue;
>  
>  		crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,
> -						       dsc ? bpp << 4 : bpp,
> -						       dsc);
> +						       bpp << 4);
>  
>  		slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
>  						      connector->port,
> @@ -975,7 +974,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  		return ret;
>  
>  	if (mode_rate > max_rate || mode->clock > max_dotclk ||
> -	    drm_dp_calc_pbn_mode(mode->clock, min_bpp, false) > port->full_pbn) {
> +	    drm_dp_calc_pbn_mode(mode->clock, min_bpp << 4) > port->full_pbn) {
>  		*status = MODE_CLOCK_HIGH;
>  		return 0;
>  	}
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index d2be40337b92e..153717e1df1a2 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -982,8 +982,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
>  		const int clock = crtc_state->adjusted_mode.clock;
>  
>  		asyh->or.bpc = connector->display_info.bpc;
> -		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3,
> -						    false);
> +		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3 << 4);
>  	}
>  
>  	mst_state = drm_atomic_get_mst_topology_state(state, &mstm->mgr);
> diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> index 545beea33e8c7..e3c818dfc0e6d 100644
> --- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> @@ -42,13 +42,13 @@ static const struct drm_dp_mst_calc_pbn_mode_test drm_dp_mst_calc_pbn_mode_cases
>  		.clock = 332880,
>  		.bpp = 24,
>  		.dsc = true,
> -		.expected = 50
> +		.expected = 1191
>  	},
>  	{
>  		.clock = 324540,
>  		.bpp = 24,
>  		.dsc = true,
> -		.expected = 49
> +		.expected = 1161
>  	},
>  };
>  
> @@ -56,7 +56,7 @@ static void drm_test_dp_mst_calc_pbn_mode(struct kunit *test)
>  {
>  	const struct drm_dp_mst_calc_pbn_mode_test *params = test->param_value;
>  
> -	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp, params->dsc),
> +	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp << 4),
>  			params->expected);
>  }
>  
> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> index 4429d3b1745b6..655862b3d2a49 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -842,7 +842,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector,
>  int drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
>  			     int link_rate, int link_lane_count);
>  
> -int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc);
> +int drm_dp_calc_pbn_mode(int clock, int bpp);
>  
>  void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_t link_encoding_cap);
>
Alex Deucher Nov. 8, 2023, 5:01 p.m. UTC | #6
[Public]

> -----Original Message-----
> From: Wentland, Harry <Harry.Wentland@amd.com>
> Sent: Wednesday, November 8, 2023 9:40 AM
> To: Imre Deak <imre.deak@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>; Manasi Navare
> <manasi.d.navare@intel.com>; Lyude Paul <lyude@redhat.com>; Francis,
> David <David.Francis@amd.com>; Mikita Lipski <mikita.lipski@amd.com>;
> Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH v4 02/30] drm/dp_mst: Fix fractional DSC bpp handling
>
>
>
> On 2023-10-30 11:58, Imre Deak wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > The current code does '(bpp << 4) / 16' in the MST PBN calculation,
> > but that is just the same as 'bpp' so the DSC codepath achieves
> > absolutely nothing. Fix it up so that the fractional part of the bpp
> > value is actually used instead of truncated away. 64*1006 has enough
> > zero lsbs that we can just shift that down in the dividend and thus
> > still manage to stick to a 32bit divisor.
> >
> > And while touching this, let's just make the whole thing more
> > straightforward by making the passed in bpp value .4 binary fixed
> > point always, instead of having to pass in different things based on
> > whether DSC is enabled or not.
> >
> > v2:
> > - Fix DSC kunit test cases.
> >
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: David Francis <David.Francis@amd.com>
> > Cc: Mikita Lipski <mikita.lipski@amd.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Fixes: dc48529fb14e ("drm/dp_mst: Add PBN calculation for DSC modes")
> > Reviewed-by: Lyude Paul <lyude@redhat.com> (v1)
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > [Imre: Fix kunit test cases]
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>
> Acked-by: Harry Wentland <harry.wentland@amd.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

>
> Harry
>
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +-
> >  .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  2 +-
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 +++++--------------
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  5 ++---
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  3 +--
> >  .../gpu/drm/tests/drm_dp_mst_helper_test.c    |  6 +++---
> >  include/drm/display/drm_dp_mst_helper.h       |  2 +-
> >  7 files changed, 14 insertions(+), 26 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 9a712791f309f..ada3773869ff0 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -6918,7 +6918,7 @@ static int
> dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
> >                                                                 max_bpc);
> >             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, false);
> > +           dm_new_connector_state->pbn =
> drm_dp_calc_pbn_mode(clock, bpp <<
> > +4);
> >     }
> >
> >     dm_new_connector_state->vcpi_slots = diff --git
> > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > index d3b13d362edac..9a58e1a4c5f49 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > +++
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > @@ -1642,7 +1642,7 @@ enum dc_status
> dm_dp_mst_is_port_support_mode(
> >     } else {
> >             /* check if mode could be supported within full_pbn */
> >             bpp = convert_dc_color_depth_into_bpc(stream-
> >timing.display_color_depth) * 3;
> > -           pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz
> / 10, bpp, false);
> > +           pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz
> / 10, bpp
> > +<< 4);
> >
> >             if (pbn > aconnector->mst_output_port->full_pbn)
> >                     return DC_FAIL_BANDWIDTH_VALIDATE; diff --git
> > a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 0e0d0e76de065..772b00ebd57bd 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4718,13 +4718,12 @@ 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
> > + * @clock: dot clock
> > + * @bpp: bpp as .4 binary fixed point
> >   *
> >   * 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, bool dsc)
> > +int drm_dp_calc_pbn_mode(int clock, int bpp)
> >  {
> >     /*
> >      * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006 @@
> > -4735,18 +4734,9 @@ int drm_dp_calc_pbn_mode(int clock, int bpp, bool
> dsc)
> >      * 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 /
> 16), 64 * 1006),
> > -                                   8 * 54 * 1000 * 1000);
> > -
> > -   return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
> > -                           8 * 54 * 1000 * 1000);
> > +   return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 >>
> 4),
> > +                           1000 * 8 * 54 * 1000);
> >  }
> >  EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 851b312bd8449..5bf45a2a85b0e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -106,8 +106,7 @@ static int
> intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
> >                     continue;
> >
> >             crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode-
> >crtc_clock,
> > -                                                  dsc ? bpp << 4 : bpp,
> > -                                                  dsc);
> > +                                                  bpp << 4);
> >
> >             slots = drm_dp_atomic_find_time_slots(state, &intel_dp-
> >mst_mgr,
> >                                                   connector->port,
> > @@ -975,7 +974,7 @@ intel_dp_mst_mode_valid_ctx(struct
> drm_connector *connector,
> >             return ret;
> >
> >     if (mode_rate > max_rate || mode->clock > max_dotclk ||
> > -       drm_dp_calc_pbn_mode(mode->clock, min_bpp, false) > port-
> >full_pbn) {
> > +       drm_dp_calc_pbn_mode(mode->clock, min_bpp << 4) >
> > +port->full_pbn) {
> >             *status = MODE_CLOCK_HIGH;
> >             return 0;
> >     }
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index d2be40337b92e..153717e1df1a2 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -982,8 +982,7 @@ nv50_msto_atomic_check(struct drm_encoder
> *encoder,
> >             const int clock = crtc_state->adjusted_mode.clock;
> >
> >             asyh->or.bpc = connector->display_info.bpc;
> > -           asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc
> * 3,
> > -                                               false);
> > +           asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc
> * 3 << 4);
> >     }
> >
> >     mst_state = drm_atomic_get_mst_topology_state(state, &mstm-
> >mgr);
> > diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > index 545beea33e8c7..e3c818dfc0e6d 100644
> > --- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > +++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > @@ -42,13 +42,13 @@ static const struct
> drm_dp_mst_calc_pbn_mode_test drm_dp_mst_calc_pbn_mode_cases
> >             .clock = 332880,
> >             .bpp = 24,
> >             .dsc = true,
> > -           .expected = 50
> > +           .expected = 1191
> >     },
> >     {
> >             .clock = 324540,
> >             .bpp = 24,
> >             .dsc = true,
> > -           .expected = 49
> > +           .expected = 1161
> >     },
> >  };
> >
> > @@ -56,7 +56,7 @@ static void drm_test_dp_mst_calc_pbn_mode(struct
> > kunit *test)  {
> >     const struct drm_dp_mst_calc_pbn_mode_test *params =
> > test->param_value;
> >
> > -   KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock,
> params->bpp, params->dsc),
> > +   KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock,
> > +params->bpp << 4),
> >                     params->expected);
> >  }
> >
> > diff --git a/include/drm/display/drm_dp_mst_helper.h
> > b/include/drm/display/drm_dp_mst_helper.h
> > index 4429d3b1745b6..655862b3d2a49 100644
> > --- a/include/drm/display/drm_dp_mst_helper.h
> > +++ b/include/drm/display/drm_dp_mst_helper.h
> > @@ -842,7 +842,7 @@ struct edid *drm_dp_mst_get_edid(struct
> > drm_connector *connector,  int drm_dp_get_vc_payload_bw(const struct
> drm_dp_mst_topology_mgr *mgr,
> >                          int link_rate, int link_lane_count);
> >
> > -int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc);
> > +int drm_dp_calc_pbn_mode(int clock, int bpp);
> >
> >  void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state
> > *mst_state, uint8_t link_encoding_cap);
> >
diff mbox series

Patch

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 9a712791f309f..ada3773869ff0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6918,7 +6918,7 @@  static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
 								    max_bpc);
 		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, false);
+		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp << 4);
 	}
 
 	dm_new_connector_state->vcpi_slots =
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index d3b13d362edac..9a58e1a4c5f49 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -1642,7 +1642,7 @@  enum dc_status dm_dp_mst_is_port_support_mode(
 	} else {
 		/* check if mode could be supported within full_pbn */
 		bpp = convert_dc_color_depth_into_bpc(stream->timing.display_color_depth) * 3;
-		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp, false);
+		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp << 4);
 
 		if (pbn > aconnector->mst_output_port->full_pbn)
 			return DC_FAIL_BANDWIDTH_VALIDATE;
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 0e0d0e76de065..772b00ebd57bd 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4718,13 +4718,12 @@  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
+ * @clock: dot clock
+ * @bpp: bpp as .4 binary fixed point
  *
  * 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, bool dsc)
+int drm_dp_calc_pbn_mode(int clock, int bpp)
 {
 	/*
 	 * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
@@ -4735,18 +4734,9 @@  int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc)
 	 * 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 / 16), 64 * 1006),
-					8 * 54 * 1000 * 1000);
-
-	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
-				8 * 54 * 1000 * 1000);
+	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 >> 4),
+				1000 * 8 * 54 * 1000);
 }
 EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 851b312bd8449..5bf45a2a85b0e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -106,8 +106,7 @@  static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
 			continue;
 
 		crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,
-						       dsc ? bpp << 4 : bpp,
-						       dsc);
+						       bpp << 4);
 
 		slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
 						      connector->port,
@@ -975,7 +974,7 @@  intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
 		return ret;
 
 	if (mode_rate > max_rate || mode->clock > max_dotclk ||
-	    drm_dp_calc_pbn_mode(mode->clock, min_bpp, false) > port->full_pbn) {
+	    drm_dp_calc_pbn_mode(mode->clock, min_bpp << 4) > port->full_pbn) {
 		*status = MODE_CLOCK_HIGH;
 		return 0;
 	}
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index d2be40337b92e..153717e1df1a2 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -982,8 +982,7 @@  nv50_msto_atomic_check(struct drm_encoder *encoder,
 		const int clock = crtc_state->adjusted_mode.clock;
 
 		asyh->or.bpc = connector->display_info.bpc;
-		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3,
-						    false);
+		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3 << 4);
 	}
 
 	mst_state = drm_atomic_get_mst_topology_state(state, &mstm->mgr);
diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
index 545beea33e8c7..e3c818dfc0e6d 100644
--- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
@@ -42,13 +42,13 @@  static const struct drm_dp_mst_calc_pbn_mode_test drm_dp_mst_calc_pbn_mode_cases
 		.clock = 332880,
 		.bpp = 24,
 		.dsc = true,
-		.expected = 50
+		.expected = 1191
 	},
 	{
 		.clock = 324540,
 		.bpp = 24,
 		.dsc = true,
-		.expected = 49
+		.expected = 1161
 	},
 };
 
@@ -56,7 +56,7 @@  static void drm_test_dp_mst_calc_pbn_mode(struct kunit *test)
 {
 	const struct drm_dp_mst_calc_pbn_mode_test *params = test->param_value;
 
-	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp, params->dsc),
+	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp << 4),
 			params->expected);
 }
 
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 4429d3b1745b6..655862b3d2a49 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -842,7 +842,7 @@  struct edid *drm_dp_mst_get_edid(struct drm_connector *connector,
 int drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
 			     int link_rate, int link_lane_count);
 
-int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc);
+int drm_dp_calc_pbn_mode(int clock, int bpp);
 
 void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_t link_encoding_cap);