Re: [PATCH 2/2] drm/dp_mst: Register aux-dev nodes for MST ports

[Lyude Paul <lyude@redhat.com>]

On Fri, 2019-04-12 at 12:05 -0400, sunpeng.li@amd.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Expose AUX devices for MST ports, similar to how they are exposed for
> SST.
> 
> The registered device will have it's MST port path appended in order to
> identify it. i.e. /dev/drm_dp_aux4_mst:0-2-1
> 
> So for a MST topology like so:
> 
>                +---------+
>                |  ASIC   |
>                +---------+
>               Conn-0|
>                     |
>                +----v----+
>           +----| MST HUB |----+
>           |    +---------+    |
>           |                   |
>           |Port-1       Port-2|
>     +-----v-----+       +-----v-----+
>     |  MST      |       |  SST      |
>     |  Display  |       |  Display  |
>     +-----------+       +-----------+
>           |Port-1
>           x
> 
> The list of AUX device names will look like:
> 
> AUX Device Name       | MST Device
> ----------------------+----------------------------------
> drm_dp_aux0           | MST Hub
> drm_dp_aux1_mst:0-1-1 | MST Display's disconnected DP out
> drm_dp_aux2_mst:0-1   | MST Display
> drm_dp_aux3_mst:0-2   | SST Display
> 
> Note that aux devices are only created for Physical Ports. Logical Ports
> are left out, since they are internally connected within the MST device
> (not connected to a DP RX or TX).
> 
> Leo Li:
> * Add missing drm_crtc_helper_internal.h include
> * Fix hard-coded offset and size in drm_dp_send_dpcd_read()
> * Only create aux devices for physical ports.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_aux_dev.c      |  13 +++-
>  drivers/gpu/drm/drm_dp_mst_topology.c | 109 ++++++++++++++++++++++++++++++-
> ---
>  include/drm/drm_dp_helper.h           |   4 ++
>  include/drm/drm_dp_mst_helper.h       |   6 ++
>  4 files changed, 117 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
> b/drivers/gpu/drm/drm_dp_aux_dev.c
> index 2310a67..f1241d1 100644
> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> @@ -34,6 +34,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/uio.h>
>  #include <drm/drm_dp_helper.h>
> +#include <drm/drm_dp_mst_helper.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drmP.h>
>  
> @@ -160,7 +161,11 @@ 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);
> +

mhhhh, is there a reason we can't just use the normal ->read and ->write
auxdev callbacks for this instead of mixing layers like this?

>  		if (res <= 0)
>  			break;
>  
> @@ -207,7 +212,11 @@ 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 2ab16c9..d5282db 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -35,6 +35,8 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  
> +#include "drm_crtc_helper_internal.h"
> +
>  /**
>   * DOC: dp mst helper
>   *
> @@ -52,6 +54,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);
> @@ -941,6 +946,8 @@ static void drm_dp_destroy_port(struct kref *kref)
>  	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
>  
>  	if (!port->input) {
> +		drm_dp_aux_unregister_devnode(&port->aux);
> +
>  		port->vcpi.num_slots = 0;
>  
>  		kfree(port->cached_edid);
> @@ -1095,6 +1102,46 @@ 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.
> + */
> +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.
> + */
> +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;
> @@ -1158,6 +1205,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;
>  		created = true;
>  	} else {
>  		old_pdt = port->pdt;
> @@ -1188,7 +1236,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
> *mstb,
>  				drm_dp_send_enum_path_resources(mstb->mgr,
> mstb, port);
>  		} else {
>  			port->available_pbn = 0;
> -			}
> +		}
>  	}
>  
>  	if (old_pdt != port->pdt && !port->input) {
> @@ -1220,6 +1268,14 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
> *mstb,
>  			drm_connector_set_tile_property(port->connector);
>  		}
>  		(*mstb->mgr->cbs->register_connector)(port->connector);
> +
> +		/* Prepend an '_' in front to suffix the aux device filename
> */
> +		memmove(proppath + 1, proppath, sizeof(proppath) - 1);
> +		proppath[0] = '_';
> +
> +		/* Only create aux devices for physical ports with a TX or
> RX*/
> +		if (port->port_num < DP_MST_LOGICAL_PORT_0)
> +			drm_dp_aux_register_devnode(&port->aux, proppath);
>  	}
>  
>  out:
> @@ -1404,7 +1460,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;
> @@ -1417,7 +1472,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)
> @@ -1994,26 +2048,55 @@ 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_get_validated_mstb_ref(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_put_mst_branch_device(mstb);
> +
> +	return ret;
>  }
> -#endif
>  
>  static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
>  				  struct drm_dp_mst_port *port,
> @@ -2041,9 +2124,9 @@ 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 == 1) {
> -			ret = -EINVAL;
> -		} else
> +		if (txmsg->reply.reply_type == 1)
> +			ret = -EIO;
> +		else
>  			ret = 0;
>  	}
>  	kfree(txmsg);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 509667e..6dea76a 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1265,6 +1265,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.
> +	 */
> +	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 371cc28..30f8c11 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -615,6 +615,12 @@ 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 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);
> +
>  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 drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
-- 
Cheers,
	Lyude Paul

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel