dri-devel Archive on lore.kernel.org
 help / color / Atom feed
From: Lyude Paul <lyude@redhat.com>
To: Sean Paul <sean@poorly.run>, dri-devel@lists.freedesktop.org
Cc: Sean Paul <seanpaul@chromium.org>, Wayne.Lin@amd.com
Subject: Re: [PATCH 0/3] drm/mst: Add support for simultaneous down replies
Date: Fri, 14 Feb 2020 18:33:17 -0500
Message-ID: <b8dbe33bc62d9cd678b5af8cde2570f6c5754aeb.camel@redhat.com> (raw)
In-Reply-To: <20200213211523.156998-1-sean@poorly.run>

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(-)
> 
-- 
Cheers,
	Lyude Paul (she/her)
	Associate Software Engineer at Red Hat

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  parent reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-13 21:15 Sean Paul
2020-02-13 21:15 ` [PATCH 1/3] drm/mst: Separate sideband packet header parsing from message building Sean Paul
2020-02-13 21:15 ` [PATCH 2/3] drm/mst: Support simultaneous down replies Sean Paul
2020-02-13 21:15 ` [PATCH 3/3] drm/dp_mst: Remove single tx msg restriction Sean Paul
2020-02-14  5:57   ` Lin, Wayne
2020-02-14 16:08     ` Sean Paul
2020-02-17  7:08       ` Lin, Wayne
2020-02-18 15:52         ` Sean Paul
2020-02-18 17:15           ` Sean Paul
2020-02-19 11:00             ` Lin, Wayne
2020-02-14 23:33 ` Lyude Paul [this message]
2020-02-18 16:20   ` [PATCH 0/3] drm/mst: Add support for simultaneous down replies Sean Paul

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b8dbe33bc62d9cd678b5af8cde2570f6c5754aeb.camel@redhat.com \
    --to=lyude@redhat.com \
    --cc=Wayne.Lin@amd.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=sean@poorly.run \
    --cc=seanpaul@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dri-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dri-devel/0 dri-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dri-devel dri-devel/ https://lore.kernel.org/dri-devel \
		dri-devel@lists.freedesktop.org
	public-inbox-index dri-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.dri-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git