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@xxxxxxx>
> ---
> 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
> */
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
