Message-ID: <143de1a1-c59f-6ff0-501c-1467872bc9d9@amd.com>
Date:   Tue, 10 Mar 2020 11:35:40 -0400
From:   Mikita Lipski <mlipski@....com>
To:     Lyude Paul <lyude@...hat.com>, dri-devel@...ts.freedesktop.org
Cc:     Mikita Lipski <mikita.lipski@....com>,
        Hans de Goede <hdegoede@...hat.com>,
        Sean Paul <sean@...rly.run>,
        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 v2 2/4] drm/dp_mst: Use full_pbn instead of available_pbn
 for bandwidth checks



On 3/6/20 6:46 PM, Lyude Paul wrote:
> DisplayPort specifications are fun. For a while, it's been really
> unclear to us what available_pbn actually does. There's a somewhat vague
> explanation in the DisplayPort spec (starting from 1.2) that partially
> explains it:
> 
>    The minimum payload bandwidth number supported by the path. Each node
>    updates this number with its available payload bandwidth number if its
>    payload bandwidth number is less than that in the Message Transaction
>    reply.
> 
> So, it sounds like available_pbn represents the smallest link rate in
> use between the source and the branch device. Cool, so full_pbn is just
> the highest possible PBN that the branch device supports right?
> 
> Well, we assumed that for quite a while until Sean Paul noticed that on
> some MST hubs, available_pbn will actually get set to 0 whenever there's
> any active payloads on the respective branch device. This caused quite a
> bit of confusion since clearing the payload ID table would end up fixing
> the available_pbn value.
> 
> So, we just went with that until commit cd82d82cbc04 ("drm/dp_mst: Add
> branch bandwidth validation to MST atomic check") started breaking
> people's setups due to us getting erroneous available_pbn values. So, we
> did some more digging and got confused until we finally looked at the
> definition for full_pbn:
> 
>    The bandwidth of the link at the trained link rate and lane count
>    between the DP Source device and the DP Sink device with no time slots
>    allocated to VC Payloads, represented as a Payload Bandwidth Number. As
>    with the Available_Payload_Bandwidth_Number, this number is determined
>    by the link with the lowest lane count and link rate.
> 
> That's what we get for not reading specs closely enough, hehe. So, since
> full_pbn is definitely what we want for doing bandwidth restriction
> checks - let's start using that instead and ignore available_pbn
> entirely.
> 


Thank you for the fix and a very detailed explanation.
I was really confused by available_pbn and why it wouldn't get updated 
and was searching for the solution in wrong places. But I'm glad you 
were able to identify the solution.
I still think the port should have an available_pbn value so it could be 
updated when driver parses RESOURCE_STATUS_NOTIFY and 
ENUM_PATH_RESOURCES messages.
With that being said it is a great find. Thanks.

Reviewed-by: Mikita Lipski <mikita.lipski@....com>

> 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: Hans de Goede <hdegoede@...hat.com>
> Cc: Sean Paul <sean@...rly.run>
> ---
>   drivers/gpu/drm/drm_dp_mst_topology.c | 15 +++++++--------
>   include/drm/drm_dp_mst_helper.h       |  4 ++--
>   2 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 6714d8a5c558..79ebb871230b 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2306,7 +2306,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
>   								port);
>   			}
>   		} else {
> -			port->available_pbn = 0;
> +			port->full_pbn = 0;
>   		}
>   	}
>   
> @@ -2401,7 +2401,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
>   		if (port->ddps) {
>   			dowork = true;
>   		} else {
> -			port->available_pbn = 0;
> +			port->full_pbn = 0;
>   		}
>   	}
>   
> @@ -2553,7 +2553,7 @@ static int drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *mg
>   		if (port->input || !port->ddps)
>   			continue;
>   
> -		if (!port->available_pbn) {
> +		if (!port->full_pbn) {
>   			drm_modeset_lock(&mgr->base.lock, NULL);
>   			drm_dp_send_enum_path_resources(mgr, mstb, port);
>   			drm_modeset_unlock(&mgr->base.lock);
> @@ -2999,8 +2999,7 @@ drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
>   				      path_res->port_number,
>   				      path_res->full_payload_bw_number,
>   				      path_res->avail_payload_bw_number);
> -			port->available_pbn =
> -				path_res->avail_payload_bw_number;
> +			port->full_pbn = path_res->full_payload_bw_number;
>   			port->fec_capable = path_res->fec_capable;
>   		}
>   	}
> @@ -3585,7 +3584,7 @@ drm_dp_mst_topology_mgr_invalidate_mstb(struct drm_dp_mst_branch *mstb)
>   
>   	list_for_each_entry(port, &mstb->ports, next) {
>   		/* The PBN for each port will also need to be re-probed */
> -		port->available_pbn = 0;
> +		port->full_pbn = 0;
>   
>   		if (port->mstb)
>   			drm_dp_mst_topology_mgr_invalidate_mstb(port->mstb);
> @@ -4853,8 +4852,8 @@ int drm_dp_mst_atomic_check_bw_limit(struct drm_dp_mst_branch *branch,
>   			if (drm_dp_mst_atomic_check_bw_limit(port->mstb, mst_state))
>   				return -ENOSPC;
>   
> -		if (port->available_pbn > 0)
> -			pbn_limit = port->available_pbn;
> +		if (port->full_pbn > 0)
> +			pbn_limit = port->full_pbn;
>   	}
>   	DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n",
>   			 branch, pbn_limit);
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 5483f888712a..1d4c996cb92d 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -81,7 +81,7 @@ struct drm_dp_vcpi {
>    * &drm_dp_mst_topology_mgr.base.lock.
>    * @num_sdp_stream_sinks: Number of stream sinks. Protected by
>    * &drm_dp_mst_topology_mgr.base.lock.
> - * @available_pbn: Available bandwidth for this port. Protected by
> + * @full_pbn: Max possible bandwidth for this port. Protected by
>    * &drm_dp_mst_topology_mgr.base.lock.
>    * @next: link to next port on this branch device
>    * @aux: i2c aux transport to talk to device connected to this port, protected
> @@ -126,7 +126,7 @@ struct drm_dp_mst_port {
>   	u8 dpcd_rev;
>   	u8 num_sdp_streams;
>   	u8 num_sdp_stream_sinks;
> -	uint16_t available_pbn;
> +	uint16_t full_pbn;
>   	struct list_head next;
>   	/**
>   	 * @mstb: the branch device connected to this port, if there is one.
> 

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

Powered by blists - more mailing lists