[Public]

________________________________________

From: Lyude Paul lyude@redhat.com Sent: Wednesday, June 16, 2021 03:44 To: Lin, Wayne; dri-devel@lists.freedesktop.org Cc: Kazlauskas, Nicholas; Wentland, Harry; Zuo, Jerry; Pillai, Aurabindo Subject: Re: [PATCH 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology

On Fri, 2021-05-28 at 21:55 +0800, Wayne Lin wrote:

...

[Why] After unplug/hotplug hub from the system, userspace might start to clear stale payloads gradually. If we call drm_dp_mst_deallocate_vcpi() to release stale VCPI of those ports which are not relating to current topology, we have chane to wrongly clear active payload table entry for current topology.

E.g. We have allocated VCPI 1 in current payload table and we call drm_dp_mst_deallocate_vcpi() to clean VCPI 1 in stale topology. In drm_dp_mst_deallocate_vcpi(), it will call drm_dp_mst_put_payload_id() tp put VCPI 1 and which means ID 1 is available again. Thereafter, if we want to allocate a new payload stream, it will find ID 1 is available by drm_dp_mst_assign_payload_id(). However, ID 1 is being used

[How] Check target sink is relating to current topology or not before doing any payload table update. Searching upward to find the target sink's relevant root branch device. If the found root branch device is not the same root of current topology, don't update payload table.

Signed-off-by: Wayne Lin Wayne.Lin@amd.com

drivers/gpu/drm/drm_dp_mst_topology.c | 29 +++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 5fc261014a20..3d988d54df89 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -94,6 +94,9 @@ static int drm_dp_mst_register_i2c_bus(struct drm_dp_mst_port *port); static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_mst_port *port); static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr);

+static bool drm_dp_mst_port_downstream_of_branch(struct drm_dp_mst_port *port,

•                                            struct drm_dp_mst_branch
  

*branch);

#define DBG_PREFIX "[dp_mst]"

#define DP_STR(x) [DP_ ## x] = #x @@ -3360,6 +3363,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) struct drm_dp_mst_port *port; int i, j; int cur_slots = 1;

•   bool skip;
  
    mutex_lock(&mgr->payload_lock);
    for (i = 0; i < mgr->max_payloads; i++) {
  

@@ -3374,6 +3378,14 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) port = container_of(vcpi, struct drm_dp_mst_port, vcpi);

•                   mutex_lock(&mgr->lock);
  

•                   skip = !drm_dp_mst_port_downstream_of_branch(port,
  

mgr->mst_primary);

•                   mutex_unlock(&mgr->lock);
  

•                   if (skip) {
  

•                           DRM_DEBUG_KMS("Virtual channel %d is not in
  

current topology\n", i);

sorry I totally missed this on my first look - but this is the wrong debugging macro (drm_dbg_kms() should be used) and as well, this patch doesn't actually apply because it needs to be rebased against drm-tip. Could you fix this, rebase the patches, and send a new version along with the fixes tags I mentioned earlier? You can generate them using the dim tool here:

https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-intel.html#...

Thanks for your time! Will do.

...

•                           continue;
  

•                   }
                    /* Validated ports don't matter if we're releasing
                     * VCPI
                     */
  

@@ -3473,6 +3485,7 @@ int drm_dp_update_payload_part2(struct drm_dp_mst_topology_mgr *mgr) struct drm_dp_mst_port *port; int i; int ret = 0;

•   bool skip;
  
    mutex_lock(&mgr->payload_lock);
    for (i = 0; i < mgr->max_payloads; i++) {
  

@@ -3482,6 +3495,13 @@ int drm_dp_update_payload_part2(struct drm_dp_mst_topology_mgr *mgr)

            port = container_of(mgr->proposed_vcpis[i], struct

drm_dp_mst_port, vcpi);

•           mutex_lock(&mgr->lock);
  

•           skip = !drm_dp_mst_port_downstream_of_branch(port, mgr-
  

...

mst_primary);

•           mutex_unlock(&mgr->lock);
  

•           if (skip)
  

•                   continue;
  

•           DRM_DEBUG_KMS("payload %d %d\n", i, mgr-
  

...

payloads[i].payload_state);

            if (mgr->payloads[i].payload_state == DP_PAYLOAD_LOCAL) {
                    ret = drm_dp_create_payload_step2(mgr, port, mgr-

...

proposed_vcpis[i]->vcpi, &mgr->payloads[i]);

@@ -4562,9 +4582,18 @@ EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots); void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port) {

•   bool skip;
  

•   if (!port->vcpi.vcpi)
            return;
  
  

•   mutex_lock(&mgr->lock);
  

•   skip = !drm_dp_mst_port_downstream_of_branch(port, mgr->mst_primary);
  

•   mutex_unlock(&mgr->lock);
  

•   if (skip)
  

•           return;
  

•   drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi);
    port->vcpi.num_slots = 0;
    port->vcpi.pbn = 0;
  

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

-- Regards, Wayne Lin