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

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

[Rafael Antognolli]

Second attempt at implementing a module that allows reading/writing arbitrary
dpcd registers. Changes to this version:
	- lseek is used to select the register to read/write;
	- read/write are used instead of ioctl;
	- no blocking_notifier is used, just a direct callback.

One thing to notice is that I am not updating the file offset during read or
write, which is kind of breaking the filesystem abstraction. But i2c-dev
doesn't do it either, so I assumed it's fine.

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.

 drivers/gpu/drm/Kconfig         |   4 +
 drivers/gpu/drm/Makefile        |   1 +
 drivers/gpu/drm/drm_aux-dev.c   | 326 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_dp_helper.c |  73 +++++++++
 drivers/gpu/drm/i915/intel_dp.c |   1 +
 include/drm/drm_dp_helper.h     |   6 +
 6 files changed, 411 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_aux-dev.c

-- 
2.4.3

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

[PATCH RFC v2 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. One function is provided to set a callback for added/removed
aux channels, and another function to iterate over the list of aux
channels.

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

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 9535c5b..41bdd93 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -746,6 +746,56 @@ 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 int (*aux_dev_cb)(int action, struct drm_dp_aux *aux) = NULL;
+
+void drm_dp_aux_set_aux_dev(int (*fn)(int, struct drm_dp_aux *))
+{
+	aux_dev_cb = fn;
+}
+EXPORT_SYMBOL(drm_dp_aux_set_aux_dev);
+
+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);
+
+static int drm_dp_aux_dev_inform(int action, struct drm_dp_aux *aux)
+{
+	if (aux_dev_cb) {
+		return aux_dev_cb(action, aux);
+	}
+	return 0;
+}
+
 /**
  * drm_dp_aux_register() - initialise and register aux channel
  * @aux: DisplayPort AUX channel
@@ -754,6 +804,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;
@@ -768,6 +819,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);
+	drm_dp_aux_dev_inform(DRM_DP_ADD_AUX, aux);
+
 	return i2c_add_adapter(&aux->ddc);
 }
 EXPORT_SYMBOL(drm_dp_aux_register);
@@ -778,6 +837,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;
+		}
+	}
+	drm_dp_aux_dev_inform(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 9ec4716..f92de26 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -763,7 +763,12 @@ 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);
+void drm_dp_aux_set_aux_dev(int (*fn)(int, struct drm_dp_aux *));
+int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *));
 
 #endif /* _DRM_DP_HELPER_H_ */
-- 
2.4.3

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

[PATCH RFC v2 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 a687250..23addc3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1079,6 +1079,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 f92de26..d762535 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.3

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

[PATCH RFC v2 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: read and write. The
address of the register to be read or written is given using lseek.
Reading or writing does not update the offset of the file.

Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
---
 drivers/gpu/drm/Kconfig       |   4 +
 drivers/gpu/drm/Makefile      |   1 +
 drivers/gpu/drm/drm_aux-dev.c | 326 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 331 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_aux-dev.c

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..fcc334a
--- /dev/null
+++ b/drivers/gpu/drm/drm_aux-dev.c
@@ -0,0 +1,326 @@
+/*
+ * 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 <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 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 ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
+			   loff_t *offset)
+{
+	char *localbuf;
+	ssize_t res;
+	struct drm_aux_dev *aux_dev = file->private_data;
+
+	localbuf = memdup_user(buf, count);
+	if (IS_ERR(localbuf))
+		return PTR_ERR(buf);
+
+	res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, count);
+	if (res < 0)
+		goto finish;
+
+	if (copy_to_user(buf, localbuf, res))
+		res = -EFAULT;
+
+finish:
+	kfree(localbuf);
+	return res;
+}
+
+static ssize_t auxdev_write(struct file *file, const char __user *buf,
+			    size_t count, loff_t *offset)
+{
+	char *localbuf;
+	ssize_t res;
+	struct drm_aux_dev *aux_dev = file->private_data;
+
+	localbuf = memdup_user(buf, count);
+	if (IS_ERR(localbuf))
+		return PTR_ERR(buf);
+
+	res = drm_dp_dpcd_write(aux_dev->aux, *offset, localbuf, count);
+	kfree(localbuf);
+
+	return res;
+}
+
+static const struct file_operations auxdev_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= generic_file_llseek,
+	.read		= auxdev_read,
+	.write		= auxdev_write,
+	.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_inform_cb(int action, struct drm_dp_aux *aux)
+{
+	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 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 */
+	drm_dp_aux_set_aux_dev(&auxdev_inform_cb);
+
+	/* Bind to already existing DP aux */
+	res = drm_dp_aux_for_each(&drm_dev_major, auxdev_attach_aux);
+
+	return 0;
+
+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_set_aux_dev(NULL);
+	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);
-- 
2.4.3

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

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

[Daniel Vetter]

On Tue, Sep 15, 2015 at 04:55:02PM -0700, Rafael Antognolli wrote:
> This list will be used to get the aux channels registered through the
> helpers. One function is provided to set a callback for added/removed
> aux channels, and another function to iterate over the list of aux
> channels.
> 
> Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 73 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     |  5 +++
>  2 files changed, 78 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 9535c5b..41bdd93 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -746,6 +746,56 @@ 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 int (*aux_dev_cb)(int action, struct drm_dp_aux *aux) = NULL;
> +
> +void drm_dp_aux_set_aux_dev(int (*fn)(int, struct drm_dp_aux *))
> +{
> +	aux_dev_cb = fn;
> +}
> +EXPORT_SYMBOL(drm_dp_aux_set_aux_dev);
> +
> +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);
> +
> +static int drm_dp_aux_dev_inform(int action, struct drm_dp_aux *aux)
> +{
> +	if (aux_dev_cb) {
> +		return aux_dev_cb(action, aux);
> +	}
> +	return 0;
> +}
> +
>  /**
>   * drm_dp_aux_register() - initialise and register aux channel
>   * @aux: DisplayPort AUX channel
> @@ -754,6 +804,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;
> @@ -768,6 +819,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);
> +	drm_dp_aux_dev_inform(DRM_DP_ADD_AUX, aux);

If you go with the direct function call approach there's no need for all
this indirection, just do something like

	drm_dp_aux_register_devnode(aux);

And drop all the list book-keeping (since there will never be a time when
the aux stuff isn't loaded yet). Essentially you're reinventing notifier
chains here (but without the required locking).

With that this patch just adds 2 lines, probably best to squash it in with
the last one.
-Daniel

> +
>  	return i2c_add_adapter(&aux->ddc);
>  }
>  EXPORT_SYMBOL(drm_dp_aux_register);
> @@ -778,6 +837,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;
> +		}
> +	}
> +	drm_dp_aux_dev_inform(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 9ec4716..f92de26 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -763,7 +763,12 @@ 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);
> +void drm_dp_aux_set_aux_dev(int (*fn)(int, struct drm_dp_aux *));
> +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *));
>  
>  #endif /* _DRM_DP_HELPER_H_ */
> -- 
> 2.4.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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 RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Daniel Vetter]

On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> 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: read and write. The
> address of the register to be read or written is given using lseek.
> Reading or writing does not update the offset of the file.

I think not updating the read position is very surprising. Would it be
hard to fix that?
-Daniel

> 
> Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> ---
>  drivers/gpu/drm/Kconfig       |   4 +
>  drivers/gpu/drm/Makefile      |   1 +
>  drivers/gpu/drm/drm_aux-dev.c | 326 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 331 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> 
> 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..fcc334a
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_aux-dev.c
> @@ -0,0 +1,326 @@
> +/*
> + * 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 <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 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 ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> +			   loff_t *offset)
> +{
> +	char *localbuf;
> +	ssize_t res;
> +	struct drm_aux_dev *aux_dev = file->private_data;
> +
> +	localbuf = memdup_user(buf, count);
> +	if (IS_ERR(localbuf))
> +		return PTR_ERR(buf);
> +
> +	res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, count);
> +	if (res < 0)
> +		goto finish;
> +
> +	if (copy_to_user(buf, localbuf, res))
> +		res = -EFAULT;
> +
> +finish:
> +	kfree(localbuf);
> +	return res;
> +}
> +
> +static ssize_t auxdev_write(struct file *file, const char __user *buf,
> +			    size_t count, loff_t *offset)
> +{
> +	char *localbuf;
> +	ssize_t res;
> +	struct drm_aux_dev *aux_dev = file->private_data;
> +
> +	localbuf = memdup_user(buf, count);
> +	if (IS_ERR(localbuf))
> +		return PTR_ERR(buf);
> +
> +	res = drm_dp_dpcd_write(aux_dev->aux, *offset, localbuf, count);
> +	kfree(localbuf);
> +
> +	return res;
> +}
> +
> +static const struct file_operations auxdev_fops = {
> +	.owner		= THIS_MODULE,
> +	.llseek		= generic_file_llseek,
> +	.read		= auxdev_read,
> +	.write		= auxdev_write,
> +	.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_inform_cb(int action, struct drm_dp_aux *aux)
> +{
> +	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 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 */
> +	drm_dp_aux_set_aux_dev(&auxdev_inform_cb);
> +
> +	/* Bind to already existing DP aux */
> +	res = drm_dp_aux_for_each(&drm_dev_major, auxdev_attach_aux);
> +
> +	return 0;
> +
> +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_set_aux_dev(NULL);
> +	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);
> -- 
> 2.4.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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 RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Daniel Vetter]

On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> 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: read and write. The
> address of the register to be read or written is given using lseek.
> Reading or writing does not update the offset of the file.
> 
> Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> ---
>  drivers/gpu/drm/Kconfig       |   4 +
>  drivers/gpu/drm/Makefile      |   1 +
>  drivers/gpu/drm/drm_aux-dev.c | 326 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 331 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> 
> 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"

Imo just make this a boolean, will simplify things a lot. Also if you
implement the drm_dp_aux_register_devnode you need to add static inline
stubs for register/unregister functions so that it all works out if this
option is disabled. See for example how the recent changes to the drm fb
helpers are done.

> +	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

I'd call the file drm_dp_aux_dev.o, drm_dp_aux for consistency in prefixes
and mixing - and _ in filenames looks strange to me ;-)
-Daniel

>  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..fcc334a
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_aux-dev.c
> @@ -0,0 +1,326 @@
> +/*
> + * 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 <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 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 ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> +			   loff_t *offset)
> +{
> +	char *localbuf;
> +	ssize_t res;
> +	struct drm_aux_dev *aux_dev = file->private_data;
> +
> +	localbuf = memdup_user(buf, count);
> +	if (IS_ERR(localbuf))
> +		return PTR_ERR(buf);
> +
> +	res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, count);
> +	if (res < 0)
> +		goto finish;
> +
> +	if (copy_to_user(buf, localbuf, res))
> +		res = -EFAULT;
> +
> +finish:
> +	kfree(localbuf);
> +	return res;
> +}
> +
> +static ssize_t auxdev_write(struct file *file, const char __user *buf,
> +			    size_t count, loff_t *offset)
> +{
> +	char *localbuf;
> +	ssize_t res;
> +	struct drm_aux_dev *aux_dev = file->private_data;
> +
> +	localbuf = memdup_user(buf, count);
> +	if (IS_ERR(localbuf))
> +		return PTR_ERR(buf);
> +
> +	res = drm_dp_dpcd_write(aux_dev->aux, *offset, localbuf, count);
> +	kfree(localbuf);
> +
> +	return res;
> +}
> +
> +static const struct file_operations auxdev_fops = {
> +	.owner		= THIS_MODULE,
> +	.llseek		= generic_file_llseek,
> +	.read		= auxdev_read,
> +	.write		= auxdev_write,
> +	.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_inform_cb(int action, struct drm_dp_aux *aux)
> +{
> +	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 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 */
> +	drm_dp_aux_set_aux_dev(&auxdev_inform_cb);
> +
> +	/* Bind to already existing DP aux */
> +	res = drm_dp_aux_for_each(&drm_dev_major, auxdev_attach_aux);
> +
> +	return 0;
> +
> +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_set_aux_dev(NULL);
> +	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);
> -- 
> 2.4.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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 RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Ville Syrjälä]

On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> 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: read and write. The
> address of the register to be read or written is given using lseek.
> Reading or writing does not update the offset of the file.
> 
> Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> ---
>  drivers/gpu/drm/Kconfig       |   4 +
>  drivers/gpu/drm/Makefile      |   1 +
>  drivers/gpu/drm/drm_aux-dev.c | 326 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 331 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> 
> 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..fcc334a
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_aux-dev.c
> @@ -0,0 +1,326 @@
> +/*
> + * 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 <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 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 ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> +			   loff_t *offset)
> +{
> +	char *localbuf;
> +	ssize_t res;
> +	struct drm_aux_dev *aux_dev = file->private_data;
> +
> +	localbuf = memdup_user(buf, count);
> +	if (IS_ERR(localbuf))
> +		return PTR_ERR(buf);
> +
> +	res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, count);
> +	if (res < 0)
> +		goto finish;

read/write will need to do the access in 16 byte chunks. That's the max
amount of data that can be transferred with a single AUX operation. I
suppose that would also avoid the need for allocating large buffers
since you can just have a 16 byte buffer on the stack and keep reusing
it while you iterate through the total count. I'm not sure what are
the expected semantics of read/write if you do it in chunks and hit
a copy_{to,from}_user() failure somewhere down the line. But I suppose
access_ok() upfront for the whole user buffer could avoid that issue.

Also the size of the DPCD address space is 1<<20 bytes, so you'll need to
limit the access to stay within that. 'size = lseek(fd, 0, SEEK_END)'
can be used to get the file size, so you should allow the offset to
reach 1<<20, but not beyond. read/write should process as much much of
the data that they can given the current file offset, and return how
many bytes made it in/out (can also be 0 if offset was already at 1<<20).

> +
> +	if (copy_to_user(buf, localbuf, res))
> +		res = -EFAULT;
> +
> +finish:
> +	kfree(localbuf);
> +	return res;
> +}
> +
> +static ssize_t auxdev_write(struct file *file, const char __user *buf,
> +			    size_t count, loff_t *offset)
> +{
> +	char *localbuf;
> +	ssize_t res;
> +	struct drm_aux_dev *aux_dev = file->private_data;
> +
> +	localbuf = memdup_user(buf, count);
> +	if (IS_ERR(localbuf))
> +		return PTR_ERR(buf);
> +
> +	res = drm_dp_dpcd_write(aux_dev->aux, *offset, localbuf, count);
> +	kfree(localbuf);
> +
> +	return res;
> +}
> +
> +static const struct file_operations auxdev_fops = {
> +	.owner		= THIS_MODULE,
> +	.llseek		= generic_file_llseek,
> +	.read		= auxdev_read,
> +	.write		= auxdev_write,
> +	.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_inform_cb(int action, struct drm_dp_aux *aux)
> +{
> +	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 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 */
> +	drm_dp_aux_set_aux_dev(&auxdev_inform_cb);
> +
> +	/* Bind to already existing DP aux */
> +	res = drm_dp_aux_for_each(&drm_dev_major, auxdev_attach_aux);
> +
> +	return 0;
> +
> +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_set_aux_dev(NULL);
> +	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);
> -- 
> 2.4.3
> 
> _______________________________________________
> 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 RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Daniel Vetter]

On Tue, Sep 22, 2015 at 03:00:54PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> > 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: read and write. The
> > address of the register to be read or written is given using lseek.
> > Reading or writing does not update the offset of the file.
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > ---
> >  drivers/gpu/drm/Kconfig       |   4 +
> >  drivers/gpu/drm/Makefile      |   1 +
> >  drivers/gpu/drm/drm_aux-dev.c | 326 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 331 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> > 
> > 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..fcc334a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_aux-dev.c
> > @@ -0,0 +1,326 @@
> > +/*
> > + * 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 <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 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 ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> > +			   loff_t *offset)
> > +{
> > +	char *localbuf;
> > +	ssize_t res;
> > +	struct drm_aux_dev *aux_dev = file->private_data;
> > +
> > +	localbuf = memdup_user(buf, count);
> > +	if (IS_ERR(localbuf))
> > +		return PTR_ERR(buf);
> > +
> > +	res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, count);
> > +	if (res < 0)
> > +		goto finish;
> 
> read/write will need to do the access in 16 byte chunks. That's the max
> amount of data that can be transferred with a single AUX operation. I
> suppose that would also avoid the need for allocating large buffers
> since you can just have a 16 byte buffer on the stack and keep reusing
> it while you iterate through the total count. I'm not sure what are
> the expected semantics of read/write if you do it in chunks and hit
> a copy_{to,from}_user() failure somewhere down the line. But I suppose
> access_ok() upfront for the whole user buffer could avoid that issue.

Iirc short reads are ok in all cases, so we could even punt the restarting
to userspace by just doing short reads/writes (like sockets do).

> Also the size of the DPCD address space is 1<<20 bytes, so you'll need to
> limit the access to stay within that. 'size = lseek(fd, 0, SEEK_END)'
> can be used to get the file size, so you should allow the offset to
> reach 1<<20, but not beyond. read/write should process as much much of
> the data that they can given the current file offset, and return how
> many bytes made it in/out (can also be 0 if offset was already at 1<<20).

Hm, sounds like a few testcases for cornercases would be neat. Best would
be to do that in igt and iterate over all available dp_aux dev nodes (igt
is now generic, so makes sense to put that stuff there). Something like
kms_dp_aux_dev would be a nice testcase name.
-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 RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Ville Syrjälä]

On Tue, Sep 22, 2015 at 02:17:51PM +0200, Daniel Vetter wrote:
> On Tue, Sep 22, 2015 at 03:00:54PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> > > 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: read and write. The
> > > address of the register to be read or written is given using lseek.
> > > Reading or writing does not update the offset of the file.
> > > 
> > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > > ---
> > >  drivers/gpu/drm/Kconfig       |   4 +
> > >  drivers/gpu/drm/Makefile      |   1 +
> > >  drivers/gpu/drm/drm_aux-dev.c | 326 ++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 331 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> > > 
> > > 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..fcc334a
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_aux-dev.c
> > > @@ -0,0 +1,326 @@
> > > +/*
> > > + * 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 <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 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 ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> > > +			   loff_t *offset)
> > > +{
> > > +	char *localbuf;
> > > +	ssize_t res;
> > > +	struct drm_aux_dev *aux_dev = file->private_data;
> > > +
> > > +	localbuf = memdup_user(buf, count);
> > > +	if (IS_ERR(localbuf))
> > > +		return PTR_ERR(buf);
> > > +
> > > +	res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, count);
> > > +	if (res < 0)
> > > +		goto finish;
> > 
> > read/write will need to do the access in 16 byte chunks. That's the max
> > amount of data that can be transferred with a single AUX operation. I
> > suppose that would also avoid the need for allocating large buffers
> > since you can just have a 16 byte buffer on the stack and keep reusing
> > it while you iterate through the total count. I'm not sure what are
> > the expected semantics of read/write if you do it in chunks and hit
> > a copy_{to,from}_user() failure somewhere down the line. But I suppose
> > access_ok() upfront for the whole user buffer could avoid that issue.
> 
> Iirc short reads are ok in all cases, so we could even punt the restarting
> to userspace by just doing short reads/writes (like sockets do).

Yeah, short writes due to -EFAULT sound more dangerous than short reads.
But I'm not sure there's any point in allowing short reads either in
this case, so just returning the error upfront if access_ok() complains
seems like a sane option to me.

> 
> > Also the size of the DPCD address space is 1<<20 bytes, so you'll need to
> > limit the access to stay within that. 'size = lseek(fd, 0, SEEK_END)'
> > can be used to get the file size, so you should allow the offset to
> > reach 1<<20, but not beyond. read/write should process as much much of
> > the data that they can given the current file offset, and return how
> > many bytes made it in/out (can also be 0 if offset was already at 1<<20).
> 
> Hm, sounds like a few testcases for cornercases would be neat. Best would
> be to do that in igt and iterate over all available dp_aux dev nodes (igt
> is now generic, so makes sense to put that stuff there). Something like
> kms_dp_aux_dev would be a nice testcase name.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

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

[Daniel Vetter]

On Tue, Sep 22, 2015 at 03:35:13PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 22, 2015 at 02:17:51PM +0200, Daniel Vetter wrote:
> > On Tue, Sep 22, 2015 at 03:00:54PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> > > > 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: read and write. The
> > > > address of the register to be read or written is given using lseek.
> > > > Reading or writing does not update the offset of the file.
> > > > 
> > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/Kconfig       |   4 +
> > > >  drivers/gpu/drm/Makefile      |   1 +
> > > >  drivers/gpu/drm/drm_aux-dev.c | 326 ++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 331 insertions(+)
> > > >  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> > > > 
> > > > 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..fcc334a
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/drm_aux-dev.c
> > > > @@ -0,0 +1,326 @@
> > > > +/*
> > > > + * 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 <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 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 ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> > > > +			   loff_t *offset)
> > > > +{
> > > > +	char *localbuf;
> > > > +	ssize_t res;
> > > > +	struct drm_aux_dev *aux_dev = file->private_data;
> > > > +
> > > > +	localbuf = memdup_user(buf, count);
> > > > +	if (IS_ERR(localbuf))
> > > > +		return PTR_ERR(buf);
> > > > +
> > > > +	res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, count);
> > > > +	if (res < 0)
> > > > +		goto finish;
> > > 
> > > read/write will need to do the access in 16 byte chunks. That's the max
> > > amount of data that can be transferred with a single AUX operation. I
> > > suppose that would also avoid the need for allocating large buffers
> > > since you can just have a 16 byte buffer on the stack and keep reusing
> > > it while you iterate through the total count. I'm not sure what are
> > > the expected semantics of read/write if you do it in chunks and hit
> > > a copy_{to,from}_user() failure somewhere down the line. But I suppose
> > > access_ok() upfront for the whole user buffer could avoid that issue.
> > 
> > Iirc short reads are ok in all cases, so we could even punt the restarting
> > to userspace by just doing short reads/writes (like sockets do).
> 
> Yeah, short writes due to -EFAULT sound more dangerous than short reads.
> But I'm not sure there's any point in allowing short reads either in
> this case, so just returning the error upfront if access_ok() complains
> seems like a sane option to me.

access_ok _only_ does static checks (on x86 it only checks that it's a
userspace address). Which means any kind of real faults will only happen
later on in the actual copy_to/from_user. I'd say we can go meh if that
happens - it's guaranteed to be userspace doing something silly since we
don't need to hold any of the mm locks ;-)
-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 RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Ville Syrjälä]

On Tue, Sep 22, 2015 at 02:59:24PM +0200, Daniel Vetter wrote:
> On Tue, Sep 22, 2015 at 03:35:13PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 22, 2015 at 02:17:51PM +0200, Daniel Vetter wrote:
> > > On Tue, Sep 22, 2015 at 03:00:54PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> > > > > 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: read and write. The
> > > > > address of the register to be read or written is given using lseek.
> > > > > Reading or writing does not update the offset of the file.
> > > > > 
> > > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/Kconfig       |   4 +
> > > > >  drivers/gpu/drm/Makefile      |   1 +
> > > > >  drivers/gpu/drm/drm_aux-dev.c | 326 ++++++++++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 331 insertions(+)
> > > > >  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> > > > > 
> > > > > 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..fcc334a
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/drm_aux-dev.c
> > > > > @@ -0,0 +1,326 @@
> > > > > +/*
> > > > > + * 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 <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 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 ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> > > > > +			   loff_t *offset)
> > > > > +{
> > > > > +	char *localbuf;
> > > > > +	ssize_t res;
> > > > > +	struct drm_aux_dev *aux_dev = file->private_data;
> > > > > +
> > > > > +	localbuf = memdup_user(buf, count);
> > > > > +	if (IS_ERR(localbuf))
> > > > > +		return PTR_ERR(buf);
> > > > > +
> > > > > +	res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, count);
> > > > > +	if (res < 0)
> > > > > +		goto finish;
> > > > 
> > > > read/write will need to do the access in 16 byte chunks. That's the max
> > > > amount of data that can be transferred with a single AUX operation. I
> > > > suppose that would also avoid the need for allocating large buffers
> > > > since you can just have a 16 byte buffer on the stack and keep reusing
> > > > it while you iterate through the total count. I'm not sure what are
> > > > the expected semantics of read/write if you do it in chunks and hit
> > > > a copy_{to,from}_user() failure somewhere down the line. But I suppose
> > > > access_ok() upfront for the whole user buffer could avoid that issue.
> > > 
> > > Iirc short reads are ok in all cases, so we could even punt the restarting
> > > to userspace by just doing short reads/writes (like sockets do).
> > 
> > Yeah, short writes due to -EFAULT sound more dangerous than short reads.
> > But I'm not sure there's any point in allowing short reads either in
> > this case, so just returning the error upfront if access_ok() complains
> > seems like a sane option to me.
> 
> access_ok _only_ does static checks (on x86 it only checks that it's a
> userspace address). Which means any kind of real faults will only happen
> later on in the actual copy_to/from_user. I'd say we can go meh if that
> happens - it's guaranteed to be userspace doing something silly since we
> don't need to hold any of the mm locks ;-)

Hmm, true. So I guess on -EFAULT we should:

if (copy_{to,from}_user())
	return num_bytes_processed ? num_bytes_processed : -EFAULT;

Sound reasonable?

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

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

[Daniel Vetter]

On Tue, Sep 22, 2015 at 04:25:17PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 22, 2015 at 02:59:24PM +0200, Daniel Vetter wrote:
> > On Tue, Sep 22, 2015 at 03:35:13PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 22, 2015 at 02:17:51PM +0200, Daniel Vetter wrote:
> > > > Iirc short reads are ok in all cases, so we could even punt the restarting
> > > > to userspace by just doing short reads/writes (like sockets do).
> > > 
> > > Yeah, short writes due to -EFAULT sound more dangerous than short reads.
> > > But I'm not sure there's any point in allowing short reads either in
> > > this case, so just returning the error upfront if access_ok() complains
> > > seems like a sane option to me.
> > 
> > access_ok _only_ does static checks (on x86 it only checks that it's a
> > userspace address). Which means any kind of real faults will only happen
> > later on in the actual copy_to/from_user. I'd say we can go meh if that
> > happens - it's guaranteed to be userspace doing something silly since we
> > don't need to hold any of the mm locks ;-)
> 
> Hmm, true. So I guess on -EFAULT we should:
> 
> if (copy_{to,from}_user())
> 	return num_bytes_processed ? num_bytes_processed : -EFAULT;
> 
> Sound reasonable?

Yeah that's what I'd go with.
-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 RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

On Tue, Sep 22, 2015 at 10:59:51AM +0200, Daniel Vetter wrote:
> On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> > 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: read and write. The
> > address of the register to be read or written is given using lseek.
> > Reading or writing does not update the offset of the file.
> 
> I think not updating the read position is very surprising. Would it be
> hard to fix that?

No, not hard at all. But I assume then I should update the write
position too, right?

BTW, i2c-dev doesn't update either of them, but I'm not sure why.

> > 
> > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > ---
> >  drivers/gpu/drm/Kconfig       |   4 +
> >  drivers/gpu/drm/Makefile      |   1 +
> >  drivers/gpu/drm/drm_aux-dev.c | 326 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 331 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> > 
> > 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..fcc334a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_aux-dev.c
> > @@ -0,0 +1,326 @@
> > +/*
> > + * 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 <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 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 ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> > +			   loff_t *offset)
> > +{
> > +	char *localbuf;
> > +	ssize_t res;
> > +	struct drm_aux_dev *aux_dev = file->private_data;
> > +
> > +	localbuf = memdup_user(buf, count);
> > +	if (IS_ERR(localbuf))
> > +		return PTR_ERR(buf);
> > +
> > +	res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, count);
> > +	if (res < 0)
> > +		goto finish;
> > +
> > +	if (copy_to_user(buf, localbuf, res))
> > +		res = -EFAULT;
> > +
> > +finish:
> > +	kfree(localbuf);
> > +	return res;
> > +}
> > +
> > +static ssize_t auxdev_write(struct file *file, const char __user *buf,
> > +			    size_t count, loff_t *offset)
> > +{
> > +	char *localbuf;
> > +	ssize_t res;
> > +	struct drm_aux_dev *aux_dev = file->private_data;
> > +
> > +	localbuf = memdup_user(buf, count);
> > +	if (IS_ERR(localbuf))
> > +		return PTR_ERR(buf);
> > +
> > +	res = drm_dp_dpcd_write(aux_dev->aux, *offset, localbuf, count);
> > +	kfree(localbuf);
> > +
> > +	return res;
> > +}
> > +
> > +static const struct file_operations auxdev_fops = {
> > +	.owner		= THIS_MODULE,
> > +	.llseek		= generic_file_llseek,
> > +	.read		= auxdev_read,
> > +	.write		= auxdev_write,
> > +	.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_inform_cb(int action, struct drm_dp_aux *aux)
> > +{
> > +	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 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 */
> > +	drm_dp_aux_set_aux_dev(&auxdev_inform_cb);
> > +
> > +	/* Bind to already existing DP aux */
> > +	res = drm_dp_aux_for_each(&drm_dev_major, auxdev_attach_aux);
> > +
> > +	return 0;
> > +
> > +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_set_aux_dev(NULL);
> > +	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);
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> 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 RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Ville Syrjälä]

On Tue, Sep 22, 2015 at 10:48:06AM -0700, Rafael Antognolli wrote:
> On Tue, Sep 22, 2015 at 10:59:51AM +0200, Daniel Vetter wrote:
> > On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> > > 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: read and write. The
> > > address of the register to be read or written is given using lseek.
> > > Reading or writing does not update the offset of the file.
> > 
> > I think not updating the read position is very surprising. Would it be
> > hard to fix that?
> 
> No, not hard at all. But I assume then I should update the write
> position too, right?
> 
> BTW, i2c-dev doesn't update either of them, but I'm not sure why.

I think there's just no standard definition of what an offset would mean
for an i2c device. For standard eeproms it would work as one would expect,
but generally it's device specific.

> 
> > > 
> > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > > ---
> > >  drivers/gpu/drm/Kconfig       |   4 +
> > >  drivers/gpu/drm/Makefile      |   1 +
> > >  drivers/gpu/drm/drm_aux-dev.c | 326 ++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 331 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> > > 
> > > 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..fcc334a
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_aux-dev.c
> > > @@ -0,0 +1,326 @@
> > > +/*
> > > + * 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 <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 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 ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> > > +			   loff_t *offset)
> > > +{
> > > +	char *localbuf;
> > > +	ssize_t res;
> > > +	struct drm_aux_dev *aux_dev = file->private_data;
> > > +
> > > +	localbuf = memdup_user(buf, count);
> > > +	if (IS_ERR(localbuf))
> > > +		return PTR_ERR(buf);
> > > +
> > > +	res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, count);
> > > +	if (res < 0)
> > > +		goto finish;
> > > +
> > > +	if (copy_to_user(buf, localbuf, res))
> > > +		res = -EFAULT;
> > > +
> > > +finish:
> > > +	kfree(localbuf);
> > > +	return res;
> > > +}
> > > +
> > > +static ssize_t auxdev_write(struct file *file, const char __user *buf,
> > > +			    size_t count, loff_t *offset)
> > > +{
> > > +	char *localbuf;
> > > +	ssize_t res;
> > > +	struct drm_aux_dev *aux_dev = file->private_data;
> > > +
> > > +	localbuf = memdup_user(buf, count);
> > > +	if (IS_ERR(localbuf))
> > > +		return PTR_ERR(buf);
> > > +
> > > +	res = drm_dp_dpcd_write(aux_dev->aux, *offset, localbuf, count);
> > > +	kfree(localbuf);
> > > +
> > > +	return res;
> > > +}
> > > +
> > > +static const struct file_operations auxdev_fops = {
> > > +	.owner		= THIS_MODULE,
> > > +	.llseek		= generic_file_llseek,
> > > +	.read		= auxdev_read,
> > > +	.write		= auxdev_write,
> > > +	.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_inform_cb(int action, struct drm_dp_aux *aux)
> > > +{
> > > +	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 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 */
> > > +	drm_dp_aux_set_aux_dev(&auxdev_inform_cb);
> > > +
> > > +	/* Bind to already existing DP aux */
> > > +	res = drm_dp_aux_for_each(&drm_dev_major, auxdev_attach_aux);
> > > +
> > > +	return 0;
> > > +
> > > +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_set_aux_dev(NULL);
> > > +	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);
> > > -- 
> > > 2.4.3
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > 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

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

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

[Rafael Antognolli]

On Tue, Sep 22, 2015 at 02:17:51PM +0200, Daniel Vetter wrote:
> On Tue, Sep 22, 2015 at 03:00:54PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> > > 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: read and write. The
> > > address of the register to be read or written is given using lseek.
> > > Reading or writing does not update the offset of the file.
> > > 
> > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > > ---
> > >  drivers/gpu/drm/Kconfig       |   4 +
> > >  drivers/gpu/drm/Makefile      |   1 +
> > >  drivers/gpu/drm/drm_aux-dev.c | 326 ++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 331 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> > > 
> > > 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..fcc334a
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_aux-dev.c
> > > @@ -0,0 +1,326 @@
> > > +/*
> > > + * 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 <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 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 ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> > > +			   loff_t *offset)
> > > +{
> > > +	char *localbuf;
> > > +	ssize_t res;
> > > +	struct drm_aux_dev *aux_dev = file->private_data;
> > > +
> > > +	localbuf = memdup_user(buf, count);
> > > +	if (IS_ERR(localbuf))
> > > +		return PTR_ERR(buf);
> > > +
> > > +	res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, count);
> > > +	if (res < 0)
> > > +		goto finish;
> > 
> > read/write will need to do the access in 16 byte chunks. That's the max
> > amount of data that can be transferred with a single AUX operation. I
> > suppose that would also avoid the need for allocating large buffers
> > since you can just have a 16 byte buffer on the stack and keep reusing
> > it while you iterate through the total count. I'm not sure what are
> > the expected semantics of read/write if you do it in chunks and hit
> > a copy_{to,from}_user() failure somewhere down the line. But I suppose
> > access_ok() upfront for the whole user buffer could avoid that issue.
> 
> Iirc short reads are ok in all cases, so we could even punt the restarting
> to userspace by just doing short reads/writes (like sockets do).
> 
> > Also the size of the DPCD address space is 1<<20 bytes, so you'll need to
> > limit the access to stay within that. 'size = lseek(fd, 0, SEEK_END)'
> > can be used to get the file size, so you should allow the offset to
> > reach 1<<20, but not beyond. read/write should process as much much of
> > the data that they can given the current file offset, and return how
> > many bytes made it in/out (can also be 0 if offset was already at 1<<20).
> 
> Hm, sounds like a few testcases for cornercases would be neat. Best would
> be to do that in igt and iterate over all available dp_aux dev nodes (igt
> is now generic, so makes sense to put that stuff there). Something like
> kms_dp_aux_dev would be a nice testcase name.

OK, I sent an updated patch set including all (I think) changes
suggested by you and Ville.

I'm working on the igt test, but I am not sure exactly what cornercases
to test, and specially how to test them. I can read a couple of
registers that I have an idea of what to expect from them, but not
corner cases. What do you have in mind?

Also, I am not sure how to identify that a given aux channel doesn't
have a display/panel attached to it, so even a read operation just fails
with a connection timeout. Should I just skip this aux channel on this
case, and not consider it a failure, when iterating over them?

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

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

[Daniel Vetter]

On Fri, Sep 25, 2015 at 04:53:47PM -0700, Rafael Antognolli wrote:
> On Tue, Sep 22, 2015 at 02:17:51PM +0200, Daniel Vetter wrote:
> > On Tue, Sep 22, 2015 at 03:00:54PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> > > > 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: read and write. The
> > > > address of the register to be read or written is given using lseek.
> > > > Reading or writing does not update the offset of the file.
> > > > 
> > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/Kconfig       |   4 +
> > > >  drivers/gpu/drm/Makefile      |   1 +
> > > >  drivers/gpu/drm/drm_aux-dev.c | 326 ++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 331 insertions(+)
> > > >  create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> > > > 
> > > > 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..fcc334a
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/drm_aux-dev.c
> > > > @@ -0,0 +1,326 @@
> > > > +/*
> > > > + * 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 <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 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 ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> > > > +			   loff_t *offset)
> > > > +{
> > > > +	char *localbuf;
> > > > +	ssize_t res;
> > > > +	struct drm_aux_dev *aux_dev = file->private_data;
> > > > +
> > > > +	localbuf = memdup_user(buf, count);
> > > > +	if (IS_ERR(localbuf))
> > > > +		return PTR_ERR(buf);
> > > > +
> > > > +	res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, count);
> > > > +	if (res < 0)
> > > > +		goto finish;
> > > 
> > > read/write will need to do the access in 16 byte chunks. That's the max
> > > amount of data that can be transferred with a single AUX operation. I
> > > suppose that would also avoid the need for allocating large buffers
> > > since you can just have a 16 byte buffer on the stack and keep reusing
> > > it while you iterate through the total count. I'm not sure what are
> > > the expected semantics of read/write if you do it in chunks and hit
> > > a copy_{to,from}_user() failure somewhere down the line. But I suppose
> > > access_ok() upfront for the whole user buffer could avoid that issue.
> > 
> > Iirc short reads are ok in all cases, so we could even punt the restarting
> > to userspace by just doing short reads/writes (like sockets do).
> > 
> > > Also the size of the DPCD address space is 1<<20 bytes, so you'll need to
> > > limit the access to stay within that. 'size = lseek(fd, 0, SEEK_END)'
> > > can be used to get the file size, so you should allow the offset to
> > > reach 1<<20, but not beyond. read/write should process as much much of
> > > the data that they can given the current file offset, and return how
> > > many bytes made it in/out (can also be 0 if offset was already at 1<<20).
> > 
> > Hm, sounds like a few testcases for cornercases would be neat. Best would
> > be to do that in igt and iterate over all available dp_aux dev nodes (igt
> > is now generic, so makes sense to put that stuff there). Something like
> > kms_dp_aux_dev would be a nice testcase name.
> 
> OK, I sent an updated patch set including all (I think) changes
> suggested by you and Ville.
> 
> I'm working on the igt test, but I am not sure exactly what cornercases
> to test, and specially how to test them. I can read a couple of
> registers that I have an idea of what to expect from them, but not
> corner cases. What do you have in mind?

Trying to read more than what fits into a single dp aux transaction (to
test the partial reads/writes stuff) and trying to read invalid offsets
(to high address). Not sure whether we should do any writing tests since
that might cause trouble ...

> Also, I am not sure how to identify that a given aux channel doesn't
> have a display/panel attached to it, so even a read operation just fails
> with a connection timeout. Should I just skip this aux channel on this
> case, and not consider it a failure, when iterating over them?

Yeah that's what we usually do. There's igt_require/skip_on for these kind
of soft failures which should just result in a testcase getting skipped.
Note that only the first (test-)read should do that, just in case there's
bugs in the real testcase later on (e.g. the partial read stuff might give
a bogus error when it shouldn't). Also it would be good to encode the
errno value we expect in that case too. ETIMEDOUT seems to be what at
least i915 will return if nothing is connected (compared to -EIO which is
more an indication of the dp aux controller being toast).
-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