[PATCH RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
Daniel Vetter
daniel at ffwll.ch
Sun Sep 27 06:11:38 PDT 2015
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 at 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 at 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
More information about the dri-devel
mailing list