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?