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;
Date:   Thu, 5 Dec 2019 13:35:01 -0500
From:   Sean Paul <sean@poorly.run>
To:     Lyude Paul <lyude@redhat.com>
Cc:     dri-devel@lists.freedesktop.org, Sean Paul <sean@poorly.run>,
        Sean Paul <seanpaul@chromium.org>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <maxime.ripard@bootlin.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] drm/dp_mst: Clear all payload id tables downstream
 when initializing
Message-ID: <20191205183501.GC162979@art_vandelay>
References: <20190829000944.20722-1-lyude@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190829000944.20722-1-lyude@redhat.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Aug 28, 2019 at 08:09:44PM -0400, Lyude Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> It seems that on certain MST hubs, namely the CableMatters USB-C 2x DP
> hub, using the DP_PAYLOAD_ALLOCATE_SET and DP_PAYLOAD_TABLE_UPDATE_STATUS
> register ranges to clear any pre-existing payload allocations on the hub isn't
> always enough to reset things if the source device has been reset unexpectedly.
> 
> Or at least, that's the current running theory. The precise behavior appears to
> be that when the source device gets reset unexpectedly, the hub begins reporting
> an available_pbn value of 0 for all of its ports. This is a bit inconsistent
> with the our theory, since this seems to happen even if previously set PBN
> allocations should have resulted in a non-zero available_pbn value. So, it's
> possible that something else may be going on here.
> 
> Strangely though, sending a CLEAR_PAYLOAD_ID_TABLE broadcast request when
> initializing the MST topology seems to bring things into working order and make
> available_pbn work again. Since this is a pretty safe solution, let's go ahead
> and implement it.
> 
> Changes since v1:
> * Change indenting on drm_dp_send_clear_payload_id_table() prototype
> * Remove some braces in drm_dp_send_clear_payload_id_table()
> * Reorganize some variable declarations in drm_dp_send_clear_payload_id_table()
> * Don't forget to handle DP_CLEAR_PAYLOAD_ID_TABLE in
>   drm_dp_sideband_parse_reply()
> * Move drm_dp_send_clear_payload_id_table() call into
>   drm_dp_mst_link_probe_work(), since we can't send sideband messages
>   while under lock in drm_dp_mst_topology_mgr_set_mst()
> * Change commit message
> 
> Change-Id: I2c763e8dae3844eca76033a41f264080052fbbfc
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Lyude Paul <lyude@redhat.com>

Pushed to drm-misc-next without that nasty Change-Id and with danvet's IRC
Acked-by to appease almighty dim.

Thanks, Lyude, for polishing this up :-)

Sean

> ---
> 
> A heads up to anyone looking at this patch: it's quite possible this
> won't be the final solution that we go with. Me and Sean would like to
> do a bit more investigation to try to figure out what exactly is
> happening here before we go ahead and push it, and hopefully figure out
> why available_pbn is being set to 0 instead of some other leftover
> non-zero allocation.
> 
>  drivers/gpu/drm/drm_dp_mst_topology.c | 63 +++++++++++++++++++++++++--
>  include/drm/drm_dp_mst_helper.h       | 16 +++++--
>  2 files changed, 72 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 82add736e17d..969e43b7eb4c 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -64,6 +64,11 @@ static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
>  
>  static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
>  				     struct drm_dp_mst_branch *mstb);
> +
> +static void
> +drm_dp_send_clear_payload_id_table(struct drm_dp_mst_topology_mgr *mgr,
> +				   struct drm_dp_mst_branch *mstb);
> +
>  static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
>  					   struct drm_dp_mst_branch *mstb,
>  					   struct drm_dp_mst_port *port);
> @@ -657,6 +662,8 @@ static bool drm_dp_sideband_parse_reply(struct drm_dp_sideband_msg_rx *raw,
>  	case DP_POWER_DOWN_PHY:
>  	case DP_POWER_UP_PHY:
>  		return drm_dp_sideband_parse_power_updown_phy_ack(raw, msg);
> +	case DP_CLEAR_PAYLOAD_ID_TABLE:
> +		return true; /* since there's nothing to parse */
>  	default:
>  		DRM_ERROR("Got unknown reply 0x%02x (%s)\n", msg->req_type,
>  			  drm_dp_mst_req_type_str(msg->req_type));
> @@ -755,6 +762,15 @@ static int build_link_address(struct drm_dp_sideband_msg_tx *msg)
>  	return 0;
>  }
>  
> +static int build_clear_payload_id_table(struct drm_dp_sideband_msg_tx *msg)
> +{
> +	struct drm_dp_sideband_msg_req_body req;
> +
> +	req.req_type = DP_CLEAR_PAYLOAD_ID_TABLE;
> +	drm_dp_encode_sideband_req(&req, msg);
> +	return 0;
> +}
> +
>  static int build_enum_path_resources(struct drm_dp_sideband_msg_tx *msg, int port_num)
>  {
>  	struct drm_dp_sideband_msg_req_body req;
> @@ -1877,8 +1893,12 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work)
>  	struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, work);
>  	struct drm_dp_mst_branch *mstb;
>  	int ret;
> +	bool clear_payload_id_table;
>  
>  	mutex_lock(&mgr->lock);
> +	clear_payload_id_table = !mgr->payload_id_table_cleared;
> +	mgr->payload_id_table_cleared = true;
> +
>  	mstb = mgr->mst_primary;
>  	if (mstb) {
>  		ret = drm_dp_mst_topology_try_get_mstb(mstb);
> @@ -1886,10 +1906,24 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work)
>  			mstb = NULL;
>  	}
>  	mutex_unlock(&mgr->lock);
> -	if (mstb) {
> -		drm_dp_check_and_send_link_address(mgr, mstb);
> -		drm_dp_mst_topology_put_mstb(mstb);
> +	if (!mstb)
> +		return;
> +
> +	/*
> +	 * Certain branch devices seem to incorrectly report an available_pbn
> +	 * of 0 on downstream sinks, even after clearing the
> +	 * DP_PAYLOAD_ALLOCATE_* registers in
> +	 * drm_dp_mst_topology_mgr_set_mst(). Namely, the CableMatters USB-C
> +	 * 2x DP hub. Sending a CLEAR_PAYLOAD_ID_TABLE message seems to make
> +	 * things work again.
> +	 */
> +	if (clear_payload_id_table) {
> +		DRM_DEBUG_KMS("Clearing payload ID table\n");
> +		drm_dp_send_clear_payload_id_table(mgr, mstb);
>  	}
> +
> +	drm_dp_check_and_send_link_address(mgr, mstb);
> +	drm_dp_mst_topology_put_mstb(mstb);
>  }
>  
>  static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
> @@ -2156,6 +2190,28 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
>  	kfree(txmsg);
>  }
>  
> +void drm_dp_send_clear_payload_id_table(struct drm_dp_mst_topology_mgr *mgr,
> +					struct drm_dp_mst_branch *mstb)
> +{
> +	struct drm_dp_sideband_msg_tx *txmsg;
> +	int len, ret;
> +
> +	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
> +	if (!txmsg)
> +		return;
> +
> +	txmsg->dst = mstb;
> +	len = build_clear_payload_id_table(txmsg);
> +
> +	drm_dp_queue_down_tx(mgr, txmsg);
> +
> +	ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
> +	if (ret > 0 && txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK)
> +		DRM_DEBUG_KMS("clear payload table id nak received\n");
> +
> +	kfree(txmsg);
> +}
> +
>  static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
>  					   struct drm_dp_mst_branch *mstb,
>  					   struct drm_dp_mst_port *port)
> @@ -2756,6 +2812,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
>  		mgr->payload_mask = 0;
>  		set_bit(0, &mgr->payload_mask);
>  		mgr->vcpi_mask = 0;
> +		mgr->payload_id_table_cleared = false;
>  	}
>  
>  out_unlock:
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 2ba6253ea6d3..ee4093c1bba3 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -494,15 +494,23 @@ struct drm_dp_mst_topology_mgr {
>  	struct drm_dp_sideband_msg_rx up_req_recv;
>  
>  	/**
> -	 * @lock: protects mst state, primary, dpcd.
> +	 * @lock: protects @mst_state, @mst_primary, @dpcd, and
> +	 * @payload_id_table_cleared.
>  	 */
>  	struct mutex lock;
>  
>  	/**
> -	 * @mst_state: If this manager is enabled for an MST capable port. False
> -	 * if no MST sink/branch devices is connected.
> +	 * @mst_state: If this manager is enabled for an MST capable port.
> +	 * False if no MST sink/branch devices is connected.
>  	 */
> -	bool mst_state;
> +	bool mst_state : 1;
> +
> +	/**
> +	 * @payload_id_table_cleared: Whether or not we've cleared the payload
> +	 * ID table for @mst_primary. Protected by @lock.
> +	 */
> +	bool payload_id_table_cleared : 1;
> +
>  	/**
>  	 * @mst_primary: Pointer to the primary/first branch device.
>  	 */
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS