* [PATCH] drm/dp_mst: Enhance DP MST topology logging
@ 2021-03-18 15:55 Eryk Brol
2021-03-18 23:16 ` Lyude Paul
2021-03-23 17:49 ` Lyude Paul
0 siblings, 2 replies; 3+ messages in thread
From: Eryk Brol @ 2021-03-18 15:55 UTC (permalink / raw)
To: lyude, manasi.d.navare, daniel, Harry.Wentland, Rodrigo.Siqueira,
nicholas.kazlauskas, jerry.zuo, wayne.lin, eryk.brol
Cc: dri-devel, amd-gfx
[why]
MST topology print was missing fec logging and pdt printed
as an int wasn't clear. vcpi and payload info were also logged as an
arbitrary series of ints which require the user to know the ordering
of the prints, making the logs difficult to use.
[how]
-add fec logging
-add pdt parsing into strings
-format vcpi and payload info into tables with headings
-clean up topology prints
Signed-off-by: Eryk Brol <eryk.brol@amd.com>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 67 ++++++++++++++++++++-------
1 file changed, 51 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 932c4641ec3e..3afeaa59cbaa 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -4720,6 +4720,24 @@ static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr)
queue_work(system_long_wq, &mgr->tx_work);
}
+static char *pdt_to_string(u8 pdt)
+{
+ switch (pdt) {
+ case DP_PEER_DEVICE_NONE:
+ return "NONE";
+ case DP_PEER_DEVICE_SOURCE_OR_SST:
+ return "SOURCE OR SST";
+ case DP_PEER_DEVICE_MST_BRANCHING:
+ return "MST BRANCHING";
+ case DP_PEER_DEVICE_SST_SINK:
+ return "SST SINK";
+ case DP_PEER_DEVICE_DP_LEGACY_CONV:
+ return "DP LEGACY CONV";
+ default:
+ return "ERR";
+ }
+}
+
static void drm_dp_mst_dump_mstb(struct seq_file *m,
struct drm_dp_mst_branch *mstb)
{
@@ -4732,9 +4750,20 @@ static void drm_dp_mst_dump_mstb(struct seq_file *m,
prefix[i] = '\t';
prefix[i] = '\0';
- seq_printf(m, "%smst: %p, %d\n", prefix, mstb, mstb->num_ports);
+ seq_printf(m, "%smstb - [%p]: num_ports: %d\n", prefix, mstb, mstb->num_ports);
list_for_each_entry(port, &mstb->ports, next) {
- seq_printf(m, "%sport: %d: input: %d: pdt: %d, ddps: %d ldps: %d, sdp: %d/%d, %p, conn: %p\n", prefix, port->port_num, port->input, port->pdt, port->ddps, port->ldps, port->num_sdp_streams, port->num_sdp_stream_sinks, port, port->connector);
+ seq_printf(m, "%sport %d - [%p] (%s - %s): ddps: %d, ldps: %d, sdp: %d/%d, fec: %s, conn: %p\n",
+ prefix,
+ port->port_num,
+ port,
+ port->input ? "input" : "output",
+ pdt_to_string(port->pdt),
+ port->ddps,
+ port->ldps,
+ port->num_sdp_streams,
+ port->num_sdp_stream_sinks,
+ port->fec_capable ? "true" : "false",
+ port->connector);
if (port->mstb)
drm_dp_mst_dump_mstb(m, port->mstb);
}
@@ -4787,33 +4816,39 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
mutex_unlock(&mgr->lock);
mutex_lock(&mgr->payload_lock);
- seq_printf(m, "vcpi: %lx %lx %d\n", mgr->payload_mask, mgr->vcpi_mask,
- mgr->max_payloads);
+ seq_printf(m, "\n *** VCPI Info ***\npayload_mask: %lx, vcpi_mask: %lx, max_payloads: %d\n",
+ mgr->payload_mask,
+ mgr->vcpi_mask,
+ mgr->max_payloads);
+ seq_printf(m, "\n| idx | port # | vcp_id | # slots | sink name |\n");
for (i = 0; i < mgr->max_payloads; i++) {
if (mgr->proposed_vcpis[i]) {
char name[14];
port = container_of(mgr->proposed_vcpis[i], struct drm_dp_mst_port, vcpi);
fetch_monitor_name(mgr, port, name, sizeof(name));
- seq_printf(m, "vcpi %d: %d %d %d sink name: %s\n", i,
- port->port_num, port->vcpi.vcpi,
- port->vcpi.num_slots,
- (*name != 0) ? name : "Unknown");
+ seq_printf(m, "%10d%10d%10d%10d%20s\n",
+ i,
+ port->port_num,
+ port->vcpi.vcpi,
+ port->vcpi.num_slots,
+ (*name != 0) ? name : "Unknown");
} else
- seq_printf(m, "vcpi %d:unused\n", i);
+ seq_printf(m, "%6d - Unused\n", i);
}
+ seq_printf(m, "\n *** Payload Info ***\n");
+ seq_printf(m, "| idx | state | start slot | # slots |\n");
for (i = 0; i < mgr->max_payloads; i++) {
- seq_printf(m, "payload %d: %d, %d, %d\n",
- i,
- mgr->payloads[i].payload_state,
- mgr->payloads[i].start_slot,
- mgr->payloads[i].num_slots);
-
-
+ seq_printf(m, "%10d%10d%15d%10d\n",
+ i,
+ mgr->payloads[i].payload_state,
+ mgr->payloads[i].start_slot,
+ mgr->payloads[i].num_slots);
}
mutex_unlock(&mgr->payload_lock);
+ seq_printf(m, "\n *** DPCD Info ***\n");
mutex_lock(&mgr->lock);
if (mgr->mst_primary) {
u8 buf[DP_PAYLOAD_TABLE_SIZE];
--
2.25.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/dp_mst: Enhance DP MST topology logging
2021-03-18 15:55 [PATCH] drm/dp_mst: Enhance DP MST topology logging Eryk Brol
@ 2021-03-18 23:16 ` Lyude Paul
2021-03-23 17:49 ` Lyude Paul
1 sibling, 0 replies; 3+ messages in thread
From: Lyude Paul @ 2021-03-18 23:16 UTC (permalink / raw)
To: Eryk Brol, manasi.d.navare, daniel, Harry.Wentland,
Rodrigo.Siqueira, nicholas.kazlauskas, jerry.zuo, wayne.lin
Cc: dri-devel, amd-gfx
(going to try to take a look at this tomorrow JFYI)
On Thu, 2021-03-18 at 11:55 -0400, Eryk Brol wrote:
> [why]
> MST topology print was missing fec logging and pdt printed
> as an int wasn't clear. vcpi and payload info were also logged as an
> arbitrary series of ints which require the user to know the ordering
> of the prints, making the logs difficult to use.
>
> [how]
> -add fec logging
> -add pdt parsing into strings
> -format vcpi and payload info into tables with headings
> -clean up topology prints
>
> Signed-off-by: Eryk Brol <eryk.brol@amd.com>
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 67 ++++++++++++++++++++-------
> 1 file changed, 51 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 932c4641ec3e..3afeaa59cbaa 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4720,6 +4720,24 @@ static void drm_dp_mst_kick_tx(struct
> drm_dp_mst_topology_mgr *mgr)
> queue_work(system_long_wq, &mgr->tx_work);
> }
>
> +static char *pdt_to_string(u8 pdt)
> +{
> + switch (pdt) {
> + case DP_PEER_DEVICE_NONE:
> + return "NONE";
> + case DP_PEER_DEVICE_SOURCE_OR_SST:
> + return "SOURCE OR SST";
> + case DP_PEER_DEVICE_MST_BRANCHING:
> + return "MST BRANCHING";
> + case DP_PEER_DEVICE_SST_SINK:
> + return "SST SINK";
> + case DP_PEER_DEVICE_DP_LEGACY_CONV:
> + return "DP LEGACY CONV";
> + default:
> + return "ERR";
> + }
> +}
> +
> static void drm_dp_mst_dump_mstb(struct seq_file *m,
> struct drm_dp_mst_branch *mstb)
> {
> @@ -4732,9 +4750,20 @@ static void drm_dp_mst_dump_mstb(struct seq_file *m,
> prefix[i] = '\t';
> prefix[i] = '\0';
>
> - seq_printf(m, "%smst: %p, %d\n", prefix, mstb, mstb->num_ports);
> + seq_printf(m, "%smstb - [%p]: num_ports: %d\n", prefix, mstb, mstb-
> >num_ports);
> list_for_each_entry(port, &mstb->ports, next) {
> - seq_printf(m, "%sport: %d: input: %d: pdt: %d, ddps: %d ldps:
> %d, sdp: %d/%d, %p, conn: %p\n", prefix, port->port_num, port->input, port-
> >pdt, port->ddps, port->ldps, port->num_sdp_streams, port-
> >num_sdp_stream_sinks, port, port->connector);
> + seq_printf(m, "%sport %d - [%p] (%s - %s): ddps: %d, ldps: %d,
> sdp: %d/%d, fec: %s, conn: %p\n",
> + prefix,
> + port->port_num,
> + port,
> + port->input ? "input" : "output",
> + pdt_to_string(port->pdt),
> + port->ddps,
> + port->ldps,
> + port->num_sdp_streams,
> + port->num_sdp_stream_sinks,
> + port->fec_capable ? "true" : "false",
> + port->connector);
> if (port->mstb)
> drm_dp_mst_dump_mstb(m, port->mstb);
> }
> @@ -4787,33 +4816,39 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
> mutex_unlock(&mgr->lock);
>
> mutex_lock(&mgr->payload_lock);
> - seq_printf(m, "vcpi: %lx %lx %d\n", mgr->payload_mask, mgr->vcpi_mask,
> - mgr->max_payloads);
> + seq_printf(m, "\n *** VCPI Info ***\npayload_mask: %lx, vcpi_mask:
> %lx, max_payloads: %d\n",
> + mgr->payload_mask,
> + mgr->vcpi_mask,
> + mgr->max_payloads);
>
> + seq_printf(m, "\n| idx | port # | vcp_id | # slots | sink
> name |\n");
> for (i = 0; i < mgr->max_payloads; i++) {
> if (mgr->proposed_vcpis[i]) {
> char name[14];
>
> port = container_of(mgr->proposed_vcpis[i], struct
> drm_dp_mst_port, vcpi);
> fetch_monitor_name(mgr, port, name, sizeof(name));
> - seq_printf(m, "vcpi %d: %d %d %d sink name: %s\n", i,
> - port->port_num, port->vcpi.vcpi,
> - port->vcpi.num_slots,
> - (*name != 0) ? name : "Unknown");
> + seq_printf(m, "%10d%10d%10d%10d%20s\n",
> + i,
> + port->port_num,
> + port->vcpi.vcpi,
> + port->vcpi.num_slots,
> + (*name != 0) ? name : "Unknown");
> } else
> - seq_printf(m, "vcpi %d:unused\n", i);
> + seq_printf(m, "%6d - Unused\n", i);
> }
> + seq_printf(m, "\n *** Payload Info ***\n");
> + seq_printf(m, "| idx | state | start slot | # slots |\n");
> for (i = 0; i < mgr->max_payloads; i++) {
> - seq_printf(m, "payload %d: %d, %d, %d\n",
> - i,
> - mgr->payloads[i].payload_state,
> - mgr->payloads[i].start_slot,
> - mgr->payloads[i].num_slots);
> -
> -
> + seq_printf(m, "%10d%10d%15d%10d\n",
> + i,
> + mgr->payloads[i].payload_state,
> + mgr->payloads[i].start_slot,
> + mgr->payloads[i].num_slots);
> }
> mutex_unlock(&mgr->payload_lock);
>
> + seq_printf(m, "\n *** DPCD Info ***\n");
> mutex_lock(&mgr->lock);
> if (mgr->mst_primary) {
> u8 buf[DP_PAYLOAD_TABLE_SIZE];
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/dp_mst: Enhance DP MST topology logging
2021-03-18 15:55 [PATCH] drm/dp_mst: Enhance DP MST topology logging Eryk Brol
2021-03-18 23:16 ` Lyude Paul
@ 2021-03-23 17:49 ` Lyude Paul
1 sibling, 0 replies; 3+ messages in thread
From: Lyude Paul @ 2021-03-23 17:49 UTC (permalink / raw)
To: Eryk Brol, manasi.d.navare, daniel, Harry.Wentland,
Rodrigo.Siqueira, nicholas.kazlauskas, jerry.zuo, wayne.lin
Cc: dri-devel, amd-gfx
Sorry for the wait! Review comments below
On Thu, 2021-03-18 at 11:55 -0400, Eryk Brol wrote:
> [why]
> MST topology print was missing fec logging and pdt printed
> as an int wasn't clear. vcpi and payload info were also logged as an
> arbitrary series of ints which require the user to know the ordering
> of the prints, making the logs difficult to use.
>
> [how]
> -add fec logging
> -add pdt parsing into strings
> -format vcpi and payload info into tables with headings
> -clean up topology prints
>
> Signed-off-by: Eryk Brol <eryk.brol@amd.com>
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 67 ++++++++++++++++++++-------
> 1 file changed, 51 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 932c4641ec3e..3afeaa59cbaa 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4720,6 +4720,24 @@ static void drm_dp_mst_kick_tx(struct
> drm_dp_mst_topology_mgr *mgr)
> queue_work(system_long_wq, &mgr->tx_work);
> }
>
> +static char *pdt_to_string(u8 pdt)
Let's make this static const char *
> +{
> + switch (pdt) {
> + case DP_PEER_DEVICE_NONE:
> + return "NONE";
> + case DP_PEER_DEVICE_SOURCE_OR_SST:
> + return "SOURCE OR SST";
> + case DP_PEER_DEVICE_MST_BRANCHING:
> + return "MST BRANCHING";
> + case DP_PEER_DEVICE_SST_SINK:
> + return "SST SINK";
> + case DP_PEER_DEVICE_DP_LEGACY_CONV:
> + return "DP LEGACY CONV";
> + default:
> + return "ERR";
> + }
> +}
> +
> static void drm_dp_mst_dump_mstb(struct seq_file *m,
> struct drm_dp_mst_branch *mstb)
> {
> @@ -4732,9 +4750,20 @@ static void drm_dp_mst_dump_mstb(struct seq_file *m,
> prefix[i] = '\t';
> prefix[i] = '\0';
>
> - seq_printf(m, "%smst: %p, %d\n", prefix, mstb, mstb->num_ports);
> + seq_printf(m, "%smstb - [%p]: num_ports: %d\n", prefix, mstb, mstb-
> >num_ports);
> list_for_each_entry(port, &mstb->ports, next) {
> - seq_printf(m, "%sport: %d: input: %d: pdt: %d, ddps: %d ldps:
> %d, sdp: %d/%d, %p, conn: %p\n", prefix, port->port_num, port->input, port->pdt,
> port->ddps, port->ldps, port->num_sdp_streams, port->num_sdp_stream_sinks, port,
> port->connector);
> + seq_printf(m, "%sport %d - [%p] (%s - %s): ddps: %d, ldps: %d,
> sdp: %d/%d, fec: %s, conn: %p\n",
> + prefix,
> + port->port_num,
> + port,
> + port->input ? "input" : "output",
> + pdt_to_string(port->pdt),
> + port->ddps,
> + port->ldps,
> + port->num_sdp_streams,
> + port->num_sdp_stream_sinks,
> + port->fec_capable ? "true" : "false",
> + port->connector);
The indenting here is wrong, "i," and all the lines up until the end of the
function call should be aligned to one col after the starting paranthesis
So like this:
seq_printf(m, "foo%d",
bar);
> if (port->mstb)
> drm_dp_mst_dump_mstb(m, port->mstb);
> }
> @@ -4787,33 +4816,39 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
> mutex_unlock(&mgr->lock);
>
> mutex_lock(&mgr->payload_lock);
> - seq_printf(m, "vcpi: %lx %lx %d\n", mgr->payload_mask, mgr->vcpi_mask,
> - mgr->max_payloads);
> + seq_printf(m, "\n *** VCPI Info ***\npayload_mask: %lx, vcpi_mask: %lx,
> max_payloads: %d\n",
There's an extra space between the \n and the ***
> + mgr->payload_mask,
> + mgr->vcpi_mask,
> + mgr->max_payloads);
IMHO I would do the seq_printf() here like this instead:
seq_printf("\n*** VCPI INFO***\n");
seq_printf("payload_mask: %lx, vcpi_mask: %lx, max_payloads: %d\n", ...);
Just to make things a bit easier to read
>
> + seq_printf(m, "\n| idx | port # | vcp_id | # slots | sink
> name |\n");
> for (i = 0; i < mgr->max_payloads; i++) {
> if (mgr->proposed_vcpis[i]) {
> char name[14];
>
> port = container_of(mgr->proposed_vcpis[i], struct
> drm_dp_mst_port, vcpi);
> fetch_monitor_name(mgr, port, name, sizeof(name));
> - seq_printf(m, "vcpi %d: %d %d %d sink name: %s\n", i,
> - port->port_num, port->vcpi.vcpi,
> - port->vcpi.num_slots,
> - (*name != 0) ? name : "Unknown");
> + seq_printf(m, "%10d%10d%10d%10d%20s\n",
> + i,
> + port->port_num,
> + port->vcpi.vcpi,
> + port->vcpi.num_slots,
> + (*name != 0) ? name : "Unknown");
Maybe cleanup the second space after the : here while you're at it. Also, the
indenting is wrong here too
> } else
> - seq_printf(m, "vcpi %d:unused\n", i);
> + seq_printf(m, "%6d - Unused\n", i);
> }
> + seq_printf(m, "\n *** Payload Info ***\n");
^
another accidental space
> + seq_printf(m, "| idx | state | start slot | # slots |\n");
> for (i = 0; i < mgr->max_payloads; i++) {
> - seq_printf(m, "payload %d: %d, %d, %d\n",
> - i,
> - mgr->payloads[i].payload_state,
> - mgr->payloads[i].start_slot,
> - mgr->payloads[i].num_slots);
> -
> -
> + seq_printf(m, "%10d%10d%15d%10d\n",
> + i,
> + mgr->payloads[i].payload_state,
> + mgr->payloads[i].start_slot,
> + mgr->payloads[i].num_slots);
Need to correct the indenting here as well
> }
> mutex_unlock(&mgr->payload_lock);
>
> + seq_printf(m, "\n *** DPCD Info ***\n");
^ another extra space
> mutex_lock(&mgr->lock);
> if (mgr->mst_primary) {
> u8 buf[DP_PAYLOAD_TABLE_SIZE];
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-23 17:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-18 15:55 [PATCH] drm/dp_mst: Enhance DP MST topology logging Eryk Brol
2021-03-18 23:16 ` Lyude Paul
2021-03-23 17:49 ` Lyude Paul
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).