[PATCH v5 08/21] gpu: host1x: Implement /dev/host1x device node
Mikko Perttunen
cyndis at kapsi.fi
Tue Mar 23 11:15:49 UTC 2021
On 3/23/21 1:02 PM, Thierry Reding wrote:
> On Mon, Jan 11, 2021 at 03:00:06PM +0200, Mikko Perttunen wrote:
>> Add the /dev/host1x device node, implementing the following
>> functionality:
>>
>> - Reading syncpoint values
>> - Allocating syncpoints (providing syncpoint FDs)
>> - Incrementing syncpoints (based on syncpoint FD)
>>
>> Signed-off-by: Mikko Perttunen <mperttunen at nvidia.com>
>> ---
>> v4:
>> * Put UAPI under CONFIG_DRM_TEGRA_STAGING
>> v3:
>> * Pass process name as syncpoint name when allocating
>> syncpoint.
>> ---
>> drivers/gpu/host1x/Makefile | 1 +
>> drivers/gpu/host1x/dev.c | 9 ++
>> drivers/gpu/host1x/dev.h | 3 +
>> drivers/gpu/host1x/uapi.c | 282 ++++++++++++++++++++++++++++++++++++
>> drivers/gpu/host1x/uapi.h | 22 +++
>> include/linux/host1x.h | 2 +
>> 6 files changed, 319 insertions(+)
>> create mode 100644 drivers/gpu/host1x/uapi.c
>> create mode 100644 drivers/gpu/host1x/uapi.h
>>
>> diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile
>> index 096017b8789d..882f928d75e1 100644
>> --- a/drivers/gpu/host1x/Makefile
>> +++ b/drivers/gpu/host1x/Makefile
>> @@ -9,6 +9,7 @@ host1x-y = \
>> job.o \
>> debug.o \
>> mipi.o \
>> + uapi.o \
>> hw/host1x01.o \
>> hw/host1x02.o \
>> hw/host1x04.o \
>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
>> index d0ebb70e2fdd..641317d23828 100644
>> --- a/drivers/gpu/host1x/dev.c
>> +++ b/drivers/gpu/host1x/dev.c
>> @@ -461,6 +461,12 @@ static int host1x_probe(struct platform_device *pdev)
>> goto deinit_syncpt;
>> }
>>
>> + err = host1x_uapi_init(&host->uapi, host);
>
> It's a bit pointless to pass &host->uapi and host to the function since
> you can access the former through the latter.
Yeah. I originally did it to separate the uapi module from the rest of
the code interface-wise as much as possible, but I don't think I have
done that consistently so it just looks weird.
>
>> + if (err) {
>> + dev_err(&pdev->dev, "failed to initialize uapi\n");
>
> s/uapi/UAPI/, and perhaps include the error code to give a better hint
> as to why things failed.
Sure (if this code is kept.)
>
>> + goto deinit_intr;
>> + }
>> +
>> host1x_debug_init(host);
>>
>> if (host->info->has_hypervisor)
>> @@ -480,6 +486,8 @@ static int host1x_probe(struct platform_device *pdev)
>> host1x_unregister(host);
>> deinit_debugfs:
>> host1x_debug_deinit(host);
>> + host1x_uapi_deinit(&host->uapi);
>> +deinit_intr:
>> host1x_intr_deinit(host);
>> deinit_syncpt:
>> host1x_syncpt_deinit(host);
>> @@ -501,6 +509,7 @@ static int host1x_remove(struct platform_device *pdev)
>>
>> host1x_unregister(host);
>> host1x_debug_deinit(host);
>> + host1x_uapi_deinit(&host->uapi);
>> host1x_intr_deinit(host);
>> host1x_syncpt_deinit(host);
>> reset_control_assert(host->rst);
>> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
>> index 63010ae37a97..7b8b7e20e32b 100644
>> --- a/drivers/gpu/host1x/dev.h
>> +++ b/drivers/gpu/host1x/dev.h
>> @@ -17,6 +17,7 @@
>> #include "intr.h"
>> #include "job.h"
>> #include "syncpt.h"
>> +#include "uapi.h"
>>
>> struct host1x_syncpt;
>> struct host1x_syncpt_base;
>> @@ -143,6 +144,8 @@ struct host1x {
>> struct list_head list;
>>
>> struct device_dma_parameters dma_parms;
>> +
>> + struct host1x_uapi uapi;
>> };
>>
>> void host1x_hypervisor_writel(struct host1x *host1x, u32 r, u32 v);
>> diff --git a/drivers/gpu/host1x/uapi.c b/drivers/gpu/host1x/uapi.c
>> new file mode 100644
>> index 000000000000..27b8761c3f35
>> --- /dev/null
>> +++ b/drivers/gpu/host1x/uapi.c
>> @@ -0,0 +1,282 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * /dev/host1x syncpoint interface
>> + *
>> + * Copyright (c) 2020, NVIDIA Corporation.
>> + */
>> +
>> +#include <linux/anon_inodes.h>
>> +#include <linux/cdev.h>
>> +#include <linux/file.h>
>> +#include <linux/fs.h>
>> +#include <linux/host1x.h>
>> +#include <linux/nospec.h>
>> +
>> +#include "dev.h"
>> +#include "syncpt.h"
>> +#include "uapi.h"
>> +
>> +#include <uapi/linux/host1x.h>
>> +
>> +static int syncpt_file_release(struct inode *inode, struct file *file)
>> +{
>> + struct host1x_syncpt *sp = file->private_data;
>> +
>> + host1x_syncpt_put(sp);
>> +
>> + return 0;
>> +}
>> +
>> +static int syncpt_file_ioctl_info(struct host1x_syncpt *sp, void __user *data)
>> +{
>> + struct host1x_syncpoint_info args;
>> + unsigned long copy_err;
>> +
>> + copy_err = copy_from_user(&args, data, sizeof(args));
>> + if (copy_err)
>> + return -EFAULT;
>> +
>> + if (args.reserved[0] || args.reserved[1] || args.reserved[2])
>> + return -EINVAL;
>
> Yes! \o/
>
>> +
>> + args.id = sp->id;
>> +
>> + copy_err = copy_to_user(data, &args, sizeof(args));
>> + if (copy_err)
>> + return -EFAULT;
>> +
>> + return 0;
>> +}
>> +
>> +static int syncpt_file_ioctl_incr(struct host1x_syncpt *sp, void __user *data)
>> +{
>> + struct host1x_syncpoint_increment args;
>> + unsigned long copy_err;
>> + u32 i;
>> +
>> + copy_err = copy_from_user(&args, data, sizeof(args));
>> + if (copy_err)
>> + return -EFAULT;
>> +
>> + for (i = 0; i < args.count; i++) {
>> + host1x_syncpt_incr(sp);
>> + if (signal_pending(current))
>> + return -EINTR;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static long syncpt_file_ioctl(struct file *file, unsigned int cmd,
>> + unsigned long arg)
>> +{
>> + void __user *data = (void __user *)arg;
>> + long err;
>> +
>> + switch (cmd) {
>> + case HOST1X_IOCTL_SYNCPOINT_INFO:
>> + err = syncpt_file_ioctl_info(file->private_data, data);
>> + break;
>> +
>> + case HOST1X_IOCTL_SYNCPOINT_INCREMENT:
>> + err = syncpt_file_ioctl_incr(file->private_data, data);
>> + break;
>> +
>> + default:
>> + err = -ENOTTY;
>> + }
>> +
>> + return err;
>> +}
>
> I wonder if it's worth adding some more logic to this demuxing. I'm
> thinking along the lines of what the DRM IOCTL demuxer does, which
> ultimately allows the IOCTLs to be extended. It does this by doing a
> bit of sanitizing and removing the parameter size field from the cmd
> argument so that the same IOCTL may handle different parameter sizes.
Yep, seems like a good idea (if we keep this).
>
>> +static const struct file_operations syncpt_file_fops = {
>> + .owner = THIS_MODULE,
>> + .release = syncpt_file_release,
>> + .unlocked_ioctl = syncpt_file_ioctl,
>> + .compat_ioctl = syncpt_file_ioctl,
>> +};
>> +
>> +struct host1x_syncpt *host1x_syncpt_fd_get(int fd)
>> +{
>> + struct host1x_syncpt *sp;
>> + struct file *file = fget(fd);
>> +
>> + if (!file)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (file->f_op != &syncpt_file_fops) {
>> + fput(file);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + sp = file->private_data;
>> +
>> + host1x_syncpt_get(sp);
>> +
>> + fput(file);
>> +
>> + return sp;
>> +}
>> +EXPORT_SYMBOL(host1x_syncpt_fd_get);
>> +
>> +static int dev_file_open(struct inode *inode, struct file *file)
>
> Maybe use the more specific host1x_ as prefix instead of the generic
> dev_? That might make things like stack traces more readable.
Yep.
>
> Otherwise looks good.
>
> Thierry
>
thanks,
Mikko
More information about the dri-devel
mailing list