On Fri, 2021-08-27 at 19:43 -0400, Fangzhi Zuo wrote:

8b/10b encoding format requires to reserve the first slot for recording metadata. Real data transmission starts from the second slot, with a total of available 63 slots available.

In 128b/132b encoding format, metadata is transmitted separately in LLCP packet before MTP. Real data transmission starts from the first slot, with a total of 64 slots available.

Signed-off-by: Fangzhi Zuo Jerry.Zuo@amd.com

drivers/gpu/drm/drm_dp_mst_topology.c | 27 ++++++++++++++++++++-------  include/drm/drm_dp_mst_helper.h       |  9 +++++++++  2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 86d13d6bc463..30544801d2b8 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3370,7 +3370,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)         struct drm_dp_payload req_payload;         struct drm_dp_mst_port *port;         int i, j; -       int cur_slots = 1; +       int cur_slots = mgr->start_slot;         bool skip;           mutex_lock(&mgr->payload_lock); @@ -4323,7 +4323,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,         num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);           /* max. time slots - one slot for MTP header */ -       if (num_slots > 63) +       if (num_slots > mgr->total_avail_slots)                 return -ENOSPC;         return num_slots;  } @@ -4335,7 +4335,7 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,         int ret;           /* max. time slots - one slot for MTP header */ -       if (slots > 63) +       if (slots > mgr->total_avail_slots)                 return -ENOSPC;           vcpi->pbn = pbn; @@ -4509,6 +4509,17 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,  }  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);   +void drm_dp_mst_update_encoding_cap(struct drm_dp_mst_topology_mgr *mgr, uint8_t link_encoding_cap) +{ +       if (link_encoding_cap == DP_CAP_ANSI_128B132B) { +               mgr->total_avail_slots = 64; +               mgr->start_slot = 0; +       } +       DRM_DEBUG_KMS("%s encoding format determined\n", +                     (link_encoding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b" : "8b/10b"); +} +EXPORT_SYMBOL(drm_dp_mst_update_encoding_cap);

This seems to be missing kdocs, can you fix that?

Also - I'm not convinced this is all of the work we have to do, as there's no locking taking place here in this function. If we're changing the number of available VCPI slots that we have, we need to be able to factor that into the atomic check logic which means that we can't rely on mgr->* for any kind of data that isn't guaranteed to remain consistent throughout the lifetime of the driver or topology. (Note that some of the old MST code didn't follow this logic, so I wouldn't be surprised if there's still exceptions to this we need to clean up).

Note that I still expect we'll have to keep some sort of track of the current total slot count in the topology mgr, but that should be reflecting the currently programmed state and not be relied on from our atomic check.

IMHO - the correct way we should go about adding support for this is to add something into drm_dp_mst_topology_state and integrate this into the atomic check helpers.

/**   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel   * @mgr: manager for this port @@ -4540,8 +4551,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,           ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);         if (ret) { -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n", -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret); +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d ret=%d\n", +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-

...

total_avail_slots, ret);

drm_dp_mst_topology_put_port(port);                 goto out;         } @@ -5228,7 +5239,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,                                          struct drm_dp_mst_topology_state *mst_state)  {         struct drm_dp_vcpi_allocation *vcpi; -       int avail_slots = 63, payload_count = 0; +       int avail_slots = mgr->total_avail_slots, payload_count = 0;           list_for_each_entry(vcpi, &mst_state->vcpis, next) {                 /* Releasing VCPI is always OK-even if the port is gone */ @@ -5257,7 +5268,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,                 }         }         drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n", -                      mgr, mst_state, avail_slots, 63 - avail_slots); +                      mgr, mst_state, avail_slots, mgr->total_avail_slots - avail_slots);           return 0;  } @@ -5529,6 +5540,8 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,         if (!mgr->proposed_vcpis)                 return -ENOMEM;         set_bit(0, &mgr->payload_mask); +       mgr->total_avail_slots = 63; +       mgr->start_slot = 1;           mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);         if (mst_state == NULL) diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index ddb9231d0309..eac5ce48f214 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -661,6 +661,15 @@ struct drm_dp_mst_topology_mgr {          */         int pbn_div;   +       /** +        * @total_avail_slots: available slots for data transmission +        */ +       u8 total_avail_slots; +       /** +        * @start_slot: first slot index for data transmission +        */ +       u8 start_slot;

/**          * @funcs: Atomic helper callbacks          */

On Tue, 2021-08-31 at 19:44 +0000, Zuo, Jerry wrote:

[AMD Official Use Only]

...

-----Original Message----- From: Lyude Paul lyude@redhat.com Sent: August 30, 2021 4:00 PM To: Zuo, Jerry Jerry.Zuo@amd.com; dri-devel@lists.freedesktop.org Cc: Wentland, Harry Harry.Wentland@amd.com; Kazlauskas, Nicholas Nicholas.Kazlauskas@amd.com; Lin, Wayne Wayne.Lin@amd.com Subject: Re: [PATCH 1/2] drm: Update MST First Link Slot Information Based on Encoding Format

On Fri, 2021-08-27 at 19:43 -0400, Fangzhi Zuo wrote:

...

8b/10b encoding format requires to reserve the first slot for recording metadata. Real data transmission starts from the second slot, with a total of available 63 slots available.

In 128b/132b encoding format, metadata is transmitted separately in LLCP packet before MTP. Real data transmission starts from the first slot, with a total of 64 slots available.

Signed-off-by: Fangzhi Zuo Jerry.Zuo@amd.com

drivers/gpu/drm/drm_dp_mst_topology.c | 27 ++++++++++++++++++++-------  include/drm/drm_dp_mst_helper.h       |  9 +++++++++  2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 86d13d6bc463..30544801d2b8 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3370,7 +3370,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)         struct drm_dp_payload req_payload;         struct drm_dp_mst_port *port;         int i, j; -       int cur_slots = 1; +       int cur_slots = mgr->start_slot;         bool skip;

mutex_lock(&mgr->payload_lock); @@ -4323,7 +4323,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,         num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);

/* max. time slots - one slot for MTP header */ -       if (num_slots > 63) +       if (num_slots > mgr->total_avail_slots)                 return -ENOSPC;         return num_slots;  } @@ -4335,7 +4335,7 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,         int ret;

/* max. time slots - one slot for MTP header */ -       if (slots > 63) +       if (slots > mgr->total_avail_slots)                 return -ENOSPC;

vcpi->pbn = pbn; @@ -4509,6 +4509,17 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,  }  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

+void drm_dp_mst_update_encoding_cap(struct

drm_dp_mst_topology_mgr

...

+*mgr, uint8_t link_encoding_cap) +{ +       if (link_encoding_cap == DP_CAP_ANSI_128B132B) { +               mgr->total_avail_slots = 64; +               mgr->start_slot = 0; +       } +       DRM_DEBUG_KMS("%s encoding format determined\n", +                     (link_encoding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b" : "8b/10b"); +} +EXPORT_SYMBOL(drm_dp_mst_update_encoding_cap);

This seems to be missing kdocs, can you fix that?

Also - I'm not convinced this is all of the work we have to do, as there's no locking taking place here in this function. If we're changing the number of available VCPI slots that we have, we need to be able to factor that into the atomic check logic which means that we can't rely on mgr->* for any kind of data that isn't guaranteed to remain consistent throughout the lifetime of the driver or topology. (Note that some of the old MST code didn't follow this logic, so I wouldn't be surprised if there's still exceptions to this we need to clean up).

Note that I still expect we'll have to keep some sort of track of the current total slot count in the topology mgr, but that should be reflecting the currently programmed state and not be relied on from our atomic check.

Thanks Lyude for your comments.

Seems I should keep existing code to keep track of current slot status in mgr. That information is getting updated each time when topology change detected. That slot information saved in mgr is a sort of static, and could only be used for debug purpose to track what is the current encoding format.

Ahh - in that case maybe we should try getting rid of it (as we could just use the atomic state for debugging output anyway).

...

IMHO - the correct way we should go about adding support for this is to add something into drm_dp_mst_topology_state and integrate this into the atomic check helpers.

The slot information should also be added into drm_dp_mst_topology_state to reflect the real-time slot status.

I'd like to confirm the best place to get slot count info. updated. Should the update be done within &drm_mode_config_funcs. atomic_check(), before new stream is created, OR should be updated within drm_dp_mst_atomic_check() ?

drm_dp_mst_atomic_check() would be better, since I imagine that we have to do this for all drivers using MST

The updated slot count will be used in drm_dp_mst_atomic_check() to check slot limit, and in drm_dp_update_payload_part1() as initial cur_slots.

...

...

/**   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel   * @mgr: manager for this port @@ -4540,8 +4551,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,

ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);         if (ret) { -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n", -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret); +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d +max=%d ret=%d\n", +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-

...

total_avail_slots, ret);

drm_dp_mst_topology_put_port(port);                 goto out;         } @@ -5228,7 +5239,7 @@

drm_dp_mst_atomic_check_vcpi_alloc_limit(struct

...

drm_dp_mst_topology_mgr *mgr,                                          struct drm_dp_mst_topology_state *mst_state)  {         struct drm_dp_vcpi_allocation *vcpi; -       int avail_slots = 63, payload_count = 0; +       int avail_slots = mgr->total_avail_slots, payload_count = 0;

list_for_each_entry(vcpi, &mst_state->vcpis, next) {                 /* Releasing VCPI is always OK-even if the port is gone */ @@ -5257,7 +5268,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,                 }         }         drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n", -                      mgr, mst_state, avail_slots, 63 - avail_slots); +                      mgr, mst_state, avail_slots, +mgr->total_avail_slots - avail_slots);

