On Fri, Feb 14, 2020 at 06:33:17PM -0500, Lyude Paul wrote: > On Thu, 2020-02-13 at 16:15 -0500, Sean Paul wrote: > > From: Sean Paul <seanpaul@xxxxxxxxxxxx> > > > > Hey all, > > Earlier this week on my 5.5 kernel (I can't seem to get a 5.6 kernel to > > behave on any of my devices), I ran into the multi-reply problem that > > Wayne fixed in January. Without realizing there was already a fix > > upstream, I went about solving it in different way. It wasn't until > > rebasing the patches on 5.6 for the list that I realized there was > > already a solution. > > > > At any rate, I think this way of handling things might be a bit more > > performant. I'm not super happy with the cleanliness of the code, I > > think this series should make things easier to read, but I don't think I > > achieved that. So suggestions are welcome on how to break this apart. > > Honestly it looks a bit cleaner to me. Sideband message parsing in MST is > pretty complex, so I'd think the code's probably always going to be messy to > some extent. > > My only suggestion with cleaning things up - maybe we should stop calling it > building a sideband reply, and call it re-assembling one? Seems a little less > confusing, since we're really just taking the rx chunks and reassembling them > into a full sideband message. I know at least I constantly find myself > forgetting those functions are for rx and not tx. Good point, something like drm_dp_sideband_append_payload() might be more descriptive and less confusing. I'm happy to change this if we decide to go through with this set. > > So, I will give my r-b for the whole series, but... > > Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx> > > ...I think we should definitely look more into what Wayne's talking about > before pushing this, and see if it's widespread enough of an issue to be a > concern. It does kind of suck how slow MST probing can be, so I'd wonder if we > could try your idea of rate limiting. My one concern there is I'm not actually > sure if there's anything in the DP MST protocol that indicates how many > messages a hub can handle at the same time - it's always supposed to just be > two iirc. I don't see an upper bound on pending downstream replies either. AFAICT from reading the spec, each endpoint must support 2 concurrent message requests. A forwarding device is responsible for reading the replies when it detects DOWN_REP_MSG_RDY. So each forwarding device has the ability (and should) rate- limit their own forwards. I guess some forwarding devices might only read one reply when they get the IRQ and not circle back for the rest? We could probably come up with a heuristic for handling this, but it'd be a bit nasty and is probably not worth it (I'd guess the vast majority of MST usecases are depth==1). Sean > > > Thanks, > > > > Sean > > > > Sean Paul (3): > > drm/mst: Separate sideband packet header parsing from message building > > drm/mst: Support simultaneous down replies > > drm/dp_mst: Remove single tx msg restriction. > > > > drivers/gpu/drm/drm_dp_mst_topology.c | 196 ++++++++++++++------------ > > include/drm/drm_dp_mst_helper.h | 65 ++++----- > > 2 files changed, 137 insertions(+), 124 deletions(-) > > > -- > Cheers, > Lyude Paul (she/her) > Associate Software Engineer at Red Hat > -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
