[PATCH 1/3] drm/dp: Use non-cyclic idr

[PATCH 1/3] drm/dp: Use non-cyclic idr

[sunpeng.li]

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

In preparation for adding aux devices for DP MST, make the IDR
non-cyclic. That way, hotplug cycling MST devices won't needlessly
increment the minor version index.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/drm_dp_aux_dev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
index 0e4f25d..6d84611 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);
-- 
2.7.4

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

[PATCH 2/3] drm/dp_mst: Expose build_mst_prop_path()

[sunpeng.li]

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

To give identifiable attributes to MST DP aux devices, we can use the
MST relative address. Expose this function for later use.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
 include/drm/drm_dp_mst_helper.h       | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 2ab16c9..86ff8e2 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1120,7 +1120,7 @@ static void drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, u8 *guid)
 	}
 }
 
-static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
+void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
 				int pnum,
 				char *proppath,
 				size_t proppath_size)
@@ -1202,7 +1202,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
 	if (created && !port->input) {
 		char proppath[255];
 
-		build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
+		drm_dp_build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
 		port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr, port, proppath);
 		if (!port->connector) {
 			/* remove it from the port list */
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 371cc28..81c8d79 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -602,6 +602,10 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
 			   int pbn);
 
+void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
+				   int pnum,
+				   char *proppath,
+				   size_t proppath_size);
 
 int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr);
 
-- 
2.7.4

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

[PATCH 3/3] drm/dp_mst: Register AUX devices for MST ports

[sunpeng.li]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

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.

Additionally, a 'path' attribute is attached to the device. This is to
allow udev to provide more identifiable symlinks to these devices,
similar to /dev/disks. For example, adding the following udev rule:

SUBSYSTEMS=="drm_dp_aux_dev", ATTR{path}=="?*", SYMLINK+="drm_dp_aux/by-path/$attr{path}"

With a MST topology like so:

               +---------+
               |  ASIC   |
               +---------+
              Conn-0|
                    |
               +----v----+
          +----| MST HUB |----+
          |    +---------+    |
          |                   |
          |Port-1       Port-2|
    +-----v-----+       +-----v-----+
    |  MST      |       |  SST      |
    |  Display  |       |  Display  |
    +-----------+       +-----------+
          |Port-1
          x

Will create the following symlinks in 'drm_dp_aux/by-path/':

AUX Device Name       | MST Device
----------------------+----------------------------------
card0_sst:0 (*)       | MST Hub
card0_mst:0-1         | MST Display
card0_mst:0-1-1       | MST Display's disconnected DP out
card0_mst:0-1-8       | MST Display's internal sink
card0_mst:0-2         | SST Display

(*) Note that the first digit is suppose to mean 'Connector ID'.
However, that's only true for 'mst:' paths. A TODO item has been left to
address this.

Although all downstream ports are exposed, only some will work. 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.

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      |  53 ++++++++++++++++-
 drivers/gpu/drm/drm_dp_mst_topology.c | 103 +++++++++++++++++++++++++++++-----
 include/drm/drm_dp_helper.h           |   4 ++
 include/drm/drm_dp_mst_helper.h       |   6 ++
 4 files changed, 151 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
index 6d84611..4218bbf 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>
 
@@ -114,10 +115,50 @@ static ssize_t name_show(struct device *dev,
 
 	return res;
 }
+
+static int is_drm_primary_device(struct device *dev, void *data)
+{
+	return strstr(dev_name(dev), "card") != NULL;
+}
+
+static ssize_t path_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct drm_dp_aux_dev *aux_dev =
+		drm_dp_aux_dev_get_by_minor(MINOR(dev->devt));
+	struct drm_dp_aux *aux = aux_dev->aux;
+	struct drm_dp_mst_port *port;
+	struct device *card;
+	char temp[64];
+	ssize_t res;
+
+	card = device_find_child(dev->parent, NULL,
+	                         is_drm_primary_device);
+
+	if (!aux->is_remote) {
+		/*
+		 * TODO: AUX index does not correlate with connector ID. See if
+		 * connector ID can be used instead.
+		 */
+		res = sprintf(buf, "%s_%s:%d\n",
+		              dev_name(card), "sst", aux_dev->index);
+		return res;
+	}
+
+	port = container_of(aux, struct drm_dp_mst_port, aux);
+	drm_dp_build_mst_prop_path(port->parent, port->port_num,
+				   temp, sizeof(temp));
+	res = sprintf(buf, "%s_%s\n", dev_name(card), temp);
+
+	return res;
+}
+
 static DEVICE_ATTR_RO(name);
+static DEVICE_ATTR_RO(path);
 
 static struct attribute *drm_dp_aux_attrs[] = {
 	&dev_attr_name.attr,
+	&dev_attr_path.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(drm_dp_aux);
@@ -160,7 +201,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 +252,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 86ff8e2..0174527 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,8 @@ 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);
+
+		drm_dp_aux_register_devnode(&port->aux);
 	}
 
 out:
@@ -1404,7 +1454,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 +1466,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 +2042,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 +2118,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 81c8d79..4de8256 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -619,6 +619,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

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

Re: [PATCH 1/3] drm/dp: Use non-cyclic idr

[Ville Syrjälä]

On Mon, Apr 22, 2019 at 07:56:26PM -0400, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> In preparation for adding aux devices for DP MST, make the IDR
> non-cyclic. That way, hotplug cycling MST devices won't needlessly
> increment the minor version index.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>

I don't recall any specific reason for the cyclic variant, so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_dp_aux_dev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
> index 0e4f25d..6d84611 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);
> -- 
> 2.7.4

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

Re: [PATCH 2/3] drm/dp_mst: Expose build_mst_prop_path()

[Ville Syrjälä]

On Mon, Apr 22, 2019 at 07:56:27PM -0400, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> To give identifiable attributes to MST DP aux devices, we can use the
> MST relative address. Expose this function for later use.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
>  include/drm/drm_dp_mst_helper.h       | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 2ab16c9..86ff8e2 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1120,7 +1120,7 @@ static void drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, u8 *guid)
>  	}
>  }
>  
> -static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>  				int pnum,
>  				char *proppath,
>  				size_t proppath_size)

I was wondering if we need to export this but looks like both 
drm_dp_mst_topology.o and drm_dp_aux_dev.o both end up in
drm_kms_helper.ko, so the answer is no.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> @@ -1202,7 +1202,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
>  	if (created && !port->input) {
>  		char proppath[255];
>  
> -		build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
> +		drm_dp_build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
>  		port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr, port, proppath);
>  		if (!port->connector) {
>  			/* remove it from the port list */
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 371cc28..81c8d79 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -602,6 +602,10 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>  int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
>  			   int pbn);
>  
> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> +				   int pnum,
> +				   char *proppath,
> +				   size_t proppath_size);
>  
>  int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr);
>  
> -- 
> 2.7.4

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

Re: [PATCH 1/3] drm/dp: Use non-cyclic idr

[Lyude Paul]

lgtm

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Mon, 2019-04-22 at 19:56 -0400, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> In preparation for adding aux devices for DP MST, make the IDR
> non-cyclic. That way, hotplug cycling MST devices won't needlessly
> increment the minor version index.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_aux_dev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
> b/drivers/gpu/drm/drm_dp_aux_dev.c
> index 0e4f25d..6d84611 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);
-- 
Cheers,
	Lyude Paul

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

Re: [PATCH 2/3] drm/dp_mst: Expose build_mst_prop_path()

[Lyude Paul]

Closer, but are we sure we want to use the MST prop path for this? Why not add
a sysfs attribute with the corresponding DRM connector name instead since the
connector itself will have a path property. That way we can associate aux
devices for eDP and DP devices with their corresponding connectors as well

On Mon, 2019-04-22 at 19:56 -0400, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> To give identifiable attributes to MST DP aux devices, we can use the
> MST relative address. Expose this function for later use.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
>  include/drm/drm_dp_mst_helper.h       | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 2ab16c9..86ff8e2 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1120,7 +1120,7 @@ static void drm_dp_check_mstb_guid(struct
> drm_dp_mst_branch *mstb, u8 *guid)
>  	}
>  }
>  
> -static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>  				int pnum,
>  				char *proppath,
>  				size_t proppath_size)
> @@ -1202,7 +1202,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
> *mstb,
>  	if (created && !port->input) {
>  		char proppath[255];
>  
> -		build_mst_prop_path(mstb, port->port_num, proppath,
> sizeof(proppath));
> +		drm_dp_build_mst_prop_path(mstb, port->port_num, proppath,
> sizeof(proppath));
>  		port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr,
> port, proppath);
>  		if (!port->connector) {
>  			/* remove it from the port list */
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index 371cc28..81c8d79 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -602,6 +602,10 @@ void drm_dp_mst_deallocate_vcpi(struct
> drm_dp_mst_topology_mgr *mgr,
>  int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
>  			   int pbn);
>  
> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> +				   int pnum,
> +				   char *proppath,
> +				   size_t proppath_size);
>  
>  int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr);
>  
-- 
Cheers,
	Lyude Paul

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

Re: [PATCH 2/3] drm/dp_mst: Expose build_mst_prop_path()

[Li, Sun peng (Leo)]

On 2019-04-24 1:26 p.m., Lyude Paul wrote:
> Closer, but are we sure we want to use the MST prop path for this? Why not add
> a sysfs attribute with the corresponding DRM connector name instead since the
> connector itself will have a path property. That way we can associate aux
> devices for eDP and DP devices with their corresponding connectors as well

I thought about that as well, but I hit a wall when trying to get the
SST connector from the aux device. Perhaps there's a simpler way that
I'm overlooking?

It's easier for MST, since the mst_port can be obtained via container_of
dp_aux. port->connector would then give what we want.

For SST though, each driver calls drm_aux_register() with an aux struct
that they've initialized. I'm not sure how I can reliably get the
drm_connector from that.

Maybe drm_dp_aux should have a back reference to the connector? FWICT
all drivers using drm_aux_register() should be able to provide the
associated connector when calling it.

Leo


> 
> On Mon, 2019-04-22 at 19:56 -0400, sunpeng.li@amd.com wrote:
>> From: Leo Li <sunpeng.li@amd.com>
>>
>> To give identifiable attributes to MST DP aux devices, we can use the
>> MST relative address. Expose this function for later use.
>>
>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>> ---
>>   drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
>>   include/drm/drm_dp_mst_helper.h       | 4 ++++
>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index 2ab16c9..86ff8e2 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -1120,7 +1120,7 @@ static void drm_dp_check_mstb_guid(struct
>> drm_dp_mst_branch *mstb, u8 *guid)
>>   	}
>>   }
>>   
>> -static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>>   				int pnum,
>>   				char *proppath,
>>   				size_t proppath_size)
>> @@ -1202,7 +1202,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
>> *mstb,
>>   	if (created && !port->input) {
>>   		char proppath[255];
>>   
>> -		build_mst_prop_path(mstb, port->port_num, proppath,
>> sizeof(proppath));
>> +		drm_dp_build_mst_prop_path(mstb, port->port_num, proppath,
>> sizeof(proppath));
>>   		port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr,
>> port, proppath);
>>   		if (!port->connector) {
>>   			/* remove it from the port list */
>> diff --git a/include/drm/drm_dp_mst_helper.h
>> b/include/drm/drm_dp_mst_helper.h
>> index 371cc28..81c8d79 100644
>> --- a/include/drm/drm_dp_mst_helper.h
>> +++ b/include/drm/drm_dp_mst_helper.h
>> @@ -602,6 +602,10 @@ void drm_dp_mst_deallocate_vcpi(struct
>> drm_dp_mst_topology_mgr *mgr,
>>   int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
>>   			   int pbn);
>>   
>> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>> +				   int pnum,
>> +				   char *proppath,
>> +				   size_t proppath_size);
>>   
>>   int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr);
>>   
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/dp_mst: Expose build_mst_prop_path()

[Ville Syrjälä]

On Wed, Apr 24, 2019 at 08:40:30PM +0000, Li, Sun peng (Leo) wrote:
> 
> 
> 
> On 2019-04-24 1:26 p.m., Lyude Paul wrote:
> > Closer, but are we sure we want to use the MST prop path for this? Why not add
> > a sysfs attribute with the corresponding DRM connector name instead since the
> > connector itself will have a path property. That way we can associate aux
> > devices for eDP and DP devices with their corresponding connectors as well
> 
> I thought about that as well, but I hit a wall when trying to get the
> SST connector from the aux device. Perhaps there's a simpler way that
> I'm overlooking?
> 
> It's easier for MST, since the mst_port can be obtained via container_of
> dp_aux. port->connector would then give what we want.
> 
> For SST though, each driver calls drm_aux_register() with an aux struct
> that they've initialized. I'm not sure how I can reliably get the
> drm_connector from that.

On i915 the aux is a child of the connector, so no extra
attributes/links needed. Maybe other drivers should/could
follow  that apporach as well?

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

Re: [PATCH 2/3] drm/dp_mst: Expose build_mst_prop_path()

[Lyude Paul]

On Wed, 2019-04-24 at 23:52 +0300, Ville Syrjälä wrote:
> On Wed, Apr 24, 2019 at 08:40:30PM +0000, Li, Sun peng (Leo) wrote:
> > 
> > 
> > On 2019-04-24 1:26 p.m., Lyude Paul wrote:
> > > Closer, but are we sure we want to use the MST prop path for this? Why
> > > not add
> > > a sysfs attribute with the corresponding DRM connector name instead
> > > since the
> > > connector itself will have a path property. That way we can associate
> > > aux
> > > devices for eDP and DP devices with their corresponding connectors as
> > > well
> > 
> > I thought about that as well, but I hit a wall when trying to get the
> > SST connector from the aux device. Perhaps there's a simpler way that
> > I'm overlooking?
> > 
> > It's easier for MST, since the mst_port can be obtained via container_of
> > dp_aux. port->connector would then give what we want.
> > 
> > For SST though, each driver calls drm_aux_register() with an aux struct
> > that they've initialized. I'm not sure how I can reliably get the
> > drm_connector from that.
> 
> On i915 the aux is a child of the connector, so no extra
> attributes/links needed. Maybe other drivers should/could
> follow  that apporach as well?

ooo, good point. Yeah that seems like it would be worth a shot since it'd be a
little nicer then just adding more sysfs attributes. But otherwise if that
doesn't work, adding a connector parameter to drm_dp_aux_register() should be
fine.

> 
-- 
Cheers,
	Lyude Paul

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