[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 @@ intdrm_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=%dmax=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 isgone */ @@ -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