RE: [PATCH 2/2] drm/dp_mst: Handle SST-only branch device case

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[AMD Public Use]



> -----Original Message-----
> From: Lyude Paul <lyude@xxxxxxxxxx>
> Sent: Wednesday, January 15, 2020 5:19 AM
> To: Lin, Wayne <Wayne.Lin@xxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx>; Wentland, Harry
> <Harry.Wentland@xxxxxxx>; Zuo, Jerry <Jerry.Zuo@xxxxxxx>; Ville Syrjälä
> <ville.syrjala@xxxxxxxxxxxxxxx>; Wentland, Harry <Harry.Wentland@xxxxxxx>
> Subject: Re: [PATCH 2/2] drm/dp_mst: Handle SST-only branch device case
> 
> On Wed, 2020-01-08 at 16:44 +0800, Wayne Lin wrote:
> > [Why]
> > While handling LINK_ADDRESS reply, current code expects a peer device
> > can handle sideband message once the peer device type is reported as
> > DP_PEER_DEVICE_MST_BRANCHING. However, when the connected device
> is a
> > SST branch case, it can't handle the sideband message(MST_CAP=0 in
> > DPCD 00021h).
> >
> > Current code will try to send LINK_ADDRESS to SST branch device and
> > end up with message timeout and monitor can't display normally. As the
> > result of that, we should take SST branch device into account.
> >
> > [How]
> > According to DP 1.4 spec, we can use Peer_Device_Type as
> > DP_PEER_DEVICE_MST_BRANCHING and Message_Capability_Status as 0 to
> > indicate peer device as a SST-only branch device.
> >
> > Fix following:
> > - Take SST-only branch device case into account in
> > drm_dp_port_set_pdt() and add a new parameter 'new_mcs'. Take sst
> > branch device case as the same case as
> DP_PEER_DEVICE_DP_LEGACY_CONV
> > and DP_PEER_DEVICE_SST_SINK. All original handling logics remain.
> > - Take SST-only branch device case into account in
> > drm_dp_mst_port_add_connector().
> > - Fix some parts in drm_dp_mst_handle_link_address_port() to have SST
> > branch device case into consideration.
> > - Fix the arguments of drm_dp_port_set_pdt() in
> > drm_dp_mst_handle_conn_stat().
> > - Have SST branch device also report
> > connector_status_connected when the ddps is true in
> > drm_dp_mst_detect_port()
> > - Fix the arguments of drm_dp_port_set_pdt() in
> > drm_dp_delayed_destroy_port()
> >
> > Fixes: c485e2c97dae ("drm/dp_mst: Refactor pdt setup/teardown, add
> > more
> > locking")
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Harry Wentland <hwentlan@xxxxxxx>
> > Cc: Lyude Paul <lyude@xxxxxxxxxx>
> > Signed-off-by: Wayne Lin <Wayne.Lin@xxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 131
> > +++++++++++++-------------
> >  1 file changed, 68 insertions(+), 63 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 8f54b241db08..4395d5cc0645 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1934,73 +1934,74 @@ static bool
> drm_dp_mst_is_dp_mst_end_device(u8
> > pdt, bool mcs)
> >  	return true;
> >  }
> >
> > -static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8
> > new_pdt)
> > +static int
> > +drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt,
> > +		    bool new_mcs)
> >  {
> >  	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> >  	struct drm_dp_mst_branch *mstb;
> >  	u8 rad[8], lct;
> >  	int ret = 0;
> >
> > -	if (port->pdt == new_pdt)
> > +	if (port->pdt == new_pdt && port->mcs == new_mcs)
> >  		return 0;
> >
> >  	/* Teardown the old pdt, if there is one */
> > -	switch (port->pdt) {
> > -	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > -	case DP_PEER_DEVICE_SST_SINK:
> > -		/*
> > -		 * If the new PDT would also have an i2c bus, don't bother
> > -		 * with reregistering it
> > -		 */
> > -		if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > -		    new_pdt == DP_PEER_DEVICE_SST_SINK) {
> > -			port->pdt = new_pdt;
> > -			return 0;
> > -		}
> > +	if (port->pdt != DP_PEER_DEVICE_NONE) {
> > +		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
> > +			/*
> > +			 * If the new PDT would also have an i2c bus,
> > +			 * don't bother with reregistering it
> > +			 */
> > +			if (new_pdt != DP_PEER_DEVICE_NONE &&
> > +			    drm_dp_mst_is_dp_mst_end_device(new_pdt, new_mcs))
> > {
> > +				port->pdt = new_pdt;
> > +				port->mcs = new_mcs;
> > +				return 0;
> > +			}
> >
> > -		/* remove i2c over sideband */
> > -		drm_dp_mst_unregister_i2c_bus(&port->aux);
> > -		break;
> > -	case DP_PEER_DEVICE_MST_BRANCHING:
> > -		mutex_lock(&mgr->lock);
> > -		drm_dp_mst_topology_put_mstb(port->mstb);
> > -		port->mstb = NULL;
> > -		mutex_unlock(&mgr->lock);
> > -		break;
> > +			/* remove i2c over sideband */
> > +			drm_dp_mst_unregister_i2c_bus(&port->aux);
> > +		} else {
> > +			mutex_lock(&mgr->lock);
> > +			drm_dp_mst_topology_put_mstb(port->mstb);
> > +			port->mstb = NULL;
> > +			mutex_unlock(&mgr->lock);
> > +		}
> >  	}
> >
> >  	port->pdt = new_pdt;
> > -	switch (port->pdt) {
> > -	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > -	case DP_PEER_DEVICE_SST_SINK:
> > -		/* add i2c over sideband */
> > -		ret = drm_dp_mst_register_i2c_bus(&port->aux);
> > -		break;
> > +	port->mcs = new_mcs;
> >
> > -	case DP_PEER_DEVICE_MST_BRANCHING:
> > -		lct = drm_dp_calculate_rad(port, rad);
> > -		mstb = drm_dp_add_mst_branch_device(lct, rad);
> > -		if (!mstb) {
> > -			ret = -ENOMEM;
> > -			DRM_ERROR("Failed to create MSTB for port %p", port);
> > -			goto out;
> > -		}
> > +	if (port->pdt != DP_PEER_DEVICE_NONE) {
> > +		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
> > +			/* add i2c over sideband */
> > +			ret = drm_dp_mst_register_i2c_bus(&port->aux);
> > +		} else {
> > +			lct = drm_dp_calculate_rad(port, rad);
> > +			mstb = drm_dp_add_mst_branch_device(lct, rad);
> > +			if (!mstb) {
> > +				ret = -ENOMEM;
> > +				DRM_ERROR("Failed to create MSTB for port %p",
> > +					  port);
> > +				goto out;
> > +			}
> >
> > -		mutex_lock(&mgr->lock);
> > -		port->mstb = mstb;
> > -		mstb->mgr = port->mgr;
> > -		mstb->port_parent = port;
> > +			mutex_lock(&mgr->lock);
> > +			port->mstb = mstb;
> > +			mstb->mgr = port->mgr;
> > +			mstb->port_parent = port;
> >
> > -		/*
> > -		 * Make sure this port's memory allocation stays
> > -		 * around until its child MSTB releases it
> > -		 */
> > -		drm_dp_mst_get_port_malloc(port);
> > -		mutex_unlock(&mgr->lock);
> > +			/*
> > +			 * Make sure this port's memory allocation stays
> > +			 * around until its child MSTB releases it
> > +			 */
> > +			drm_dp_mst_get_port_malloc(port);
> > +			mutex_unlock(&mgr->lock);
> >
> > -		/* And make sure we send a link address for this */
> > -		ret = 1;
> > -		break;
> > +			/* And make sure we send a link address for this */
> > +			ret = 1;
> > +		}
> >  	}
> >
> >  out:
> > @@ -2153,12 +2154,12 @@ drm_dp_mst_port_add_connector(struct
> > drm_dp_mst_branch *mstb,
> >  		goto error;
> >  	}
> >
> > -	if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > -	     port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> > -	    port->port_num >= DP_MST_LOGICAL_PORT_0) {
> > -		port->cached_edid = drm_get_edid(port->connector,
> > -						 &port->aux.ddc);
> > -		drm_connector_set_tile_property(port->connector);
> > +	if (port->pdt != DP_PEER_DEVICE_NONE) {
> > +		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
> > +			port->cached_edid = drm_get_edid(port->connector,
> > +							 &port->aux.ddc);
> > +			drm_connector_set_tile_property(port->connector);
> > +		}
> 
> I'd combine these two if statements here into one, otherwise this looks great.
> Thank you for all of the great fixes recently :)
> 
> Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
Thanks for your time!
I will fix it in the new version.
Thanks.
> 
> 
> >  	}
> >
> >  	mgr->cbs->register_connector(port->connector);
> > @@ -2223,6 +2224,7 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> >  	struct drm_dp_mst_port *port;
> >  	int old_ddps = 0, ret;
> >  	u8 new_pdt = DP_PEER_DEVICE_NONE;
> > +	bool new_mcs = 0;
> >  	bool created = false, send_link_addr = false, changed = false;
> >
> >  	port = drm_dp_get_port(mstb, port_msg->port_number); @@ -2267,7
> > +2269,7 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> >  	port->input = port_msg->input_port;
> >  	if (!port->input)
> >  		new_pdt = port_msg->peer_device_type;
> > -	port->mcs = port_msg->mcs;
> > +	new_mcs = port_msg->mcs;
> >  	port->ddps = port_msg->ddps;
> >  	port->ldps = port_msg->legacy_device_plug_status;
> >  	port->dpcd_rev = port_msg->dpcd_revision; @@ -2295,7 +2297,7 @@
> > drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> >  		}
> >  	}
> >
> > -	ret = drm_dp_port_set_pdt(port, new_pdt);
> > +	ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs);
> >  	if (ret == 1) {
> >  		send_link_addr = true;
> >  	} else if (ret < 0) {
> > @@ -2309,7 +2311,8 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> >  	 * we're coming out of suspend. In this case, always resend the link
> >  	 * address if there's an MSTB on this port
> >  	 */
> > -	if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING)
> > +	if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING &&
> > +	    port->mcs)
> >  		send_link_addr = true;
> >
> >  	if (port->connector)
> > @@ -2346,6 +2349,7 @@ drm_dp_mst_handle_conn_stat(struct
> > drm_dp_mst_branch *mstb,
> >  	struct drm_dp_mst_port *port;
> >  	int old_ddps, old_input, ret, i;
> >  	u8 new_pdt;
> > +	bool new_mcs;
> >  	bool dowork = false, create_connector = false;
> >
> >  	port = drm_dp_get_port(mstb, conn_stat->port_number); @@ -2377,7
> > +2381,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
> *mstb,
> >  	old_ddps = port->ddps;
> >  	old_input = port->input;
> >  	port->input = conn_stat->input_port;
> > -	port->mcs = conn_stat->message_capability_status;
> >  	port->ldps = conn_stat->legacy_device_plug_status;
> >  	port->ddps = conn_stat->displayport_device_plug_status;
> >
> > @@ -2390,8 +2393,8 @@ drm_dp_mst_handle_conn_stat(struct
> > drm_dp_mst_branch *mstb,
> >  	}
> >
> >  	new_pdt = port->input ? DP_PEER_DEVICE_NONE : conn_stat-
> > >peer_device_type;
> > -
> > -	ret = drm_dp_port_set_pdt(port, new_pdt);
> > +	new_mcs = conn_stat->message_capability_status;
> > +	ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs);
> >  	if (ret == 1) {
> >  		dowork = true;
> >  	} else if (ret < 0) {
> > @@ -3958,6 +3961,8 @@ drm_dp_mst_detect_port(struct drm_connector
> > *connector,
> >  	switch (port->pdt) {
> >  	case DP_PEER_DEVICE_NONE:
> >  	case DP_PEER_DEVICE_MST_BRANCHING:
> > +		if (!port->mcs)
> > +			ret = connector_status_connected;
> >  		break;
> >
> >  	case DP_PEER_DEVICE_SST_SINK:
> > @@ -4597,7 +4602,7 @@ drm_dp_delayed_destroy_port(struct
> > drm_dp_mst_port
> > *port)
> >  	if (port->connector)
> >  		port->mgr->cbs->destroy_connector(port->mgr, port->connector);
> >
> > -	drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE);
> > +	drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE, port->mcs);
> >  	drm_dp_mst_put_port_malloc(port);
> >  }
> >
> --
> Cheers,
> 	Lyude Paul
--
Best regards,
Wayne Lin
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux