[PATCH 0/3] RFC: Add a drm_aux-dev module.

[PATCH 0/3] RFC: Add a drm_aux-dev module.

[Rafael Antognolli]

This is a tentative implementation of a module that allows reading/writing
arbitrary dpcd registers, following the suggestion from Daniel Vetter. It
assumes the drm aux helpers were used by the driver.

I tried to follow the i2c-dev implementation as close as possible, but the only
operations that are provided on the dev node are two different ioctl's, one for
reading a register and another one for writing it.

I have at least 2 open questions:

 * Right now the AUX channels are stored in a global list inside the
   drm_dp_helper implementation, and I assume that's not ideal. A different
   approach would be to iterate over the list of connectors, instead of the
   list of AUX channels, but that would require the struct drm_connector or
   similar to know about the respective aux helper. It could be added as a
   function to register that, or as a new method on the drm_connector_funcs to
   retrieve the aux channel helper.

 * From the created sysfs drm_aux-dev device it's possible to know what is the
   respective connector, but not the other way around. Is this good enough?

Please provide any feedback you have and tell me if this is the idea you had
initially for this kind of implementation. Or, if it's nothing like this, let
me know what else you had in mind.

If I'm going in the right direction, I'll refine the patch to provide full
documentation and tests if needed.

Rafael Antognolli (3):
  drm/dp: Keep a list of drm_dp_aux helper.
  drm/dp: Store the drm_connector device pointer on the helper.
  drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

 Documentation/ioctl/ioctl-number.txt |   1 +
 drivers/gpu/drm/Kconfig              |   4 +
 drivers/gpu/drm/Makefile             |   1 +
 drivers/gpu/drm/drm_aux-dev.c        | 390 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_dp_helper.c      |  71 +++++++
 drivers/gpu/drm/i915/intel_dp.c      |   1 +
 include/drm/drm_dp_helper.h          |   7 +
 include/uapi/linux/Kbuild            |   1 +
 include/uapi/linux/drm_aux-dev.h     |  45 ++++
 9 files changed, 521 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_aux-dev.c
 create mode 100644 include/uapi/linux/drm_aux-dev.h

-- 
2.4.0

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

[PATCH 1/3] drm/dp: Keep a list of drm_dp_aux helper.

[Rafael Antognolli]

This list will be used to get the aux channels registered through the
helpers. Two functions are provided to register/unregister notifier
listeners on the list, and another functiont to iterate over the list of
aux channels.

Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 71 +++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     |  6 ++++
 2 files changed, 77 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 291734e..01a1489 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -710,6 +710,54 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
 	.master_xfer = drm_dp_i2c_xfer,
 };
 
+struct drm_dp_aux_node {
+	struct klist_node list;
+	struct drm_dp_aux *aux;
+};
+
+static DEFINE_KLIST(drm_dp_aux_list, NULL, NULL);
+
+static BLOCKING_NOTIFIER_HEAD(aux_notifier);
+
+int drm_dp_aux_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&aux_notifier, nb);
+}
+EXPORT_SYMBOL(drm_dp_aux_register_notifier);
+
+int drm_dp_aux_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&aux_notifier, nb);
+}
+EXPORT_SYMBOL(drm_dp_aux_unregister_notifier);
+
+static struct drm_dp_aux *next_aux(struct klist_iter *i)
+{
+	struct klist_node *n = klist_next(i);
+	struct drm_dp_aux *aux = NULL;
+	struct drm_dp_aux_node *aux_node;
+
+	if (n) {
+		aux_node = container_of(n, struct drm_dp_aux_node, list);
+		aux = aux_node->aux;
+	}
+	return aux;
+}
+
+int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *))
+{
+	struct klist_iter i;
+	struct drm_dp_aux *aux;
+	int error = 0;
+
+	klist_iter_init(&drm_dp_aux_list, &i);
+	while ((aux = next_aux(&i)) && !error)
+		error = fn(aux, data);
+	klist_iter_exit(&i);
+	return error;
+}
+EXPORT_SYMBOL(drm_dp_aux_for_each);
+
 /**
  * drm_dp_aux_register() - initialise and register aux channel
  * @aux: DisplayPort AUX channel
@@ -718,6 +766,7 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
  */
 int drm_dp_aux_register(struct drm_dp_aux *aux)
 {
+	struct drm_dp_aux_node *aux_node;
 	mutex_init(&aux->hw_mutex);
 
 	aux->ddc.algo = &drm_dp_i2c_algo;
@@ -732,6 +781,14 @@ 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));
 
+	/* add aux to list and notify listeners */
+	aux_node = kzalloc(sizeof(*aux_node), GFP_KERNEL);
+	if (!aux_node)
+		return -ENOMEM;
+	aux_node->aux = aux;
+	klist_add_tail(&aux_node->list, &drm_dp_aux_list);
+	blocking_notifier_call_chain(&aux_notifier, DRM_DP_ADD_AUX, aux);
+
 	return i2c_add_adapter(&aux->ddc);
 }
 EXPORT_SYMBOL(drm_dp_aux_register);
@@ -742,6 +799,20 @@ EXPORT_SYMBOL(drm_dp_aux_register);
  */
 void drm_dp_aux_unregister(struct drm_dp_aux *aux)
 {
+	struct klist_iter i;
+	struct klist_node *n;
+
+	klist_iter_init(&drm_dp_aux_list, &i);
+	while ((n = klist_next(&i))) {
+		struct drm_dp_aux_node *aux_node =
+			container_of(n, struct drm_dp_aux_node, list);
+		if (aux_node->aux == aux) {
+			klist_del(n);
+			kfree(aux_node);
+			break;
+		}
+	}
+	blocking_notifier_call_chain(&aux_notifier, DRM_DP_DEL_AUX, aux);
 	i2c_del_adapter(&aux->ddc);
 }
 EXPORT_SYMBOL(drm_dp_aux_unregister);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 8c52d0ef1..023620c 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -763,7 +763,13 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
 int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
 int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
 
+#define DRM_DP_ADD_AUX 0x01
+#define DRM_DP_DEL_AUX 0x02
+
 int drm_dp_aux_register(struct drm_dp_aux *aux);
 void drm_dp_aux_unregister(struct drm_dp_aux *aux);
+int drm_dp_aux_register_notifier(struct notifier_block *nb);
+int drm_dp_aux_unregister_notifier(struct notifier_block *nb);
+int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *));
 
 #endif /* _DRM_DP_HELPER_H_ */
-- 
2.4.0

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

[PATCH 2/3] drm/dp: Store the drm_connector device pointer on the helper.

[Rafael Antognolli]

This is useful to determine which connector owns this AUX channel.

Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 1 +
 include/drm/drm_dp_helper.h     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f8f4d99..6f481fc 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1078,6 +1078,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
 
 	intel_dp->aux.name = name;
 	intel_dp->aux.dev = dev->dev;
+	intel_dp->aux.connector = connector->base.kdev;
 	intel_dp->aux.transfer = intel_dp_aux_transfer;
 
 	DRM_DEBUG_KMS("registering %s bus for %s\n", name,
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 023620c..e481fbd 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -702,6 +702,7 @@ struct drm_dp_aux {
 	const char *name;
 	struct i2c_adapter ddc;
 	struct device *dev;
+	struct device *connector;
 	struct mutex hw_mutex;
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);
-- 
2.4.0

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

[PATCH 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_aux-N.

It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux-dev/drm_aux-N/connector, if
the connector device pointer was correctly set in the aux helper struct.

Two main operations are provided on the registers through ioctls: read
and write; and a helper struct is provided for that, used as:

	- address is the address of the register to read/write;
	- len is the number of bytes to read/write;
	- buf is a pointer to a buffer where to store the read data, or
	  where the data to be written is already stored;
	- the return value is the number of bytes read/written, on
	  success, or the error code otherwise. The number of
	  read/written bytes is also stored on the len field of the
	  struct.

Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
---
 Documentation/ioctl/ioctl-number.txt |   1 +
 drivers/gpu/drm/Kconfig              |   4 +
 drivers/gpu/drm/Makefile             |   1 +
 drivers/gpu/drm/drm_aux-dev.c        | 390 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/Kbuild            |   1 +
 include/uapi/linux/drm_aux-dev.h     |  45 ++++
 6 files changed, 442 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_aux-dev.c
 create mode 100644 include/uapi/linux/drm_aux-dev.h

diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 611c522..ea76980 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -93,6 +93,7 @@ Code  Seq#(hex)	Include File		Comments
 ';'	64-7F	linux/vfio.h
 '@'	00-0F	linux/radeonfb.h	conflict!
 '@'	00-0F	drivers/video/aty/aty128fb.c	conflict!
+'@'	10-2F	drm_aux-dev
 'A'	00-1F	linux/apm_bios.h	conflict!
 'A'	00-0F	linux/agpgart.h		conflict!
 		and drivers/char/agp/compat_ioctl.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1a0a8df..eae847c 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,10 @@ config DRM_MIPI_DSI
 	bool
 	depends on DRM
 
+config DRM_AUX_CHARDEV
+	tristate "DRM DP AUX Interface"
+	depends on DRM
+
 config DRM_KMS_HELPER
 	tristate
 	depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 45e7719..a1a94306 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -32,6 +32,7 @@ CFLAGS_drm_trace_points.o := -I$(src)
 
 obj-$(CONFIG_DRM)	+= drm.o
 obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
+obj-$(CONFIG_DRM_AUX_CHARDEV) += drm_aux-dev.o
 obj-$(CONFIG_DRM_TTM)	+= ttm/
 obj-$(CONFIG_DRM_TDFX)	+= tdfx/
 obj-$(CONFIG_DRM_R128)	+= r128/
diff --git a/drivers/gpu/drm/drm_aux-dev.c b/drivers/gpu/drm/drm_aux-dev.c
new file mode 100644
index 0000000..3ae9064
--- /dev/null
+++ b/drivers/gpu/drm/drm_aux-dev.c
@@ -0,0 +1,390 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Rafael Antognolli <rafael.antognolli@intel.com>
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/drm_aux-dev.h>
+#include <asm/uaccess.h>
+#include <drm/drm_dp_helper.h>
+#include <drm/drm_crtc.h>
+
+struct drm_aux_dev {
+	struct list_head list;
+	unsigned index;
+	struct drm_dp_aux *aux;
+	struct device *dev;
+};
+
+#define DRM_AUX_MINORS	256
+static int drm_aux_dev_count = 0;
+static LIST_HEAD(drm_aux_dev_list);
+static DEFINE_SPINLOCK(drm_aux_dev_list_lock);
+
+static struct drm_aux_dev *drm_aux_dev_get_by_minor(unsigned index)
+{
+	struct drm_aux_dev *aux_dev;
+
+	spin_lock(&drm_aux_dev_list_lock);
+	list_for_each_entry(aux_dev, &drm_aux_dev_list, list) {
+		if (aux_dev->index == index)
+			goto found;
+	}
+
+	aux_dev = NULL;
+found:
+	spin_unlock(&drm_aux_dev_list_lock);
+	return aux_dev;
+}
+
+static struct drm_aux_dev *drm_aux_dev_get_by_aux(struct drm_dp_aux *aux)
+{
+	struct drm_aux_dev *aux_dev;
+
+	spin_lock(&drm_aux_dev_list_lock);
+	list_for_each_entry(aux_dev, &drm_aux_dev_list, list) {
+		if (aux_dev->aux == aux)
+			goto found;
+	}
+
+	aux_dev = NULL;
+found:
+	spin_unlock(&drm_aux_dev_list_lock);
+	return aux_dev;
+}
+
+static struct drm_aux_dev *get_free_drm_aux_dev(struct drm_dp_aux *aux)
+{
+	struct drm_aux_dev *aux_dev;
+	int index;
+
+	spin_lock(&drm_aux_dev_list_lock);
+	index = drm_aux_dev_count;
+	spin_unlock(&drm_aux_dev_list_lock);
+	if (index >= DRM_AUX_MINORS) {
+		printk(KERN_ERR "i2c-dev: Out of device minors (%d)\n",
+		       index);
+		return ERR_PTR(-ENODEV);
+	}
+
+	aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL);
+	if (!aux_dev)
+		return ERR_PTR(-ENOMEM);
+	aux_dev->aux = aux;
+	aux_dev->index = index;
+
+	spin_lock(&drm_aux_dev_list_lock);
+	drm_aux_dev_count++;
+	list_add_tail(&aux_dev->list, &drm_aux_dev_list);
+	spin_unlock(&drm_aux_dev_list_lock);
+	return aux_dev;
+}
+
+static void return_drm_aux_dev(struct drm_aux_dev *aux_dev)
+{
+	spin_lock(&drm_aux_dev_list_lock);
+	list_del(&aux_dev->list);
+	spin_unlock(&drm_aux_dev_list_lock);
+	kfree(aux_dev);
+}
+
+static ssize_t name_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct drm_aux_dev *aux_dev = drm_aux_dev_get_by_minor(MINOR(dev->devt));
+
+	if (!aux_dev)
+		return -ENODEV;
+	return sprintf(buf, "%s\n", aux_dev->aux->name);
+}
+static DEVICE_ATTR_RO(name);
+
+static ssize_t connector_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct drm_aux_dev *aux_dev = drm_aux_dev_get_by_minor(MINOR(dev->devt));
+	struct drm_dp_aux *aux;
+	struct device *conn_dev;
+	struct drm_connector *connector = NULL;
+
+	if (!aux_dev)
+		return -ENODEV;
+	aux = aux_dev->aux;
+	conn_dev = aux->connector;
+	if (!conn_dev)
+		return sprintf(buf, "unknown\n");
+
+	connector = dev_get_drvdata(aux->connector);
+
+	return sprintf(buf, "%s\n", connector->name);
+}
+static DEVICE_ATTR_RO(connector);
+
+static struct attribute *drm_aux_attrs[] = {
+	&dev_attr_name.attr,
+	&dev_attr_connector.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(drm_aux);
+
+static long auxdev_ioctl_read(struct drm_aux_dev *aux_dev, unsigned long arg)
+{
+	struct drm_aux_dev_data rdwr_arg;
+	__u8 *buf;
+	int res;
+
+	/* Get register address, buffer and read length from user */
+	if (copy_from_user(&rdwr_arg,
+			   (struct drm_aux_dev_data __user *)arg,
+			   sizeof(rdwr_arg)))
+		return -EFAULT;
+
+	buf = memdup_user(rdwr_arg.buf, rdwr_arg.len * sizeof(__u8));
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
+
+	/* read register */
+	res = drm_dp_dpcd_read(aux_dev->aux, rdwr_arg.address, buf, rdwr_arg.len);
+	if (res < 0) {
+		kfree(buf);
+		return res;
+	}
+
+	/* return number of read bytes to user */
+	rdwr_arg.len = res;
+	if (copy_to_user((struct drm_aux_dev_data __user *)arg,
+			 &rdwr_arg, sizeof(rdwr_arg))) {
+		kfree(buf);
+		return -EFAULT;
+	}
+
+	/* return the read bytes to user */
+	if (copy_to_user((__u8 __user *)rdwr_arg.buf,
+			 buf, rdwr_arg.len)) {
+		kfree(buf);
+		return -EFAULT;
+	}
+
+	kfree(buf);
+	return res;
+}
+
+static long auxdev_ioctl_write(struct drm_aux_dev *aux_dev, unsigned long arg)
+{
+	struct drm_aux_dev_data rdwr_arg;
+	__u8 *buf;
+	int res;
+
+	/* Get register address and data to write */
+	if (copy_from_user(&rdwr_arg,
+			   (struct drm_aux_dev_data __user *)arg,
+			   sizeof(rdwr_arg)))
+		return -EFAULT;
+
+	buf = memdup_user(rdwr_arg.buf, rdwr_arg.len * sizeof(__u8));
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
+
+	/* write register */
+	res = drm_dp_dpcd_write(aux_dev->aux, rdwr_arg.address, buf, rdwr_arg.len);
+	if (res < 0) {
+		kfree(buf);
+		return res;
+	}
+
+	/* return length of written data */
+	rdwr_arg.len = res;
+	if (copy_to_user((struct drm_aux_dev_data __user *)arg,
+			 &rdwr_arg, sizeof(rdwr_arg))) {
+		kfree(buf);
+		return -EFAULT;
+	}
+
+	kfree(buf);
+	return res;
+}
+
+static long auxdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct drm_aux_dev *aux_dev = file->private_data;
+	dev_dbg(aux_dev->aux->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n",
+		cmd, arg);
+	switch (cmd) {
+	case DRM_AUX_DEV_READ:
+		return auxdev_ioctl_read(aux_dev, arg);
+	case DRM_AUX_DEV_WRITE:
+		return auxdev_ioctl_write(aux_dev, arg);
+	default:
+		return -ENOTTY;
+	}
+
+	return 0;
+}
+
+static int auxdev_open(struct inode *inode, struct file *file)
+{
+	unsigned int minor = iminor(inode);
+	struct drm_aux_dev *aux_dev;
+
+	aux_dev = drm_aux_dev_get_by_minor(minor);
+	if (!aux_dev)
+		return -ENODEV;
+
+	file->private_data = aux_dev;
+	return 0;
+}
+
+static const struct file_operations auxdev_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.unlocked_ioctl	= auxdev_ioctl,
+	.open		= auxdev_open,
+};
+
+static struct class *drm_aux_dev_class;
+static int drm_dev_major = -1;
+
+#define to_auxdev(d) container_of(d, struct drm_aux_dev, aux)
+
+static int auxdev_attach_aux(struct drm_dp_aux *aux, void *data)
+{
+	int *major = data;
+	struct drm_aux_dev *aux_dev;
+	int res;
+
+	aux_dev = get_free_drm_aux_dev(aux);
+	if (IS_ERR(aux_dev))
+		return PTR_ERR(aux_dev);
+
+	aux_dev->dev = device_create(drm_aux_dev_class, aux->dev,
+				     MKDEV(*major, aux_dev->index), NULL,
+				     "drm_aux-%d", aux_dev->index);
+	if (IS_ERR(aux_dev->dev)) {
+		res = PTR_ERR(aux_dev->dev);
+		goto error;
+	}
+
+	pr_debug("drm_aux-dev: aux [%s] registered as minor %d\n",
+		 aux->name, aux_dev->index);
+	return 0;
+error:
+	return_drm_aux_dev(aux_dev);
+	return res;
+
+}
+
+static int auxdev_detach_aux(struct drm_dp_aux *aux, void *data)
+{
+	int *major = data;
+	int minor;
+	struct drm_aux_dev *aux_dev;
+
+	aux_dev = drm_aux_dev_get_by_aux(aux);
+	if (!aux_dev) /* attach must have failed */
+		return 0;
+
+	minor = aux_dev->index;
+	return_drm_aux_dev(aux_dev);
+	device_destroy(drm_aux_dev_class, MKDEV(*major, minor));
+
+	pr_debug("drm_aux-dev: aux [%s] unregistered\n", aux->name);
+	return 0;
+}
+
+static int auxdev_notifier_call(struct notifier_block *nb, unsigned long action,
+				void *data)
+{
+	struct drm_dp_aux *aux = data;
+	switch (action) {
+	case DRM_DP_ADD_AUX:
+		return auxdev_attach_aux(aux, &drm_dev_major);
+	case DRM_DP_DEL_AUX:
+		return auxdev_detach_aux(aux, &drm_dev_major);
+	}
+
+	return 0;
+}
+
+static struct notifier_block auxdev_notifier = {
+	.notifier_call = auxdev_notifier_call,
+};
+
+static int __init drm_aux_dev_init(void)
+{
+	int res;
+
+	printk(KERN_INFO "drm dp aux /dev entries driver\n");
+
+	res = register_chrdev(0, "aux", &auxdev_fops);
+	if (res < 0)
+		goto out;
+	drm_dev_major = res;
+
+	drm_aux_dev_class = class_create(THIS_MODULE, "drm_aux-dev");
+	if (IS_ERR(drm_aux_dev_class)) {
+		res = PTR_ERR(drm_aux_dev_class);
+		goto out_unreg_chrdev;
+	}
+	drm_aux_dev_class->dev_groups = drm_aux_groups;
+
+	/* Keep track of DP aux that will be added or removed later */
+	res = drm_dp_aux_register_notifier(&auxdev_notifier);
+	if (res)
+		goto out_unreg_class;
+
+	/* Bind to already existing DP aux */
+	res = drm_dp_aux_for_each(&drm_dev_major, auxdev_attach_aux);
+
+	return 0;
+
+out_unreg_class:
+	class_destroy(drm_aux_dev_class);
+out_unreg_chrdev:
+	unregister_chrdev(drm_dev_major, "aux");
+out:
+	printk(KERN_ERR "%s: Driver Initialisation failed\n", __FILE__);
+	return res;
+}
+
+static void __exit drm_aux_dev_exit(void)
+{
+	printk(KERN_INFO "drm dp aux /dev entries driver - unloading\n");
+	drm_dp_aux_unregister_notifier(&auxdev_notifier);
+	drm_dp_aux_for_each(&drm_dev_major, auxdev_detach_aux);
+	class_destroy(drm_aux_dev_class);
+	unregister_chrdev(drm_dev_major, "aux");
+}
+
+MODULE_AUTHOR("Rafael Antognolli <rafael.antognolli@intel.com>");
+MODULE_DESCRIPTION("DRM DP AUX /dev entries driver");
+MODULE_LICENSE("GPL");
+
+module_init(drm_aux_dev_init);
+module_exit(drm_aux_dev_exit);
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 1ff9942..07cdc8c 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -105,6 +105,7 @@ header-y += dm-ioctl.h
 header-y += dm-log-userspace.h
 header-y += dn.h
 header-y += dqblk_xfs.h
+header-y += drm_aux-dev.h
 header-y += edd.h
 header-y += efs_fs_sb.h
 header-y += elfcore.h
diff --git a/include/uapi/linux/drm_aux-dev.h b/include/uapi/linux/drm_aux-dev.h
new file mode 100644
index 0000000..668b631
--- /dev/null
+++ b/include/uapi/linux/drm_aux-dev.h
@@ -0,0 +1,45 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Rafael Antognolli <rafael.antognolli@intel.com>
+ *
+ */
+
+#ifndef _UAPI_LINUX_DRM_AUX_DEV_H
+#define _UAPI_LINUX_DRM_AUX_DEV_H
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+#define DRM_AUX_DEV_MAGIC '@'
+
+struct drm_aux_dev_data {
+	__u32 address; /* of the register */
+	__u16 len; /* size of the read/write */
+	__u8 *buf;
+};
+
+#define DRM_AUX_DEV_READ _IOWR(DRM_AUX_DEV_MAGIC, 0x10, struct drm_aux_dev_data)
+#define DRM_AUX_DEV_WRITE _IOW(DRM_AUX_DEV_MAGIC, 0x11, struct drm_aux_dev_data)
+
+#endif // _UAPI_LINUX_DRM_AUX_DEV_H
-- 
2.4.0

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

Re: [PATCH 0/3] RFC: Add a drm_aux-dev module.

[Ville Syrjälä]

On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote:
> This is a tentative implementation of a module that allows reading/writing
> arbitrary dpcd registers, following the suggestion from Daniel Vetter. It
> assumes the drm aux helpers were used by the driver.
> 
> I tried to follow the i2c-dev implementation as close as possible, but the only
> operations that are provided on the dev node are two different ioctl's, one for
> reading a register and another one for writing it.

Why would you use ioctls instead of normal read/write syscalls?

> 
> I have at least 2 open questions:
> 
>  * Right now the AUX channels are stored in a global list inside the
>    drm_dp_helper implementation, and I assume that's not ideal. A different
>    approach would be to iterate over the list of connectors, instead of the
>    list of AUX channels, but that would require the struct drm_connector or
>    similar to know about the respective aux helper. It could be added as a
>    function to register that, or as a new method on the drm_connector_funcs to
>    retrieve the aux channel helper.
> 
>  * From the created sysfs drm_aux-dev device it's possible to know what is the
>    respective connector, but not the other way around. Is this good enough?
> 
> Please provide any feedback you have and tell me if this is the idea you had
> initially for this kind of implementation. Or, if it's nothing like this, let
> me know what else you had in mind.
> 
> If I'm going in the right direction, I'll refine the patch to provide full
> documentation and tests if needed.
> 
> Rafael Antognolli (3):
>   drm/dp: Keep a list of drm_dp_aux helper.
>   drm/dp: Store the drm_connector device pointer on the helper.
>   drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
> 
>  Documentation/ioctl/ioctl-number.txt |   1 +
>  drivers/gpu/drm/Kconfig              |   4 +
>  drivers/gpu/drm/Makefile             |   1 +
>  drivers/gpu/drm/drm_aux-dev.c        | 390 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_dp_helper.c      |  71 +++++++
>  drivers/gpu/drm/i915/intel_dp.c      |   1 +
>  include/drm/drm_dp_helper.h          |   7 +
>  include/uapi/linux/Kbuild            |   1 +
>  include/uapi/linux/drm_aux-dev.h     |  45 ++++
>  9 files changed, 521 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
>  create mode 100644 include/uapi/linux/drm_aux-dev.h
> 
> -- 
> 2.4.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 1/3] drm/dp: Keep a list of drm_dp_aux helper.

[Ville Syrjälä]

On Mon, Sep 14, 2015 at 04:12:30PM -0700, Rafael Antognolli wrote:
> This list will be used to get the aux channels registered through the
> helpers. Two functions are provided to register/unregister notifier
> listeners on the list, and another functiont to iterate over the list of
> aux channels.
> 
> Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 71 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     |  6 ++++
>  2 files changed, 77 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 291734e..01a1489 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -710,6 +710,54 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
>  	.master_xfer = drm_dp_i2c_xfer,
>  };
>  
> +struct drm_dp_aux_node {
> +	struct klist_node list;
> +	struct drm_dp_aux *aux;
> +};
> +
> +static DEFINE_KLIST(drm_dp_aux_list, NULL, NULL);
> +
> +static BLOCKING_NOTIFIER_HEAD(aux_notifier);
> +
> +int drm_dp_aux_register_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&aux_notifier, nb);
> +}
> +EXPORT_SYMBOL(drm_dp_aux_register_notifier);

Why is this notifier stuff needed? Why not just register/unregister the
aux-dev directly?

>+
> +int drm_dp_aux_unregister_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_unregister(&aux_notifier, nb);
> +}
> +EXPORT_SYMBOL(drm_dp_aux_unregister_notifier);
> +
> +static struct drm_dp_aux *next_aux(struct klist_iter *i)
> +{
> +	struct klist_node *n = klist_next(i);
> +	struct drm_dp_aux *aux = NULL;
> +	struct drm_dp_aux_node *aux_node;
> +
> +	if (n) {
> +		aux_node = container_of(n, struct drm_dp_aux_node, list);
> +		aux = aux_node->aux;
> +	}
> +	return aux;
> +}
> +
> +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *))
> +{
> +	struct klist_iter i;
> +	struct drm_dp_aux *aux;
> +	int error = 0;
> +
> +	klist_iter_init(&drm_dp_aux_list, &i);
> +	while ((aux = next_aux(&i)) && !error)
> +		error = fn(aux, data);
> +	klist_iter_exit(&i);
> +	return error;
> +}
> +EXPORT_SYMBOL(drm_dp_aux_for_each);
> +
>  /**
>   * drm_dp_aux_register() - initialise and register aux channel
>   * @aux: DisplayPort AUX channel
> @@ -718,6 +766,7 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
>   */
>  int drm_dp_aux_register(struct drm_dp_aux *aux)
>  {
> +	struct drm_dp_aux_node *aux_node;
>  	mutex_init(&aux->hw_mutex);
>  
>  	aux->ddc.algo = &drm_dp_i2c_algo;
> @@ -732,6 +781,14 @@ 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));
>  
> +	/* add aux to list and notify listeners */
> +	aux_node = kzalloc(sizeof(*aux_node), GFP_KERNEL);
> +	if (!aux_node)
> +		return -ENOMEM;
> +	aux_node->aux = aux;
> +	klist_add_tail(&aux_node->list, &drm_dp_aux_list);
> +	blocking_notifier_call_chain(&aux_notifier, DRM_DP_ADD_AUX, aux);
> +
>  	return i2c_add_adapter(&aux->ddc);
>  }
>  EXPORT_SYMBOL(drm_dp_aux_register);
> @@ -742,6 +799,20 @@ EXPORT_SYMBOL(drm_dp_aux_register);
>   */
>  void drm_dp_aux_unregister(struct drm_dp_aux *aux)
>  {
> +	struct klist_iter i;
> +	struct klist_node *n;
> +
> +	klist_iter_init(&drm_dp_aux_list, &i);
> +	while ((n = klist_next(&i))) {
> +		struct drm_dp_aux_node *aux_node =
> +			container_of(n, struct drm_dp_aux_node, list);
> +		if (aux_node->aux == aux) {
> +			klist_del(n);
> +			kfree(aux_node);
> +			break;
> +		}
> +	}
> +	blocking_notifier_call_chain(&aux_notifier, DRM_DP_DEL_AUX, aux);
>  	i2c_del_adapter(&aux->ddc);
>  }
>  EXPORT_SYMBOL(drm_dp_aux_unregister);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 8c52d0ef1..023620c 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -763,7 +763,13 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
>  int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
>  int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
>  
> +#define DRM_DP_ADD_AUX 0x01
> +#define DRM_DP_DEL_AUX 0x02
> +
>  int drm_dp_aux_register(struct drm_dp_aux *aux);
>  void drm_dp_aux_unregister(struct drm_dp_aux *aux);
> +int drm_dp_aux_register_notifier(struct notifier_block *nb);
> +int drm_dp_aux_unregister_notifier(struct notifier_block *nb);
> +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *));
>  
>  #endif /* _DRM_DP_HELPER_H_ */
> -- 
> 2.4.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 1/3] drm/dp: Keep a list of drm_dp_aux helper.

[Rafael Antognolli]

On Tue, Sep 15, 2015 at 10:46:43AM +0300, Ville Syrjälä wrote:
> On Mon, Sep 14, 2015 at 04:12:30PM -0700, Rafael Antognolli wrote:
> > This list will be used to get the aux channels registered through the
> > helpers. Two functions are provided to register/unregister notifier
> > listeners on the list, and another functiont to iterate over the list of
> > aux channels.
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c | 71 +++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_dp_helper.h     |  6 ++++
> >  2 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 291734e..01a1489 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -710,6 +710,54 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
> >  	.master_xfer = drm_dp_i2c_xfer,
> >  };
> >  
> > +struct drm_dp_aux_node {
> > +	struct klist_node list;
> > +	struct drm_dp_aux *aux;
> > +};
> > +
> > +static DEFINE_KLIST(drm_dp_aux_list, NULL, NULL);
> > +
> > +static BLOCKING_NOTIFIER_HEAD(aux_notifier);
> > +
> > +int drm_dp_aux_register_notifier(struct notifier_block *nb)
> > +{
> > +	return blocking_notifier_chain_register(&aux_notifier, nb);
> > +}
> > +EXPORT_SYMBOL(drm_dp_aux_register_notifier);
> 
> Why is this notifier stuff needed? Why not just register/unregister the
> aux-dev directly?
> 

I am not sure it's needed, I was just looking for the best way of
informing aux-dev that a new aux channel was added.

By register/unregister the aux-dev directly, do you mean making this
drm_dp_helper aware of the aux dev, when it's registered, and directly
calling some callback to inform that new aux channels were added?

> >+
> > +int drm_dp_aux_unregister_notifier(struct notifier_block *nb)
> > +{
> > +	return blocking_notifier_chain_unregister(&aux_notifier, nb);
> > +}
> > +EXPORT_SYMBOL(drm_dp_aux_unregister_notifier);
> > +
> > +static struct drm_dp_aux *next_aux(struct klist_iter *i)
> > +{
> > +	struct klist_node *n = klist_next(i);
> > +	struct drm_dp_aux *aux = NULL;
> > +	struct drm_dp_aux_node *aux_node;
> > +
> > +	if (n) {
> > +		aux_node = container_of(n, struct drm_dp_aux_node, list);
> > +		aux = aux_node->aux;
> > +	}
> > +	return aux;
> > +}
> > +
> > +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *))
> > +{
> > +	struct klist_iter i;
> > +	struct drm_dp_aux *aux;
> > +	int error = 0;
> > +
> > +	klist_iter_init(&drm_dp_aux_list, &i);
> > +	while ((aux = next_aux(&i)) && !error)
> > +		error = fn(aux, data);
> > +	klist_iter_exit(&i);
> > +	return error;
> > +}
> > +EXPORT_SYMBOL(drm_dp_aux_for_each);
> > +
> >  /**
> >   * drm_dp_aux_register() - initialise and register aux channel
> >   * @aux: DisplayPort AUX channel
> > @@ -718,6 +766,7 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
> >   */
> >  int drm_dp_aux_register(struct drm_dp_aux *aux)
> >  {
> > +	struct drm_dp_aux_node *aux_node;
> >  	mutex_init(&aux->hw_mutex);
> >  
> >  	aux->ddc.algo = &drm_dp_i2c_algo;
> > @@ -732,6 +781,14 @@ 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));
> >  
> > +	/* add aux to list and notify listeners */
> > +	aux_node = kzalloc(sizeof(*aux_node), GFP_KERNEL);
> > +	if (!aux_node)
> > +		return -ENOMEM;
> > +	aux_node->aux = aux;
> > +	klist_add_tail(&aux_node->list, &drm_dp_aux_list);
> > +	blocking_notifier_call_chain(&aux_notifier, DRM_DP_ADD_AUX, aux);
> > +
> >  	return i2c_add_adapter(&aux->ddc);
> >  }
> >  EXPORT_SYMBOL(drm_dp_aux_register);
> > @@ -742,6 +799,20 @@ EXPORT_SYMBOL(drm_dp_aux_register);
> >   */
> >  void drm_dp_aux_unregister(struct drm_dp_aux *aux)
> >  {
> > +	struct klist_iter i;
> > +	struct klist_node *n;
> > +
> > +	klist_iter_init(&drm_dp_aux_list, &i);
> > +	while ((n = klist_next(&i))) {
> > +		struct drm_dp_aux_node *aux_node =
> > +			container_of(n, struct drm_dp_aux_node, list);
> > +		if (aux_node->aux == aux) {
> > +			klist_del(n);
> > +			kfree(aux_node);
> > +			break;
> > +		}
> > +	}
> > +	blocking_notifier_call_chain(&aux_notifier, DRM_DP_DEL_AUX, aux);
> >  	i2c_del_adapter(&aux->ddc);
> >  }
> >  EXPORT_SYMBOL(drm_dp_aux_unregister);
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 8c52d0ef1..023620c 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -763,7 +763,13 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
> >  int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
> >  int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
> >  
> > +#define DRM_DP_ADD_AUX 0x01
> > +#define DRM_DP_DEL_AUX 0x02
> > +
> >  int drm_dp_aux_register(struct drm_dp_aux *aux);
> >  void drm_dp_aux_unregister(struct drm_dp_aux *aux);
> > +int drm_dp_aux_register_notifier(struct notifier_block *nb);
> > +int drm_dp_aux_unregister_notifier(struct notifier_block *nb);
> > +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *));
> >  
> >  #endif /* _DRM_DP_HELPER_H_ */
> > -- 
> > 2.4.0
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] RFC: Add a drm_aux-dev module.

[Rafael Antognolli]

On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote:
> On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote:
> > This is a tentative implementation of a module that allows reading/writing
> > arbitrary dpcd registers, following the suggestion from Daniel Vetter. It
> > assumes the drm aux helpers were used by the driver.
> > 
> > I tried to follow the i2c-dev implementation as close as possible, but the only
> > operations that are provided on the dev node are two different ioctl's, one for
> > reading a register and another one for writing it.
> 
> Why would you use ioctls instead of normal read/write syscalls?
> 

For writing, that should work fine, I can easily change that if you
prefer.

For reading, one has to first tell which register address is going to be
read. So it would require to first write the address on the file, to
then read. It was suggested that an ioctl to be used, and I understood
it was to solve this, but maybe I'm wrong.

I don't have any particular preference for the API, could easily change
this code to use just read/writes.

Thanks,
Rafael

> > 
> > I have at least 2 open questions:
> > 
> >  * Right now the AUX channels are stored in a global list inside the
> >    drm_dp_helper implementation, and I assume that's not ideal. A different
> >    approach would be to iterate over the list of connectors, instead of the
> >    list of AUX channels, but that would require the struct drm_connector or
> >    similar to know about the respective aux helper. It could be added as a
> >    function to register that, or as a new method on the drm_connector_funcs to
> >    retrieve the aux channel helper.
> > 
> >  * From the created sysfs drm_aux-dev device it's possible to know what is the
> >    respective connector, but not the other way around. Is this good enough?
> > 
> > Please provide any feedback you have and tell me if this is the idea you had
> > initially for this kind of implementation. Or, if it's nothing like this, let
> > me know what else you had in mind.
> > 
> > If I'm going in the right direction, I'll refine the patch to provide full
> > documentation and tests if needed.
> > 
> > Rafael Antognolli (3):
> >   drm/dp: Keep a list of drm_dp_aux helper.
> >   drm/dp: Store the drm_connector device pointer on the helper.
> >   drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
> > 
> >  Documentation/ioctl/ioctl-number.txt |   1 +
> >  drivers/gpu/drm/Kconfig              |   4 +
> >  drivers/gpu/drm/Makefile             |   1 +
> >  drivers/gpu/drm/drm_aux-dev.c        | 390 +++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_dp_helper.c      |  71 +++++++
> >  drivers/gpu/drm/i915/intel_dp.c      |   1 +
> >  include/drm/drm_dp_helper.h          |   7 +
> >  include/uapi/linux/Kbuild            |   1 +
> >  include/uapi/linux/drm_aux-dev.h     |  45 ++++
> >  9 files changed, 521 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> >  create mode 100644 include/uapi/linux/drm_aux-dev.h
> > 
> > -- 
> > 2.4.0
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] RFC: Add a drm_aux-dev module.

[Ville Syrjälä]

On Tue, Sep 15, 2015 at 09:34:15AM -0700, Rafael Antognolli wrote:
> On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote:
> > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote:
> > > This is a tentative implementation of a module that allows reading/writing
> > > arbitrary dpcd registers, following the suggestion from Daniel Vetter. It
> > > assumes the drm aux helpers were used by the driver.
> > > 
> > > I tried to follow the i2c-dev implementation as close as possible, but the only
> > > operations that are provided on the dev node are two different ioctl's, one for
> > > reading a register and another one for writing it.
> > 
> > Why would you use ioctls instead of normal read/write syscalls?
> > 
> 
> For writing, that should work fine, I can easily change that if you
> prefer.
> 
> For reading, one has to first tell which register address is going to be
> read.

seek()

> So it would require to first write the address on the file, to
> then read. It was suggested that an ioctl to be used, and I understood
> it was to solve this, but maybe I'm wrong.
> 
> I don't have any particular preference for the API, could easily change
> this code to use just read/writes.
> 
> Thanks,
> Rafael
> 
> > > 
> > > I have at least 2 open questions:
> > > 
> > >  * Right now the AUX channels are stored in a global list inside the
> > >    drm_dp_helper implementation, and I assume that's not ideal. A different
> > >    approach would be to iterate over the list of connectors, instead of the
> > >    list of AUX channels, but that would require the struct drm_connector or
> > >    similar to know about the respective aux helper. It could be added as a
> > >    function to register that, or as a new method on the drm_connector_funcs to
> > >    retrieve the aux channel helper.
> > > 
> > >  * From the created sysfs drm_aux-dev device it's possible to know what is the
> > >    respective connector, but not the other way around. Is this good enough?
> > > 
> > > Please provide any feedback you have and tell me if this is the idea you had
> > > initially for this kind of implementation. Or, if it's nothing like this, let
> > > me know what else you had in mind.
> > > 
> > > If I'm going in the right direction, I'll refine the patch to provide full
> > > documentation and tests if needed.
> > > 
> > > Rafael Antognolli (3):
> > >   drm/dp: Keep a list of drm_dp_aux helper.
> > >   drm/dp: Store the drm_connector device pointer on the helper.
> > >   drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
> > > 
> > >  Documentation/ioctl/ioctl-number.txt |   1 +
> > >  drivers/gpu/drm/Kconfig              |   4 +
> > >  drivers/gpu/drm/Makefile             |   1 +
> > >  drivers/gpu/drm/drm_aux-dev.c        | 390 +++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_dp_helper.c      |  71 +++++++
> > >  drivers/gpu/drm/i915/intel_dp.c      |   1 +
> > >  include/drm/drm_dp_helper.h          |   7 +
> > >  include/uapi/linux/Kbuild            |   1 +
> > >  include/uapi/linux/drm_aux-dev.h     |  45 ++++
> > >  9 files changed, 521 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> > >  create mode 100644 include/uapi/linux/drm_aux-dev.h
> > > 
> > > -- 
> > > 2.4.0
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

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

Re: [PATCH 0/3] RFC: Add a drm_aux-dev module.

[Ville Syrjälä]

On Tue, Sep 15, 2015 at 07:41:06PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 15, 2015 at 09:34:15AM -0700, Rafael Antognolli wrote:
> > On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote:
> > > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote:
> > > > This is a tentative implementation of a module that allows reading/writing
> > > > arbitrary dpcd registers, following the suggestion from Daniel Vetter. It
> > > > assumes the drm aux helpers were used by the driver.
> > > > 
> > > > I tried to follow the i2c-dev implementation as close as possible, but the only
> > > > operations that are provided on the dev node are two different ioctl's, one for
> > > > reading a register and another one for writing it.
> > > 
> > > Why would you use ioctls instead of normal read/write syscalls?
> > > 
> > 
> > For writing, that should work fine, I can easily change that if you
> > prefer.
> > 
> > For reading, one has to first tell which register address is going to be
> > read.
> 
> seek()

Sorry typo. Make that lseek(). 

> 
> > So it would require to first write the address on the file, to
> > then read. It was suggested that an ioctl to be used, and I understood
> > it was to solve this, but maybe I'm wrong.
> > 
> > I don't have any particular preference for the API, could easily change
> > this code to use just read/writes.
> > 
> > Thanks,
> > Rafael
> > 
> > > > 
> > > > I have at least 2 open questions:
> > > > 
> > > >  * Right now the AUX channels are stored in a global list inside the
> > > >    drm_dp_helper implementation, and I assume that's not ideal. A different
> > > >    approach would be to iterate over the list of connectors, instead of the
> > > >    list of AUX channels, but that would require the struct drm_connector or
> > > >    similar to know about the respective aux helper. It could be added as a
> > > >    function to register that, or as a new method on the drm_connector_funcs to
> > > >    retrieve the aux channel helper.
> > > > 
> > > >  * From the created sysfs drm_aux-dev device it's possible to know what is the
> > > >    respective connector, but not the other way around. Is this good enough?
> > > > 
> > > > Please provide any feedback you have and tell me if this is the idea you had
> > > > initially for this kind of implementation. Or, if it's nothing like this, let
> > > > me know what else you had in mind.
> > > > 
> > > > If I'm going in the right direction, I'll refine the patch to provide full
> > > > documentation and tests if needed.
> > > > 
> > > > Rafael Antognolli (3):
> > > >   drm/dp: Keep a list of drm_dp_aux helper.
> > > >   drm/dp: Store the drm_connector device pointer on the helper.
> > > >   drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
> > > > 
> > > >  Documentation/ioctl/ioctl-number.txt |   1 +
> > > >  drivers/gpu/drm/Kconfig              |   4 +
> > > >  drivers/gpu/drm/Makefile             |   1 +
> > > >  drivers/gpu/drm/drm_aux-dev.c        | 390 +++++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/drm_dp_helper.c      |  71 +++++++
> > > >  drivers/gpu/drm/i915/intel_dp.c      |   1 +
> > > >  include/drm/drm_dp_helper.h          |   7 +
> > > >  include/uapi/linux/Kbuild            |   1 +
> > > >  include/uapi/linux/drm_aux-dev.h     |  45 ++++
> > > >  9 files changed, 521 insertions(+)
> > > >  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> > > >  create mode 100644 include/uapi/linux/drm_aux-dev.h
> > > > 
> > > > -- 
> > > > 2.4.0
> > > > 
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

Re: [PATCH 1/3] drm/dp: Keep a list of drm_dp_aux helper.

[Ville Syrjälä]

On Tue, Sep 15, 2015 at 09:27:27AM -0700, Rafael Antognolli wrote:
> On Tue, Sep 15, 2015 at 10:46:43AM +0300, Ville Syrjälä wrote:
> > On Mon, Sep 14, 2015 at 04:12:30PM -0700, Rafael Antognolli wrote:
> > > This list will be used to get the aux channels registered through the
> > > helpers. Two functions are provided to register/unregister notifier
> > > listeners on the list, and another functiont to iterate over the list of
> > > aux channels.
> > > 
> > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_helper.c | 71 +++++++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_dp_helper.h     |  6 ++++
> > >  2 files changed, 77 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > index 291734e..01a1489 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -710,6 +710,54 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
> > >  	.master_xfer = drm_dp_i2c_xfer,
> > >  };
> > >  
> > > +struct drm_dp_aux_node {
> > > +	struct klist_node list;
> > > +	struct drm_dp_aux *aux;
> > > +};
> > > +
> > > +static DEFINE_KLIST(drm_dp_aux_list, NULL, NULL);
> > > +
> > > +static BLOCKING_NOTIFIER_HEAD(aux_notifier);
> > > +
> > > +int drm_dp_aux_register_notifier(struct notifier_block *nb)
> > > +{
> > > +	return blocking_notifier_chain_register(&aux_notifier, nb);
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_aux_register_notifier);
> > 
> > Why is this notifier stuff needed? Why not just register/unregister the
> > aux-dev directly?
> > 
> 
> I am not sure it's needed, I was just looking for the best way of
> informing aux-dev that a new aux channel was added.
> 
> By register/unregister the aux-dev directly, do you mean making this
> drm_dp_helper aware of the aux dev, when it's registered, and directly
> calling some callback to inform that new aux channels were added?

That was my thought, yes. It would mean the auxdev module can't be
unloaded like i2c-dev, but I'm not sure that's a use case worth
worrying about.

> 
> > >+
> > > +int drm_dp_aux_unregister_notifier(struct notifier_block *nb)
> > > +{
> > > +	return blocking_notifier_chain_unregister(&aux_notifier, nb);
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_aux_unregister_notifier);
> > > +
> > > +static struct drm_dp_aux *next_aux(struct klist_iter *i)
> > > +{
> > > +	struct klist_node *n = klist_next(i);
> > > +	struct drm_dp_aux *aux = NULL;
> > > +	struct drm_dp_aux_node *aux_node;
> > > +
> > > +	if (n) {
> > > +		aux_node = container_of(n, struct drm_dp_aux_node, list);
> > > +		aux = aux_node->aux;
> > > +	}
> > > +	return aux;
> > > +}
> > > +
> > > +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *))
> > > +{
> > > +	struct klist_iter i;
> > > +	struct drm_dp_aux *aux;
> > > +	int error = 0;
> > > +
> > > +	klist_iter_init(&drm_dp_aux_list, &i);
> > > +	while ((aux = next_aux(&i)) && !error)
> > > +		error = fn(aux, data);
> > > +	klist_iter_exit(&i);
> > > +	return error;
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_aux_for_each);
> > > +
> > >  /**
> > >   * drm_dp_aux_register() - initialise and register aux channel
> > >   * @aux: DisplayPort AUX channel
> > > @@ -718,6 +766,7 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
> > >   */
> > >  int drm_dp_aux_register(struct drm_dp_aux *aux)
> > >  {
> > > +	struct drm_dp_aux_node *aux_node;
> > >  	mutex_init(&aux->hw_mutex);
> > >  
> > >  	aux->ddc.algo = &drm_dp_i2c_algo;
> > > @@ -732,6 +781,14 @@ 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));
> > >  
> > > +	/* add aux to list and notify listeners */
> > > +	aux_node = kzalloc(sizeof(*aux_node), GFP_KERNEL);
> > > +	if (!aux_node)
> > > +		return -ENOMEM;
> > > +	aux_node->aux = aux;
> > > +	klist_add_tail(&aux_node->list, &drm_dp_aux_list);
> > > +	blocking_notifier_call_chain(&aux_notifier, DRM_DP_ADD_AUX, aux);
> > > +
> > >  	return i2c_add_adapter(&aux->ddc);
> > >  }
> > >  EXPORT_SYMBOL(drm_dp_aux_register);
> > > @@ -742,6 +799,20 @@ EXPORT_SYMBOL(drm_dp_aux_register);
> > >   */
> > >  void drm_dp_aux_unregister(struct drm_dp_aux *aux)
> > >  {
> > > +	struct klist_iter i;
> > > +	struct klist_node *n;
> > > +
> > > +	klist_iter_init(&drm_dp_aux_list, &i);
> > > +	while ((n = klist_next(&i))) {
> > > +		struct drm_dp_aux_node *aux_node =
> > > +			container_of(n, struct drm_dp_aux_node, list);
> > > +		if (aux_node->aux == aux) {
> > > +			klist_del(n);
> > > +			kfree(aux_node);
> > > +			break;
> > > +		}
> > > +	}
> > > +	blocking_notifier_call_chain(&aux_notifier, DRM_DP_DEL_AUX, aux);
> > >  	i2c_del_adapter(&aux->ddc);
> > >  }
> > >  EXPORT_SYMBOL(drm_dp_aux_unregister);
> > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > > index 8c52d0ef1..023620c 100644
> > > --- a/include/drm/drm_dp_helper.h
> > > +++ b/include/drm/drm_dp_helper.h
> > > @@ -763,7 +763,13 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
> > >  int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
> > >  int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
> > >  
> > > +#define DRM_DP_ADD_AUX 0x01
> > > +#define DRM_DP_DEL_AUX 0x02
> > > +
> > >  int drm_dp_aux_register(struct drm_dp_aux *aux);
> > >  void drm_dp_aux_unregister(struct drm_dp_aux *aux);
> > > +int drm_dp_aux_register_notifier(struct notifier_block *nb);
> > > +int drm_dp_aux_unregister_notifier(struct notifier_block *nb);
> > > +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *));
> > >  
> > >  #endif /* _DRM_DP_HELPER_H_ */
> > > -- 
> > > 2.4.0
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

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

Re: [PATCH 0/3] RFC: Add a drm_aux-dev module.

[Rafael Antognolli]

On Tue, Sep 15, 2015 at 07:46:55PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 15, 2015 at 07:41:06PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 15, 2015 at 09:34:15AM -0700, Rafael Antognolli wrote:
> > > On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote:
> > > > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote:
> > > > > This is a tentative implementation of a module that allows reading/writing
> > > > > arbitrary dpcd registers, following the suggestion from Daniel Vetter. It
> > > > > assumes the drm aux helpers were used by the driver.
> > > > > 
> > > > > I tried to follow the i2c-dev implementation as close as possible, but the only
> > > > > operations that are provided on the dev node are two different ioctl's, one for
> > > > > reading a register and another one for writing it.
> > > > 
> > > > Why would you use ioctls instead of normal read/write syscalls?
> > > > 
> > > 
> > > For writing, that should work fine, I can easily change that if you
> > > prefer.
> > > 
> > > For reading, one has to first tell which register address is going to be
> > > read.
> > 
> > seek()
> 
> Sorry typo. Make that lseek(). 
> 
Oh, nice, I'll update the patch with this and remove the notifier stuff,
thanks!

> > 
> > > So it would require to first write the address on the file, to
> > > then read. It was suggested that an ioctl to be used, and I understood
> > > it was to solve this, but maybe I'm wrong.
> > > 
> > > I don't have any particular preference for the API, could easily change
> > > this code to use just read/writes.
> > > 
> > > Thanks,
> > > Rafael
> > > 
> > > > > 
> > > > > I have at least 2 open questions:
> > > > > 
> > > > >  * Right now the AUX channels are stored in a global list inside the
> > > > >    drm_dp_helper implementation, and I assume that's not ideal. A different
> > > > >    approach would be to iterate over the list of connectors, instead of the
> > > > >    list of AUX channels, but that would require the struct drm_connector or
> > > > >    similar to know about the respective aux helper. It could be added as a
> > > > >    function to register that, or as a new method on the drm_connector_funcs to
> > > > >    retrieve the aux channel helper.
> > > > > 
> > > > >  * From the created sysfs drm_aux-dev device it's possible to know what is the
> > > > >    respective connector, but not the other way around. Is this good enough?
> > > > > 
> > > > > Please provide any feedback you have and tell me if this is the idea you had
> > > > > initially for this kind of implementation. Or, if it's nothing like this, let
> > > > > me know what else you had in mind.
> > > > > 
> > > > > If I'm going in the right direction, I'll refine the patch to provide full
> > > > > documentation and tests if needed.
> > > > > 
> > > > > Rafael Antognolli (3):
> > > > >   drm/dp: Keep a list of drm_dp_aux helper.
> > > > >   drm/dp: Store the drm_connector device pointer on the helper.
> > > > >   drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
> > > > > 
> > > > >  Documentation/ioctl/ioctl-number.txt |   1 +
> > > > >  drivers/gpu/drm/Kconfig              |   4 +
> > > > >  drivers/gpu/drm/Makefile             |   1 +
> > > > >  drivers/gpu/drm/drm_aux-dev.c        | 390 +++++++++++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/drm_dp_helper.c      |  71 +++++++
> > > > >  drivers/gpu/drm/i915/intel_dp.c      |   1 +
> > > > >  include/drm/drm_dp_helper.h          |   7 +
> > > > >  include/uapi/linux/Kbuild            |   1 +
> > > > >  include/uapi/linux/drm_aux-dev.h     |  45 ++++
> > > > >  9 files changed, 521 insertions(+)
> > > > >  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> > > > >  create mode 100644 include/uapi/linux/drm_aux-dev.h
> > > > > 
> > > > > -- 
> > > > > 2.4.0
> > > > > 
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel OTC
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/dp: Keep a list of drm_dp_aux helper.

[Daniel Vetter]

On Tue, Sep 15, 2015 at 07:57:19PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 15, 2015 at 09:27:27AM -0700, Rafael Antognolli wrote:
> > On Tue, Sep 15, 2015 at 10:46:43AM +0300, Ville Syrjälä wrote:
> > > On Mon, Sep 14, 2015 at 04:12:30PM -0700, Rafael Antognolli wrote:
> > > > This list will be used to get the aux channels registered through the
> > > > helpers. Two functions are provided to register/unregister notifier
> > > > listeners on the list, and another functiont to iterate over the list of
> > > > aux channels.
> > > > 
> > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_dp_helper.c | 71 +++++++++++++++++++++++++++++++++++++++++
> > > >  include/drm/drm_dp_helper.h     |  6 ++++
> > > >  2 files changed, 77 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > > index 291734e..01a1489 100644
> > > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > > @@ -710,6 +710,54 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
> > > >  	.master_xfer = drm_dp_i2c_xfer,
> > > >  };
> > > >  
> > > > +struct drm_dp_aux_node {
> > > > +	struct klist_node list;
> > > > +	struct drm_dp_aux *aux;
> > > > +};
> > > > +
> > > > +static DEFINE_KLIST(drm_dp_aux_list, NULL, NULL);
> > > > +
> > > > +static BLOCKING_NOTIFIER_HEAD(aux_notifier);
> > > > +
> > > > +int drm_dp_aux_register_notifier(struct notifier_block *nb)
> > > > +{
> > > > +	return blocking_notifier_chain_register(&aux_notifier, nb);
> > > > +}
> > > > +EXPORT_SYMBOL(drm_dp_aux_register_notifier);
> > > 
> > > Why is this notifier stuff needed? Why not just register/unregister the
> > > aux-dev directly?
> > > 
> > 
> > I am not sure it's needed, I was just looking for the best way of
> > informing aux-dev that a new aux channel was added.
> > 
> > By register/unregister the aux-dev directly, do you mean making this
> > drm_dp_helper aware of the aux dev, when it's registered, and directly
> > calling some callback to inform that new aux channels were added?
> 
> That was my thought, yes. It would mean the auxdev module can't be
> unloaded like i2c-dev, but I'm not sure that's a use case worth
> worrying about.

Yeah imo notifiers are evil because of locking inversion issues that
usually grop up, and having a use-case to unload dp_aux-dev seems
unlikely. Maybe we can do a Kconfig knob or something like that if some
people want to make the dev nodes optional.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] RFC: Add a drm_aux-dev module.

[Daniel Vetter]

On Tue, Sep 15, 2015 at 10:03:09AM -0700, Rafael Antognolli wrote:
> On Tue, Sep 15, 2015 at 07:46:55PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 15, 2015 at 07:41:06PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 15, 2015 at 09:34:15AM -0700, Rafael Antognolli wrote:
> > > > On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote:
> > > > > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote:
> > > > > > This is a tentative implementation of a module that allows reading/writing
> > > > > > arbitrary dpcd registers, following the suggestion from Daniel Vetter. It
> > > > > > assumes the drm aux helpers were used by the driver.
> > > > > > 
> > > > > > I tried to follow the i2c-dev implementation as close as possible, but the only
> > > > > > operations that are provided on the dev node are two different ioctl's, one for
> > > > > > reading a register and another one for writing it.
> > > > > 
> > > > > Why would you use ioctls instead of normal read/write syscalls?
> > > > > 
> > > > 
> > > > For writing, that should work fine, I can easily change that if you
> > > > prefer.
> > > > 
> > > > For reading, one has to first tell which register address is going to be
> > > > read.
> > > 
> > > seek()
> > 
> > Sorry typo. Make that lseek(). 
> > 
> Oh, nice, I'll update the patch with this and remove the notifier stuff,
> thanks!

Well there's also other modes like i2c over aux, and that would be easier
to support with an ioctl in a uniform way. So not sure how much value
there is in reusing read/write for i2c ...

But I really don't have a strong opinion about this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] RFC: Add a drm_aux-dev module.

[Ville Syrjälä]

