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 >