return 0;  } @@ -5529,6 +5540,8 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,         if (!mgr->proposed_vcpis)                 return -ENOMEM;         set_bit(0, &mgr->payload_mask); +       mgr->total_avail_slots = 63; +       mgr->start_slot = 1;

mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);         if (mst_state == NULL) diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index ddb9231d0309..eac5ce48f214 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -661,6 +661,15 @@ struct drm_dp_mst_topology_mgr {          */         int pbn_div;

+       /** +        * @total_avail_slots: available slots for data transmission +        */ +       u8 total_avail_slots; +       /** +        * @start_slot: first slot index for data transmission +        */ +       u8 start_slot;

/**          * @funcs: Atomic helper callbacks          */

-- Cheers,  Lyude Paul (she/her)  Software Engineer at Red Hat

8b/10b encoding format requires to reserve the first slot for recording metadata. Real data transmission starts from the second slot, with a total of available 63 slots available.

In 128b/132b encoding format, metadata is transmitted separately in LLCP packet before MTP. Real data transmission starts from the first slot, with a total of 64 slots available.

v2: * Remove get_mst_link_encoding_cap * Move total/start slots to mst_state, and copy it to mst_mgr in atomic_check

Signed-off-by: Fangzhi Zuo Jerry.Zuo@amd.com Signed-off-by: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++ drivers/gpu/drm/drm_dp_mst_topology.c | 35 +++++++++++++++---- include/drm/drm_dp_mst_helper.h | 13 +++++++ 3 files changed, 69 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 5020f2d36fe1..4ad50eb0091a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, #if defined(CONFIG_DRM_AMD_DC_DCN) struct dsc_mst_fairness_vars vars[MAX_PIPES]; #endif + struct drm_dp_mst_topology_state *mst_state; + struct drm_dp_mst_topology_mgr *mgr;

trace_amdgpu_dm_atomic_check_begin(state);

@@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, lock_and_validation_needed = true; }

+#if defined(CONFIG_DRM_AMD_DC_DCN) + for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) { + struct amdgpu_dm_connector *aconnector; + struct drm_connector *connector; + struct drm_connector_list_iter iter; + u8 link_coding_cap; + + if (!mgr->mst_state ) + continue; + + drm_connector_list_iter_begin(dev, &iter); + drm_for_each_connector_iter(connector, &iter) { + int id = connector->index; + + if (id == mst_state->mgr->conn_base_id) { + aconnector = to_amdgpu_dm_connector(connector); + link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link); + drm_dp_mst_update_coding_cap(mst_state, link_coding_cap); + + break; + } + } + drm_connector_list_iter_end(&iter); + + } +#endif /** * Streams and planes are reset when there are changes that affect * bandwidth. Anything that affects bandwidth needs to go through diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index ad0795afc21c..fb5c47c4cb2e 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) struct drm_dp_payload req_payload; struct drm_dp_mst_port *port; int i, j; - int cur_slots = 1; + int cur_slots = mgr->start_slot; bool skip;

mutex_lock(&mgr->payload_lock); @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);

/* max. time slots - one slot for MTP header */ - if (num_slots > 63) + if (num_slots > mgr->total_avail_slots) return -ENOSPC; return num_slots; } @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr, int ret;

/* max. time slots - one slot for MTP header */ - if (slots > 63) + if (slots > mgr->total_avail_slots) return -ENOSPC;

vcpi->pbn = pbn; @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, } EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

+void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap) +{ + if (link_coding_cap == DP_CAP_ANSI_128B132B) { + mst_state->total_avail_slots = 64; + mst_state->start_slot = 0; + } + + DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n", + (link_coding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b":"8b/10b", mst_state->mgr); +} +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap); + /** * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel * @mgr: manager for this port @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,

ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots); if (ret) { - drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n", - DIV_ROUND_UP(pbn, mgr->pbn_div), ret); + drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d ret=%d\n", + DIV_ROUND_UP(pbn, mgr->pbn_div), mgr->total_avail_slots, ret); drm_dp_mst_topology_put_port(port); goto out; } @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_topology_state *mst_state) { struct drm_dp_vcpi_allocation *vcpi; - int avail_slots = 63, payload_count = 0; + int avail_slots = mgr->total_avail_slots, payload_count = 0;

list_for_each_entry(vcpi, &mst_state->vcpis, next) { /* Releasing VCPI is always OK-even if the port is gone */ @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr, } } drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n", - mgr, mst_state, avail_slots, 63 - avail_slots); + mgr, mst_state, avail_slots, mgr->total_avail_slots - avail_slots);

return 0; } @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state) break;

mutex_lock(&mgr->lock); + + mgr->start_slot = mst_state->start_slot; + mgr->total_avail_slots = mst_state->total_avail_slots; + ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr->mst_primary, mst_state); mutex_unlock(&mgr->lock); @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, if (!mgr->proposed_vcpis) return -ENOMEM; set_bit(0, &mgr->payload_mask); + mgr->total_avail_slots = 63; + mgr->start_slot = 1;

mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL); if (mst_state == NULL) return -ENOMEM;

+ mst_state->total_avail_slots = 63; + mst_state->start_slot = 1; + mst_state->mgr = mgr; INIT_LIST_HEAD(&mst_state->vcpis);

diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index ddb9231d0309..f8152dfb34ed 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state { struct drm_private_state base; struct list_head vcpis; struct drm_dp_mst_topology_mgr *mgr; + u8 total_avail_slots; + u8 start_slot; };

#define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base) @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr { */ int pbn_div;

+ /** + * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b + */ + u8 total_avail_slots; + + /** + * @start_slot: 1 for 8b/10b, 0 for 128/132b + */ + u8 start_slot; + /** * @funcs: Atomic helper callbacks */ @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp

void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);

+void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap);

void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);

No problem, thanks for the patches! Will take a look at this tomorrow

On Tue, 2021-10-12 at 22:02 +0000, Lakha, Bhawanpreet wrote:

[AMD Official Use Only]

Hi Lyude,

Jerry is busy these few weeks so I will be taking a look at this. I added the start/total slots to the mst_state and am updating them in atomic_check.

Also ignore the V2 "* Remove get_mst_link_encoding_cap" I got a bit lost in trying to figure out the patch layout that was sent before.

Thanks, Bhawan

From: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com Sent: October 12, 2021 5:58 PM To: Zuo, Jerry Jerry.Zuo@amd.com; dri-devel@lists.freedesktop.org <dri- devel@lists.freedesktop.org>; lyude@redhat.com lyude@redhat.com Cc: Wentland, Harry Harry.Wentland@amd.com; Lin, Wayne Wayne.Lin@amd.com; Kazlauskas, Nicholas Nicholas.Kazlauskas@amd.com; Lakha, Bhawanpreet Bhawanpreet.Lakha@amd.com Subject: [PATCH] drm: Update MST First Link Slot Information Based on Encoding Format  8b/10b encoding format requires to reserve the first slot for recording metadata. Real data transmission starts from the second slot, with a total of available 63 slots available.

In 128b/132b encoding format, metadata is transmitted separately in LLCP packet before MTP. Real data transmission starts from the first slot, with a total of 64 slots available.

v2:

• Remove get_mst_link_encoding_cap
• Move total/start slots to mst_state, and copy it to mst_mgr in

atomic_check

Signed-off-by: Fangzhi Zuo Jerry.Zuo@amd.com Signed-off-by: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com

---

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++  drivers/gpu/drm/drm_dp_mst_topology.c         | 35 +++++++++++++++----  include/drm/drm_dp_mst_helper.h               | 13 +++++++  3 files changed, 69 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 5020f2d36fe1..4ad50eb0091a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,  #if defined(CONFIG_DRM_AMD_DC_DCN)          struct dsc_mst_fairness_vars vars[MAX_PIPES];  #endif +       struct drm_dp_mst_topology_state *mst_state; +       struct drm_dp_mst_topology_mgr *mgr;            trace_amdgpu_dm_atomic_check_begin(state);   @@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,                  lock_and_validation_needed = true;          }   +#if defined(CONFIG_DRM_AMD_DC_DCN) +       for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) { +               struct amdgpu_dm_connector *aconnector; +               struct drm_connector *connector; +               struct drm_connector_list_iter iter; +               u8 link_coding_cap;

+               if (!mgr->mst_state ) +                       continue;

+               drm_connector_list_iter_begin(dev, &iter); +               drm_for_each_connector_iter(connector, &iter) { +                       int id = connector->index;

+                       if (id == mst_state->mgr->conn_base_id) { +                               aconnector = to_amdgpu_dm_connector(connector); +                               link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link); +                               drm_dp_mst_update_coding_cap(mst_state, link_coding_cap);

+                               break; +                       } +               } +               drm_connector_list_iter_end(&iter);

+       } +#endif          /**           * Streams and planes are reset when there are changes that affect           * bandwidth. Anything that affects bandwidth needs to go through diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index ad0795afc21c..fb5c47c4cb2e 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)          struct drm_dp_payload req_payload;          struct drm_dp_mst_port *port;          int i, j; -       int cur_slots = 1; +       int cur_slots = mgr->start_slot;          bool skip;            mutex_lock(&mgr->payload_lock); @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,          num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);            /* max. time slots - one slot for MTP header */ -       if (num_slots > 63) +       if (num_slots > mgr->total_avail_slots)                  return -ENOSPC;          return num_slots;  } @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,          int ret;            /* max. time slots - one slot for MTP header */ -       if (slots > 63) +       if (slots > mgr->total_avail_slots)                  return -ENOSPC;            vcpi->pbn = pbn; @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,  }  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);   +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap) +{ +       if (link_coding_cap == DP_CAP_ANSI_128B132B) { +               mst_state->total_avail_slots = 64; +               mst_state->start_slot = 0; +       }

+       DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n", +                       (link_coding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b":"8b/10b", mst_state->mgr); +} +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);

/**   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel   * @mgr: manager for this port @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,            ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);          if (ret) { -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n", -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret); +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d ret=%d\n", +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-

...

total_avail_slots, ret);

drm_dp_mst_topology_put_port(port);                  goto out;          } @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,                                           struct drm_dp_mst_topology_state *mst_state)  {          struct drm_dp_vcpi_allocation *vcpi; -       int avail_slots = 63, payload_count = 0; +       int avail_slots = mgr->total_avail_slots, payload_count = 0;            list_for_each_entry(vcpi, &mst_state->vcpis, next) {                  /* Releasing VCPI is always OK-even if the port is gone */ @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,                  }          }          drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n", -                      mgr, mst_state, avail_slots, 63 - avail_slots); +                      mgr, mst_state, avail_slots, mgr->total_avail_slots - avail_slots);            return 0;  } @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state)                          break;                    mutex_lock(&mgr->lock);

+               mgr->start_slot = mst_state->start_slot; +               mgr->total_avail_slots = mst_state->total_avail_slots;

ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-

...

mst_primary,

mst_state);                  mutex_unlock(&mgr->lock); @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,          if (!mgr->proposed_vcpis)                  return -ENOMEM;          set_bit(0, &mgr->payload_mask); +       mgr->total_avail_slots = 63; +       mgr->start_slot = 1;            mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);          if (mst_state == NULL)                  return -ENOMEM;   +       mst_state->total_avail_slots = 63; +       mst_state->start_slot = 1;

mst_state->mgr = mgr;          INIT_LIST_HEAD(&mst_state->vcpis);   diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index ddb9231d0309..f8152dfb34ed 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {          struct drm_private_state base;          struct list_head vcpis;          struct drm_dp_mst_topology_mgr *mgr; +       u8 total_avail_slots; +       u8 start_slot;  };    #define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base) @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {           */          int pbn_div;   +       /** +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b +        */ +       u8 total_avail_slots;

+       /** +        * @start_slot: 1 for 8b/10b, 0 for 128/132b +        */ +       u8 start_slot;

/**           * @funcs: Atomic helper callbacks           */ @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp    void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);   +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap);    void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,                                  struct drm_dp_mst_port *port);

On 2021-10-13 12:09 p.m., Jani Nikula wrote:

On Tue, 12 Oct 2021, Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com wrote:

...

8b/10b encoding format requires to reserve the first slot for recording metadata. Real data transmission starts from the second slot, with a total of available 63 slots available.

In 128b/132b encoding format, metadata is transmitted separately in LLCP packet before MTP. Real data transmission starts from the first slot, with a total of 64 slots available.

v2:

• Remove get_mst_link_encoding_cap
• Move total/start slots to mst_state, and copy it to mst_mgr in

atomic_check

Signed-off-by: Fangzhi Zuo Jerry.Zuo@amd.com Signed-off-by: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com

---

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++ drivers/gpu/drm/drm_dp_mst_topology.c | 35 +++++++++++++++---- include/drm/drm_dp_mst_helper.h | 13 +++++++ 3 files changed, 69 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 5020f2d36fe1..4ad50eb0091a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, #if defined(CONFIG_DRM_AMD_DC_DCN) struct dsc_mst_fairness_vars vars[MAX_PIPES]; #endif

• struct drm_dp_mst_topology_state *mst_state;

• struct drm_dp_mst_topology_mgr *mgr;

  trace_amdgpu_dm_atomic_check_begin(state);

@@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, lock_and_validation_needed = true; }

+#if defined(CONFIG_DRM_AMD_DC_DCN)

• for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {

• struct amdgpu_dm_connector *aconnector;
  

• struct drm_connector *connector;
  

• struct drm_connector_list_iter iter;
  

• u8 link_coding_cap;
  

• if (!mgr->mst_state )
  

• 	continue;
  

• drm_connector_list_iter_begin(dev, &iter);
  

• drm_for_each_connector_iter(connector, &iter) {
  

• 	int id = connector->index;
  

• 	if (id == mst_state->mgr->conn_base_id) {
  

• 		aconnector = to_amdgpu_dm_connector(connector);
  

• 		link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
  

• 		drm_dp_mst_update_coding_cap(mst_state, link_coding_cap);
  

• 		break;
  

• 	}
  

• }
  

• drm_connector_list_iter_end(&iter);
  

• }

+#endif

I wonder if we could split this to separate drm dp helper and amd driver patches?

I believe that was the original structure but, lyude recommended to put them into the same patch to see how it is being used

...

/** * Streams and planes are reset when there are changes that affect * bandwidth. Anything that affects bandwidth needs to go through diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index ad0795afc21c..fb5c47c4cb2e 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) struct drm_dp_payload req_payload; struct drm_dp_mst_port *port; int i, j;

• int cur_slots = 1;

• int cur_slots = mgr->start_slot; bool skip;

  mutex_lock(&mgr->payload_lock);

@@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);

/* max. time slots - one slot for MTP header */

• if (num_slots > 63)

• if (num_slots > mgr->total_avail_slots) return -ENOSPC; return num_slots; }

@@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr, int ret;

/* max. time slots - one slot for MTP header */

• if (slots > 63)

• if (slots > mgr->total_avail_slots) return -ENOSPC;

  vcpi->pbn = pbn;

@@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, } EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

+void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap) +{

• if (link_coding_cap == DP_CAP_ANSI_128B132B) {

• mst_state->total_avail_slots = 64;
  

• mst_state->start_slot = 0;
  

• }

The values never change AFAICT, should we store the channel encoding instead, and use that information to initialize the values?

(Alternatively, why aren't the 8b/10b values initialized here if 128b/132b are?)

I agree, 8b/10 are the default, but in case where we switch from 128b/132 -> 8b/10b we should be updating them here aswell.

...

• DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n",

• 	(link_coding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b":"8b/10b", mst_state->mgr);
  

+} +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);

• /**
  • drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
  • @mgr: manager for this port

@@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,

ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots); if (ret) {

• drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n",
  

• 	    DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
  

• drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d ret=%d\n",
  

• 	    DIV_ROUND_UP(pbn, mgr->pbn_div), mgr->total_avail_slots, ret);
  

  drm_dp_mst_topology_put_port(port); goto out; }

@@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_topology_state *mst_state) { struct drm_dp_vcpi_allocation *vcpi;

• int avail_slots = 63, payload_count = 0;

• int avail_slots = mgr->total_avail_slots, payload_count = 0;

  list_for_each_entry(vcpi, &mst_state->vcpis, next) { /* Releasing VCPI is always OK-even if the port is gone */

@@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr, } } drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n",

•        mgr, mst_state, avail_slots, 63 - avail_slots);
  

•        mgr, mst_state, avail_slots, mgr->total_avail_slots - avail_slots);
  
  

  return 0; }

@@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state) break;

mutex_lock(&mgr->lock);

• mgr->start_slot = mst_state->start_slot;
  

• mgr->total_avail_slots = mst_state->total_avail_slots;
  

It feels wrong to me to be copying stuff from mst_state to mgr in general, and in atomic check hook in particular.

previously we were setting it directly in the mgr, and this was suggested by lyude. I am not sure what the alternative is.

...

ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr->mst_primary,
					    mst_state);
mutex_unlock(&mgr->lock);

@@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, if (!mgr->proposed_vcpis) return -ENOMEM; set_bit(0, &mgr->payload_mask);

• mgr->total_avail_slots = 63;

• mgr->start_slot = 1;

  mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL); if (mst_state == NULL) return -ENOMEM;

• mst_state->total_avail_slots = 63;

• mst_state->start_slot = 1;

The magic values for 8b/10b are now duplicated to two places, with the 128b/132b values in a separate place.

8b/10b is the default (to make sure we don't break any existing driver that doesn't use 128b/132b). Are you against setting it as the default here? or do you mean we should use #define for this? the magic numbers are currently being used directly right now (inside drm_dp_find_vcpi_slots, drm_dp_init_vcpi).

Bhawan

BR, Jani.

...

mst_state->mgr = mgr; INIT_LIST_HEAD(&mst_state->vcpis);

diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index ddb9231d0309..f8152dfb34ed 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state { struct drm_private_state base; struct list_head vcpis; struct drm_dp_mst_topology_mgr *mgr;

• u8 total_avail_slots;

• u8 start_slot; };

  #define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base)

@@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr { */ int pbn_div;

• /**

• * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
  

• */
  

• u8 total_avail_slots;
• /**

• * @start_slot: 1 for 8b/10b, 0 for 128/132b
  

• */
  

• u8 start_slot;
• /**
  • @funcs: Atomic helper callbacks
  */

@@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp

void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);

+void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap);

void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);

Some comments below (also, sorry again for the mixup on the last review!)

On Tue, 2021-10-12 at 17:58 -0400, Bhawanpreet Lakha wrote:

8b/10b encoding format requires to reserve the first slot for recording metadata. Real data transmission starts from the second slot, with a total of available 63 slots available.

In 128b/132b encoding format, metadata is transmitted separately in LLCP packet before MTP. Real data transmission starts from the first slot, with a total of 64 slots available.

v2:

• Remove get_mst_link_encoding_cap
• Move total/start slots to mst_state, and copy it to mst_mgr in

atomic_check

Signed-off-by: Fangzhi Zuo Jerry.Zuo@amd.com Signed-off-by: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com

---

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++  drivers/gpu/drm/drm_dp_mst_topology.c         | 35 +++++++++++++++----  include/drm/drm_dp_mst_helper.h               | 13 +++++++  3 files changed, 69 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 5020f2d36fe1..4ad50eb0091a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,  #if defined(CONFIG_DRM_AMD_DC_DCN)         struct dsc_mst_fairness_vars vars[MAX_PIPES];  #endif +       struct drm_dp_mst_topology_state *mst_state; +       struct drm_dp_mst_topology_mgr *mgr;           trace_amdgpu_dm_atomic_check_begin(state);   @@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,                 lock_and_validation_needed = true;         }   +#if defined(CONFIG_DRM_AMD_DC_DCN) +       for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) { +               struct amdgpu_dm_connector *aconnector; +               struct drm_connector *connector; +               struct drm_connector_list_iter iter; +               u8 link_coding_cap;

+               if (!mgr->mst_state ) +                       continue;

extraneous space

+               drm_connector_list_iter_begin(dev, &iter); +               drm_for_each_connector_iter(connector, &iter) { +                       int id = connector->index;

+                       if (id == mst_state->mgr->conn_base_id) { +                               aconnector = to_amdgpu_dm_connector(connector); +                               link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link); +                               drm_dp_mst_update_coding_cap(mst_state, link_coding_cap);

+                               break; +                       } +               } +               drm_connector_list_iter_end(&iter);

+       } +#endif         /**          * Streams and planes are reset when there are changes that affect          * bandwidth. Anything that affects bandwidth needs to go through diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index ad0795afc21c..fb5c47c4cb2e 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)         struct drm_dp_payload req_payload;         struct drm_dp_mst_port *port;         int i, j; -       int cur_slots = 1; +       int cur_slots = mgr->start_slot;         bool skip;           mutex_lock(&mgr->payload_lock); @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,         num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);           /* max. time slots - one slot for MTP header */ -       if (num_slots > 63) +       if (num_slots > mgr->total_avail_slots)                 return -ENOSPC;

For reasons I will explain a little further in this email, we might want to drop this…

return num_slots;  } @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,         int ret;           /* max. time slots - one slot for MTP header */ -       if (slots > 63) +       if (slots > mgr->total_avail_slots)

…and this

return -ENOSPC;           vcpi->pbn = pbn; @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,  }  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);   +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap)

Need some kdocs here

+{ +       if (link_coding_cap == DP_CAP_ANSI_128B132B) { +               mst_state->total_avail_slots = 64; +               mst_state->start_slot = 0; +       }

+       DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n", +                       (link_coding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b":"8b/10b", mst_state->mgr);

need to fix indenting here, and wrap this to 100 chars

+} +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);

/**   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel   * @mgr: manager for this port @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,           ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);         if (ret) { -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n", -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret); +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d ret=%d\n", +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-

...

total_avail_slots, ret);

drm_dp_mst_topology_put_port(port);                 goto out;         } @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,                                          struct drm_dp_mst_topology_state *mst_state)  {         struct drm_dp_vcpi_allocation *vcpi; -       int avail_slots = 63, payload_count = 0; +       int avail_slots = mgr->total_avail_slots, payload_count = 0;           list_for_each_entry(vcpi, &mst_state->vcpis, next) {                 /* Releasing VCPI is always OK-even if the port is gone */ @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,                 }         }         drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n", -                      mgr, mst_state, avail_slots, 63 - avail_slots); +                      mgr, mst_state, avail_slots, mgr->total_avail_slots - avail_slots);           return 0;  } @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state)                         break;                   mutex_lock(&mgr->lock);

+               mgr->start_slot = mst_state->start_slot; +               mgr->total_avail_slots = mst_state->total_avail_slots;

this isn't correct - atomic checks aren't allowed to mutate anything besides the atomic state structure that we're checking since we don't know whether or not the display state is going to be committed when checking it. If we're storing state in mgr, that state needs to be written to mgr during the atomic commit instead of the atomic check.

...but, coming back to this MST code after being gone for a while, I think it might be time for us to stop worrying about the non-atomic state. Especially since there's only one driver using it; the legacy radeon.ko; and right now the plan is either to just drop MST support on radeon.ko or move the MST code it uses into radeon.ko.Which brings me to say - I think we can drop some of the hunks I mentioned above (e.g. the changes to drm_dp_init_vcpi and drm_dp_find_vcpi_slots I mentioned above). We can then just update the kdocs for these functions in a separate patch to clarify that now in addition to being deprecated, these functions are just broken and will eventually be removed.

So - doing that allows us to get rid of mgr->total_avail_slots and mgr-

start_slot entirely, just set the slot info in the atomic state during atomic

check, and then just rely on the atomic state for drm_dp_atomic_find_vcpi_slots() and friends. Which seems much simpler to me, does this sound alrght with you?

ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-

...

mst_primary,

mst_state);                 mutex_unlock(&mgr->lock); @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,         if (!mgr->proposed_vcpis)                 return -ENOMEM;         set_bit(0, &mgr->payload_mask); +       mgr->total_avail_slots = 63; +       mgr->start_slot = 1;           mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);         if (mst_state == NULL)                 return -ENOMEM;   +       mst_state->total_avail_slots = 63; +       mst_state->start_slot = 1;

mst_state->mgr = mgr;         INIT_LIST_HEAD(&mst_state->vcpis);   diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index ddb9231d0309..f8152dfb34ed 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {         struct drm_private_state base;         struct list_head vcpis;         struct drm_dp_mst_topology_mgr *mgr; +       u8 total_avail_slots; +       u8 start_slot;  };    #define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base) @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {          */         int pbn_div;   +       /** +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b +        */ +       u8 total_avail_slots;

+       /** +        * @start_slot: 1 for 8b/10b, 0 for 128/132b +        */ +       u8 start_slot;

/**          * @funcs: Atomic helper callbacks          */ @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp    void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);   +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap);    void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,                                 struct drm_dp_mst_port *port);

Adding Mikita aswell

On 2021-10-14 4:21 p.m., Bhawanpreet Lakha wrote:

On 2021-10-13 6:25 p.m., Lyude Paul wrote:

...

Some comments below (also, sorry again for the mixup on the last review!)

On Tue, 2021-10-12 at 17:58 -0400, Bhawanpreet Lakha wrote:

...

8b/10b encoding format requires to reserve the first slot for recording metadata. Real data transmission starts from the second slot, with a total of available 63 slots available.

In 128b/132b encoding format, metadata is transmitted separately in LLCP packet before MTP. Real data transmission starts from the first slot, with a total of 64 slots available.

v2:

• Remove get_mst_link_encoding_cap
• Move total/start slots to mst_state, and copy it to mst_mgr in

atomic_check

Signed-off-by: Fangzhi Zuo Jerry.Zuo@amd.com Signed-off-by: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com

---

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++   drivers/gpu/drm/drm_dp_mst_topology.c         | 35 +++++++++++++++----   include/drm/drm_dp_mst_helper.h               | 13 +++++++   3 files changed, 69 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 5020f2d36fe1..4ad50eb0091a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,   #if defined(CONFIG_DRM_AMD_DC_DCN)          struct dsc_mst_fairness_vars vars[MAX_PIPES];   #endif +       struct drm_dp_mst_topology_state *mst_state; +       struct drm_dp_mst_topology_mgr *mgr;            trace_amdgpu_dm_atomic_check_begin(state);   @@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,                  lock_and_validation_needed = true;          }   +#if defined(CONFIG_DRM_AMD_DC_DCN) +       for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) { +               struct amdgpu_dm_connector *aconnector; +               struct drm_connector *connector; +               struct drm_connector_list_iter iter; +               u8 link_coding_cap;

+               if (!mgr->mst_state ) +                       continue;

extraneous space

...

+               drm_connector_list_iter_begin(dev, &iter); +               drm_for_each_connector_iter(connector, &iter) { +                       int id = connector->index;

+                       if (id == mst_state->mgr->conn_base_id) { +                               aconnector = to_amdgpu_dm_connector(connector); +                               link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link); +                               drm_dp_mst_update_coding_cap(mst_state, link_coding_cap);

+                               break; +                       } +               } +               drm_connector_list_iter_end(&iter);

+       } +#endif          /**           * Streams and planes are reset when there are changes that affect           * bandwidth. Anything that affects bandwidth needs to go through diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index ad0795afc21c..fb5c47c4cb2e 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)          struct drm_dp_payload req_payload;          struct drm_dp_mst_port *port;          int i, j; -       int cur_slots = 1; +       int cur_slots = mgr->start_slot;          bool skip;            mutex_lock(&mgr->payload_lock); @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,          num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);            /* max. time slots - one slot for MTP header */ -       if (num_slots > 63) +       if (num_slots > mgr->total_avail_slots)                  return -ENOSPC;

For reasons I will explain a little further in this email, we might want to drop this…

...

return num_slots;   } @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,          int ret;            /* max. time slots - one slot for MTP header */ -       if (slots > 63) +       if (slots > mgr->total_avail_slots)

…and this

...

return -ENOSPC;            vcpi->pbn = pbn; @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,   }   EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);   +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap)

Need some kdocs here

...

+{ +       if (link_coding_cap == DP_CAP_ANSI_128B132B) { +               mst_state->total_avail_slots = 64; +               mst_state->start_slot = 0; +       }

+       DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n", +                       (link_coding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b":"8b/10b", mst_state->mgr);

need to fix indenting here, and wrap this to 100 chars

...

+} +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);

/**    * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel    * @mgr: manager for this port @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,            ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);          if (ret) { -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n", -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret); +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d ret=%d\n", +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-

...

total_avail_slots, ret);

drm_dp_mst_topology_put_port(port);                  goto out;          } @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,                                           struct drm_dp_mst_topology_state *mst_state)   {          struct drm_dp_vcpi_allocation *vcpi; -       int avail_slots = 63, payload_count = 0; +       int avail_slots = mgr->total_avail_slots, payload_count = 0;            list_for_each_entry(vcpi, &mst_state->vcpis, next) {                  /* Releasing VCPI is always OK-even if the port is gone */ @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,                  }          }          drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n", -                      mgr, mst_state, avail_slots, 63 - avail_slots); +                      mgr, mst_state, avail_slots, mgr->total_avail_slots - avail_slots);            return 0;   } @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state)                          break;                    mutex_lock(&mgr->lock);

+               mgr->start_slot = mst_state->start_slot; +               mgr->total_avail_slots = mst_state->total_avail_slots;

this isn't correct - atomic checks aren't allowed to mutate anything besides the atomic state structure that we're checking since we don't know whether or not the display state is going to be committed when checking it. If we're storing state in mgr, that state needs to be written to mgr during the atomic commit instead of the atomic check.

...but, coming back to this MST code after being gone for a while, I think it might be time for us to stop worrying about the non-atomic state. Especially since there's only one driver using it; the legacy radeon.ko; and right now the plan is either to just drop MST support on radeon.ko or move the MST code it uses into radeon.ko.Which brings me to say - I think we can drop some of the hunks I mentioned above (e.g. the changes to drm_dp_init_vcpi and drm_dp_find_vcpi_slots I mentioned above). We can then just update the kdocs for these functions in a separate patch to clarify that now in addition to being deprecated, these functions are just broken and will eventually be removed.

So - doing that allows us to get rid of mgr->total_avail_slots and mgr-

...

start_slot entirely, just set the slot info in the atomic state during atomic

check, and then just rely on the atomic state for drm_dp_atomic_find_vcpi_slots() and friends. Which seems much simpler to me, does this sound alrght with you?

Thanks for the response,

That function is per port (drm_dp_atomic_find_vcpi_slots) so not sure how that will work, maybe I don't understand what you mean?

Also we only need to check this inside drm_dp_mst_atomic_check_vcpi_alloc_limit(), which doesn't have a state, so we still need to have this on the mgr somehow.

I was thinking we could add the slots(or some DP version indicator) inside the drm_connector, and add a parameter to drm_dp_mst_atomic_check_vcpi_alloc_limit(int slots) and call it with this info via drm_dp_mst_atomic_check().

Bhawan

...

...

ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-

...

mst_primary,

mst_state);                  mutex_unlock(&mgr->lock); @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,          if (!mgr->proposed_vcpis)                  return -ENOMEM;          set_bit(0, &mgr->payload_mask); +       mgr->total_avail_slots = 63; +       mgr->start_slot = 1;            mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);          if (mst_state == NULL)                  return -ENOMEM;   +       mst_state->total_avail_slots = 63; +       mst_state->start_slot = 1;

mst_state->mgr = mgr;          INIT_LIST_HEAD(&mst_state->vcpis);   diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index ddb9231d0309..f8152dfb34ed 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {          struct drm_private_state base;          struct list_head vcpis;          struct drm_dp_mst_topology_mgr *mgr; +       u8 total_avail_slots; +       u8 start_slot;   };     #define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base) @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {           */          int pbn_div;   +       /** +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b +        */ +       u8 total_avail_slots;

+       /** +        * @start_slot: 1 for 8b/10b, 0 for 128/132b +        */ +       u8 start_slot;

/**           * @funcs: Atomic helper callbacks           */ @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp     void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);   +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap);     void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,                                  struct drm_dp_mst_port *port);

On 2021-10-13 6:25 p.m., Lyude Paul wrote:

Some comments below (also, sorry again for the mixup on the last review!)

On Tue, 2021-10-12 at 17:58 -0400, Bhawanpreet Lakha wrote:

...

8b/10b encoding format requires to reserve the first slot for recording metadata. Real data transmission starts from the second slot, with a total of available 63 slots available.

In 128b/132b encoding format, metadata is transmitted separately in LLCP packet before MTP. Real data transmission starts from the first slot, with a total of 64 slots available.

v2:

• Remove get_mst_link_encoding_cap
• Move total/start slots to mst_state, and copy it to mst_mgr in

atomic_check

Signed-off-by: Fangzhi Zuo Jerry.Zuo@amd.com Signed-off-by: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com

---

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++  drivers/gpu/drm/drm_dp_mst_topology.c         | 35 +++++++++++++++----  include/drm/drm_dp_mst_helper.h               | 13 +++++++  3 files changed, 69 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 5020f2d36fe1..4ad50eb0091a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,  #if defined(CONFIG_DRM_AMD_DC_DCN)         struct dsc_mst_fairness_vars vars[MAX_PIPES];  #endif +       struct drm_dp_mst_topology_state *mst_state; +       struct drm_dp_mst_topology_mgr *mgr;

trace_amdgpu_dm_atomic_check_begin(state);

@@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,                 lock_and_validation_needed = true;         }

+#if defined(CONFIG_DRM_AMD_DC_DCN) +       for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) { +               struct amdgpu_dm_connector *aconnector; +               struct drm_connector *connector; +               struct drm_connector_list_iter iter; +               u8 link_coding_cap;

+               if (!mgr->mst_state ) +                       continue;

extraneous space

...

+               drm_connector_list_iter_begin(dev, &iter); +               drm_for_each_connector_iter(connector, &iter) { +                       int id = connector->index;

+                       if (id == mst_state->mgr->conn_base_id) { +                               aconnector = to_amdgpu_dm_connector(connector); +                               link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link); +                               drm_dp_mst_update_coding_cap(mst_state, link_coding_cap);

+                               break; +                       } +               } +               drm_connector_list_iter_end(&iter);

+       } +#endif         /**          * Streams and planes are reset when there are changes that affect          * bandwidth. Anything that affects bandwidth needs to go through diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index ad0795afc21c..fb5c47c4cb2e 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)         struct drm_dp_payload req_payload;         struct drm_dp_mst_port *port;         int i, j; -       int cur_slots = 1; +       int cur_slots = mgr->start_slot;         bool skip;

mutex_lock(&mgr->payload_lock); @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,         num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);

/* max. time slots - one slot for MTP header */ -       if (num_slots > 63) +       if (num_slots > mgr->total_avail_slots)                 return -ENOSPC;

For reasons I will explain a little further in this email, we might want to drop this…

...

return num_slots;  } @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,         int ret;

/* max. time slots - one slot for MTP header */ -       if (slots > 63) +       if (slots > mgr->total_avail_slots)

…and this

...

return -ENOSPC;

vcpi->pbn = pbn; @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,  }  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

+void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap)

Need some kdocs here

...

+{ +       if (link_coding_cap == DP_CAP_ANSI_128B132B) { +               mst_state->total_avail_slots = 64; +               mst_state->start_slot = 0; +       }

+       DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n", +                       (link_coding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b":"8b/10b", mst_state->mgr);

need to fix indenting here, and wrap this to 100 chars

...

+} +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);

/**   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel   * @mgr: manager for this port @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,

ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);         if (ret) { -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n", -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret); +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d ret=%d\n", +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-

...

total_avail_slots, ret);

drm_dp_mst_topology_put_port(port);                 goto out;         } @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,                                          struct drm_dp_mst_topology_state *mst_state)  {         struct drm_dp_vcpi_allocation *vcpi; -       int avail_slots = 63, payload_count = 0; +       int avail_slots = mgr->total_avail_slots, payload_count = 0;

list_for_each_entry(vcpi, &mst_state->vcpis, next) {                 /* Releasing VCPI is always OK-even if the port is gone */ @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,                 }         }         drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n", -                      mgr, mst_state, avail_slots, 63 - avail_slots); +                      mgr, mst_state, avail_slots, mgr->total_avail_slots - avail_slots);

return 0;  } @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state)                         break;

mutex_lock(&mgr->lock);

+               mgr->start_slot = mst_state->start_slot; +               mgr->total_avail_slots = mst_state->total_avail_slots;

this isn't correct - atomic checks aren't allowed to mutate anything besides the atomic state structure that we're checking since we don't know whether or not the display state is going to be committed when checking it. If we're storing state in mgr, that state needs to be written to mgr during the atomic commit instead of the atomic check.

...but, coming back to this MST code after being gone for a while, I think it might be time for us to stop worrying about the non-atomic state. Especially since there's only one driver using it; the legacy radeon.ko; and right now the plan is either to just drop MST support on radeon.ko or move the MST code it uses into radeon.ko.Which brings me to say - I think we can drop some of the hunks I mentioned above (e.g. the changes to drm_dp_init_vcpi and drm_dp_find_vcpi_slots I mentioned above). We can then just update the kdocs for these functions in a separate patch to clarify that now in addition to being deprecated, these functions are just broken and will eventually be removed.

So - doing that allows us to get rid of mgr->total_avail_slots and mgr-

...

start_slot entirely, just set the slot info in the atomic state during atomic

check, and then just rely on the atomic state for drm_dp_atomic_find_vcpi_slots() and friends. Which seems much simpler to me, does this sound alrght with you?

Thanks for the response,

That function is per port so not sure how that will work. Also we only need to check this inside drm_dp_mst_atomic_check_vcpi_alloc_limit(), which doesn't have a state.

We could add the slots(or some DP version indicator) inside the drm_connector, and add a parameter to drm_dp_mst_atomic_check_vcpi_alloc_limit(int slots)? and call it with this info via drm_dp_mst_atomic_check() and then update the mgr->slot in commit.

Bhawan

...

ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-

...

mst_primary,

mst_state);                 mutex_unlock(&mgr->lock); @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,         if (!mgr->proposed_vcpis)                 return -ENOMEM;         set_bit(0, &mgr->payload_mask); +       mgr->total_avail_slots = 63; +       mgr->start_slot = 1;

mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);         if (mst_state == NULL)                 return -ENOMEM;

+       mst_state->total_avail_slots = 63; +       mst_state->start_slot = 1;

mst_state->mgr = mgr;         INIT_LIST_HEAD(&mst_state->vcpis);

diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index ddb9231d0309..f8152dfb34ed 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {         struct drm_private_state base;         struct list_head vcpis;         struct drm_dp_mst_topology_mgr *mgr; +       u8 total_avail_slots; +       u8 start_slot;  };

#define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base) @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {          */         int pbn_div;

+       /** +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b +        */ +       u8 total_avail_slots;

+       /** +        * @start_slot: 1 for 8b/10b, 0 for 128/132b +        */ +       u8 start_slot;

/**          * @funcs: Atomic helper callbacks          */ @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp

void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);

+void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap);

void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,                                 struct drm_dp_mst_port *port);

I understand the mst_state argument its just that most of the mst functions are using mst_mgr/port structs and there is no easy way to extract the mst_state using mgr/port in these places (drm_dp_update_payload_part1, drm_dp_mst_allocate_vcpi, drm_dp_init_vcpi etc) where we need the slot info.

So either we need to keep a copy of the slots in the mgr because that's what most of the code is using right now or pass around the atomic state to get the mgr->state mapping. (I don't have much experience with the mst code so maybe I am missing some key detail here?)

Thanks,

Bhawan

On 2021-10-15 4:41 p.m., Lyude Paul wrote:

[more snip]

On Fri, 2021-10-15 at 15:43 -0400, Bhawanpreet Lakha wrote:

...

Thanks for the response,

That function is per port so not sure how that will work. Also we only need to check this inside drm_dp_mst_atomic_check_vcpi_alloc_limit(), which doesn't have a state.

We could add the slots(or some DP version indicator) inside the drm_connector, and add a parameter to drm_dp_mst_atomic_check_vcpi_alloc_limit(int slots)? and call it with this info via drm_dp_mst_atomic_check() and then update the mgr->slot in commit.

TBH - I think we can actually just get away with having all of this info in drm_dp_mst_topology_state

...

Bhawan

...

...

ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-

...

mst_primary,

mst_state);                  mutex_unlock(&mgr->lock); @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,          if (!mgr->proposed_vcpis)                  return -ENOMEM;          set_bit(0, &mgr->payload_mask); +       mgr->total_avail_slots = 63; +       mgr->start_slot = 1;

mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);          if (mst_state == NULL)                  return -ENOMEM;

+       mst_state->total_avail_slots = 63; +       mst_state->start_slot = 1;

mst_state->mgr = mgr;          INIT_LIST_HEAD(&mst_state->vcpis);

diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index ddb9231d0309..f8152dfb34ed 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {          struct drm_private_state base;          struct list_head vcpis;          struct drm_dp_mst_topology_mgr *mgr; +       u8 total_avail_slots; +       u8 start_slot;   };

#define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base) @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {           */          int pbn_div;

+       /** +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b +        */ +       u8 total_avail_slots;

+       /** +        * @start_slot: 1 for 8b/10b, 0 for 128/132b +        */ +       u8 start_slot;

/**           * @funcs: Atomic helper callbacks           */ @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp

void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);

+void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap);

void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,                                  struct drm_dp_mst_port *port);

8b/10b encoding format requires to reserve the first slot for recording metadata. Real data transmission starts from the second slot, with a total of available 63 slots available.

In 128b/132b encoding format, metadata is transmitted separately in LLCP packet before MTP. Real data transmission starts from the first slot, with a total of 64 slots available.

v2: * Move total/start slots to mst_state, and copy it to mst_mgr in atomic_check

v3: * Only keep the slot info on the mst_state * add a start_slot parameter to the payload function, to facilitate non atomic drivers (this is a temporary workaround and should be removed when we are moving out the non atomic driver helpers)

Signed-off-by: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com Signed-off-by: Fangzhi Zuo Jerry.Zuo@amd.com --- .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +- drivers/gpu/drm/drm_dp_mst_topology.c | 34 ++++++++++++++++--- drivers/gpu/drm/i915/display/intel_dp_mst.c | 4 +-- drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- drivers/gpu/drm/radeon/radeon_dp_mst.c | 4 +-- include/drm/drm_dp_mst_helper.h | 5 ++- 6 files changed, 40 insertions(+), 11 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 ff0f91c93ba4..6169488e2011 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 @@ -251,7 +251,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table( }

/* It's OK for this to fail */ - drm_dp_update_payload_part1(mst_mgr); + drm_dp_update_payload_part1(mst_mgr, 1);

/* mst_mgr->->payloads are VC payload notify MST branch using DPCD or * AUX message. The sequence is slot 1-63 allocated sequence for each diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 5ab3b3a46e89..d188a5269070 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3353,6 +3353,9 @@ static int drm_dp_destroy_payload_step2(struct drm_dp_mst_topology_mgr *mgr, /** * drm_dp_update_payload_part1() - Execute payload update part 1 * @mgr: manager to use. + * @start_slot: this is the cur slot + * NOTE: start_slot is a temporary workaround for non-atomic drivers, + * this will be removed when non-atomic mst helpers are moved out of the helper * * This iterates over all proposed virtual channels, and tries to * allocate space in the link for them. For 0->slots transitions, @@ -3363,12 +3366,12 @@ static int drm_dp_destroy_payload_step2(struct drm_dp_mst_topology_mgr *mgr, * after calling this the driver should generate ACT and payload * packets. */ -int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) +int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr, int start_slot) { struct drm_dp_payload req_payload; struct drm_dp_mst_port *port; int i, j; - int cur_slots = 1; + int cur_slots = start_slot; bool skip;

mutex_lock(&mgr->payload_lock); @@ -4503,6 +4506,26 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, } EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

+/** + * drm_dp_mst_update_slots() - updates the slot info depending on the DP ecoding format + * @mst_state: mst_state to update + * @link_ecoding_cap: the ecoding format on the link + */ +void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_t link_ecoding_cap) +{ + if (link_ecoding_cap == DP_CAP_ANSI_128B132B) { + mst_state->total_avail_slots = 64; + mst_state->start_slot = 0; + } else { + mst_state->total_avail_slots = 63; + mst_state->start_slot = 1; + } + + DRM_DEBUG_KMS("%s ecoding format on mst_state 0x%p\n", + (link_ecoding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b":"8b/10b", mst_state->mgr); +} +EXPORT_SYMBOL(drm_dp_mst_update_slots); + /** * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel * @mgr: manager for this port @@ -5222,7 +5245,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_topology_state *mst_state) { struct drm_dp_vcpi_allocation *vcpi; - int avail_slots = 63, payload_count = 0; + int avail_slots = mst_state->total_avail_slots, payload_count = 0;

list_for_each_entry(vcpi, &mst_state->vcpis, next) { /* Releasing VCPI is always OK-even if the port is gone */ @@ -5251,7 +5274,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr, } } drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n", - mgr, mst_state, avail_slots, 63 - avail_slots); + mgr, mst_state, avail_slots, mst_state->total_avail_slots - avail_slots);

return 0; } @@ -5528,6 +5551,9 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, if (mst_state == NULL) return -ENOMEM;

+ mst_state->total_avail_slots = 63; + mst_state->start_slot = 1; + mst_state->mgr = mgr; INIT_LIST_HEAD(&mst_state->vcpis);

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index b170e272bdee..d3a24189a12c 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -368,7 +368,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,

drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, connector->port);

- ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); + ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr, 1); if (ret) { drm_dbg_kms(&i915->drm, "failed to update payload %d\n", ret); } @@ -516,7 +516,7 @@ static void intel_mst_pre_enable_dp(struct intel_atomic_state *state,

intel_dp->active_mst_links++;

- ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); + ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr, 1);

/* * Before Gen 12 this is not done as part of diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index f949767698fc..6c8c59c26dbf 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1413,7 +1413,7 @@ nv50_mstm_prepare(struct nv50_mstm *mstm) int ret;

NV_ATOMIC(drm, "%s: mstm prepare\n", mstm->outp->base.base.name); - ret = drm_dp_update_payload_part1(&mstm->mgr); + ret = drm_dp_update_payload_part1(&mstm->mgr, 1);

drm_for_each_encoder(encoder, mstm->outp->base.base.dev) { if (encoder->encoder_type == DRM_MODE_ENCODER_DPMST) { diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c index ec867fa880a4..751c2c075e09 100644 --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c @@ -423,7 +423,7 @@ radeon_mst_encoder_dpms(struct drm_encoder *encoder, int mode) drm_dp_mst_allocate_vcpi(&radeon_connector->mst_port->mst_mgr, radeon_connector->port, mst_enc->pbn, slots); - drm_dp_update_payload_part1(&radeon_connector->mst_port->mst_mgr); + drm_dp_update_payload_part1(&radeon_connector->mst_port->mst_mgr, 1);

radeon_dp_mst_set_be_cntl(primary, mst_enc, radeon_connector->mst_port->hpd.hpd, true); @@ -452,7 +452,7 @@ radeon_mst_encoder_dpms(struct drm_encoder *encoder, int mode) return;

drm_dp_mst_reset_vcpi_slots(&radeon_connector->mst_port->mst_mgr, mst_enc->port); - drm_dp_update_payload_part1(&radeon_connector->mst_port->mst_mgr); + drm_dp_update_payload_part1(&radeon_connector->mst_port->mst_mgr, 1);

drm_dp_check_act_status(&radeon_connector->mst_port->mst_mgr); /* and this can also fail */ diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index ddb9231d0309..3207b49586fc 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state { struct drm_private_state base; struct list_head vcpis; struct drm_dp_mst_topology_mgr *mgr; + u8 total_avail_slots; + u8 start_slot; };

#define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base) @@ -806,6 +808,7 @@ int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp

void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);

+void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_t link_ecoding_cap);

void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port); @@ -815,7 +818,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, int pbn);

-int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); +int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr, int start_slot);

int drm_dp_update_payload_part2(struct drm_dp_mst_topology_mgr *mgr);

(sorry - I just wrote a second response to your email I meant to send to your coworker Jerry Zuo! You can ignore it)

On Tue, 2021-08-31 at 19:44 +0000, Zuo, Jerry wrote:

[AMD Official Use Only]

...

-----Original Message----- From: Lyude Paul lyude@redhat.com Sent: August 30, 2021 4:00 PM To: Zuo, Jerry Jerry.Zuo@amd.com; dri-devel@lists.freedesktop.org Cc: Wentland, Harry Harry.Wentland@amd.com; Kazlauskas, Nicholas Nicholas.Kazlauskas@amd.com; Lin, Wayne Wayne.Lin@amd.com Subject: Re: [PATCH 1/2] drm: Update MST First Link Slot Information Based on Encoding Format

On Fri, 2021-08-27 at 19:43 -0400, Fangzhi Zuo wrote:

...

8b/10b encoding format requires to reserve the first slot for recording metadata. Real data transmission starts from the second slot, with a total of available 63 slots available.

In 128b/132b encoding format, metadata is transmitted separately in LLCP packet before MTP. Real data transmission starts from the first slot, with a total of 64 slots available.

Signed-off-by: Fangzhi Zuo Jerry.Zuo@amd.com

drivers/gpu/drm/drm_dp_mst_topology.c | 27 ++++++++++++++++++++-------  include/drm/drm_dp_mst_helper.h       |  9 +++++++++  2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 86d13d6bc463..30544801d2b8 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3370,7 +3370,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)         struct drm_dp_payload req_payload;         struct drm_dp_mst_port *port;         int i, j; -       int cur_slots = 1; +       int cur_slots = mgr->start_slot;         bool skip;

mutex_lock(&mgr->payload_lock); @@ -4323,7 +4323,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,         num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);

/* max. time slots - one slot for MTP header */ -       if (num_slots > 63) +       if (num_slots > mgr->total_avail_slots)                 return -ENOSPC;         return num_slots;  } @@ -4335,7 +4335,7 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,         int ret;

/* max. time slots - one slot for MTP header */ -       if (slots > 63) +       if (slots > mgr->total_avail_slots)                 return -ENOSPC;

vcpi->pbn = pbn; @@ -4509,6 +4509,17 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,  }  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

+void drm_dp_mst_update_encoding_cap(struct

drm_dp_mst_topology_mgr

...

+*mgr, uint8_t link_encoding_cap) +{ +       if (link_encoding_cap == DP_CAP_ANSI_128B132B) { +               mgr->total_avail_slots = 64; +               mgr->start_slot = 0; +       } +       DRM_DEBUG_KMS("%s encoding format determined\n", +                     (link_encoding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b" : "8b/10b"); +} +EXPORT_SYMBOL(drm_dp_mst_update_encoding_cap);

This seems to be missing kdocs, can you fix that?

Also - I'm not convinced this is all of the work we have to do, as there's no locking taking place here in this function. If we're changing the number of available VCPI slots that we have, we need to be able to factor that into the atomic check logic which means that we can't rely on mgr->* for any kind of data that isn't guaranteed to remain consistent throughout the lifetime of the driver or topology. (Note that some of the old MST code didn't follow this logic, so I wouldn't be surprised if there's still exceptions to this we need to clean up).

Note that I still expect we'll have to keep some sort of track of the current total slot count in the topology mgr, but that should be reflecting the currently programmed state and not be relied on from our atomic check.

Thanks Lyude for your comments.

Seems I should keep existing code to keep track of current slot status in mgr. That information is getting updated each time when topology change detected. That slot information saved in mgr is a sort of static, and could only be used for debug purpose to track what is the current encoding format.

...

IMHO - the correct way we should go about adding support for this is to add something into drm_dp_mst_topology_state and integrate this into the atomic check helpers.

The slot information should also be added into drm_dp_mst_topology_state to reflect the real-time slot status.

I'd like to confirm the best place to get slot count info. updated. Should the update be done within &drm_mode_config_funcs. atomic_check(), before new stream is created, OR should be updated within drm_dp_mst_atomic_check() ?

The updated slot count will be used in drm_dp_mst_atomic_check() to check slot limit, and in drm_dp_update_payload_part1() as initial cur_slots.

...

...

/**   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel   * @mgr: manager for this port @@ -4540,8 +4551,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,

ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);         if (ret) { -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n", -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret); +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d +max=%d ret=%d\n", +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-

...

total_avail_slots, ret);

drm_dp_mst_topology_put_port(port);                 goto out;         } @@ -5228,7 +5239,7 @@

drm_dp_mst_atomic_check_vcpi_alloc_limit(struct

...

drm_dp_mst_topology_mgr *mgr,                                          struct drm_dp_mst_topology_state *mst_state)  {         struct drm_dp_vcpi_allocation *vcpi; -       int avail_slots = 63, payload_count = 0; +       int avail_slots = mgr->total_avail_slots, payload_count = 0;

list_for_each_entry(vcpi, &mst_state->vcpis, next) {                 /* Releasing VCPI is always OK-even if the port is gone */ @@ -5257,7 +5268,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,                 }         }         drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n", -                      mgr, mst_state, avail_slots, 63 - avail_slots); +                      mgr, mst_state, avail_slots, +mgr->total_avail_slots - avail_slots);

return 0;  } @@ -5529,6 +5540,8 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,         if (!mgr->proposed_vcpis)                 return -ENOMEM;         set_bit(0, &mgr->payload_mask); +       mgr->total_avail_slots = 63; +       mgr->start_slot = 1;

mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);         if (mst_state == NULL) diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index ddb9231d0309..eac5ce48f214 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -661,6 +661,15 @@ struct drm_dp_mst_topology_mgr {          */         int pbn_div;

+       /** +        * @total_avail_slots: available slots for data transmission +        */ +       u8 total_avail_slots; +       /** +        * @start_slot: first slot index for data transmission +        */ +       u8 start_slot;

/**          * @funcs: Atomic helper callbacks          */

-- Cheers,  Lyude Paul (she/her)  Software Engineer at Red Hat