[PATCH] drm/dp_mst: Enhance DP MST topology logging
Lyude Paul
lyude at redhat.com
Tue Mar 23 17:49:39 UTC 2021
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 at 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!
More information about the amd-gfx
mailing list