Message-ID: <8272905f-3966-4fcd-d78f-e88063cf53f8@amd.com>
Date:   Tue, 10 Mar 2020 13:51:58 -0400
From:   Mikita Lipski <mlipski@....com>
To:     Lyude Paul <lyude@...hat.com>, dri-devel@...ts.freedesktop.org
Cc:     Mikita Lipski <mikita.lipski@....com>,
        Sean Paul <seanpaul@...gle.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Alex Deucher <alexander.deucher@....com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] drm/dp_mst: Rewrite and fix bandwidth limit checks



On 3/9/20 5:01 PM, Lyude Paul 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 → |          | ← 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()
> 

Thank you for rewriting the bandwidth check helper!

My initial understanding of available_pbn was completely wrong and 
therefore the bandwidth validation was not doing what it intended.
This version is much cleaner and  easier to follow than the initial one, 
since you're separating branch and port validation into 2 different 
functions, and also go down the device topology rather than starting 
from the end nodes. Also the explanation and the diagram help a lot to 
understand how it should have be done initially.

This change makes sense and looks correct to me, therefore:
Reviewed-by: Mikita Lipski <mikita.lipski@....com>

Thanks,
Mikita


> Signed-off-by: Lyude Paul <lyude@...hat.com>
> Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check")
> Cc: Mikita Lipski <mikita.lipski@....com>
> Cc: Sean Paul <seanpaul@...gle.com>
> Cc: Hans de Goede <hdegoede@...hat.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 *mst_state)
> +static int
> +drm_dp_mst_atomic_check_port_bw_limit(struct drm_dp_mst_port *port,
> +				      struct drm_dp_mst_topology_state *state);
> +
> +static int
> +drm_dp_mst_atomic_check_mstb_bw_limit(struct drm_dp_mst_branch *mstb,
> +				      struct drm_dp_mst_topology_state *state)
>   {
> -	struct drm_dp_mst_port *port;
>   	struct drm_dp_vcpi_allocation *vcpi;
> -	int pbn_limit = 0, pbn_used = 0;
> +	struct drm_dp_mst_port *port;
> +	int pbn_used = 0, ret;
> +	bool found = 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 downstream
> +	 * 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, mstb))
> +			continue;
>   
> -		if (port->full_pbn > 0)
> -			pbn_limit = port->full_pbn;
> +		found = 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 bandwidth limits on [MSTB:%p]\n",
> +				 mstb->port_parent->parent, mstb->port_parent,
> +				 mstb);
> +	else
> +		DRM_DEBUG_ATOMIC("[MSTB:%p] Checking bandwidth limits\n",
> +				 mstb);
>   
> -		if (drm_dp_mst_port_downstream_of_branch(vcpi->port, branch))
> -			pbn_used += vcpi->pbn;
> +	list_for_each_entry(port, &mstb->ports, next) {
> +		ret = drm_dp_mst_atomic_check_port_bw_limit(port, state);
> +		if (ret < 0)
> +			return ret;
> +
> +		pbn_used += 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 *state)
> +{
> +	struct drm_dp_vcpi_allocation *vcpi;
> +	int pbn_used = 0;
> +
> +	if (port->pdt == DP_PEER_DEVICE_NONE)
> +		return 0;
> +
> +	if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
> +		bool found = false;
> +
> +		list_for_each_entry(vcpi, &state->vcpis, next) {
> +			if (vcpi->port != port)
> +				continue;
> +			if (!vcpi->pbn)
> +				return 0;
> +
> +			found = 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 = vcpi->pbn;
> +	} else {
> +		pbn_used = drm_dp_mst_atomic_check_mstb_bw_limit(port->mstb,
> +								 state);
> +		if (pbn_used <= 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_state *state)
>   		ret = drm_dp_mst_atomic_check_vcpi_alloc_limit(mgr, mst_state);
>   		if (ret)
>   			break;
> -		ret = drm_dp_mst_atomic_check_bw_limit(mgr->mst_primary, mst_state);
> -		if (ret)
> +
> +		mutex_lock(&mgr->lock);
> +		ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr->mst_primary,
> +							    mst_state);
> +		mutex_unlock(&mgr->lock);
> +		if (ret < 0)
>   			break;
> +		else
> +			ret = 0;
>   	}
>   
>   	return ret;
> 

-- 
Thanks,
Mikita Lipski
Software Engineer 2, AMD
mikita.lipski@....com

Powered by blists - more mailing lists