RE: [PATCH v8 01/17] drm/dp_mst: Add PBN calculation for DSC modes
Hi Lyude,
Thanks for the review.
I’ve updated this patch in a v9 iteration of it, I’ve sent separately from the
series this past Monday. The new version has the updated selftest.
Thanks,
Mikita
From: Lyude Paul<mailto:ly...@redhat.com>
Sent: December 6, 2019 7:17 PM
To: Lipski, Mikita<mailto:mikita.lip...@amd.com>;
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Cc: dri-de...@lists.freedesktop.org<mailto:dri-de...@lists.freedesktop.org>;
David Francis<mailto:david.fran...@amd.com>
Subject: Re: [PATCH v8 01/17] drm/dp_mst: Add PBN calculation for DSC modes
This patch still needs to be rebased, and the selftests for the PBN
calculation that got added still need to be updated to reflect the changes for
dsc. The code for PBN calculation changed quite a bit upstream since this
series started.
On Tue, 2019-12-03 at 09:35 -0500, mikita.lip...@amd.com wrote:
> From: David Francis
>
> 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
>
> v3: Keep the calculation in a single equation
>
> Cc: Lyude Paul
> Reviewed-by: Manasi Navare
> Reviewed-by: Lyude Paul
> Reviewed-by: Harry Wentland
> Signed-off-by: David Francis
> Signed-off-by: Mikita Lipski
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
> drivers/gpu/drm/drm_dp_mst_topology.c | 38 ++-
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +-
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +-
> drivers/gpu/drm/radeon/radeon_dp_mst.c| 2 +-
> include/drm/drm_dp_mst_helper.h | 3 +-
> 6 files changed, 43 insertions(+), 7 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 455c51c38720..9fc03fc1017d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4967,7 +4967,7 @@ static int dm_encoder_helper_atomic_check(struct
> drm_encoder *encoder,
>is_y420);
>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 ae5809a1f19a..261e2c1828c6 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4342,10 +4342,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)
> {
>/*
> * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
> @@ -4356,12 +4357,47 @@ int drm_dp_calc_pbn_mode(int clock, int bpp)
> * peak_kbps *= (1006/1000)
> * peak_kbps *= (64/54)
> * peak_kbps *= 8convert 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, 64 * 1006 /
> 16),
> + 8 * 54 * 1000 * 1000);
> +
>return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
>8 * 54 * 1000 * 1000);
> +
> }
> EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>
> +static int test_calc_pbn_mode(void)
> +{
> + int ret;
> + 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;
> +
Re: [PATCH v8 01/17] drm/dp_mst: Add PBN calculation for DSC modes
This patch still needs to be rebased, and the selftests for the PBN
calculation that got added still need to be updated to reflect the changes for
dsc. The code for PBN calculation changed quite a bit upstream since this
series started.
On Tue, 2019-12-03 at 09:35 -0500, mikita.lip...@amd.com wrote:
> From: David Francis
>
> 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
>
> v3: Keep the calculation in a single equation
>
> Cc: Lyude Paul
> Reviewed-by: Manasi Navare
> Reviewed-by: Lyude Paul
> Reviewed-by: Harry Wentland
> Signed-off-by: David Francis
> Signed-off-by: Mikita Lipski
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
> drivers/gpu/drm/drm_dp_mst_topology.c | 38 ++-
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +-
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +-
> drivers/gpu/drm/radeon/radeon_dp_mst.c| 2 +-
> include/drm/drm_dp_mst_helper.h | 3 +-
> 6 files changed, 43 insertions(+), 7 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 455c51c38720..9fc03fc1017d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4967,7 +4967,7 @@ static int dm_encoder_helper_atomic_check(struct
> drm_encoder *encoder,
> is_y420);
> 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 ae5809a1f19a..261e2c1828c6 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4342,10 +4342,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)
> {
> /*
>* margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
> @@ -4356,12 +4357,47 @@ int drm_dp_calc_pbn_mode(int clock, int bpp)
>* peak_kbps *= (1006/1000)
>* peak_kbps *= (64/54)
>* peak_kbps *= 8convert 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, 64 * 1006 /
> 16),
> + 8 * 54 * 1000 * 1000);
> +
> return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
> 8 * 54 * 1000 * 1000);
> +
> }
> EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>
> +static int test_calc_pbn_mode(void)
> +{
> + int ret;
> + 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, 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, 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);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +
> /* we want to kick the TX after we've ack the up/down IRQs. */
> static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr)
> {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 03d1cba0b696..92be17711287 100644
> ---
Re: [PATCH v8 01/17] drm/dp_mst: Add PBN calculation for DSC modes
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20191203]
[cannot apply to drm-intel/for-linux-next linus/master v5.4-rc8
drm-exynos/exynos-drm-next v5.4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/mikita-lipski-amd-com/DSC-MST-support-for-DRM-and-AMDGPU/20191204-020604
base:1ab75b2e415a29dba9aec94f203c6f88dbfc0ba0
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-91-g817270f-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c:28:43: sparse: sparse:
>> not enough arguments for function drm_dp_calc_pbn_mode
vim +28 drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
7cbce45d624322 Lyude Paul 2019-09-03 13
7cbce45d624322 Lyude Paul 2019-09-03 14 int igt_dp_mst_calc_pbn_mode(void
*ignored)
7cbce45d624322 Lyude Paul 2019-09-03 15 {
7cbce45d624322 Lyude Paul 2019-09-03 16int pbn, i;
7cbce45d624322 Lyude Paul 2019-09-03 17const struct {
7cbce45d624322 Lyude Paul 2019-09-03 18int rate;
7cbce45d624322 Lyude Paul 2019-09-03 19int bpp;
7cbce45d624322 Lyude Paul 2019-09-03 20int expected;
7cbce45d624322 Lyude Paul 2019-09-03 21} test_params[] = {
7cbce45d624322 Lyude Paul 2019-09-03 22{ 154000, 30, 689 },
7cbce45d624322 Lyude Paul 2019-09-03 23{ 234000, 30, 1047 },
7cbce45d624322 Lyude Paul 2019-09-03 24{ 297000, 24, 1063 },
7cbce45d624322 Lyude Paul 2019-09-03 25};
7cbce45d624322 Lyude Paul 2019-09-03 26
7cbce45d624322 Lyude Paul 2019-09-03 27for (i = 0; i <
ARRAY_SIZE(test_params); i++) {
7cbce45d624322 Lyude Paul 2019-09-03 @28pbn =
drm_dp_calc_pbn_mode(test_params[i].rate,
7cbce45d624322 Lyude Paul 2019-09-03 29
test_params[i].bpp);
7cbce45d624322 Lyude Paul 2019-09-03 30FAIL(pbn !=
test_params[i].expected,
7cbce45d624322 Lyude Paul 2019-09-03 31 "Expected PBN %d
for clock %d bpp %d, got %d\n",
7cbce45d624322 Lyude Paul 2019-09-03 32
test_params[i].expected, test_params[i].rate,
7cbce45d624322 Lyude Paul 2019-09-03 33
test_params[i].bpp, pbn);
7cbce45d624322 Lyude Paul 2019-09-03 34}
7cbce45d624322 Lyude Paul 2019-09-03 35
7cbce45d624322 Lyude Paul 2019-09-03 36return 0;
7cbce45d624322 Lyude Paul 2019-09-03 37 }
2f015ec6eab693 Lyude Paul 2019-09-03 38
:: The code at line 28 was first introduced by commit
:: 7cbce45d6243225914b5c967b4ee927a2327842a drm/dp_mst: Move
test_calc_pbn_mode() into an actual selftest
:: TO: Lyude Paul
:: CC: Lyude Paul
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org Intel Corporation
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v8 01/17] drm/dp_mst: Add PBN calculation for DSC modes
From: David Francis
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
v3: Keep the calculation in a single equation
Cc: Lyude Paul
Reviewed-by: Manasi Navare
Reviewed-by: Lyude Paul
Reviewed-by: Harry Wentland
Signed-off-by: David Francis
Signed-off-by: Mikita Lipski
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
drivers/gpu/drm/drm_dp_mst_topology.c | 38 ++-
drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +-
drivers/gpu/drm/radeon/radeon_dp_mst.c| 2 +-
include/drm/drm_dp_mst_helper.h | 3 +-
6 files changed, 43 insertions(+), 7 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 455c51c38720..9fc03fc1017d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4967,7 +4967,7 @@ static int dm_encoder_helper_atomic_check(struct
drm_encoder *encoder,
is_y420);
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 ae5809a1f19a..261e2c1828c6 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -4342,10 +4342,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)
{
/*
* margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
@@ -4356,12 +4357,47 @@ int drm_dp_calc_pbn_mode(int clock, int bpp)
* peak_kbps *= (1006/1000)
* peak_kbps *= (64/54)
* peak_kbps *= 8convert 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, 64 * 1006 /
16),
+ 8 * 54 * 1000 * 1000);
+
return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
8 * 54 * 1000 * 1000);
+
}
EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
+static int test_calc_pbn_mode(void)
+{
+ int ret;
+ 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, 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, 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);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+
/* we want to kick the TX after we've ack the up/down IRQs. */
static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr)
{
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 03d1cba0b696..92be17711287 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);
+
