Re: [PATCH v2 06/14] drm/dp-mst: Use dc and drm helpers to compute timeslots
On Wed, 2019-08-21 at 12:27 +, Kazlauskas, Nicholas wrote:
> On 8/20/19 4:43 PM, Lyude Paul wrote:
> > Some nitpicks below
> >
> > On Tue, 2019-08-20 at 15:11 -0400, David Francis wrote:
> > > We were using drm helpers to convert a timing into its
> > > bandwidth, its bandwidth into pbn, and its pbn into timeslots
> > >
> > > These helpers
> > > -Did not take DSC timings into account
> > > -Used the link rate and lane count of the link's aux device,
> > > which are not the same as the link's current cap
> > > -Did not take FEC into account (FEC reduces the PBN per timeslot)
> > >
> > > For converting timing into PBN, add a new function
> > > drm_dp_calc_pbn_mode_dsc that handles the DSC case
> > >
> > > For converting PBN into time slots, amdgpu doesn't use the
> > > 'correct' atomic method (drm_dp_atomic_find_vcpi_slots), so
> > > don't add a new helper to cover our approach. Use the same
> > > means of calculating pbn per time slot as the DSC code.
> > >
> > > v2: Add drm helper for clock to pbn conversion
> > >
> > > Cc: Jerry Zuo
> > > Cc: Nicholas Kazlauskas
> > > Signed-off-by: David Francis
>
> I would like ot see Lyude's nitpicks addressed but also having this
> patch split into one that just adds the helper and another that uses it
> in amdgpu.
Sgtm!
>
> Nicholas Kazlauskas
>
> > > ---
> > > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 18 +---
> > > drivers/gpu/drm/drm_dp_mst_topology.c | 41 +++
> > > include/drm/drm_dp_mst_helper.h | 2 +-
> > > 3 files changed, 54 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > index 5f2c315b18ba..dfa99e0d6e64 100644
> > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > @@ -189,8 +189,8 @@ bool
> > > dm_helpers_dp_mst_write_payload_allocation_table(
> > > int slots = 0;
> > > bool ret;
> > > int clock;
> > > - int bpp = 0;
> > > int pbn = 0;
> > > + int pbn_per_timeslot, bpp = 0;
> > >
> > > aconnector = (struct amdgpu_dm_connector *)stream-
> > > >dm_stream_context;
> > >
> > > @@ -208,7 +208,6 @@ bool
> > > dm_helpers_dp_mst_write_payload_allocation_table(
> > > clock = stream->timing.pix_clk_100hz / 10;
> > >
> > > switch (stream->timing.display_color_depth) {
> > > -
> > > case COLOR_DEPTH_666:
> > > bpp = 6;
> > > break;
> > > @@ -234,11 +233,18 @@ bool
> > > dm_helpers_dp_mst_write_payload_allocation_table(
> > >
> > > bpp = bpp * 3;
> > >
> > > - /* TODO need to know link rate */
> > > -
> > > - pbn = drm_dp_calc_pbn_mode(clock, bpp);
> > > +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
> > > + if (stream->timing.flags.DSC)
> > > + pbn = drm_dp_calc_pbn_mode_dsc(clock,
> > > + stream-
> > > > timing.dsc_cfg.bits_per_pixel);
> > > + else
> > > +#endif
> > > + pbn = drm_dp_calc_pbn_mode(clock, bpp);
> > >
> > > - slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
> > > + /* Convert kilobits per second / 64 (for 64 timeslots) to pbn
> > > (54/64 megabytes per second) */
> > > + pbn_per_timeslot = dc_link_bandwidth_kbps(
> > > + stream->link, dc_link_get_link_cap(stream-
> > > > link)) / (8 * 1000 * 54);
> > > + slots = DIV_ROUND_UP(pbn, pbn_per_timeslot);
> > > ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn,
> > > slots);
> > >
> > > if (!ret)
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 398e7314ea8b..d789b7af7dbf 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -3588,6 +3588,47 @@ static int test_calc_pbn_mode(void)
> > > return 0;
> > > }
> > >
> > > +/**
> > > + * drm_dp_calc_pbn_mode_dsc() - Calculate the PBN for a mode with DSC
> > > enabled.
> > > + * @clock: dot clock for the mode
> > > + * @dsc_bpp: dsc bits per pixel x16 (e.g. dsc_bpp = 136 is 8.5 bpp)
> > > + *
> > > + * This uses the formula in the spec to calculate the PBN value for a
> > > mode,
> > > + * given that the mode is using DSC
> >
> > You should use the proper kdoc formatting for documenting return values
> > (not
> > all of the DP MST code does this currently, but that's a bug I haven't
> > taken
> > the time to fix :):
> >
> > /*
> > * foo_bar() - foo the bar
> > *
> > * foos the bar, guaranteed
> > * Returns:
> > * Some magic number
> > */
> >
> > > + */
> > > +int drm_dp_calc_pbn_mode_dsc(int clock, int dsc_bpp)
> > > +{
> > > + u64 kbps;
> > > + s6
Re: [PATCH v2 06/14] drm/dp-mst: Use dc and drm helpers to compute timeslots
On 8/20/19 4:43 PM, Lyude Paul wrote:
> Some nitpicks below
>
> On Tue, 2019-08-20 at 15:11 -0400, David Francis wrote:
>> We were using drm helpers to convert a timing into its
>> bandwidth, its bandwidth into pbn, and its pbn into timeslots
>>
>> These helpers
>> -Did not take DSC timings into account
>> -Used the link rate and lane count of the link's aux device,
>> which are not the same as the link's current cap
>> -Did not take FEC into account (FEC reduces the PBN per timeslot)
>>
>> For converting timing into PBN, add a new function
>> drm_dp_calc_pbn_mode_dsc that handles the DSC case
>>
>> For converting PBN into time slots, amdgpu doesn't use the
>> 'correct' atomic method (drm_dp_atomic_find_vcpi_slots), so
>> don't add a new helper to cover our approach. Use the same
>> means of calculating pbn per time slot as the DSC code.
>>
>> v2: Add drm helper for clock to pbn conversion
>>
>> Cc: Jerry Zuo
>> Cc: Nicholas Kazlauskas
>> Signed-off-by: David Francis
I would like ot see Lyude's nitpicks addressed but also having this
patch split into one that just adds the helper and another that uses it
in amdgpu.
Nicholas Kazlauskas
>> ---
>> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 18 +---
>> drivers/gpu/drm/drm_dp_mst_topology.c | 41 +++
>> include/drm/drm_dp_mst_helper.h | 2 +-
>> 3 files changed, 54 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> index 5f2c315b18ba..dfa99e0d6e64 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> @@ -189,8 +189,8 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>> int slots = 0;
>> bool ret;
>> int clock;
>> -int bpp = 0;
>> int pbn = 0;
>> +int pbn_per_timeslot, bpp = 0;
>>
>> aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
>>
>> @@ -208,7 +208,6 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>> clock = stream->timing.pix_clk_100hz / 10;
>>
>> switch (stream->timing.display_color_depth) {
>> -
>> case COLOR_DEPTH_666:
>> bpp = 6;
>> break;
>> @@ -234,11 +233,18 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>>
>> bpp = bpp * 3;
>>
>> -/* TODO need to know link rate */
>> -
>> -pbn = drm_dp_calc_pbn_mode(clock, bpp);
>> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
>> +if (stream->timing.flags.DSC)
>> +pbn = drm_dp_calc_pbn_mode_dsc(clock,
>> +stream-
>>> timing.dsc_cfg.bits_per_pixel);
>> +else
>> +#endif
>> +pbn = drm_dp_calc_pbn_mode(clock, bpp);
>>
>> -slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
>> +/* Convert kilobits per second / 64 (for 64 timeslots) to pbn
>> (54/64 megabytes per second) */
>> +pbn_per_timeslot = dc_link_bandwidth_kbps(
>> +stream->link, dc_link_get_link_cap(stream-
>>> link)) / (8 * 1000 * 54);
>> +slots = DIV_ROUND_UP(pbn, pbn_per_timeslot);
>> ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots);
>>
>> if (!ret)
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index 398e7314ea8b..d789b7af7dbf 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -3588,6 +3588,47 @@ static int test_calc_pbn_mode(void)
>> return 0;
>> }
>>
>> +/**
>> + * drm_dp_calc_pbn_mode_dsc() - Calculate the PBN for a mode with DSC
>> enabled.
>> + * @clock: dot clock for the mode
>> + * @dsc_bpp: dsc bits per pixel x16 (e.g. dsc_bpp = 136 is 8.5 bpp)
>> + *
>> + * This uses the formula in the spec to calculate the PBN value for a mode,
>> + * given that the mode is using DSC
>
> You should use the proper kdoc formatting for documenting return values (not
> all of the DP MST code does this currently, but that's a bug I haven't taken
> the time to fix :):
>
> /*
> * foo_bar() - foo the bar
> *
> * foos the bar, guaranteed
> * Returns:
> * Some magic number
> */
>
>> + */
>> +int drm_dp_calc_pbn_mode_dsc(int clock, int dsc_bpp)
>> +{
>> +u64 kbps;
>> +s64 peak_kbps;
>> +u32 numerator;
>> +u32 denominator;
>> +
>> +kbps = clock * dsc_bpp;
>> +
>> +/*
>> + * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
>> + * The unit of 54/64Mbytes/sec is an arbitrary unit chosen based on
>> + * common multiplier to render an integer PBN for all link rate/lane
>> + * counts combinations
>> + * calculate
>> + * peak_kbps *= (1/16) bppx16 to bpp
>> + * peak_kbps *= (1006/1000)
>> + * peak_kbps
Re: [PATCH v2 06/14] drm/dp-mst: Use dc and drm helpers to compute timeslots
Some nitpicks below
On Tue, 2019-08-20 at 15:11 -0400, David Francis wrote:
> We were using drm helpers to convert a timing into its
> bandwidth, its bandwidth into pbn, and its pbn into timeslots
>
> These helpers
> -Did not take DSC timings into account
> -Used the link rate and lane count of the link's aux device,
> which are not the same as the link's current cap
> -Did not take FEC into account (FEC reduces the PBN per timeslot)
>
> For converting timing into PBN, add a new function
> drm_dp_calc_pbn_mode_dsc that handles the DSC case
>
> For converting PBN into time slots, amdgpu doesn't use the
> 'correct' atomic method (drm_dp_atomic_find_vcpi_slots), so
> don't add a new helper to cover our approach. Use the same
> means of calculating pbn per time slot as the DSC code.
>
> v2: Add drm helper for clock to pbn conversion
>
> Cc: Jerry Zuo
> Cc: Nicholas Kazlauskas
> Signed-off-by: David Francis
> ---
> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 18 +---
> drivers/gpu/drm/drm_dp_mst_topology.c | 41 +++
> include/drm/drm_dp_mst_helper.h | 2 +-
> 3 files changed, 54 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 5f2c315b18ba..dfa99e0d6e64 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -189,8 +189,8 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
> int slots = 0;
> bool ret;
> int clock;
> - int bpp = 0;
> int pbn = 0;
> + int pbn_per_timeslot, bpp = 0;
>
> aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
>
> @@ -208,7 +208,6 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
> clock = stream->timing.pix_clk_100hz / 10;
>
> switch (stream->timing.display_color_depth) {
> -
> case COLOR_DEPTH_666:
> bpp = 6;
> break;
> @@ -234,11 +233,18 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>
> bpp = bpp * 3;
>
> - /* TODO need to know link rate */
> -
> - pbn = drm_dp_calc_pbn_mode(clock, bpp);
> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
> + if (stream->timing.flags.DSC)
> + pbn = drm_dp_calc_pbn_mode_dsc(clock,
> + stream-
> >timing.dsc_cfg.bits_per_pixel);
> + else
> +#endif
> + pbn = drm_dp_calc_pbn_mode(clock, bpp);
>
> - slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
> + /* Convert kilobits per second / 64 (for 64 timeslots) to pbn
> (54/64 megabytes per second) */
> + pbn_per_timeslot = dc_link_bandwidth_kbps(
> + stream->link, dc_link_get_link_cap(stream-
> >link)) / (8 * 1000 * 54);
> + slots = DIV_ROUND_UP(pbn, pbn_per_timeslot);
> ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots);
>
> if (!ret)
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 398e7314ea8b..d789b7af7dbf 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3588,6 +3588,47 @@ static int test_calc_pbn_mode(void)
> return 0;
> }
>
> +/**
> + * drm_dp_calc_pbn_mode_dsc() - Calculate the PBN for a mode with DSC
> enabled.
> + * @clock: dot clock for the mode
> + * @dsc_bpp: dsc bits per pixel x16 (e.g. dsc_bpp = 136 is 8.5 bpp)
> + *
> + * This uses the formula in the spec to calculate the PBN value for a mode,
> + * given that the mode is using DSC
You should use the proper kdoc formatting for documenting return values (not
all of the DP MST code does this currently, but that's a bug I haven't taken
the time to fix :):
/*
* foo_bar() - foo the bar
*
* foos the bar, guaranteed
* Returns:
* Some magic number
*/
> + */
> +int drm_dp_calc_pbn_mode_dsc(int clock, int dsc_bpp)
> +{
> + u64 kbps;
> + s64 peak_kbps;
> + u32 numerator;
> + u32 denominator;
> +
> + kbps = clock * dsc_bpp;
> +
> + /*
> + * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
> + * The unit of 54/64Mbytes/sec is an arbitrary unit chosen based on
> + * common multiplier to render an integer PBN for all link rate/lane
> + * counts combinations
> + * calculate
> + * peak_kbps *= (1/16) bppx16 to bpp
> + * peak_kbps *= (1006/1000)
> + * peak_kbps *= (64/54)
> + * peak_kbps *= 8convert to bytes
> + *
> + * Divide numerator and denominator by 16 to avoid overflow
> + */
> +
> + numerator = 64 * 1006 / 16;
> + denominator = 54 * 8 * 1000 * 1000;
> +
> + kbps *= numerator;
> + peak_kbps = drm_fixp_from_fraction(kbps, denominator);
> +
[PATCH v2 06/14] drm/dp-mst: Use dc and drm helpers to compute timeslots
We were using drm helpers to convert a timing into its
bandwidth, its bandwidth into pbn, and its pbn into timeslots
These helpers
-Did not take DSC timings into account
-Used the link rate and lane count of the link's aux device,
which are not the same as the link's current cap
-Did not take FEC into account (FEC reduces the PBN per timeslot)
For converting timing into PBN, add a new function
drm_dp_calc_pbn_mode_dsc that handles the DSC case
For converting PBN into time slots, amdgpu doesn't use the
'correct' atomic method (drm_dp_atomic_find_vcpi_slots), so
don't add a new helper to cover our approach. Use the same
means of calculating pbn per time slot as the DSC code.
v2: Add drm helper for clock to pbn conversion
Cc: Jerry Zuo
Cc: Nicholas Kazlauskas
Signed-off-by: David Francis
---
.../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 18 +---
drivers/gpu/drm/drm_dp_mst_topology.c | 41 +++
include/drm/drm_dp_mst_helper.h | 2 +-
3 files changed, 54 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 5f2c315b18ba..dfa99e0d6e64 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -189,8 +189,8 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
int slots = 0;
bool ret;
int clock;
- int bpp = 0;
int pbn = 0;
+ int pbn_per_timeslot, bpp = 0;
aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
@@ -208,7 +208,6 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
clock = stream->timing.pix_clk_100hz / 10;
switch (stream->timing.display_color_depth) {
-
case COLOR_DEPTH_666:
bpp = 6;
break;
@@ -234,11 +233,18 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
bpp = bpp * 3;
- /* TODO need to know link rate */
-
- pbn = drm_dp_calc_pbn_mode(clock, bpp);
+#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
+ if (stream->timing.flags.DSC)
+ pbn = drm_dp_calc_pbn_mode_dsc(clock,
+ stream->timing.dsc_cfg.bits_per_pixel);
+ else
+#endif
+ pbn = drm_dp_calc_pbn_mode(clock, bpp);
- slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
+ /* Convert kilobits per second / 64 (for 64 timeslots) to pbn
(54/64 megabytes per second) */
+ pbn_per_timeslot = dc_link_bandwidth_kbps(
+ stream->link,
dc_link_get_link_cap(stream->link)) / (8 * 1000 * 54);
+ slots = DIV_ROUND_UP(pbn, pbn_per_timeslot);
ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots);
if (!ret)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 398e7314ea8b..d789b7af7dbf 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3588,6 +3588,47 @@ static int test_calc_pbn_mode(void)
return 0;
}
+/**
+ * drm_dp_calc_pbn_mode_dsc() - Calculate the PBN for a mode with DSC enabled.
+ * @clock: dot clock for the mode
+ * @dsc_bpp: dsc bits per pixel x16 (e.g. dsc_bpp = 136 is 8.5 bpp)
+ *
+ * This uses the formula in the spec to calculate the PBN value for a mode,
+ * given that the mode is using DSC
+ */
+int drm_dp_calc_pbn_mode_dsc(int clock, int dsc_bpp)
+{
+ u64 kbps;
+ s64 peak_kbps;
+ u32 numerator;
+ u32 denominator;
+
+ kbps = clock * dsc_bpp;
+
+ /*
+* margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
+* The unit of 54/64Mbytes/sec is an arbitrary unit chosen based on
+* common multiplier to render an integer PBN for all link rate/lane
+* counts combinations
+* calculate
+* peak_kbps *= (1/16) bppx16 to bpp
+* peak_kbps *= (1006/1000)
+* peak_kbps *= (64/54)
+* peak_kbps *= 8convert to bytes
+*
+* Divide numerator and denominator by 16 to avoid overflow
+*/
+
+ numerator = 64 * 1006 / 16;
+ denominator = 54 * 8 * 1000 * 1000;
+
+ kbps *= numerator;
+ peak_kbps = drm_fixp_from_fraction(kbps, denominator);
+
+ return drm_fixp2int_ceil(peak_kbps);
+}
+EXPORT_SYMBOL(drm_dp_calc_pbn_mode_dsc);
+
/* 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/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 2ba6253ea6d3..ddb518f2157a 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -611,7 +611,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector
