* [PATCH v2 0/2] Fix observed mst problems with StarTech hub
@ 2021-06-16 3:54 Wayne Lin
2021-06-16 3:55 ` Wayne Lin
2021-06-16 3:55 ` Wayne Lin
0 siblings, 2 replies; 12+ messages in thread
From: Wayne Lin @ 2021-06-16 3:54 UTC (permalink / raw)
To: dri-devel; +Cc: jerry.zuo, aurabindo.pillai, Wayne Lin, Nicholas.Kazlauskas
Use Startech 1to3 DP hub to do some mst hotplug tests and find some
light up issues.
1. ACT polling timeout:
Which is due to we didn't update DPCD payload table but still try
to send ACT and polling for "ACT Handled" bit
2. Not all monitors light up:
Due to we wrongly set unavailable VCP ID for new streams
Changes since v1:
* Add appropriate tags: Fixes & Cc
* Change debug macro to use drm_dbg_kms() instead
Wayne Lin (2):
drm/dp_mst: Do not set proposed vcpi directly
drm/dp_mst: Avoid to mess up payload table by ports in stale topology
drivers/gpu/drm/drm_dp_mst_topology.c | 65 ++++++++++++++++-----------
1 file changed, 39 insertions(+), 26 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly
2021-06-16 3:54 [PATCH v2 0/2] Fix observed mst problems with StarTech hub Wayne Lin
@ 2021-06-16 3:55 ` Wayne Lin
2021-06-16 3:55 ` Wayne Lin
1 sibling, 0 replies; 12+ messages in thread
From: Wayne Lin @ 2021-06-16 3:55 UTC (permalink / raw)
To: dri-devel
Cc: lyude, Nicholas.Kazlauskas, harry.wentland, jerry.zuo,
aurabindo.pillai, Wayne Lin, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, stable
[Why]
When we receive CSN message to notify one port is disconnected, we will
implicitly set its corresponding num_slots to 0. Later on, we will
eventually call drm_dp_update_payload_part1() to arrange down streams.
In drm_dp_update_payload_part1(), we iterate over all proposed_vcpis[]
to do the update. Not specific to a target sink only. For example, if we
light up 2 monitors, Monitor_A and Monitor_B, and then we unplug
Monitor_B. Later on, when we call drm_dp_update_payload_part1() to try
to update payload for Monitor_A, we'll also implicitly clean payload for
Monitor_B at the same time. And finally, when we try to call
drm_dp_update_payload_part1() to clean payload for Monitor_B, we will do
nothing at this time since payload for Monitor_B has been cleaned up
previously.
For StarTech 1to3 DP hub, it seems like if we didn't update DPCD payload
ID table then polling for "ACT Handled"(BIT_1 of DPCD 002C0h) will fail
and this polling will last for 3 seconds.
Therefore, guess the best way is we don't set the proposed_vcpi[]
diretly. Let user of these herlper functions to set the proposed_vcpi
directly.
[How]
1. Revert commit 7617e9621bf2 ("drm/dp_mst: clear time slots for ports
invalid")
2. Tackle the issue in previous commit by skipping those trasient
proposed VCPIs. These stale VCPIs shoulde be explicitly cleared by
user later on.
Changes since v1:
* Change debug macro to use drm_dbg_kms() instead
* Amend the commit message to add Fixed & Cc tags
Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
Fixes: 7617e9621bf2 ("drm/dp_mst: clear time slots for ports invalid")
Cc: Lyude Paul <lyude@redhat.com>
Cc: Wayne Lin <Wayne.Lin@amd.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.5+
---
drivers/gpu/drm/drm_dp_mst_topology.c | 36 ++++++++-------------------
1 file changed, 10 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 32b7f8983b94..b41b837db66d 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2501,7 +2501,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
{
struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
struct drm_dp_mst_port *port;
- int old_ddps, old_input, ret, i;
+ int old_ddps, ret;
u8 new_pdt;
bool new_mcs;
bool dowork = false, create_connector = false;
@@ -2533,7 +2533,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
}
old_ddps = port->ddps;
- old_input = port->input;
port->input = conn_stat->input_port;
port->ldps = conn_stat->legacy_device_plug_status;
port->ddps = conn_stat->displayport_device_plug_status;
@@ -2555,28 +2554,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
dowork = false;
}
- if (!old_input && old_ddps != port->ddps && !port->ddps) {
- for (i = 0; i < mgr->max_payloads; i++) {
- struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i];
- struct drm_dp_mst_port *port_validated;
-
- if (!vcpi)
- continue;
-
- port_validated =
- container_of(vcpi, struct drm_dp_mst_port, vcpi);
- port_validated =
- drm_dp_mst_topology_get_port_validated(mgr, port_validated);
- if (!port_validated) {
- mutex_lock(&mgr->payload_lock);
- vcpi->num_slots = 0;
- mutex_unlock(&mgr->payload_lock);
- } else {
- drm_dp_mst_topology_put_port(port_validated);
- }
- }
- }
-
if (port->connector)
drm_modeset_unlock(&mgr->base.lock);
else if (create_connector)
@@ -3410,8 +3387,15 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
port = drm_dp_mst_topology_get_port_validated(
mgr, port);
if (!port) {
- mutex_unlock(&mgr->payload_lock);
- return -EINVAL;
+ if (vcpi->num_slots == payload->num_slots) {
+ cur_slots += vcpi->num_slots;
+ payload->start_slot = req_payload.start_slot;
+ continue;
+ } else {
+ drm_dbg_kms("Fail:set payload to invalid sink");
+ mutex_unlock(&mgr->payload_lock);
+ return -EINVAL;
+ }
}
put_port = true;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly
@ 2021-06-16 3:55 ` Wayne Lin
0 siblings, 0 replies; 12+ messages in thread
From: Wayne Lin @ 2021-06-16 3:55 UTC (permalink / raw)
To: dri-devel
Cc: jerry.zuo, aurabindo.pillai, stable, Thomas Zimmermann,
Wayne Lin, Nicholas.Kazlauskas
[Why]
When we receive CSN message to notify one port is disconnected, we will
implicitly set its corresponding num_slots to 0. Later on, we will
eventually call drm_dp_update_payload_part1() to arrange down streams.
In drm_dp_update_payload_part1(), we iterate over all proposed_vcpis[]
to do the update. Not specific to a target sink only. For example, if we
light up 2 monitors, Monitor_A and Monitor_B, and then we unplug
Monitor_B. Later on, when we call drm_dp_update_payload_part1() to try
to update payload for Monitor_A, we'll also implicitly clean payload for
Monitor_B at the same time. And finally, when we try to call
drm_dp_update_payload_part1() to clean payload for Monitor_B, we will do
nothing at this time since payload for Monitor_B has been cleaned up
previously.
For StarTech 1to3 DP hub, it seems like if we didn't update DPCD payload
ID table then polling for "ACT Handled"(BIT_1 of DPCD 002C0h) will fail
and this polling will last for 3 seconds.
Therefore, guess the best way is we don't set the proposed_vcpi[]
diretly. Let user of these herlper functions to set the proposed_vcpi
directly.
[How]
1. Revert commit 7617e9621bf2 ("drm/dp_mst: clear time slots for ports
invalid")
2. Tackle the issue in previous commit by skipping those trasient
proposed VCPIs. These stale VCPIs shoulde be explicitly cleared by
user later on.
Changes since v1:
* Change debug macro to use drm_dbg_kms() instead
* Amend the commit message to add Fixed & Cc tags
Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
Fixes: 7617e9621bf2 ("drm/dp_mst: clear time slots for ports invalid")
Cc: Lyude Paul <lyude@redhat.com>
Cc: Wayne Lin <Wayne.Lin@amd.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.5+
---
drivers/gpu/drm/drm_dp_mst_topology.c | 36 ++++++++-------------------
1 file changed, 10 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 32b7f8983b94..b41b837db66d 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2501,7 +2501,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
{
struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
struct drm_dp_mst_port *port;
- int old_ddps, old_input, ret, i;
+ int old_ddps, ret;
u8 new_pdt;
bool new_mcs;
bool dowork = false, create_connector = false;
@@ -2533,7 +2533,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
}
old_ddps = port->ddps;
- old_input = port->input;
port->input = conn_stat->input_port;
port->ldps = conn_stat->legacy_device_plug_status;
port->ddps = conn_stat->displayport_device_plug_status;
@@ -2555,28 +2554,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
dowork = false;
}
- if (!old_input && old_ddps != port->ddps && !port->ddps) {
- for (i = 0; i < mgr->max_payloads; i++) {
- struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i];
- struct drm_dp_mst_port *port_validated;
-
- if (!vcpi)
- continue;
-
- port_validated =
- container_of(vcpi, struct drm_dp_mst_port, vcpi);
- port_validated =
- drm_dp_mst_topology_get_port_validated(mgr, port_validated);
- if (!port_validated) {
- mutex_lock(&mgr->payload_lock);
- vcpi->num_slots = 0;
- mutex_unlock(&mgr->payload_lock);
- } else {
- drm_dp_mst_topology_put_port(port_validated);
- }
- }
- }
-
if (port->connector)
drm_modeset_unlock(&mgr->base.lock);
else if (create_connector)
@@ -3410,8 +3387,15 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
port = drm_dp_mst_topology_get_port_validated(
mgr, port);
if (!port) {
- mutex_unlock(&mgr->payload_lock);
- return -EINVAL;
+ if (vcpi->num_slots == payload->num_slots) {
+ cur_slots += vcpi->num_slots;
+ payload->start_slot = req_payload.start_slot;
+ continue;
+ } else {
+ drm_dbg_kms("Fail:set payload to invalid sink");
+ mutex_unlock(&mgr->payload_lock);
+ return -EINVAL;
+ }
}
put_port = true;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology
2021-06-16 3:54 [PATCH v2 0/2] Fix observed mst problems with StarTech hub Wayne Lin
@ 2021-06-16 3:55 ` Wayne Lin
2021-06-16 3:55 ` Wayne Lin
1 sibling, 0 replies; 12+ messages in thread
From: Wayne Lin @ 2021-06-16 3:55 UTC (permalink / raw)
To: dri-devel
Cc: lyude, Nicholas.Kazlauskas, harry.wentland, jerry.zuo,
aurabindo.pillai, Wayne Lin, stable
[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.
Changes since v1:
* Change debug macro to use drm_dbg_kms() instead
* Amend the commit message to add Cc tag.
Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
Cc: stable@vger.kernel.org
---
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 b41b837db66d..9ac148efd9e4 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
@@ -3366,6 +3369,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++) {
@@ -3380,6 +3384,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_dbg_kms("Virtual channel %d is not in current topology\n", i);
+ continue;
+ }
/* Validated ports don't matter if we're releasing
* VCPI
*/
@@ -3479,6 +3491,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++) {
@@ -3488,6 +3501,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_dbg_kms(mgr->dev, "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]);
@@ -4574,9 +4594,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;
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology
@ 2021-06-16 3:55 ` Wayne Lin
0 siblings, 0 replies; 12+ messages in thread
From: Wayne Lin @ 2021-06-16 3:55 UTC (permalink / raw)
To: dri-devel
Cc: stable, jerry.zuo, aurabindo.pillai, Wayne Lin, Nicholas.Kazlauskas
[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.
Changes since v1:
* Change debug macro to use drm_dbg_kms() instead
* Amend the commit message to add Cc tag.
Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
Cc: stable@vger.kernel.org
---
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 b41b837db66d..9ac148efd9e4 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
@@ -3366,6 +3369,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++) {
@@ -3380,6 +3384,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_dbg_kms("Virtual channel %d is not in current topology\n", i);
+ continue;
+ }
/* Validated ports don't matter if we're releasing
* VCPI
*/
@@ -3479,6 +3491,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++) {
@@ -3488,6 +3501,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_dbg_kms(mgr->dev, "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]);
@@ -4574,9 +4594,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;
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology
2021-06-16 3:55 ` Wayne Lin
@ 2021-06-16 19:26 ` Souza, Jose
-1 siblings, 0 replies; 12+ messages in thread
From: Souza, Jose @ 2021-06-16 19:26 UTC (permalink / raw)
To: dri-devel, Wayne.Lin, lyude
Cc: stable, aurabindo.pillai, jerry.zuo, Nicholas.Kazlauskas
On Wed, 2021-06-16 at 11: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.
>
> Changes since v1:
> * Change debug macro to use drm_dbg_kms() instead
> * Amend the commit message to add Cc tag.
>
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> Cc: stable@vger.kernel.org
> ---
> 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 b41b837db66d..9ac148efd9e4 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
> @@ -3366,6 +3369,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++) {
> @@ -3380,6 +3384,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_dbg_kms("Virtual channel %d is not in current topology\n", i);
Missing dev/drm parameter and breaking the build.
> + continue;
> + }
> /* Validated ports don't matter if we're releasing
> * VCPI
> */
> @@ -3479,6 +3491,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++) {
> @@ -3488,6 +3501,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_dbg_kms(mgr->dev, "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]);
> @@ -4574,9 +4594,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;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology
@ 2021-06-16 19:26 ` Souza, Jose
0 siblings, 0 replies; 12+ messages in thread
From: Souza, Jose @ 2021-06-16 19:26 UTC (permalink / raw)
To: dri-devel, Wayne.Lin, lyude
Cc: jerry.zuo, aurabindo.pillai, Nicholas.Kazlauskas, stable
On Wed, 2021-06-16 at 11: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.
>
> Changes since v1:
> * Change debug macro to use drm_dbg_kms() instead
> * Amend the commit message to add Cc tag.
>
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> Cc: stable@vger.kernel.org
> ---
> 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 b41b837db66d..9ac148efd9e4 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
> @@ -3366,6 +3369,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++) {
> @@ -3380,6 +3384,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_dbg_kms("Virtual channel %d is not in current topology\n", i);
Missing dev/drm parameter and breaking the build.
> + continue;
> + }
> /* Validated ports don't matter if we're releasing
> * VCPI
> */
> @@ -3479,6 +3491,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++) {
> @@ -3488,6 +3501,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_dbg_kms(mgr->dev, "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]);
> @@ -4574,9 +4594,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;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly
2021-06-16 3:55 ` Wayne Lin
@ 2021-06-16 19:53 ` Harry Wentland
-1 siblings, 0 replies; 12+ messages in thread
From: Harry Wentland @ 2021-06-16 19:53 UTC (permalink / raw)
To: Wayne Lin, dri-devel
Cc: lyude, Nicholas.Kazlauskas, jerry.zuo, aurabindo.pillai,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, stable
On 2021-06-15 11:55 p.m., Wayne Lin wrote:
> [Why]
> When we receive CSN message to notify one port is disconnected, we will
> implicitly set its corresponding num_slots to 0. Later on, we will
> eventually call drm_dp_update_payload_part1() to arrange down streams.
>
> In drm_dp_update_payload_part1(), we iterate over all proposed_vcpis[]
> to do the update. Not specific to a target sink only. For example, if we
> light up 2 monitors, Monitor_A and Monitor_B, and then we unplug
> Monitor_B. Later on, when we call drm_dp_update_payload_part1() to try
> to update payload for Monitor_A, we'll also implicitly clean payload for
> Monitor_B at the same time. And finally, when we try to call
> drm_dp_update_payload_part1() to clean payload for Monitor_B, we will do
> nothing at this time since payload for Monitor_B has been cleaned up
> previously.
>
> For StarTech 1to3 DP hub, it seems like if we didn't update DPCD payload
> ID table then polling for "ACT Handled"(BIT_1 of DPCD 002C0h) will fail
> and this polling will last for 3 seconds.
>
> Therefore, guess the best way is we don't set the proposed_vcpi[]
> diretly. Let user of these herlper functions to set the proposed_vcpi
> directly.
>
> [How]
> 1. Revert commit 7617e9621bf2 ("drm/dp_mst: clear time slots for ports
> invalid")
> 2. Tackle the issue in previous commit by skipping those trasient
> proposed VCPIs. These stale VCPIs shoulde be explicitly cleared by
> user later on.
>
> Changes since v1:
> * Change debug macro to use drm_dbg_kms() instead
> * Amend the commit message to add Fixed & Cc tags
>
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> Fixes: 7617e9621bf2 ("drm/dp_mst: clear time slots for ports invalid")
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Wayne Lin <Wayne.Lin@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v5.5+
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 36 ++++++++-------------------
> 1 file changed, 10 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 32b7f8983b94..b41b837db66d 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2501,7 +2501,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> {
> struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> struct drm_dp_mst_port *port;
> - int old_ddps, old_input, ret, i;
> + int old_ddps, ret;
> u8 new_pdt;
> bool new_mcs;
> bool dowork = false, create_connector = false;
> @@ -2533,7 +2533,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> }
>
> old_ddps = port->ddps;
> - old_input = port->input;
> port->input = conn_stat->input_port;
> port->ldps = conn_stat->legacy_device_plug_status;
> port->ddps = conn_stat->displayport_device_plug_status;
> @@ -2555,28 +2554,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> dowork = false;
> }
>
> - if (!old_input && old_ddps != port->ddps && !port->ddps) {
> - for (i = 0; i < mgr->max_payloads; i++) {
> - struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i];
> - struct drm_dp_mst_port *port_validated;
> -
> - if (!vcpi)
> - continue;
> -
> - port_validated =
> - container_of(vcpi, struct drm_dp_mst_port, vcpi);
> - port_validated =
> - drm_dp_mst_topology_get_port_validated(mgr, port_validated);
> - if (!port_validated) {
> - mutex_lock(&mgr->payload_lock);
> - vcpi->num_slots = 0;
> - mutex_unlock(&mgr->payload_lock);
> - } else {
> - drm_dp_mst_topology_put_port(port_validated);
> - }
> - }
> - }
> -
> if (port->connector)
> drm_modeset_unlock(&mgr->base.lock);
> else if (create_connector)
> @@ -3410,8 +3387,15 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
> port = drm_dp_mst_topology_get_port_validated(
> mgr, port);
> if (!port) {
> - mutex_unlock(&mgr->payload_lock);
> - return -EINVAL;
> + if (vcpi->num_slots == payload->num_slots) {
> + cur_slots += vcpi->num_slots;
> + payload->start_slot = req_payload.start_slot;
> + continue;
> + } else {
> + drm_dbg_kms("Fail:set payload to invalid sink");
drm_dbg_kms takes a drm_device as first parameter.
Harry
> + mutex_unlock(&mgr->payload_lock);
> + return -EINVAL;
> + }
> }
> put_port = true;
> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly
@ 2021-06-16 19:53 ` Harry Wentland
0 siblings, 0 replies; 12+ messages in thread
From: Harry Wentland @ 2021-06-16 19:53 UTC (permalink / raw)
To: Wayne Lin, dri-devel
Cc: jerry.zuo, aurabindo.pillai, stable, Thomas Zimmermann,
Nicholas.Kazlauskas
On 2021-06-15 11:55 p.m., Wayne Lin wrote:
> [Why]
> When we receive CSN message to notify one port is disconnected, we will
> implicitly set its corresponding num_slots to 0. Later on, we will
> eventually call drm_dp_update_payload_part1() to arrange down streams.
>
> In drm_dp_update_payload_part1(), we iterate over all proposed_vcpis[]
> to do the update. Not specific to a target sink only. For example, if we
> light up 2 monitors, Monitor_A and Monitor_B, and then we unplug
> Monitor_B. Later on, when we call drm_dp_update_payload_part1() to try
> to update payload for Monitor_A, we'll also implicitly clean payload for
> Monitor_B at the same time. And finally, when we try to call
> drm_dp_update_payload_part1() to clean payload for Monitor_B, we will do
> nothing at this time since payload for Monitor_B has been cleaned up
> previously.
>
> For StarTech 1to3 DP hub, it seems like if we didn't update DPCD payload
> ID table then polling for "ACT Handled"(BIT_1 of DPCD 002C0h) will fail
> and this polling will last for 3 seconds.
>
> Therefore, guess the best way is we don't set the proposed_vcpi[]
> diretly. Let user of these herlper functions to set the proposed_vcpi
> directly.
>
> [How]
> 1. Revert commit 7617e9621bf2 ("drm/dp_mst: clear time slots for ports
> invalid")
> 2. Tackle the issue in previous commit by skipping those trasient
> proposed VCPIs. These stale VCPIs shoulde be explicitly cleared by
> user later on.
>
> Changes since v1:
> * Change debug macro to use drm_dbg_kms() instead
> * Amend the commit message to add Fixed & Cc tags
>
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> Fixes: 7617e9621bf2 ("drm/dp_mst: clear time slots for ports invalid")
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Wayne Lin <Wayne.Lin@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v5.5+
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 36 ++++++++-------------------
> 1 file changed, 10 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 32b7f8983b94..b41b837db66d 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2501,7 +2501,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> {
> struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> struct drm_dp_mst_port *port;
> - int old_ddps, old_input, ret, i;
> + int old_ddps, ret;
> u8 new_pdt;
> bool new_mcs;
> bool dowork = false, create_connector = false;
> @@ -2533,7 +2533,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> }
>
> old_ddps = port->ddps;
> - old_input = port->input;
> port->input = conn_stat->input_port;
> port->ldps = conn_stat->legacy_device_plug_status;
> port->ddps = conn_stat->displayport_device_plug_status;
> @@ -2555,28 +2554,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> dowork = false;
> }
>
> - if (!old_input && old_ddps != port->ddps && !port->ddps) {
> - for (i = 0; i < mgr->max_payloads; i++) {
> - struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i];
> - struct drm_dp_mst_port *port_validated;
> -
> - if (!vcpi)
> - continue;
> -
> - port_validated =
> - container_of(vcpi, struct drm_dp_mst_port, vcpi);
> - port_validated =
> - drm_dp_mst_topology_get_port_validated(mgr, port_validated);
> - if (!port_validated) {
> - mutex_lock(&mgr->payload_lock);
> - vcpi->num_slots = 0;
> - mutex_unlock(&mgr->payload_lock);
> - } else {
> - drm_dp_mst_topology_put_port(port_validated);
> - }
> - }
> - }
> -
> if (port->connector)
> drm_modeset_unlock(&mgr->base.lock);
> else if (create_connector)
> @@ -3410,8 +3387,15 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
> port = drm_dp_mst_topology_get_port_validated(
> mgr, port);
> if (!port) {
> - mutex_unlock(&mgr->payload_lock);
> - return -EINVAL;
> + if (vcpi->num_slots == payload->num_slots) {
> + cur_slots += vcpi->num_slots;
> + payload->start_slot = req_payload.start_slot;
> + continue;
> + } else {
> + drm_dbg_kms("Fail:set payload to invalid sink");
drm_dbg_kms takes a drm_device as first parameter.
Harry
> + mutex_unlock(&mgr->payload_lock);
> + return -EINVAL;
> + }
> }
> put_port = true;
> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology
2021-06-16 3:55 ` Wayne Lin
(?)
(?)
@ 2021-06-17 4:01 ` kernel test robot
-1 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2021-06-17 4:01 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 8666 bytes --]
Hi Wayne,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-exynos/exynos-drm-next next-20210616]
[cannot apply to drm-tip/drm-tip tegra-drm/drm/tegra/for-next linus/master drm/drm-next v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Wayne-Lin/Fix-observed-mst-problems-with-StarTech-hub/20210617-053552
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: nios2-randconfig-r026-20210617 (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/a904fdf8679b7eb026e3919cd14678ba9a06c8bc
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Wayne-Lin/Fix-observed-mst-problems-with-StarTech-hub/20210617-053552
git checkout a904fdf8679b7eb026e3919cd14678ba9a06c8bc
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nios2
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/drm_dp_mst_topology.c:45:
drivers/gpu/drm/drm_dp_mst_topology.c: In function 'drm_dp_update_payload_part1':
include/drm/drm_print.h:450:27: error: request for member 'dev' in something not a structure or union
450 | drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_KMS, fmt, ##__VA_ARGS__)
| ^~
drivers/gpu/drm/drm_dp_mst_topology.c:3392:5: note: in expansion of macro 'drm_dbg_kms'
3392 | drm_dbg_kms("Virtual channel %d is not in current topology\n", i);
| ^~~~~~~~~~~
>> drivers/gpu/drm/drm_dp_mst_topology.c:3392:68: warning: passing argument 3 of 'drm_dev_dbg' makes pointer from integer without a cast [-Wint-conversion]
3392 | drm_dbg_kms("Virtual channel %d is not in current topology\n", i);
| ^
| |
| int
include/drm/drm_print.h:450:53: note: in definition of macro 'drm_dbg_kms'
450 | drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_KMS, fmt, ##__VA_ARGS__)
| ^~~
include/drm/drm_print.h:338:16: note: expected 'const char *' but argument is of type 'int'
338 | const char *format, ...);
| ~~~~~~~~~~~~^~~~~~
drivers/gpu/drm/drm_dp_mst_topology.c:3407:53: error: macro "drm_dbg_kms" requires 3 arguments, but only 1 given
3407 | drm_dbg_kms("Fail:set payload to invalid sink");
| ^
In file included from drivers/gpu/drm/drm_dp_mst_topology.c:45:
include/drm/drm_print.h:449: note: macro "drm_dbg_kms" defined here
449 | #define drm_dbg_kms(drm, fmt, ...) \
|
drivers/gpu/drm/drm_dp_mst_topology.c:3407:7: error: 'drm_dbg_kms' undeclared (first use in this function)
3407 | drm_dbg_kms("Fail:set payload to invalid sink");
| ^~~~~~~~~~~
drivers/gpu/drm/drm_dp_mst_topology.c:3407:7: note: each undeclared identifier is reported only once for each function it appears in
vim +/drm_dev_dbg +3392 drivers/gpu/drm/drm_dp_mst_topology.c
3352
3353 /**
3354 * drm_dp_update_payload_part1() - Execute payload update part 1
3355 * @mgr: manager to use.
3356 *
3357 * This iterates over all proposed virtual channels, and tries to
3358 * allocate space in the link for them. For 0->slots transitions,
3359 * this step just writes the VCPI to the MST device. For slots->0
3360 * transitions, this writes the updated VCPIs and removes the
3361 * remote VC payloads.
3362 *
3363 * after calling this the driver should generate ACT and payload
3364 * packets.
3365 */
3366 int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
3367 {
3368 struct drm_dp_payload req_payload;
3369 struct drm_dp_mst_port *port;
3370 int i, j;
3371 int cur_slots = 1;
3372 bool skip;
3373
3374 mutex_lock(&mgr->payload_lock);
3375 for (i = 0; i < mgr->max_payloads; i++) {
3376 struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i];
3377 struct drm_dp_payload *payload = &mgr->payloads[i];
3378 bool put_port = false;
3379
3380 /* solve the current payloads - compare to the hw ones
3381 - update the hw view */
3382 req_payload.start_slot = cur_slots;
3383 if (vcpi) {
3384 port = container_of(vcpi, struct drm_dp_mst_port,
3385 vcpi);
3386
3387 mutex_lock(&mgr->lock);
3388 skip = !drm_dp_mst_port_downstream_of_branch(port, mgr->mst_primary);
3389 mutex_unlock(&mgr->lock);
3390
3391 if (skip) {
> 3392 drm_dbg_kms("Virtual channel %d is not in current topology\n", i);
3393 continue;
3394 }
3395 /* Validated ports don't matter if we're releasing
3396 * VCPI
3397 */
3398 if (vcpi->num_slots) {
3399 port = drm_dp_mst_topology_get_port_validated(
3400 mgr, port);
3401 if (!port) {
3402 if (vcpi->num_slots == payload->num_slots) {
3403 cur_slots += vcpi->num_slots;
3404 payload->start_slot = req_payload.start_slot;
3405 continue;
3406 } else {
3407 drm_dbg_kms("Fail:set payload to invalid sink");
3408 mutex_unlock(&mgr->payload_lock);
3409 return -EINVAL;
3410 }
3411 }
3412 put_port = true;
3413 }
3414
3415 req_payload.num_slots = vcpi->num_slots;
3416 req_payload.vcpi = vcpi->vcpi;
3417 } else {
3418 port = NULL;
3419 req_payload.num_slots = 0;
3420 }
3421
3422 payload->start_slot = req_payload.start_slot;
3423 /* work out what is required to happen with this payload */
3424 if (payload->num_slots != req_payload.num_slots) {
3425
3426 /* need to push an update for this payload */
3427 if (req_payload.num_slots) {
3428 drm_dp_create_payload_step1(mgr, vcpi->vcpi,
3429 &req_payload);
3430 payload->num_slots = req_payload.num_slots;
3431 payload->vcpi = req_payload.vcpi;
3432
3433 } else if (payload->num_slots) {
3434 payload->num_slots = 0;
3435 drm_dp_destroy_payload_step1(mgr, port,
3436 payload->vcpi,
3437 payload);
3438 req_payload.payload_state =
3439 payload->payload_state;
3440 payload->start_slot = 0;
3441 }
3442 payload->payload_state = req_payload.payload_state;
3443 }
3444 cur_slots += req_payload.num_slots;
3445
3446 if (put_port)
3447 drm_dp_mst_topology_put_port(port);
3448 }
3449
3450 for (i = 0; i < mgr->max_payloads; /* do nothing */) {
3451 if (mgr->payloads[i].payload_state != DP_PAYLOAD_DELETE_LOCAL) {
3452 i++;
3453 continue;
3454 }
3455
3456 drm_dbg_kms(mgr->dev, "removing payload %d\n", i);
3457 for (j = i; j < mgr->max_payloads - 1; j++) {
3458 mgr->payloads[j] = mgr->payloads[j + 1];
3459 mgr->proposed_vcpis[j] = mgr->proposed_vcpis[j + 1];
3460
3461 if (mgr->proposed_vcpis[j] &&
3462 mgr->proposed_vcpis[j]->num_slots) {
3463 set_bit(j + 1, &mgr->payload_mask);
3464 } else {
3465 clear_bit(j + 1, &mgr->payload_mask);
3466 }
3467 }
3468
3469 memset(&mgr->payloads[mgr->max_payloads - 1], 0,
3470 sizeof(struct drm_dp_payload));
3471 mgr->proposed_vcpis[mgr->max_payloads - 1] = NULL;
3472 clear_bit(mgr->max_payloads, &mgr->payload_mask);
3473 }
3474 mutex_unlock(&mgr->payload_lock);
3475
3476 return 0;
3477 }
3478 EXPORT_SYMBOL(drm_dp_update_payload_part1);
3479
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 26991 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly
2021-06-16 19:53 ` Harry Wentland
@ 2021-06-17 8:47 ` Lin, Wayne
-1 siblings, 0 replies; 12+ messages in thread
From: Lin, Wayne @ 2021-06-17 8:47 UTC (permalink / raw)
To: Wentland, Harry, dri-devel
Cc: lyude, Kazlauskas, Nicholas, Zuo, Jerry, Pillai, Aurabindo,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, stable
[AMD Official Use Only]
________________________________________
> From: Wentland, Harry <Harry.Wentland@amd.com>
> Sent: Thursday, June 17, 2021 03:53
> To: Lin, Wayne; dri-devel@lists.freedesktop.org
> Cc: lyude@redhat.com; Kazlauskas, Nicholas; Zuo, Jerry; Pillai, Aurabindo; Maarten Lankhorst; Maxime Ripard; Thomas Zimmermann; stable@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly
>
>
>
> On 2021-06-15 11:55 p.m., Wayne Lin wrote:
> > [Why]
> > When we receive CSN message to notify one port is disconnected, we will
> > implicitly set its corresponding num_slots to 0. Later on, we will
> > eventually call drm_dp_update_payload_part1() to arrange down streams.
> >
> > In drm_dp_update_payload_part1(), we iterate over all proposed_vcpis[]
> > to do the update. Not specific to a target sink only. For example, if we
> > light up 2 monitors, Monitor_A and Monitor_B, and then we unplug
> > Monitor_B. Later on, when we call drm_dp_update_payload_part1() to try
> > to update payload for Monitor_A, we'll also implicitly clean payload for
> > Monitor_B at the same time. And finally, when we try to call
> > drm_dp_update_payload_part1() to clean payload for Monitor_B, we will do
> > nothing at this time since payload for Monitor_B has been cleaned up
> > previously.
> >
> > For StarTech 1to3 DP hub, it seems like if we didn't update DPCD payload
> > ID table then polling for "ACT Handled"(BIT_1 of DPCD 002C0h) will fail
> > and this polling will last for 3 seconds.
> >
> > Therefore, guess the best way is we don't set the proposed_vcpi[]
> > diretly. Let user of these herlper functions to set the proposed_vcpi
> > directly.
> >
> > [How]
> > 1. Revert commit 7617e9621bf2 ("drm/dp_mst: clear time slots for ports
> > invalid")
> > 2. Tackle the issue in previous commit by skipping those trasient
> > proposed VCPIs. These stale VCPIs shoulde be explicitly cleared by
> > user later on.
> >
> > Changes since v1:
> > * Change debug macro to use drm_dbg_kms() instead
> > * Amend the commit message to add Fixed & Cc tags
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > Fixes: 7617e9621bf2 ("drm/dp_mst: clear time slots for ports invalid")
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Wayne Lin <Wayne.Lin@amd.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: <stable@vger.kernel.org> # v5.5+
> > ---
> > drivers/gpu/drm/drm_dp_mst_topology.c | 36 ++++++++-------------------
> > 1 file changed, 10 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 32b7f8983b94..b41b837db66d 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2501,7 +2501,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> > {
> > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> > struct drm_dp_mst_port *port;
> > - int old_ddps, old_input, ret, i;
> > + int old_ddps, ret;
> > u8 new_pdt;
> > bool new_mcs;
> > bool dowork = false, create_connector = false;
> > @@ -2533,7 +2533,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> > }
> >
> > old_ddps = port->ddps;
> > - old_input = port->input;
> > port->input = conn_stat->input_port;
> > port->ldps = conn_stat->legacy_device_plug_status;
> > port->ddps = conn_stat->displayport_device_plug_status;
> > @@ -2555,28 +2554,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> > dowork = false;
> > }
> >
> > - if (!old_input && old_ddps != port->ddps && !port->ddps) {
> > - for (i = 0; i < mgr->max_payloads; i++) {
> > - struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i];
> > - struct drm_dp_mst_port *port_validated;
> > -
> > - if (!vcpi)
> > - continue;
> > -
> > - port_validated =
> > - container_of(vcpi, struct drm_dp_mst_port, vcpi);
> > - port_validated =
> > - drm_dp_mst_topology_get_port_validated(mgr, port_validated);
> > - if (!port_validated) {
> > - mutex_lock(&mgr->payload_lock);
> > - vcpi->num_slots = 0;
> > - mutex_unlock(&mgr->payload_lock);
> > - } else {
> > - drm_dp_mst_topology_put_port(port_validated);
> > - }
> > - }
> > - }
> > -
> > if (port->connector)
> > drm_modeset_unlock(&mgr->base.lock);
> > else if (create_connector)
> > @@ -3410,8 +3387,15 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
> > port = drm_dp_mst_topology_get_port_validated(
> > mgr, port);
> > if (!port) {
> > - mutex_unlock(&mgr->payload_lock);
> > - return -EINVAL;
> > + if (vcpi->num_slots == payload->num_slots) {
> > + cur_slots += vcpi->num_slots;
> > + payload->start_slot = req_payload.start_slot;
> > + continue;
> > + } else {
> > + drm_dbg_kms("Fail:set payload to invalid sink");
>
> drm_dbg_kms takes a drm_device as first parameter.
>
> Harry
Thanks Harry. Should be addressed by another patch provided by José.
>
> > + mutex_unlock(&mgr->payload_lock);
> > + return -EINVAL;
> > + }
> > }
> > put_port = true;
> > }
> >
--
Regards,
Wayne
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly
@ 2021-06-17 8:47 ` Lin, Wayne
0 siblings, 0 replies; 12+ messages in thread
From: Lin, Wayne @ 2021-06-17 8:47 UTC (permalink / raw)
To: Wentland, Harry, dri-devel
Cc: Zuo, Jerry, Pillai, Aurabindo, stable, Thomas Zimmermann,
Kazlauskas, Nicholas
[AMD Official Use Only]
________________________________________
> From: Wentland, Harry <Harry.Wentland@amd.com>
> Sent: Thursday, June 17, 2021 03:53
> To: Lin, Wayne; dri-devel@lists.freedesktop.org
> Cc: lyude@redhat.com; Kazlauskas, Nicholas; Zuo, Jerry; Pillai, Aurabindo; Maarten Lankhorst; Maxime Ripard; Thomas Zimmermann; stable@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly
>
>
>
> On 2021-06-15 11:55 p.m., Wayne Lin wrote:
> > [Why]
> > When we receive CSN message to notify one port is disconnected, we will
> > implicitly set its corresponding num_slots to 0. Later on, we will
> > eventually call drm_dp_update_payload_part1() to arrange down streams.
> >
> > In drm_dp_update_payload_part1(), we iterate over all proposed_vcpis[]
> > to do the update. Not specific to a target sink only. For example, if we
> > light up 2 monitors, Monitor_A and Monitor_B, and then we unplug
> > Monitor_B. Later on, when we call drm_dp_update_payload_part1() to try
> > to update payload for Monitor_A, we'll also implicitly clean payload for
> > Monitor_B at the same time. And finally, when we try to call
> > drm_dp_update_payload_part1() to clean payload for Monitor_B, we will do
> > nothing at this time since payload for Monitor_B has been cleaned up
> > previously.
> >
> > For StarTech 1to3 DP hub, it seems like if we didn't update DPCD payload
> > ID table then polling for "ACT Handled"(BIT_1 of DPCD 002C0h) will fail
> > and this polling will last for 3 seconds.
> >
> > Therefore, guess the best way is we don't set the proposed_vcpi[]
> > diretly. Let user of these herlper functions to set the proposed_vcpi
> > directly.
> >
> > [How]
> > 1. Revert commit 7617e9621bf2 ("drm/dp_mst: clear time slots for ports
> > invalid")
> > 2. Tackle the issue in previous commit by skipping those trasient
> > proposed VCPIs. These stale VCPIs shoulde be explicitly cleared by
> > user later on.
> >
> > Changes since v1:
> > * Change debug macro to use drm_dbg_kms() instead
> > * Amend the commit message to add Fixed & Cc tags
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > Fixes: 7617e9621bf2 ("drm/dp_mst: clear time slots for ports invalid")
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Wayne Lin <Wayne.Lin@amd.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: <stable@vger.kernel.org> # v5.5+
> > ---
> > drivers/gpu/drm/drm_dp_mst_topology.c | 36 ++++++++-------------------
> > 1 file changed, 10 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 32b7f8983b94..b41b837db66d 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2501,7 +2501,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> > {
> > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> > struct drm_dp_mst_port *port;
> > - int old_ddps, old_input, ret, i;
> > + int old_ddps, ret;
> > u8 new_pdt;
> > bool new_mcs;
> > bool dowork = false, create_connector = false;
> > @@ -2533,7 +2533,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> > }
> >
> > old_ddps = port->ddps;
> > - old_input = port->input;
> > port->input = conn_stat->input_port;
> > port->ldps = conn_stat->legacy_device_plug_status;
> > port->ddps = conn_stat->displayport_device_plug_status;
> > @@ -2555,28 +2554,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> > dowork = false;
> > }
> >
> > - if (!old_input && old_ddps != port->ddps && !port->ddps) {
> > - for (i = 0; i < mgr->max_payloads; i++) {
> > - struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i];
> > - struct drm_dp_mst_port *port_validated;
> > -
> > - if (!vcpi)
> > - continue;
> > -
> > - port_validated =
> > - container_of(vcpi, struct drm_dp_mst_port, vcpi);
> > - port_validated =
> > - drm_dp_mst_topology_get_port_validated(mgr, port_validated);
> > - if (!port_validated) {
> > - mutex_lock(&mgr->payload_lock);
> > - vcpi->num_slots = 0;
> > - mutex_unlock(&mgr->payload_lock);
> > - } else {
> > - drm_dp_mst_topology_put_port(port_validated);
> > - }
> > - }
> > - }
> > -
> > if (port->connector)
> > drm_modeset_unlock(&mgr->base.lock);
> > else if (create_connector)
> > @@ -3410,8 +3387,15 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
> > port = drm_dp_mst_topology_get_port_validated(
> > mgr, port);
> > if (!port) {
> > - mutex_unlock(&mgr->payload_lock);
> > - return -EINVAL;
> > + if (vcpi->num_slots == payload->num_slots) {
> > + cur_slots += vcpi->num_slots;
> > + payload->start_slot = req_payload.start_slot;
> > + continue;
> > + } else {
> > + drm_dbg_kms("Fail:set payload to invalid sink");
>
> drm_dbg_kms takes a drm_device as first parameter.
>
> Harry
Thanks Harry. Should be addressed by another patch provided by José.
>
> > + mutex_unlock(&mgr->payload_lock);
> > + return -EINVAL;
> > + }
> > }
> > put_port = true;
> > }
> >
--
Regards,
Wayne
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-06-17 8:47 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-16 3:54 [PATCH v2 0/2] Fix observed mst problems with StarTech hub Wayne Lin
2021-06-16 3:55 ` [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly Wayne Lin
2021-06-16 3:55 ` Wayne Lin
2021-06-16 19:53 ` Harry Wentland
2021-06-16 19:53 ` Harry Wentland
2021-06-17 8:47 ` Lin, Wayne
2021-06-17 8:47 ` Lin, Wayne
2021-06-16 3:55 ` [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology Wayne Lin
2021-06-16 3:55 ` Wayne Lin
2021-06-16 19:26 ` Souza, Jose
2021-06-16 19:26 ` Souza, Jose
2021-06-17 4:01 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.