[RFC 0/2] Add AUX device entries for DP MST devices

[RFC 0/2] Add AUX device entries for DP MST devices

[sunpeng.li]

From: Leo Li <sunpeng.li@amd.com>

Hi all,

This is a follup to this change made by Ville to add MST aux nodes:
https://github.com/vsyrjala/linux/commit/cac63f799ee2f5fbbe4f0a375383f13e03d640a5
Patch 2/2 describes what I added on top.

Sending as an RFC since there are some items I'm not certain on:

1) Only expose aux devices for physical ports. FWICT, only DPTX and DPRX
   can handle AUX transactions, leaving logical ports out.
2) Naming of exposed AUX devices. I'm not sure if the scheme implemented
   here is the best approach.

Let me know your thoughts,
Leo

Leo Li (1):
  drm/dp_mst: Use non-cyclic idr, add suffix option for aux device

Ville Syrjälä (1):
  drm/dp_mst: Register aux-dev nodes for MST ports

 drivers/gpu/drm/drm_crtc_helper_internal.h |   5 +-
 drivers/gpu/drm/drm_dp_aux_dev.c           |  21 ++++--
 drivers/gpu/drm/drm_dp_helper.c            |   2 +-
 drivers/gpu/drm/drm_dp_mst_topology.c      | 109 +++++++++++++++++++++++++----
 include/drm/drm_dp_helper.h                |   4 ++
 include/drm/drm_dp_mst_helper.h            |   6 ++
 6 files changed, 125 insertions(+), 22 deletions(-)

-- 
2.7.4

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

[PATCH 1/2] drm/dp_aux: Use non-cyclic idr, add suffix option for aux device

[sunpeng.li]

From: Leo Li <sunpeng.li@amd.com>

In preparation for adding aux devices for DP MST:

1. A non-cyclic idr is used for the device minor version. That way,
   hotplug cycling MST devices won't needlessly increment the minor
   version index.
2. A suffix option is added to the aux device file name. It can be used
   to identify the corresponding MST device.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/drm_crtc_helper_internal.h | 5 +++--
 drivers/gpu/drm/drm_dp_aux_dev.c           | 8 ++++----
 drivers/gpu/drm/drm_dp_helper.c            | 2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h b/drivers/gpu/drm/drm_crtc_helper_internal.h
index b5ac158..9f0907b 100644
--- a/drivers/gpu/drm/drm_crtc_helper_internal.h
+++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
@@ -46,7 +46,7 @@ static inline int drm_fb_helper_modinit(void)
 #ifdef CONFIG_DRM_DP_AUX_CHARDEV
 int drm_dp_aux_dev_init(void);
 void drm_dp_aux_dev_exit(void);
-int drm_dp_aux_register_devnode(struct drm_dp_aux *aux);
+int drm_dp_aux_register_devnode(struct drm_dp_aux *aux, const char *suffix);
 void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux);
 #else
 static inline int drm_dp_aux_dev_init(void)
@@ -58,7 +58,8 @@ static inline void drm_dp_aux_dev_exit(void)
 {
 }
 
-static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
+static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux,
+					      const char *suffix)
 {
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
index 0e4f25d..2310a67 100644
--- a/drivers/gpu/drm/drm_dp_aux_dev.c
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -80,8 +80,7 @@ static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux)
 	kref_init(&aux_dev->refcount);
 
 	mutex_lock(&aux_idr_mutex);
-	index = idr_alloc_cyclic(&aux_idr, aux_dev, 0, DRM_AUX_MINORS,
-				 GFP_KERNEL);
+	index = idr_alloc(&aux_idr, aux_dev, 0, DRM_AUX_MINORS, GFP_KERNEL);
 	mutex_unlock(&aux_idr_mutex);
 	if (index < 0) {
 		kfree(aux_dev);
@@ -290,7 +289,7 @@ void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
 	kref_put(&aux_dev->refcount, release_drm_dp_aux_dev);
 }
 
-int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
+int drm_dp_aux_register_devnode(struct drm_dp_aux *aux, const char *suffix)
 {
 	struct drm_dp_aux_dev *aux_dev;
 	int res;
@@ -301,7 +300,8 @@ int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
 
 	aux_dev->dev = device_create(drm_dp_aux_dev_class, aux->dev,
 				     MKDEV(drm_dev_major, aux_dev->index), NULL,
-				     "drm_dp_aux%d", aux_dev->index);
+				     "drm_dp_aux%d%s", aux_dev->index,
+				     suffix ? suffix : "");
 	if (IS_ERR(aux_dev->dev)) {
 		res = PTR_ERR(aux_dev->dev);
 		aux_dev->dev = NULL;
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index e2266ac..13f1005 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -1143,7 +1143,7 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
 	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
 		sizeof(aux->ddc.name));
 
-	ret = drm_dp_aux_register_devnode(aux);
+	ret = drm_dp_aux_register_devnode(aux, NULL);
 	if (ret)
 		return ret;
 
-- 
2.7.4

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

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

[sunpeng.li-5C7GfCeVMHo]

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);
+
 		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,
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC 0/2] Add AUX device entries for DP MST devices

[Ville Syrjälä]

On Fri, Apr 12, 2019 at 12:05:29PM -0400, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> Hi all,
> 
> This is a follup to this change made by Ville to add MST aux nodes:
> https://github.com/vsyrjala/linux/commit/cac63f799ee2f5fbbe4f0a375383f13e03d640a5
> Patch 2/2 describes what I added on top.

Cool. I take you got it to actually work? IIRC I never managed that :P

> 
> Sending as an RFC since there are some items I'm not certain on:
> 
> 1) Only expose aux devices for physical ports. FWICT, only DPTX and DPRX
>    can handle AUX transactions, leaving logical ports out.

Hmm. My MST-foo is admittedly weak so I'm not sure. A quick trawl through
the spec didn't provide any solid explanations either :( However eg.
"Figure 2-83: Example Multi-function MST Branch-Sink Device Enumeration"
in the DP 1.4 spec does appear to show kind of virtual DPCD thing behind
a logical port. But I'm not really sure what than means.

> 2) Naming of exposed AUX devices. I'm not sure if the scheme implemented
>    here is the best approach.

I'm not sure magic naming schemes are the best. I believe we do have
the sysfs hierarchy which allows you to find the right aux dev for
the connector, but I do admit that can be a bit cumbersome to use.
Also I'm not sure if all the things we might want to talk to are
even represented by a connector, so maybe we do need something else.

> 
> Let me know your thoughts,
> Leo
> 
> Leo Li (1):
>   drm/dp_mst: Use non-cyclic idr, add suffix option for aux device
> 
> Ville Syrjälä (1):
>   drm/dp_mst: Register aux-dev nodes for MST ports
> 
>  drivers/gpu/drm/drm_crtc_helper_internal.h |   5 +-
>  drivers/gpu/drm/drm_dp_aux_dev.c           |  21 ++++--
>  drivers/gpu/drm/drm_dp_helper.c            |   2 +-
>  drivers/gpu/drm/drm_dp_mst_topology.c      | 109 +++++++++++++++++++++++++----
>  include/drm/drm_dp_helper.h                |   4 ++
>  include/drm/drm_dp_mst_helper.h            |   6 ++
>  6 files changed, 125 insertions(+), 22 deletions(-)
> 
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC 0/2] Add AUX device entries for DP MST devices

[Li, Sun peng (Leo)]

On 2019-04-12 1:30 p.m., Ville Syrjälä wrote:
> On Fri, Apr 12, 2019 at 12:05:29PM -0400, sunpeng.li@amd.com wrote:
>> From: Leo Li <sunpeng.li@amd.com>
>>
>> Hi all,
>>
>> This is a follup to this change made by Ville to add MST aux nodes:
>> https://github.com/vsyrjala/linux/commit/cac63f799ee2f5fbbe4f0a375383f13e03d640a5
>> Patch 2/2 describes what I added on top.
> 
> Cool. I take you got it to actually work? IIRC I never managed that :P

Yeah, I got it to work on my system :) The diagram in Patch2/2 is what I
was testing with. Both reading and writing worked with the help of
Lyude's auxrw.py script:

https://gitlab.freedesktop.org/lyudess/auxrw/blob/master/auxrw.py

> 
>>
>> Sending as an RFC since there are some items I'm not certain on:
>>
>> 1) Only expose aux devices for physical ports. FWICT, only DPTX and DPRX
>>     can handle AUX transactions, leaving logical ports out.
> 
> Hmm. My MST-foo is admittedly weak so I'm not sure. A quick trawl through
> the spec didn't provide any solid explanations either :( However eg.
> "Figure 2-83: Example Multi-function MST Branch-Sink Device Enumeration"
> in the DP 1.4 spec does appear to show kind of virtual DPCD thing behind
> a logical port. But I'm not really sure what than means.

Good point, I'm gonna dig more into that. It sounds like we should be
able to access that with the relative addressing as defined by the spec
(2.11.5). I'll have to see why that's currently getting nacked though.

>> 2) Naming of exposed AUX devices. I'm not sure if the scheme implemented
>>     here is the best approach.
> 
> I'm not sure magic naming schemes are the best. I believe we do have
> the sysfs hierarchy which allows you to find the right aux dev for
> the connector, but I do admit that can be a bit cumbersome to use.
> Also I'm not sure if all the things we might want to talk to are
> even represented by a connector, so maybe we do need something else.
> 

The spec does define two methods of identifying devices (GUID or RAD) in
section 2.5.3. I think the method we choose should make use of those.
FWIW the scheme used here is based upon the RAD as generated by
build_mst_prop_path().

Thanks,
Leo

>>
>> Let me know your thoughts,
>> Leo
>>
>> Leo Li (1):
>>    drm/dp_mst: Use non-cyclic idr, add suffix option for aux device
>>
>> Ville Syrjälä (1):
>>    drm/dp_mst: Register aux-dev nodes for MST ports
>>
>>   drivers/gpu/drm/drm_crtc_helper_internal.h |   5 +-
>>   drivers/gpu/drm/drm_dp_aux_dev.c           |  21 ++++--
>>   drivers/gpu/drm/drm_dp_helper.c            |   2 +-
>>   drivers/gpu/drm/drm_dp_mst_topology.c      | 109 +++++++++++++++++++++++++----
>>   include/drm/drm_dp_helper.h                |   4 ++
>>   include/drm/drm_dp_mst_helper.h            |   6 ++
>>   6 files changed, 125 insertions(+), 22 deletions(-)
>>
>> -- 
>> 2.7.4
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC 0/2] Add AUX device entries for DP MST devices

[Li, Sun peng (Leo)]

>> Hmm. My MST-foo is admittedly weak so I'm not sure. A quick trawl through
>> the spec didn't provide any solid explanations either :( However eg.
>> "Figure 2-83: Example Multi-function MST Branch-Sink Device Enumeration"
>> in the DP 1.4 spec does appear to show kind of virtual DPCD thing behind
>> a logical port. But I'm not really sure what than means.
> 
> Good point, I'm gonna dig more into that. It sounds like we should be
> able to access that with the relative addressing as defined by the spec
> (2.11.5). I'll have to see why that's currently getting nacked though.
> 

It looks like DPCD reads on logical ports work on some devices, and not
others... I swapped my MST display out with a different one, and it read
just fine.

More specifically - in a daisy chain setup with 2 MST displays - with
the one that works at the end:

# auxrw.py read /dev/drm_dp_aux4_mst\:0-1-8 0x30-0x3f -> ACK
# auxrw.py read /dev/drm_dp_aux6_mst\:0-1 0x30-0x3f -> ACK
(The GUIDs returned are identical)

With the one that doesn't at the end:

# auxrw.py read /dev/drm_dp_aux4_mst\:0-1-8 0x30-0x3f -> *NAK*
# auxrw.py read /dev/drm_dp_aux6_mst\:0-1 0x30-0x3f -> ACK

So it seems there's some device dependent behavior here. I'm not sure if
there's a better way of handling this besides exposing all the
downstream ports: If it works, great. If not, just don't use it?

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

Re: [RFC 0/2] Add AUX device entries for DP MST devices

[Ville Syrjälä]

On Tue, Apr 16, 2019 at 03:28:24PM +0000, Li, Sun peng (Leo) wrote:
> >> Hmm. My MST-foo is admittedly weak so I'm not sure. A quick trawl through
> >> the spec didn't provide any solid explanations either :( However eg.
> >> "Figure 2-83: Example Multi-function MST Branch-Sink Device Enumeration"
> >> in the DP 1.4 spec does appear to show kind of virtual DPCD thing behind
> >> a logical port. But I'm not really sure what than means.
> > 
> > Good point, I'm gonna dig more into that. It sounds like we should be
> > able to access that with the relative addressing as defined by the spec
> > (2.11.5). I'll have to see why that's currently getting nacked though.
> > 
> 
> It looks like DPCD reads on logical ports work on some devices, and not
> others... I swapped my MST display out with a different one, and it read
> just fine.
> 
> More specifically - in a daisy chain setup with 2 MST displays - with
> the one that works at the end:
> 
> # auxrw.py read /dev/drm_dp_aux4_mst\:0-1-8 0x30-0x3f -> ACK
> # auxrw.py read /dev/drm_dp_aux6_mst\:0-1 0x30-0x3f -> ACK
> (The GUIDs returned are identical)
> 
> With the one that doesn't at the end:
> 
> # auxrw.py read /dev/drm_dp_aux4_mst\:0-1-8 0x30-0x3f -> *NAK*
> # auxrw.py read /dev/drm_dp_aux6_mst\:0-1 0x30-0x3f -> ACK
> 
> So it seems there's some device dependent behavior here. I'm not sure if
> there's a better way of handling this besides exposing all the
> downstream ports: If it works, great. If not, just don't use it?

Yeah, I think that's fine. It's really meant for debugging anyway
so doesn't really matter if we expose something that's not
guaranteed to work.

-- 
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/dp_aux: Use non-cyclic idr, add suffix option for aux device

[Lyude Paul]

Sorry for the slow response, I've been really busy ;_;

On Fri, 2019-04-12 at 12:05 -0400, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> In preparation for adding aux devices for DP MST:
> 
> 1. A non-cyclic idr is used for the device minor version. That way,
>    hotplug cycling MST devices won't needlessly increment the minor
>    version index.

I really like this btw, cyclic idrs are really annoying for drm_dp_aux_dev,
even outside of MST (try reloading a drm driver a couple of times and you'll
understand ;). I think we should split this into another commit though

> 2. A suffix option is added to the aux device file name. It can be used
>    to identify the corresponding MST device.

I like this idea, but I think there may be a better way that we can do this.
Using /dev/nvme* as an example, we have the standard dev paths (/dev/nvme0,
/dev/nvme0n1, etc.). But we can also access them through /dev/disk/by-
{id,label,partlabel,partuuid,path,uuid}.

So what if we added something similar for aux devices? We start off with the
standard /dev/drm_dp_aux*, then provide more descriptive symlinks and
directories:

(feel free to come up with better naming then this if you can)

/dev/drm_dp_aux/card0-DP-1 -> /dev/drm_dp_aux1
/dev/drm_dp_aux/card0-DP-2 -> /dev/drm_dp_aux2
etc.

> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>  drivers/gpu/drm/drm_crtc_helper_internal.h | 5 +++--
>  drivers/gpu/drm/drm_dp_aux_dev.c           | 8 ++++----
>  drivers/gpu/drm/drm_dp_helper.c            | 2 +-
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h
> b/drivers/gpu/drm/drm_crtc_helper_internal.h
> index b5ac158..9f0907b 100644
> --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> @@ -46,7 +46,7 @@ static inline int drm_fb_helper_modinit(void)
>  #ifdef CONFIG_DRM_DP_AUX_CHARDEV
>  int drm_dp_aux_dev_init(void);
>  void drm_dp_aux_dev_exit(void);
> -int drm_dp_aux_register_devnode(struct drm_dp_aux *aux);
> +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux, const char
> *suffix);
>  void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux);
>  #else
>  static inline int drm_dp_aux_dev_init(void)
> @@ -58,7 +58,8 @@ static inline void drm_dp_aux_dev_exit(void)
>  {
>  }
>  
> -static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
> +static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux,
> +					      const char *suffix)
>  {
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
> b/drivers/gpu/drm/drm_dp_aux_dev.c
> index 0e4f25d..2310a67 100644
> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> @@ -80,8 +80,7 @@ static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct
> drm_dp_aux *aux)
>  	kref_init(&aux_dev->refcount);
>  
>  	mutex_lock(&aux_idr_mutex);
> -	index = idr_alloc_cyclic(&aux_idr, aux_dev, 0, DRM_AUX_MINORS,
> -				 GFP_KERNEL);
> +	index = idr_alloc(&aux_idr, aux_dev, 0, DRM_AUX_MINORS, GFP_KERNEL);
>  	mutex_unlock(&aux_idr_mutex);
>  	if (index < 0) {
>  		kfree(aux_dev);
> @@ -290,7 +289,7 @@ void drm_dp_aux_unregister_devnode(struct drm_dp_aux
> *aux)
>  	kref_put(&aux_dev->refcount, release_drm_dp_aux_dev);
>  }
>  
> -int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
> +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux, const char *suffix)
>  {
>  	struct drm_dp_aux_dev *aux_dev;
>  	int res;
> @@ -301,7 +300,8 @@ int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
>  
>  	aux_dev->dev = device_create(drm_dp_aux_dev_class, aux->dev,
>  				     MKDEV(drm_dev_major, aux_dev->index),
> NULL,
> -				     "drm_dp_aux%d", aux_dev->index);
> +				     "drm_dp_aux%d%s", aux_dev->index,
> +				     suffix ? suffix : "");
>  	if (IS_ERR(aux_dev->dev)) {
>  		res = PTR_ERR(aux_dev->dev);
>  		aux_dev->dev = NULL;
> diff --git a/drivers/gpu/drm/drm_dp_helper.c
> b/drivers/gpu/drm/drm_dp_helper.c
> index e2266ac..13f1005 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1143,7 +1143,7 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
>  	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
>  		sizeof(aux->ddc.name));
>  
> -	ret = drm_dp_aux_register_devnode(aux);
> +	ret = drm_dp_aux_register_devnode(aux, NULL);
>  	if (ret)
>  		return ret;
>  
-- 
Cheers,
	Lyude Paul

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Lyude Paul]

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

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

[Ville Syrjälä]

On Tue, Apr 16, 2019 at 06:22:33PM -0400, Lyude Paul wrote:
> 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?

IIRC I did think about that when I initially did this, but decided 
there was actual work involved so didn't bother with it, especially
since the code wasn't even working yet. Sadly I've forgotten what
the "acutal work" part was supposed to be.

-- 
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/dp_aux: Use non-cyclic idr, add suffix option for aux device

[Li, Sun peng (Leo)]

On 2019-04-16 6:16 p.m., Lyude Paul wrote:
> Sorry for the slow response, I've been really busy ;_;

No worries :)

> 
> On Fri, 2019-04-12 at 12:05 -0400, sunpeng.li@amd.com wrote:
>> From: Leo Li <sunpeng.li@amd.com>
>>
>> In preparation for adding aux devices for DP MST:
>>
>> 1. A non-cyclic idr is used for the device minor version. That way,
>>     hotplug cycling MST devices won't needlessly increment the minor
>>     version index.
> 
> I really like this btw, cyclic idrs are really annoying for drm_dp_aux_dev,
> even outside of MST (try reloading a drm driver a couple of times and you'll
> understand ;). I think we should split this into another commit though
> 
>> 2. A suffix option is added to the aux device file name. It can be used
>>     to identify the corresponding MST device.
> 
> I like this idea, but I think there may be a better way that we can do this.
> Using /dev/nvme* as an example, we have the standard dev paths (/dev/nvme0,
> /dev/nvme0n1, etc.). But we can also access them through /dev/disk/by-
> {id,label,partlabel,partuuid,path,uuid}.
> 
> So what if we added something similar for aux devices? We start off with the
> standard /dev/drm_dp_aux*, then provide more descriptive symlinks and
> directories:
> 
> (feel free to come up with better naming then this if you can)
> 
> /dev/drm_dp_aux/card0-DP-1 -> /dev/drm_dp_aux1
> /dev/drm_dp_aux/card0-DP-2 -> /dev/drm_dp_aux2
> etc.

That does sound neater - although FWICT, this will have to be done with
udev rules?

I took a brief look at how that's done for storage devices. It looks
like they have rules defined in
/lib/udev/rules.d/60-persistent-storage.rules that manages the "by-x"
symlinks.

To make this happen for aux devices, what we could do is attach sysfs
attributes to the device. It can then be parsed by udev in a similar
fashion. Currently, only 'name' attribute is attached, as seen in
drm_dp_aux_dev.c (after name_show()).

Thanks,
Leo

> 
>>
>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>> ---
>>   drivers/gpu/drm/drm_crtc_helper_internal.h | 5 +++--
>>   drivers/gpu/drm/drm_dp_aux_dev.c           | 8 ++++----
>>   drivers/gpu/drm/drm_dp_helper.c            | 2 +-
>>   3 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h
>> b/drivers/gpu/drm/drm_crtc_helper_internal.h
>> index b5ac158..9f0907b 100644
>> --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
>> +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
>> @@ -46,7 +46,7 @@ static inline int drm_fb_helper_modinit(void)
>>   #ifdef CONFIG_DRM_DP_AUX_CHARDEV
>>   int drm_dp_aux_dev_init(void);
>>   void drm_dp_aux_dev_exit(void);
>> -int drm_dp_aux_register_devnode(struct drm_dp_aux *aux);
>> +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux, const char
>> *suffix);
>>   void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux);
>>   #else
>>   static inline int drm_dp_aux_dev_init(void)
>> @@ -58,7 +58,8 @@ static inline void drm_dp_aux_dev_exit(void)
>>   {
>>   }
>>   
>> -static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
>> +static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux,
>> +					      const char *suffix)
>>   {
>>   	return 0;
>>   }
>> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
>> b/drivers/gpu/drm/drm_dp_aux_dev.c
>> index 0e4f25d..2310a67 100644
>> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
>> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
>> @@ -80,8 +80,7 @@ static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct
>> drm_dp_aux *aux)
>>   	kref_init(&aux_dev->refcount);
>>   
>>   	mutex_lock(&aux_idr_mutex);
>> -	index = idr_alloc_cyclic(&aux_idr, aux_dev, 0, DRM_AUX_MINORS,
>> -				 GFP_KERNEL);
>> +	index = idr_alloc(&aux_idr, aux_dev, 0, DRM_AUX_MINORS, GFP_KERNEL);
>>   	mutex_unlock(&aux_idr_mutex);
>>   	if (index < 0) {
>>   		kfree(aux_dev);
>> @@ -290,7 +289,7 @@ void drm_dp_aux_unregister_devnode(struct drm_dp_aux
>> *aux)
>>   	kref_put(&aux_dev->refcount, release_drm_dp_aux_dev);
>>   }
>>   
>> -int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
>> +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux, const char *suffix)
>>   {
>>   	struct drm_dp_aux_dev *aux_dev;
>>   	int res;
>> @@ -301,7 +300,8 @@ int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
>>   
>>   	aux_dev->dev = device_create(drm_dp_aux_dev_class, aux->dev,
>>   				     MKDEV(drm_dev_major, aux_dev->index),
>> NULL,
>> -				     "drm_dp_aux%d", aux_dev->index);
>> +				     "drm_dp_aux%d%s", aux_dev->index,
>> +				     suffix ? suffix : "");
>>   	if (IS_ERR(aux_dev->dev)) {
>>   		res = PTR_ERR(aux_dev->dev);
>>   		aux_dev->dev = NULL;
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c
>> b/drivers/gpu/drm/drm_dp_helper.c
>> index e2266ac..13f1005 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -1143,7 +1143,7 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
>>   	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
>>   		sizeof(aux->ddc.name));
>>   
>> -	ret = drm_dp_aux_register_devnode(aux);
>> +	ret = drm_dp_aux_register_devnode(aux, NULL);
>>   	if (ret)
>>   		return ret;
>>   
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/dp_aux: Use non-cyclic idr, add suffix option for aux device

[Lyude Paul]

On Wed, 2019-04-17 at 23:10 +0000, Li, Sun peng (Leo) wrote:
> 
> 
> On 2019-04-16 6:16 p.m., Lyude Paul wrote:
> > Sorry for the slow response, I've been really busy ;_;
> 
> No worries :)
> 
> > On Fri, 2019-04-12 at 12:05 -0400, sunpeng.li@amd.com wrote:
> > > From: Leo Li <sunpeng.li@amd.com>
> > > 
> > > In preparation for adding aux devices for DP MST:
> > > 
> > > 1. A non-cyclic idr is used for the device minor version. That way,
> > >     hotplug cycling MST devices won't needlessly increment the minor
> > >     version index.
> > 
> > I really like this btw, cyclic idrs are really annoying for drm_dp_aux_dev,
> > even outside of MST (try reloading a drm driver a couple of times and you'll
> > understand ;). I think we should split this into another commit though
> > 
> > > 2. A suffix option is added to the aux device file name. It can be used
> > >     to identify the corresponding MST device.
> > 
> > I like this idea, but I think there may be a better way that we can do this.
> > Using /dev/nvme* as an example, we have the standard dev paths (/dev/nvme0,
> > /dev/nvme0n1, etc.). But we can also access them through /dev/disk/by-
> > {id,label,partlabel,partuuid,path,uuid}.
> > 
> > So what if we added something similar for aux devices? We start off with the
> > standard /dev/drm_dp_aux*, then provide more descriptive symlinks and
> > directories:
> > 
> > (feel free to come up with better naming then this if you can)
> > 
> > /dev/drm_dp_aux/card0-DP-1 -> /dev/drm_dp_aux1
> > /dev/drm_dp_aux/card0-DP-2 -> /dev/drm_dp_aux2
> > etc.
> 
> That does sound neater - although FWICT, this will have to be done with
> udev rules?
> 
> I took a brief look at how that's done for storage devices. It looks
> like they have rules defined in
> /lib/udev/rules.d/60-persistent-storage.rules that manages the "by-x"
> symlinks.
> 
> To make this happen for aux devices, what we could do is attach sysfs
> attributes to the device. It can then be parsed by udev in a similar
> fashion. Currently, only 'name' attribute is attached, as seen in
> drm_dp_aux_dev.c (after name_show()).

Yeah-that sounds perfect to me!

> 
> Thanks,
> Leo
> 
> > > Signed-off-by: Leo Li <sunpeng.li@amd.com>
> > > ---
> > >   drivers/gpu/drm/drm_crtc_helper_internal.h | 5 +++--
> > >   drivers/gpu/drm/drm_dp_aux_dev.c           | 8 ++++----
> > >   drivers/gpu/drm/drm_dp_helper.c            | 2 +-
> > >   3 files changed, 8 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > index b5ac158..9f0907b 100644
> > > --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > @@ -46,7 +46,7 @@ static inline int drm_fb_helper_modinit(void)
> > >   #ifdef CONFIG_DRM_DP_AUX_CHARDEV
> > >   int drm_dp_aux_dev_init(void);
> > >   void drm_dp_aux_dev_exit(void);
> > > -int drm_dp_aux_register_devnode(struct drm_dp_aux *aux);
> > > +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux, const char
> > > *suffix);
> > >   void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux);
> > >   #else
> > >   static inline int drm_dp_aux_dev_init(void)
> > > @@ -58,7 +58,8 @@ static inline void drm_dp_aux_dev_exit(void)
> > >   {
> > >   }
> > >   
> > > -static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
> > > +static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux,
> > > +					      const char *suffix)
> > >   {
> > >   	return 0;
> > >   }
> > > diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
> > > b/drivers/gpu/drm/drm_dp_aux_dev.c
> > > index 0e4f25d..2310a67 100644
> > > --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> > > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> > > @@ -80,8 +80,7 @@ static struct drm_dp_aux_dev
> > > *alloc_drm_dp_aux_dev(struct
> > > drm_dp_aux *aux)
> > >   	kref_init(&aux_dev->refcount);
> > >   
> > >   	mutex_lock(&aux_idr_mutex);
> > > -	index = idr_alloc_cyclic(&aux_idr, aux_dev, 0, DRM_AUX_MINORS,
> > > -				 GFP_KERNEL);
> > > +	index = idr_alloc(&aux_idr, aux_dev, 0, DRM_AUX_MINORS, GFP_KERNEL);
> > >   	mutex_unlock(&aux_idr_mutex);
> > >   	if (index < 0) {
> > >   		kfree(aux_dev);
> > > @@ -290,7 +289,7 @@ void drm_dp_aux_unregister_devnode(struct drm_dp_aux
> > > *aux)
> > >   	kref_put(&aux_dev->refcount, release_drm_dp_aux_dev);
> > >   }
> > >   
> > > -int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
> > > +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux, const char
> > > *suffix)
> > >   {
> > >   	struct drm_dp_aux_dev *aux_dev;
> > >   	int res;
> > > @@ -301,7 +300,8 @@ int drm_dp_aux_register_devnode(struct drm_dp_aux
> > > *aux)
> > >   
> > >   	aux_dev->dev = device_create(drm_dp_aux_dev_class, aux->dev,
> > >   				     MKDEV(drm_dev_major, aux_dev-
> > > >index),
> > > NULL,
> > > -				     "drm_dp_aux%d", aux_dev->index);
> > > +				     "drm_dp_aux%d%s", aux_dev->index,
> > > +				     suffix ? suffix : "");
> > >   	if (IS_ERR(aux_dev->dev)) {
> > >   		res = PTR_ERR(aux_dev->dev);
> > >   		aux_dev->dev = NULL;
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > > b/drivers/gpu/drm/drm_dp_helper.c
> > > index e2266ac..13f1005 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -1143,7 +1143,7 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
> > >   	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux-
> > > >dev),
> > >   		sizeof(aux->ddc.name));
> > >   
> > > -	ret = drm_dp_aux_register_devnode(aux);
> > > +	ret = drm_dp_aux_register_devnode(aux, NULL);
> > >   	if (ret)
> > >   		return ret;
> > >   

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx