Re: [PATCH 02/10] drm/dp_mst: Enable registration of AUX devices for MST ports (v2)

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

 



Some small nitpicks

On Thu, 2019-07-04 at 15:05 -0400, sunpeng.li@xxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> All available downstream ports - physical and logical - are exposed for
> each MST device. They are listed in /dev/, following the same naming
> scheme as SST devices by appending an incremental ID.
> 
> Although all downstream ports are exposed, only some will work as
> expected. Consider the following topology:
> 
>                +---------+
>                |  ASIC   |
>                +---------+
>               Conn-0|
>                     |
>                +----v----+
>           +----| MST HUB |----+
>           |    +---------+    |
>           |                   |
>           |Port-1       Port-2|
>     +-----v-----+       +-----v-----+
>     |  MST      |       |  SST      |
>     |  Display  |       |  Display  |
>     +-----------+       +-----------+
>           |Port-1
>           x
> 
>  MST Path  | MST Device
>  ----------+----------------------------------
>  sst:0     | MST Hub
>  mst:0-1   | MST Display
>  mst:0-1-1 | MST Display's disconnected DP out
>  mst:0-1-8 | MST Display's internal sink
>  mst:0-2   | SST Display
> 
> On certain MST displays, the upstream physical port will ACK DPCD reads.
> However, reads on the local logical port to the internal sink will
> *NAK*. i.e. reading mst:0-1 ACKs, but mst:0-1-8 NAKs.
> 
> There may also be duplicates. Some displays will return the same GUID
> when reading DPCD from both mst:0-1 and mst:0-1-8.
> 
> There are some device-dependent behavior as well. The MST hub used
> during testing will actually *ACK* read requests on a disconnected
> physical port, whereas the MST displays will NAK.
> 
> In light of these discrepancies, it's simpler to expose all downstream
> ports - both physical and logical - and let the user decide what to use.
> 
> (v2) changes:
> 
> Moved remote aux device (un)registration to new mst connector late
> register and early unregister helpers. Drivers should call these from
> their own mst connector function hooks.
> 
> This is to solve an issue during driver unload, where mst connector
> devices are unregistered before the remote aux devices are. In a setup
> where aux devices are created as children of connector devices, the aux
> device would be removed too early, and uncleanly. Doing so in
> early_unregister solves this issue, as that is called before connector
> unregistration.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Leo Li <sunpeng.li@xxxxxxx>
> ---
>  drivers/gpu/drm/drm_dp_aux_dev.c      |  16 ++-
>  drivers/gpu/drm/drm_dp_mst_topology.c | 137 ++++++++++++++++++++++++--
>  include/drm/drm_dp_helper.h           |   4 +
>  include/drm/drm_dp_mst_helper.h       |  11 +++
>  4 files changed, 156 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
> b/drivers/gpu/drm/drm_dp_aux_dev.c
> index 26e38dacf654..4aa5e455e894 100644
> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> @@ -37,6 +37,7 @@
>  
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_dp_helper.h>
> +#include <drm/drm_dp_mst_helper.h>
>  #include <drm/drm_print.h>
>  
>  #include "drm_crtc_helper_internal.h"
> @@ -116,6 +117,7 @@ static ssize_t name_show(struct device *dev,
>  
>  	return res;
>  }
> +
>  static DEVICE_ATTR_RO(name);
>  
>  static struct attribute *drm_dp_aux_attrs[] = {
> @@ -162,7 +164,12 @@ static ssize_t auxdev_read_iter(struct kiocb *iocb,
> struct iov_iter *to)
>  			break;
>  		}
>  
> -		res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
> +		if (aux_dev->aux->is_remote)
> +			res = drm_dp_mst_dpcd_read(aux_dev->aux, pos, buf,
> +						   todo);
> +		else
> +			res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
> +
>  		if (res <= 0)
>  			break;
>  
> @@ -209,7 +216,12 @@ static ssize_t auxdev_write_iter(struct kiocb *iocb,
> struct iov_iter *from)
>  			break;
>  		}
>  
> -		res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
> +		if (aux_dev->aux->is_remote)
> +			res = drm_dp_mst_dpcd_write(aux_dev->aux, pos, buf,
> +						    todo);
> +		else
> +			res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
> +
>  		if (res <= 0)
>  			break;
>  
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 0984b9a34d55..dde79c44b625 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -36,6 +36,8 @@
>  #include <drm/drm_print.h>
>  #include <drm/drm_probe_helper.h>
>  
> +#include "drm_crtc_helper_internal.h"
> +
>  /**
>   * DOC: dp mst helper
>   *
> @@ -53,6 +55,9 @@ static int drm_dp_dpcd_write_payload(struct
> drm_dp_mst_topology_mgr *mgr,
>  				     int id,
>  				     struct drm_dp_payload *payload);
>  
> +static int drm_dp_send_dpcd_read(struct drm_dp_mst_topology_mgr *mgr,
> +				 struct drm_dp_mst_port *port,
> +				 int offset, int size, u8 *bytes);
>  static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
>  				  struct drm_dp_mst_port *port,
>  				  int offset, int size, u8 *bytes);
> @@ -1238,6 +1243,9 @@ static void drm_dp_destroy_port(struct kref *kref)
>  	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
>  
>  	if (!port->input) {
> +

We should get rid of the extra newline right here ^

> +		port->vcpi.num_slots = 0;
> +
>  		kfree(port->cached_edid);
>  
>  		/*
> @@ -1483,6 +1491,48 @@ static bool drm_dp_port_setup_pdt(struct
> drm_dp_mst_port *port)
>  	return send_link;
>  }
>  
> +/**
> + * drm_dp_mst_dpcd_read() - read a series of bytes from the DPCD via
> sideband
> + * @aux: Fake sideband AUX CH
> + * @offset: address of the (first) register to read
> + * @buffer: buffer to store the register values
> + * @size: number of bytes in @buffer
> + *
> + * Performs the same functionality for remote devices via
> + * sideband messaging as drm_dp_dpcd_read() does for local
> + * devices via actual AUX CH.
> + */

You forgot to document the return value

> +ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux,
> +			     unsigned int offset, void *buffer, size_t size)
> +{
> +	struct drm_dp_mst_port *port = container_of(aux, struct
> drm_dp_mst_port,
> +						    aux);
> +
> +	return drm_dp_send_dpcd_read(port->mgr, port,
> +				     offset, size, buffer);
> +}
> +
> +/**
> + * drm_dp_mst_dpcd_write() - write a series of bytes to the DPCD via
> sideband
> + * @aux: Fake sideband AUX CH
> + * @offset: address of the (first) register to write
> + * @buffer: buffer containing the values to write
> + * @size: number of bytes in @buffer
> + *
> + * Performs the same functionality for remote devices via
> + * sideband messaging as drm_dp_dpcd_write() does for local
> + * devices via actual AUX CH.
> + */

Same for this one…

> +ssize_t drm_dp_mst_dpcd_write(struct drm_dp_aux *aux,
> +			      unsigned int offset, void *buffer, size_t size)
> +{
> +	struct drm_dp_mst_port *port = container_of(aux, struct
> drm_dp_mst_port,
> +						    aux);
> +
> +	return drm_dp_send_dpcd_write(port->mgr, port,
> +				      offset, size, buffer);
> +}
> +
>  static void drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, u8
> *guid)
>  {
>  	int ret;
> @@ -1526,6 +1576,44 @@ static void build_mst_prop_path(const struct
> drm_dp_mst_branch *mstb,
>  	strlcat(proppath, temp, proppath_size);
>  }
>  
> +/**
> + * drm_dp_mst_connector_late_register() - Late MST connector registration
> + * @drm_connector: The MST connector
> + * @port: The MST port for this connector
> + *
> + * Helper to register the remote aux device for this MST port. Drivers
> should
> + * call this from their mst connector's late_register hook to enable MST
> aux
> + * devices.
> + */

…and this one…

> +int drm_dp_mst_connector_late_register(struct drm_connector *connector,
> +				       struct drm_dp_mst_port *port)
> +{
> +	DRM_DEBUG_KMS("registering %s remote bus for %s\n",
> +		      port->aux.name, connector->kdev->kobj.name);
> +
> +	port->aux.dev = connector->kdev;
> +	return drm_dp_aux_register_devnode(&port->aux);
> +}
> +EXPORT_SYMBOL(drm_dp_mst_connector_late_register);
> +
> +/**
> + * drm_dp_mst_connector_early_unregister() - Early MST connector
> unregistration
> + * @drm_connector: The MST connector
> + * @port: The MST port for this connector
> + *
> + * Helper to unregister the remote aux device for this MST port, registered
> by
> + * drm_dp_mst_connector_late_register(). Drivers should call this from
> their mst
> + * connector's early_unregister hook.
> + */

…and this one.

> +void drm_dp_mst_connector_early_unregister(struct drm_connector *connector,
> +					   struct drm_dp_mst_port *port)
> +{
> +	DRM_DEBUG_KMS("unregistering %s remote bus for %s\n",
> +		      port->aux.name, connector->kdev->kobj.name);
> +	drm_dp_aux_unregister_devnode(&port->aux);
> +}
> +EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
> +
>  static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
>  			    struct drm_device *dev,
>  			    struct drm_dp_link_addr_reply_port *port_msg)
> @@ -1548,6 +1636,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
> *mstb,
>  		port->mgr = mstb->mgr;
>  		port->aux.name = "DPMST";
>  		port->aux.dev = dev->dev;
> +		port->aux.is_remote = true;
>  
>  		/*
>  		 * Make sure the memory allocation for our parent branch stays
> @@ -1816,7 +1905,6 @@ static bool drm_dp_validate_guid(struct
> drm_dp_mst_topology_mgr *mgr,
>  	return false;
>  }
>  
> -#if 0
>  static int build_dpcd_read(struct drm_dp_sideband_msg_tx *msg, u8 port_num,
> u32 offset, u8 num_bytes)
>  {
>  	struct drm_dp_sideband_msg_req_body req;
> @@ -1829,7 +1917,6 @@ static int build_dpcd_read(struct
> drm_dp_sideband_msg_tx *msg, u8 port_num, u32
>  
>  	return 0;
>  }
> -#endif
>  
>  static int drm_dp_send_sideband_msg(struct drm_dp_mst_topology_mgr *mgr,
>  				    bool up, u8 *msg, int len)
> @@ -2441,26 +2528,56 @@ int drm_dp_update_payload_part2(struct
> drm_dp_mst_topology_mgr *mgr)
>  }
>  EXPORT_SYMBOL(drm_dp_update_payload_part2);
>  
> -#if 0 /* unused as of yet */
>  static int drm_dp_send_dpcd_read(struct drm_dp_mst_topology_mgr *mgr,
>  				 struct drm_dp_mst_port *port,
> -				 int offset, int size)
> +				 int offset, int size, u8 *bytes)
>  {
>  	int len;
> +	int ret = 0;
>  	struct drm_dp_sideband_msg_tx *txmsg;
> +	struct drm_dp_mst_branch *mstb;
> +
> +	mstb = drm_dp_mst_topology_get_mstb_validated(mgr, port->parent);
> +	if (!mstb)
> +		return -EINVAL;
>  
>  	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
> -	if (!txmsg)
> -		return -ENOMEM;
> +	if (!txmsg) {
> +		ret = -ENOMEM;
> +		goto fail_put;
> +	}
>  
> -	len = build_dpcd_read(txmsg, port->port_num, 0, 8);
> +	len = build_dpcd_read(txmsg, port->port_num, offset, size);
>  	txmsg->dst = port->parent;
>  
>  	drm_dp_queue_down_tx(mgr, txmsg);
>  
> -	return 0;
> +	ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
> +	if (ret < 0)
> +		goto fail_free;
> +
> +	/* DPCD read should never be NACKed */
> +	if (WARN_ON_ONCE(txmsg->reply.reply_type == 1)) {
> +		ret = -EIO;
> +		goto fail_free;
> +	}
> +
> +	if (txmsg->reply.u.remote_dpcd_read_ack.num_bytes != size) {
> +		ret = -EPROTO;
> +		goto fail_free;
> +	}
> +
> +	ret = min_t(size_t, txmsg->reply.u.remote_dpcd_read_ack.num_bytes,
> +		    size);
> +	memcpy(bytes, txmsg->reply.u.remote_dpcd_read_ack.bytes, ret);
> +
> +fail_free:
> +	kfree(txmsg);
> +fail_put:
> +	drm_dp_mst_topology_put_mstb(mstb);
> +
> +	return ret;
>  }
> -#endif
>  
>  static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
>  				  struct drm_dp_mst_port *port,
> @@ -2489,7 +2606,7 @@ static int drm_dp_send_dpcd_write(struct
> drm_dp_mst_topology_mgr *mgr,
>  	ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
>  	if (ret > 0) {
>  		if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK)
> -			ret = -EINVAL;
> +			ret = -EIO;
>  		else
>  			ret = 0;
>  	}
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 397896b5b21a..cb2deface950 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1309,6 +1309,10 @@ struct drm_dp_aux {
>  	 * @cec: struct containing fields used for CEC-Tunneling-over-AUX.
>  	 */
>  	struct drm_dp_aux_cec cec;
> +	/**
> +	 * @is_remote: Is this "AUX CH" actually using sideband messaging.

I'd remove the quotations around AUX CH, but that's up to you. With those
small whitespace and kernel doc changes addressed:

Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>

Thanks for the great work with this!

> +	 */
> +	bool is_remote;
>  };
>  
>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index 8c97a5f92c47..2ba6253ea6d3 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -643,6 +643,17 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
>  void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
>  int __must_check
>  drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
> +
> +ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux,
> +			     unsigned int offset, void *buffer, size_t size);
> +ssize_t drm_dp_mst_dpcd_write(struct drm_dp_aux *aux,
> +			      unsigned int offset, void *buffer, size_t size);
> +
> +int drm_dp_mst_connector_late_register(struct drm_connector *connector,
> +				       struct drm_dp_mst_port *port);
> +void drm_dp_mst_connector_early_unregister(struct drm_connector *connector,
> +					   struct drm_dp_mst_port *port);
> +
>  struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct
> drm_atomic_state *state,
>  								    struct
> drm_dp_mst_topology_mgr *mgr);
>  int __must_check
-- 
Cheers,
	Lyude Paul

_______________________________________________
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