Re: [Intel-gfx] [PATCH v2 01/11] drm/dp_mst: Store the MST PBN divider value in fixed point format
On Thu, Nov 16, 2023 at 03:18:31PM +0200, Imre Deak wrote:
[...]
> 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 ed784cf27d396..63024393b516e 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
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
>
> #include "dm_services.h"
> #include "amdgpu.h"
> @@ -210,7 +211,7 @@ static void dm_helpers_construct_old_payload(
> struct drm_dp_mst_atomic_payload *old_payload)
> {
> struct drm_dp_mst_atomic_payload *pos;
> - int pbn_per_slot = mst_state->pbn_div;
> + int pbn_per_slot = dfixed_trunc(mst_state->pbn_div);
> u8 next_payload_vc_start = mgr->next_start_slot;
> u8 payload_vc_start = new_payload->vc_start_slot;
> u8 allocated_time_slots;
I'm planning to merge this patchset today via drm-intel-next and for
that I'll need to rebase the above change to:
@@ -205,13 +206,14 @@ void dm_helpers_dp_update_branch_info(
static void dm_helpers_construct_old_payload(
struct dc_link *link,
- int pbn_per_slot,
+ fixed20_12 pbn_per_slot_fp,
struct drm_dp_mst_atomic_payload *new_payload,
struct drm_dp_mst_atomic_payload *old_payload)
{
struct link_mst_stream_allocation_table current_link_table =
link->mst_stream_alloc_table;
struct link_mst_stream_allocation *dc_alloc;
+ int pbn_per_slot = dfixed_trunc(pbn_per_slot_fp);
int i;
*old_payload = *new_payload;
and then apply the original changes in the patch above while merging
drm-intel-next to drm-tip. This is required due to
commit 9031e0013f819c ("drm/amd/display: Fix mst hub unplug warning")
being only in drm-misc-next, but not yet in drm-intel-next.
Let me know if you have a concern with this.
--Imre
Re: [Intel-gfx] [PATCH v2 01/11] drm/dp_mst: Store the MST PBN divider value in fixed point format
On Fri, Nov 17, 2023 at 12:56:36PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 16, 2023 at 03:18:31PM +0200, Imre Deak wrote:
> > On UHBR links the PBN divider is a fractional number, accordingly store
> > it in fixed point format. For now drm_dp_get_vc_payload_bw() always
> > returns a whole number and all callers will use only the integer part of
> > it which should preserve the current behavior. The next patch will fix
> > drm_dp_get_vc_payload_bw() for UHBR rates returning a fractional number
> > for those (also accounting for the channel coding efficiency correctly).
> >
> > Cc: Lyude Paul
> > Cc: Harry Wentland
> > Cc: Alex Deucher
> > Cc: Wayne Lin
> > Cc: amd-...@lists.freedesktop.org
> > Cc: dri-de...@lists.freedesktop.org
> > Signed-off-by: Imre Deak
> > ---
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++--
> > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 3 ++-
> > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 +++--
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +--
> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 ++-
> > drivers/gpu/drm/nouveau/dispnv50/disp.c | 6 +++--
> > include/drm/display/drm_dp_mst_helper.h | 7 +++---
> > 7 files changed, 33 insertions(+), 18 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 74f9f02abcdec..12346b21d0b05 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -85,6 +85,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -6909,8 +6910,8 @@ static int dm_encoder_helper_atomic_check(struct
> > drm_encoder *encoder,
> > if (IS_ERR(mst_state))
> > return PTR_ERR(mst_state);
> >
> > - if (!mst_state->pbn_div)
> > - mst_state->pbn_div =
> > dm_mst_get_pbn_divider(aconnector->mst_root->dc_link);
> > + if (!mst_state->pbn_div.full)
> > + mst_state->pbn_div.full =
> > dfixed_const(dm_mst_get_pbn_divider(aconnector->mst_root->dc_link));
>
> Why doesn't that dfixed stuff return the correct type?
AFAICS a follow up change could make dfixed_init() return the correct
type and then change all
fp.full = dfixed_const(A)
to
fp = dfixed_init(A)
>
> Anyways looks mostly mechanical
> Reviewed-by: Ville Syrjälä
>
> >
> > if (!state->duplicated) {
> > int max_bpc = conn_state->max_requested_bpc;
> > 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 ed784cf27d396..63024393b516e 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
> > @@ -31,6 +31,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "dm_services.h"
> > #include "amdgpu.h"
> > @@ -210,7 +211,7 @@ static void dm_helpers_construct_old_payload(
> > struct drm_dp_mst_atomic_payload *old_payload)
> > {
> > struct drm_dp_mst_atomic_payload *pos;
> > - int pbn_per_slot = mst_state->pbn_div;
> > + int pbn_per_slot = dfixed_trunc(mst_state->pbn_div);
> > u8 next_payload_vc_start = mgr->next_start_slot;
> > u8 payload_vc_start = new_payload->vc_start_slot;
> > u8 allocated_time_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 9a58e1a4c5f49..d1ba3ae228b08 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
> > @@ -27,6 +27,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "dm_services.h"
> > #include "amdgpu.h"
> > #include "amdgpu_dm.h"
> > @@ -941,10 +942,10 @@ static int increase_dsc_bpp(struct drm_atomic_state
> > *state,
> > link_timeslots_used = 0;
> >
> > for (i = 0; i < count; i++)
> > - link_timeslots_used += DIV_ROUND_UP(vars[i + k].pbn,
> > mst_state->pbn_div);
> > + link_timeslots_used += DIV_ROUND_UP(vars[i + k].pbn,
> > dfixed_trunc(mst_state->pbn_div));
> >
> > fair_pbn_alloc =
> > - (63 - link_timeslots_used) / remaining_to_increase *
> > mst_state->pbn_div;
> > + (63 - link_timeslots_used) / remaining_to_increase *
> > dfixed_trunc(mst_state->pbn_div);
> >
> > if (initial_slack[next_index] > fair_pbn_alloc) {
> > vars[next_index].pbn += fair_pbn_alloc;
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 4d72c9a32026e..000d05e80352a 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++
Re: [Intel-gfx] [PATCH v2 01/11] drm/dp_mst: Store the MST PBN divider value in fixed point format
On Thu, Nov 16, 2023 at 03:18:31PM +0200, Imre Deak wrote:
> On UHBR links the PBN divider is a fractional number, accordingly store
> it in fixed point format. For now drm_dp_get_vc_payload_bw() always
> returns a whole number and all callers will use only the integer part of
> it which should preserve the current behavior. The next patch will fix
> drm_dp_get_vc_payload_bw() for UHBR rates returning a fractional number
> for those (also accounting for the channel coding efficiency correctly).
>
> Cc: Lyude Paul
> Cc: Harry Wentland
> Cc: Alex Deucher
> Cc: Wayne Lin
> Cc: amd-...@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Signed-off-by: Imre Deak
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++--
> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 3 ++-
> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 +++--
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +--
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 ++-
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 6 +++--
> include/drm/display/drm_dp_mst_helper.h | 7 +++---
> 7 files changed, 33 insertions(+), 18 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 74f9f02abcdec..12346b21d0b05 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -85,6 +85,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -6909,8 +6910,8 @@ static int dm_encoder_helper_atomic_check(struct
> drm_encoder *encoder,
> if (IS_ERR(mst_state))
> return PTR_ERR(mst_state);
>
> - if (!mst_state->pbn_div)
> - mst_state->pbn_div =
> dm_mst_get_pbn_divider(aconnector->mst_root->dc_link);
> + if (!mst_state->pbn_div.full)
> + mst_state->pbn_div.full =
> dfixed_const(dm_mst_get_pbn_divider(aconnector->mst_root->dc_link));
Why doesn't that dfixed stuff return the correct type?
Anyways looks mostly mechanical
Reviewed-by: Ville Syrjälä
>
> if (!state->duplicated) {
> int max_bpc = conn_state->max_requested_bpc;
> 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 ed784cf27d396..63024393b516e 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
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
>
> #include "dm_services.h"
> #include "amdgpu.h"
> @@ -210,7 +211,7 @@ static void dm_helpers_construct_old_payload(
> struct drm_dp_mst_atomic_payload *old_payload)
> {
> struct drm_dp_mst_atomic_payload *pos;
> - int pbn_per_slot = mst_state->pbn_div;
> + int pbn_per_slot = dfixed_trunc(mst_state->pbn_div);
> u8 next_payload_vc_start = mgr->next_start_slot;
> u8 payload_vc_start = new_payload->vc_start_slot;
> u8 allocated_time_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 9a58e1a4c5f49..d1ba3ae228b08 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
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
> #include "dm_services.h"
> #include "amdgpu.h"
> #include "amdgpu_dm.h"
> @@ -941,10 +942,10 @@ static int increase_dsc_bpp(struct drm_atomic_state
> *state,
> link_timeslots_used = 0;
>
> for (i = 0; i < count; i++)
> - link_timeslots_used += DIV_ROUND_UP(vars[i + k].pbn,
> mst_state->pbn_div);
> + link_timeslots_used += DIV_ROUND_UP(vars[i + k].pbn,
> dfixed_trunc(mst_state->pbn_div));
>
> fair_pbn_alloc =
> - (63 - link_timeslots_used) / remaining_to_increase *
> mst_state->pbn_div;
> + (63 - link_timeslots_used) / remaining_to_increase *
> dfixed_trunc(mst_state->pbn_div);
>
> if (initial_slack[next_index] > fair_pbn_alloc) {
> vars[next_index].pbn += fair_pbn_alloc;
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 4d72c9a32026e..000d05e80352a 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -43,6 +43,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -3578,16 +3579,22 @@ static int drm_dp_send_up_ack_reply(struct
> drm_dp_mst_topology_mgr *mgr,
> * value is in units of PBNs/(timeslots/1 MTP). This value can be used to
> * convert the number of PBNs required for a given stream to the number of
>
[Intel-gfx] [PATCH v2 01/11] drm/dp_mst: Store the MST PBN divider value in fixed point format
On UHBR links the PBN divider is a fractional number, accordingly store
it in fixed point format. For now drm_dp_get_vc_payload_bw() always
returns a whole number and all callers will use only the integer part of
it which should preserve the current behavior. The next patch will fix
drm_dp_get_vc_payload_bw() for UHBR rates returning a fractional number
for those (also accounting for the channel coding efficiency correctly).
Cc: Lyude Paul
Cc: Harry Wentland
Cc: Alex Deucher
Cc: Wayne Lin
Cc: amd-...@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Imre Deak
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++--
.../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 3 ++-
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 +++--
drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +--
drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 ++-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 6 +++--
include/drm/display/drm_dp_mst_helper.h | 7 +++---
7 files changed, 33 insertions(+), 18 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 74f9f02abcdec..12346b21d0b05 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -85,6 +85,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -6909,8 +6910,8 @@ static int dm_encoder_helper_atomic_check(struct
drm_encoder *encoder,
if (IS_ERR(mst_state))
return PTR_ERR(mst_state);
- if (!mst_state->pbn_div)
- mst_state->pbn_div =
dm_mst_get_pbn_divider(aconnector->mst_root->dc_link);
+ if (!mst_state->pbn_div.full)
+ mst_state->pbn_div.full =
dfixed_const(dm_mst_get_pbn_divider(aconnector->mst_root->dc_link));
if (!state->duplicated) {
int max_bpc = conn_state->max_requested_bpc;
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 ed784cf27d396..63024393b516e 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
@@ -31,6 +31,7 @@
#include
#include
#include
+#include
#include "dm_services.h"
#include "amdgpu.h"
@@ -210,7 +211,7 @@ static void dm_helpers_construct_old_payload(
struct drm_dp_mst_atomic_payload *old_payload)
{
struct drm_dp_mst_atomic_payload *pos;
- int pbn_per_slot = mst_state->pbn_div;
+ int pbn_per_slot = dfixed_trunc(mst_state->pbn_div);
u8 next_payload_vc_start = mgr->next_start_slot;
u8 payload_vc_start = new_payload->vc_start_slot;
u8 allocated_time_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 9a58e1a4c5f49..d1ba3ae228b08 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
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "dm_services.h"
#include "amdgpu.h"
#include "amdgpu_dm.h"
@@ -941,10 +942,10 @@ static int increase_dsc_bpp(struct drm_atomic_state
*state,
link_timeslots_used = 0;
for (i = 0; i < count; i++)
- link_timeslots_used += DIV_ROUND_UP(vars[i + k].pbn,
mst_state->pbn_div);
+ link_timeslots_used += DIV_ROUND_UP(vars[i + k].pbn,
dfixed_trunc(mst_state->pbn_div));
fair_pbn_alloc =
- (63 - link_timeslots_used) / remaining_to_increase *
mst_state->pbn_div;
+ (63 - link_timeslots_used) / remaining_to_increase *
dfixed_trunc(mst_state->pbn_div);
if (initial_slack[next_index] > fair_pbn_alloc) {
vars[next_index].pbn += fair_pbn_alloc;
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 4d72c9a32026e..000d05e80352a 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -43,6 +43,7 @@
#include
#include
#include
+#include
#include
#include
@@ -3578,16 +3579,22 @@ static int drm_dp_send_up_ack_reply(struct
drm_dp_mst_topology_mgr *mgr,
* value is in units of PBNs/(timeslots/1 MTP). This value can be used to
* convert the number of PBNs required for a given stream to the number of
* timeslots this stream requires in each MTP.
+ *
+ * Returns the BW / timeslot value in 20.12 fixed point format.
*/
-int drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
-int link_rate, int link_lane_count)
+fixed20_12 drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
+
