Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200306234623.547525-5-lyude@redhat.com> <20200309210131.1497545-1-lyude@redhat.com>
In-Reply-To: <20200309210131.1497545-1-lyude@redhat.com>
From:   Alex Deucher <alexdeucher@gmail.com>
Date:   Mon, 9 Mar 2020 17:15:19 -0400
Message-ID: <CADnq5_PAaCxF_Wq4huxPEKuWY00zi110KzLzNb6ucsd5zb_7vQ@mail.gmail.com>
Subject: Re: [PATCH v3] drm/dp_mst: Rewrite and fix bandwidth limit checks
To:     Lyude Paul <lyude@redhat.com>
Cc:     Maling list - DRI developers <dri-devel@lists.freedesktop.org>,
        Sean Paul <seanpaul@google.com>,
        David Airlie <airlied@linux.ie>,
        LKML <linux-kernel@vger.kernel.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Alex Deucher <alexander.deucher@amd.com>,
        Mikita Lipski <mikita.lipski@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 9, 2020 at 5:01 PM Lyude Paul <lyude@redhat.com> wrote:
>
> Sigh, this is mostly my fault for not giving commit cd82d82cbc04
> ("drm/dp_mst: Add branch bandwidth validation to MST atomic check")
> enough scrutiny during review. The way we're checking bandwidth
> limitations here is mostly wrong:
>
> For starters, drm_dp_mst_atomic_check_bw_limit() determines the
> pbn_limit of a branch by simply scanning each port on the current branch
> device, then uses the last non-zero full_pbn value that it finds. It
> then counts the sum of the PBN used on each branch device for that
> level, and compares against the full_pbn value it found before.
>
> This is wrong because ports can and will have different PBN limitations
> on many hubs, especially since a number of DisplayPort hubs out there
> will be clever and only use the smallest link rate required for each
> downstream sink - potentially giving every port a different full_pbn
> value depending on what link rate it's trained at. This means with our
> current code, which max PBN value we end up with is not well defined.
>
> Additionally, we also need to remember when checking bandwidth
> limitations that the top-most device in any MST topology is a branch
> device, not a port. This means that the first level of a topology
> doesn't technically have a full_pbn value that needs to be checked.
> Instead, we should assume that so long as our VCPI allocations fit we're
> within the bandwidth limitations of the primary MSTB.
>
> We do however, want to check full_pbn on every port including those of
> the primary MSTB. However, it's important to keep in mind that this
> value represents the minimum link rate /between a port's sink or mstb,
> and the mstb itself/. A quick diagram to explain:
>
>                                 MSTB #1
>                                /       \
>                               /         \
>                            Port #1    Port #2
>        full_pbn for Port #1 =E2=86=92 |          | =E2=86=90 full_pbn for=
 Port #2
>                            Sink #1    MSTB #2
>                                          |
>                                        etc...
>
> Note that in the above diagram, the combined PBN from all VCPI
> allocations on said hub should not exceed the full_pbn value of port #2,
> and the display configuration on sink #1 should not exceed the full_pbn
> value of port #1. However, port #1 and port #2 can otherwise consume as
> much bandwidth as they want so long as their VCPI allocations still fit.
>
> And finally - our current bandwidth checking code also makes the mistake
> of not checking whether something is an end device or not before trying
> to traverse down it.
>
> So, let's fix it by rewriting our bandwidth checking helpers. We split
> the function into one part for handling branches which simply adds up
> the total PBN on each branch and returns it, and one for checking each
> port to ensure we're not going over its PBN limit. Phew.
>
> This should fix regressions seen, where we erroneously reject display
> configurations due to thinking they're going over our bandwidth limits
> when they're not.
>
> Changes since v1:
> * Took an even closer look at how PBN limitations are supposed to be
>   handled, and did some experimenting with Sean Paul. Ended up rewriting
>   these helpers again, but this time they should actually be correct!
> Changes since v2:
> * Small indenting fix
> * Fix pbn_used check in drm_dp_mst_atomic_check_port_bw_limit()
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST =
atomic check")
> Cc: Mikita Lipski <mikita.lipski@amd.com>
> Cc: Sean Paul <seanpaul@google.com>
> Cc: Hans de Goede <hdegoede@redhat.com>

Thanks for the detailed descriptions.  The changes make sense to me,
but I don't know the DP MST code that well, so patches 2-4 are:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 119 ++++++++++++++++++++------
>  1 file changed, 93 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_=
dp_mst_topology.c
> index b81ad444c24f..d2f464bdcfff 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4841,41 +4841,102 @@ static bool drm_dp_mst_port_downstream_of_branch=
(struct drm_dp_mst_port *port,
>         return false;
>  }
>
> -static inline
> -int drm_dp_mst_atomic_check_bw_limit(struct drm_dp_mst_branch *branch,
> -                                    struct drm_dp_mst_topology_state *ms=
t_state)
> +static int
> +drm_dp_mst_atomic_check_port_bw_limit(struct drm_dp_mst_port *port,
> +                                     struct drm_dp_mst_topology_state *s=
tate);
> +
> +static int
> +drm_dp_mst_atomic_check_mstb_bw_limit(struct drm_dp_mst_branch *mstb,
> +                                     struct drm_dp_mst_topology_state *s=
tate)
>  {
> -       struct drm_dp_mst_port *port;
>         struct drm_dp_vcpi_allocation *vcpi;
> -       int pbn_limit =3D 0, pbn_used =3D 0;
> +       struct drm_dp_mst_port *port;
> +       int pbn_used =3D 0, ret;
> +       bool found =3D false;
>
> -       list_for_each_entry(port, &branch->ports, next) {
> -               if (port->mstb)
> -                       if (drm_dp_mst_atomic_check_bw_limit(port->mstb, =
mst_state))
> -                               return -ENOSPC;
> +       /* Check that we have at least one port in our state that's downs=
tream
> +        * of this branch, otherwise we can skip this branch
> +        */
> +       list_for_each_entry(vcpi, &state->vcpis, next) {
> +               if (!vcpi->pbn ||
> +                   !drm_dp_mst_port_downstream_of_branch(vcpi->port, mst=
b))
> +                       continue;
>
> -               if (port->full_pbn > 0)
> -                       pbn_limit =3D port->full_pbn;
> +               found =3D true;
> +               break;
>         }
> -       DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n",
> -                        branch, pbn_limit);
> +       if (!found)
> +               return 0;
>
> -       list_for_each_entry(vcpi, &mst_state->vcpis, next) {
> -               if (!vcpi->pbn)
> -                       continue;
> +       if (mstb->port_parent)
> +               DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] Checking bandwi=
dth limits on [MSTB:%p]\n",
> +                                mstb->port_parent->parent, mstb->port_pa=
rent,
> +                                mstb);
> +       else
> +               DRM_DEBUG_ATOMIC("[MSTB:%p] Checking bandwidth limits\n",
> +                                mstb);
>
> -               if (drm_dp_mst_port_downstream_of_branch(vcpi->port, bran=
ch))
> -                       pbn_used +=3D vcpi->pbn;
> +       list_for_each_entry(port, &mstb->ports, next) {
> +               ret =3D drm_dp_mst_atomic_check_port_bw_limit(port, state=
);
> +               if (ret < 0)
> +                       return ret;
> +
> +               pbn_used +=3D ret;
>         }
> -       DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch used %d PBN\n",
> -                        branch, pbn_used);
>
> -       if (pbn_used > pbn_limit) {
> -               DRM_DEBUG_ATOMIC("[MST BRANCH:%p] No available bandwidth\=
n",
> -                                branch);
> +       return pbn_used;
> +}
> +
> +static int
> +drm_dp_mst_atomic_check_port_bw_limit(struct drm_dp_mst_port *port,
> +                                     struct drm_dp_mst_topology_state *s=
tate)
> +{
> +       struct drm_dp_vcpi_allocation *vcpi;
> +       int pbn_used =3D 0;
> +
> +       if (port->pdt =3D=3D DP_PEER_DEVICE_NONE)
> +               return 0;
> +
> +       if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
> +               bool found =3D false;
> +
> +               list_for_each_entry(vcpi, &state->vcpis, next) {
> +                       if (vcpi->port !=3D port)
> +                               continue;
> +                       if (!vcpi->pbn)
> +                               return 0;
> +
> +                       found =3D true;
> +                       break;
> +               }
> +               if (!found)
> +                       return 0;
> +
> +               /* This should never happen, as it means we tried to
> +                * set a mode before querying the full_pbn
> +                */
> +               if (WARN_ON(!port->full_pbn))
> +                       return -EINVAL;
> +
> +               pbn_used =3D vcpi->pbn;
> +       } else {
> +               pbn_used =3D drm_dp_mst_atomic_check_mstb_bw_limit(port->=
mstb,
> +                                                                state);
> +               if (pbn_used <=3D 0)
> +                       return pbn_used;
> +       }
> +
> +       if (pbn_used > port->full_pbn) {
> +               DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] required PBN of=
 %d exceeds port limit of %d\n",
> +                                port->parent, port, pbn_used,
> +                                port->full_pbn);
>                 return -ENOSPC;
>         }
> -       return 0;
> +
> +       DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] uses %d out of %d PBN\n=
",
> +                        port->parent, port, pbn_used, port->full_pbn);
> +
> +       return pbn_used;
>  }
>
>  static inline int
> @@ -5073,9 +5134,15 @@ int drm_dp_mst_atomic_check(struct drm_atomic_stat=
e *state)
>                 ret =3D drm_dp_mst_atomic_check_vcpi_alloc_limit(mgr, mst=
_state);
>                 if (ret)
>                         break;
> -               ret =3D drm_dp_mst_atomic_check_bw_limit(mgr->mst_primary=
, mst_state);
> -               if (ret)
> +
> +               mutex_lock(&mgr->lock);
> +               ret =3D drm_dp_mst_atomic_check_mstb_bw_limit(mgr->mst_pr=
imary,
> +                                                           mst_state);
> +               mutex_unlock(&mgr->lock);
> +               if (ret < 0)
>                         break;
> +               else
> +                       ret =3D 0;
>         }
>
>         return ret;
> --
> 2.24.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel