<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> [AMD Official Use Only - Internal Distribution Only] <div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Hi Lyude,</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Yes, I would appreciate it if you could push this to drm-misc-next for me.</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Thank you for your comments and review!</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Best,</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Eryk </div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Lyude Paul <lyude@redhat.com> Sent: Thursday, March 25, 2021 6:30 PM To: Brol, Eryk <Eryk.Brol@amd.com>; manasi.d.navare@intel.com <manasi.d.navare@intel.com>; daniel@ffwll.ch <daniel@ffwll.ch>; Wentland, Harry <Harry.Wentland@amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>; Lin, Wayne <Wayne.Lin@amd.com> Cc: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH v2] drm/mst: Enhance MST topology logging <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">Reviewed-by: Lyude Paul <lyude@redhat.com> Let me know if you need me to push this to drm-misc-next for you On Thu, 2021-03-25 at 14:06 -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 was printed as an > arbitrary series of ints which requires 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 > > --- > > v2: Addressed Lyude's comments > -made helper function return const > -fixed indentation and spacing issues > > Signed-off-by: Eryk Brol <eryk.brol@amd.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 59 ++++++++++++++++++++++----- > 1 file changed, 48 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 932c4641ec3e..de5124ce42cb 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -4720,6 +4720,28 @@ static void drm_dp_mst_kick_tx(struct > drm_dp_mst_topology_mgr *mgr) > queue_work(system_long_wq, &mgr->tx_work); > } > > +/* > + * Helper function for parsing DP device types into convenient strings > + * for use with dp_mst_topology > + */ > +static const 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 +4754,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 +4820,37 @@ 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 ***\n"); > + seq_printf(m, "payload_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, > + seq_printf(m, "%10d%10d%10d%10d%20s\n", > + i, > + port->port_num, > + port->vcpi.vcpi, > port->vcpi.num_slots, > - (*name != 0) ? name : "Unknown"); > + (*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", > + 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! </div> </div> </div> </body> </html>