Re: [PATCH 0/3] drm/mst: Add support for simultaneous down replies

[Sean Paul <sean@poorly.run>]

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
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel