| Message ID | 20200213211523.156998-1-sean@poorly.run (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/mst: Add support for simultaneous down replies | expand |
On Thu, 2020-02-13 at 16:15 -0500, Sean Paul wrote: > From: Sean Paul <seanpaul@chromium.org> > > 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. So, I will give my r-b for the whole series, but... Reviewed-by: Lyude Paul <lyude@redhat.com> ...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. > 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(-) >
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@chromium.org> > > > > 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@redhat.com> > > ...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 >
From: Sean Paul <seanpaul@chromium.org> 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. 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(-)