On Wed, Sep 16, 2015 at 01:09:54PM -0700, Daniel Vetter wrote:
> On Tue, Sep 15, 2015 at 10:03:09AM -0700, Rafael Antognolli wrote:
> > On Tue, Sep 15, 2015 at 07:46:55PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 15, 2015 at 07:41:06PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Sep 15, 2015 at 09:34:15AM -0700, Rafael Antognolli wrote:
> > > > > On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote:
> > > > > > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote:
> > > > > > > This is a tentative implementation of a module that allows reading/writing
> > > > > > > arbitrary dpcd registers, following the suggestion from Daniel Vetter. It
> > > > > > > assumes the drm aux helpers were used by the driver.
> > > > > > > 
> > > > > > > I tried to follow the i2c-dev implementation as close as possible, but the only
> > > > > > > operations that are provided on the dev node are two different ioctl's, one for
> > > > > > > reading a register and another one for writing it.
> > > > > > 
> > > > > > Why would you use ioctls instead of normal read/write syscalls?
> > > > > > 
> > > > > 
> > > > > For writing, that should work fine, I can easily change that if you
> > > > > prefer.
> > > > > 
> > > > > For reading, one has to first tell which register address is going to be
> > > > > read.
> > > > 
> > > > seek()
> > > 
> > > Sorry typo. Make that lseek(). 
> > > 
> > Oh, nice, I'll update the patch with this and remove the notifier stuff,
> > thanks!
> 
> Well there's also other modes like i2c over aux, and that would be easier
> to support with an ioctl in a uniform way. So not sure how much value
> there is in reusing read/write for i2c ...

We already have i2c-dev for i2c. So not sure what you're after here.

i2c is a bit of a mess on the protocol level. Lots of devices do
interesting things with it, so it can't really make do with just
read/write/lseek. Apart from i2c-over-aux, the rest is all about
DPCD access, and for that read/write/lseek is all you ever need.

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

Re: [PATCH 0/3] RFC: Add a drm_aux-dev module.

[Rafael Antognolli]

On Wed, Sep 16, 2015 at 11:47:21PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 16, 2015 at 01:09:54PM -0700, Daniel Vetter wrote:
> > On Tue, Sep 15, 2015 at 10:03:09AM -0700, Rafael Antognolli wrote:
> > > On Tue, Sep 15, 2015 at 07:46:55PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Sep 15, 2015 at 07:41:06PM +0300, Ville Syrjälä wrote:
> > > > > On Tue, Sep 15, 2015 at 09:34:15AM -0700, Rafael Antognolli wrote:
> > > > > > On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote:
> > > > > > > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote:
> > > > > > > > This is a tentative implementation of a module that allows reading/writing
> > > > > > > > arbitrary dpcd registers, following the suggestion from Daniel Vetter. It
> > > > > > > > assumes the drm aux helpers were used by the driver.
> > > > > > > > 
> > > > > > > > I tried to follow the i2c-dev implementation as close as possible, but the only
> > > > > > > > operations that are provided on the dev node are two different ioctl's, one for
> > > > > > > > reading a register and another one for writing it.
> > > > > > > 
> > > > > > > Why would you use ioctls instead of normal read/write syscalls?
> > > > > > > 
> > > > > > 
> > > > > > For writing, that should work fine, I can easily change that if you
> > > > > > prefer.
> > > > > > 
> > > > > > For reading, one has to first tell which register address is going to be
> > > > > > read.
> > > > > 
> > > > > seek()
> > > > 
> > > > Sorry typo. Make that lseek(). 
> > > > 
> > > Oh, nice, I'll update the patch with this and remove the notifier stuff,
> > > thanks!
> > 
> > Well there's also other modes like i2c over aux, and that would be easier
> > to support with an ioctl in a uniform way. So not sure how much value
> > there is in reusing read/write for i2c ...
> 
> We already have i2c-dev for i2c. So not sure what you're after here.
> 
> i2c is a bit of a mess on the protocol level. Lots of devices do
> interesting things with it, so it can't really make do with just
> read/write/lseek. Apart from i2c-over-aux, the rest is all about
> DPCD access, and for that read/write/lseek is all you ever need.

I just noticed that I forgot to cc you guys, but yesterday I sent an
updated version of the patch set to this list:

http://lists.freedesktop.org/archives/dri-devel/2015-September/090366.html

I also don't have a strong opinion about ioctl vs read/write/lseek, but
at least my second implementation did get a little cleaner.

Thanks,
Rafael
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] RFC: Add a drm_aux-dev module.

[Jani Nikula]

On Wed, 16 Sep 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Sep 16, 2015 at 01:09:54PM -0700, Daniel Vetter wrote:
>> On Tue, Sep 15, 2015 at 10:03:09AM -0700, Rafael Antognolli wrote:
>> > On Tue, Sep 15, 2015 at 07:46:55PM +0300, Ville Syrjälä wrote:
>> > > On Tue, Sep 15, 2015 at 07:41:06PM +0300, Ville Syrjälä wrote:
>> > > > On Tue, Sep 15, 2015 at 09:34:15AM -0700, Rafael Antognolli wrote:
>> > > > > On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote:
>> > > > > > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote:
>> > > > > > > This is a tentative implementation of a module that allows reading/writing
>> > > > > > > arbitrary dpcd registers, following the suggestion from Daniel Vetter. It
>> > > > > > > assumes the drm aux helpers were used by the driver.
>> > > > > > > 
>> > > > > > > I tried to follow the i2c-dev implementation as close as possible, but the only
>> > > > > > > operations that are provided on the dev node are two different ioctl's, one for
>> > > > > > > reading a register and another one for writing it.
>> > > > > > 
>> > > > > > Why would you use ioctls instead of normal read/write syscalls?
>> > > > > > 
>> > > > > 
>> > > > > For writing, that should work fine, I can easily change that if you
>> > > > > prefer.
>> > > > > 
>> > > > > For reading, one has to first tell which register address is going to be
>> > > > > read.
>> > > > 
>> > > > seek()
>> > > 
>> > > Sorry typo. Make that lseek(). 
>> > > 
>> > Oh, nice, I'll update the patch with this and remove the notifier stuff,
>> > thanks!
>> 
>> Well there's also other modes like i2c over aux, and that would be easier
>> to support with an ioctl in a uniform way. So not sure how much value
>> there is in reusing read/write for i2c ...
>
> We already have i2c-dev for i2c. So not sure what you're after here.

Yeah, definitely don't reinvent the wheel for this.

BR,
Jani.

>
> i2c is a bit of a mess on the protocol level. Lots of devices do
> interesting things with it, so it can't really make do with just
> read/write/lseek. Apart from i2c-over-aux, the rest is all about
> DPCD access, and for that read/write/lseek is all you ever need.
>
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] RFC: Add a drm_aux-dev module.

[Daniel Vetter]

On Thu, Sep 17, 2015 at 12:01 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Wed, 16 Sep 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Wed, Sep 16, 2015 at 01:09:54PM -0700, Daniel Vetter wrote:
>>> On Tue, Sep 15, 2015 at 10:03:09AM -0700, Rafael Antognolli wrote:
>>> > On Tue, Sep 15, 2015 at 07:46:55PM +0300, Ville Syrjälä wrote:
>>> > > On Tue, Sep 15, 2015 at 07:41:06PM +0300, Ville Syrjälä wrote:
>>> > > > On Tue, Sep 15, 2015 at 09:34:15AM -0700, Rafael Antognolli wrote:
>>> > > > > On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote:
>>> > > > > > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote:
>>> > > > > > > This is a tentative implementation of a module that allows reading/writing
>>> > > > > > > arbitrary dpcd registers, following the suggestion from Daniel Vetter. It
>>> > > > > > > assumes the drm aux helpers were used by the driver.
>>> > > > > > >
>>> > > > > > > I tried to follow the i2c-dev implementation as close as possible, but the only
>>> > > > > > > operations that are provided on the dev node are two different ioctl's, one for
>>> > > > > > > reading a register and another one for writing it.
>>> > > > > >
>>> > > > > > Why would you use ioctls instead of normal read/write syscalls?
>>> > > > > >
>>> > > > >
>>> > > > > For writing, that should work fine, I can easily change that if you
>>> > > > > prefer.
>>> > > > >
>>> > > > > For reading, one has to first tell which register address is going to be
>>> > > > > read.
>>> > > >
>>> > > > seek()
>>> > >
>>> > > Sorry typo. Make that lseek().
>>> > >
>>> > Oh, nice, I'll update the patch with this and remove the notifier stuff,
>>> > thanks!
>>>
>>> Well there's also other modes like i2c over aux, and that would be easier
>>> to support with an ioctl in a uniform way. So not sure how much value
>>> there is in reusing read/write for i2c ...
>>
>> We already have i2c-dev for i2c. So not sure what you're after here.
>
> Yeah, definitely don't reinvent the wheel for this.

Of course I didn't mean to reinvent the i2c-dev wheel, but just expose
the underlying low-level dp aux ops to do i2c transaction - that might
be useful for hacking around to figure out some of the recent i2c
changes we've done. Otoh we could add such a raw mode later on as an
ioctl, for plain dpcd reads/writes read/write/lseek seems indeed a
good fit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel