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
