Re: [RFC PATCH 2/9] [media] v4l2-core: add core jobs API support

[Hans Verkuil]

On 09/28/2017 11:50 AM, Alexandre Courbot wrote:
> Add core support code for jobs API. This manages the life cycle of jobs
> and creation of a jobs queue, as well as the interface for job states.
> 
> It also exposes the user-space jobs API.
> 
> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxxxx>
> ---
>  drivers/media/v4l2-core/Makefile            |   3 +-
>  drivers/media/v4l2-core/v4l2-dev.c          |   6 +
>  drivers/media/v4l2-core/v4l2-jobqueue-dev.c | 173 +++++++
>  drivers/media/v4l2-core/v4l2-jobqueue.c     | 764 ++++++++++++++++++++++++++++
>  include/media/v4l2-dev.h                    |   4 +
>  include/media/v4l2-fh.h                     |   4 +
>  include/media/v4l2-job-state.h              |  75 +++
>  include/media/v4l2-jobqueue-dev.h           |  24 +
>  include/media/v4l2-jobqueue.h               |  54 ++
>  include/uapi/linux/v4l2-jobs.h              |  40 ++
>  include/uapi/linux/videodev2.h              |   2 +
>  11 files changed, 1148 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/media/v4l2-core/v4l2-jobqueue-dev.c
>  create mode 100644 drivers/media/v4l2-core/v4l2-jobqueue.c
>  create mode 100644 include/media/v4l2-job-state.h
>  create mode 100644 include/media/v4l2-jobqueue-dev.h
>  create mode 100644 include/media/v4l2-jobqueue.h
>  create mode 100644 include/uapi/linux/v4l2-jobs.h
> 
> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> index 098ad5fd5231..a717bb8f1a25 100644
> --- a/drivers/media/v4l2-core/Makefile
> +++ b/drivers/media/v4l2-core/Makefile
> @@ -6,7 +6,8 @@ tuner-objs	:=	tuner-core.o
>  
>  videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
>  			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \
> -			v4l2-async.o
> +			v4l2-async.o v4l2-jobqueue.o v4l2-jobqueue-dev.o
> +
>  ifeq ($(CONFIG_COMPAT),y)
>    videodev-objs += v4l2-compat-ioctl32.o
>  endif
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 5a7063886c93..fb229b671b9d 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -30,6 +30,7 @@
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
> +#include <media/v4l2-jobqueue-dev.h>
>  
>  #define VIDEO_NUM_DEVICES	256
>  #define VIDEO_NAME              "video4linux"
> @@ -1058,6 +1059,10 @@ static int __init videodev_init(void)
>  		return -EIO;
>  	}
>  
> +	ret = v4l2_jobqueue_device_init();
> +	if (ret < 0)
> +		printk(KERN_WARNING "video_dev: channel initialization failed\n");
> +
>  	return 0;
>  }
>  
> @@ -1065,6 +1070,7 @@ static void __exit videodev_exit(void)
>  {
>  	dev_t dev = MKDEV(VIDEO_MAJOR, 0);
>  
> +	v4l2_jobqueue_device_exit();
>  	class_unregister(&video_class);
>  	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
>  }
> diff --git a/drivers/media/v4l2-core/v4l2-jobqueue-dev.c b/drivers/media/v4l2-core/v4l2-jobqueue-dev.c
> new file mode 100644
> index 000000000000..688c4ba275a6
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-jobqueue-dev.c
> @@ -0,0 +1,173 @@
> +/*
> +    V4L2 job queue device
> +
> +    Copyright (C) 2017  The Chromium project
> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 2 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-jobqueue.h>
> +#include <uapi/linux/v4l2-jobs.h>
> +
> +#define CLASS_NAME "v4l2_jobqueue"
> +#define DEVICE_NAME "v4l2_jobqueue"
> +
> +static int major;
> +static struct class *jobqueue_class;
> +static struct device *jobqueue_device;
> +
> +static int v4l2_jobqueue_device_open(struct inode *inode, struct file *filp)
> +{
> +	struct v4l2_jobqueue *jq;
> +
> +	jq = v4l2_jobqueue_new();
> +	if (IS_ERR(jq))
> +		return PTR_ERR(jq);
> +
> +	filp->private_data = jq;
> +
> +	return 0;
> +}
> +
> +static int v4l2_jobqueue_device_release(struct inode *inode, struct file *filp)
> +{
> +	struct v4l2_jobqueue *jq = filp->private_data;
> +
> +	return v4l2_jobqueue_del(jq);
> +}
> +
> +static long v4l2_jobqueue_ioctl_init(struct file *filp, void *arg)
> +{
> +	struct v4l2_jobqueue *jq = filp->private_data;
> +	struct v4l2_jobqueue_init *cinit = arg;
> +
> +	return v4l2_jobqueue_init(jq, cinit);
> +}
> +
> +static long v4l2_jobqueue_device_ioctl_qjob(struct file *filp, void *arg)
> +{
> +	struct v4l2_jobqueue *jq = filp->private_data;
> +
> +	return v4l2_jobqueue_qjob(jq);
> +}
> +
> +static long v4l2_jobqueue_device_ioctl_dqjob(struct file *filp, void *arg)
> +{
> +	struct v4l2_jobqueue *jq = filp->private_data;
> +
> +	return v4l2_jobqueue_dqjob(jq);
> +}
> +
> +static long v4l2_jobqueue_device_ioctl_export_job(struct file *filp, void *arg)
> +{
> +	struct v4l2_jobqueue *jq = filp->private_data;
> +	struct v4l2_jobqueue_job *job = arg;
> +
> +	return v4l2_jobqueue_export_job(jq, job);
> +}
> +
> +static long v4l2_jobqueue_device_ioctl_import_job(struct file *filp, void *arg)
> +{
> +	struct v4l2_jobqueue *jq = filp->private_data;
> +	struct v4l2_jobqueue_job *job = arg;
> +
> +	return v4l2_jobqueue_import_job(jq, job);
> +}
> +
> +static long v4l2_jobqueue_device_do_ioctl(struct file *filp, unsigned int cmd,
> +					  void *arg)
> +{
> +	switch (cmd) {
> +		case VIDIOC_JOBQUEUE_INIT:
> +			return v4l2_jobqueue_ioctl_init(filp, arg);
> +
> +		case VIDIOC_JOBQUEUE_QJOB:
> +			return v4l2_jobqueue_device_ioctl_qjob(filp, arg);
> +
> +		case VIDIOC_JOBQUEUE_DQJOB:
> +			return v4l2_jobqueue_device_ioctl_dqjob(filp, arg);
> +
> +		case VIDIOC_JOBQUEUE_EXPORT_JOB:
> +			return v4l2_jobqueue_device_ioctl_export_job(filp, arg);
> +
> +		case VIDIOC_JOBQUEUE_IMPORT_JOB:
> +			return v4l2_jobqueue_device_ioctl_import_job(filp, arg);

There really is no need for these stub functions, just inline them for each
case.

> +
> +		default:
> +			pr_err("Invalid ioctl!\n");
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static long v4l2_jobqueue_device_ioctl(struct file *filp, unsigned int cmd,
> +				       unsigned long arg)
> +{
> +	return video_usercopy(filp, cmd, arg, v4l2_jobqueue_device_do_ioctl);
> +}
> +
> +static const struct file_operations v4l2_jobqueue_devnode_fops = {
> +	.owner = THIS_MODULE,
> +	.open = v4l2_jobqueue_device_open,
> +	.unlocked_ioctl = v4l2_jobqueue_device_ioctl,
> +#ifdef CONFIG_COMPAT
> +	/* TODO */
> +	/* .compat_ioctl = jobqueue_compat_ioctl, */
> +#endif
> +	.release = v4l2_jobqueue_device_release,
> +};
> +
> +int __init v4l2_jobqueue_device_init(void)
> +{
> +	/* Set to error value so v4l2_jobqueue_device_exit does nothing if we
> +	 * don't initialize properly */
> +	jobqueue_device = ERR_PTR(-EINVAL);
> +
> +	major = register_chrdev(0, DEVICE_NAME, &v4l2_jobqueue_devnode_fops);
> +	if (major < 0) {
> +		pr_err("unable to allocate major\n");
> +		return major;
> +	}
> +
> +	jobqueue_class = class_create(THIS_MODULE, CLASS_NAME);
> +	if (IS_ERR(jobqueue_class)) {
> +		pr_err("cannot create class\n");
> +		unregister_chrdev(major, DEVICE_NAME);
> +		return PTR_ERR(jobqueue_class);
> +	}
> +
> +	jobqueue_device = device_create(jobqueue_class, NULL, MKDEV(major, 0),
> +					NULL, DEVICE_NAME);
> +	if (IS_ERR(jobqueue_device)) {
> +		pr_err("cannot create device\n");
> +		class_destroy(jobqueue_class);
> +		unregister_chrdev(major, DEVICE_NAME);
> +		return PTR_ERR(jobqueue_device);
> +	}
> +
> +	return 0;
> +}
> +
> +void __exit v4l2_jobqueue_device_exit(void)
> +{
> +	if (IS_ERR(jobqueue_device))
> +		return;
> +
> +	device_destroy(jobqueue_class, MKDEV(major, 0));
> +	class_destroy(jobqueue_class);
> +	unregister_chrdev(major, DEVICE_NAME);
> +}

<snip>

> diff --git a/drivers/media/v4l2-core/v4l2-jobqueue.c b/drivers/media/v4l2-core/v4l2-jobqueue.c
> new file mode 100644
> index 000000000000..36d2dd48b086
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-jobqueue.c
> @@ -0,0 +1,764 @@
> +/*
> +    V4L2 job queue implementation
> +
> +    Copyright (C) 2017  The Chromium project
> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 2 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> + */
> +
> +#include <linux/compat.h>
> +#include <linux/export.h>
> +#include <linux/string.h>
> +#include <linux/file.h>
> +#include <linux/list.h>
> +#include <linux/kref.h>
> +#include <linux/anon_inodes.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/workqueue.h>
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-fh.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-jobqueue.h>
> +#include <media/v4l2-job-state.h>
> +#include <uapi/linux/v4l2-jobs.h>
> +
> +/* Limited by the size of atomic_t to track devices that completed a job */
> +#define V4L2_JOBQUEUE_MAX_DEVICES sizeof(atomic_t)
> +
> +/*
> + * State of all managed devices for a given job
> + */
> +struct v4l2_job {
> +	struct kref refcount;
> +	struct v4l2_jobqueue *jq;
> +	/* node in v4l2_jobqueue's queued_jobs or completed_jobs */
> +	struct list_head node;
> +	/* global list of existing jobs for this queue */
> +	struct list_head jobs_list;
> +	/* mask of devices that completed this job */
> +	atomic_t completed;
> +	/* fd exported to user-space */
> +	int fd;
> +	enum v4l2_job_status status;
> +
> +	/* per-device states */
> +	struct v4l2_job_state *state[0];
> +};
> +
> +/*
> + * A job queue manages the job flow for a given set of devices, applies their
> + * state, and activates them in lockstep.
> + *
> + * A job goes through the following stages through its life:
> + *
> + * * current_job: the job has been created and is waiting to be queued. S_CTRL
> + *   will apply to it. Once queued, it is pushed into
> + * * queued_jobs: a queue of jobs to be processed in sequential order. The head
> + *   of this list becomes the
> + * * active_job: the job currently being processed by the hardware. Once
> + *   completed, the next job in queued_job becomes active, and the previous
> + *   active job goes into
> + * * completed_jobs: a list of completed jobs waiting to be dequeued by
> + *   user-space. As user-space called the DQJOB ioctl, the head becomes the
> + * * dequeued_job: the job on which G_CTRL will be performed on. A job stays
> + *   in this state until another one is dequeued, at which point it is deleted.
> + */
> +struct v4l2_jobqueue {
> +	/* List of all jobs created for this queue, regardless of state */
> +	struct list_head jobs_list;
> +	/*
> +	 * Job that user-space is currently preparing, to be added to
> +	 * queued_jobs upon QJOB ioctl.
> +	 */
> +	struct v4l2_job *current_job;
> +
> +	/* List of jobs that are ready to be processed */
> +	struct list_head queued_jobs;
> +
> +	/* Job that is currently processed by the devices */
> +	struct v4l2_job *active_job;

Shouldn't this be a list as well? I interpret 'active job' as being a
job that is passed to the various drivers that need to process it.

Just as with video buffers where the hardware may need a minimum of
buffers before it can start the DMA (min_buffers_needed), so the same
is true for jobs. E.g. a driver may have to look ahead by a few frames
to see what changes are requested. Some changes (esp. sensor related)
can take a few frames before they take effect and the hardware has to
be programmed ahead of time.

It would make more sense if this was a list of active jobs. It would
likely also solve the TODOs you have in the code w.r.t. min_buffers_needed:
after STREAMON you'd just queue the jobs to the active list, and also
queue any associated buffers. Once the minimum number of buffers has been
reached the DMA is started.

> +
> +	/* List of completed jobs, ready to be dequeued */
> +	struct list_head completed_jobs;
> +
> +	/* Job that has last been dequeued and can be queried by user-space */
> +	struct v4l2_job *dequeued_job;
> +
> +	/* Projects the *_job[s] lists/pointers above */
> +	struct mutex lock;
> +	struct work_struct job_complete_work;
> +
> +	wait_queue_head_t done_wq;
> +
> +	unsigned int nb_devs;
> +	struct {
> +		struct file *f;
> +		struct v4l2_job_state_handler *state_handler;
> +	} *devs;
> +};

<snip>

> diff --git a/include/uapi/linux/v4l2-jobs.h b/include/uapi/linux/v4l2-jobs.h
> new file mode 100644
> index 000000000000..2cba4d20e62f
> --- /dev/null
> +++ b/include/uapi/linux/v4l2-jobs.h
> @@ -0,0 +1,40 @@
> +/*
> + * V4L2 jobs API
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __LINUX_V4L2_JOBS_H
> +#define __LINUX_V4L2_JOBS_H
> +
> +#ifndef __KERNEL__
> +#include <stdint.h>
> +#endif
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +struct v4l2_jobqueue_init {
> +	__u32 nb_devs;
> +	__s32 *fd;
> +};
> +
> +struct v4l2_jobqueue_job {
> +	__s32 fd;
> +};
> +
> +#define VIDIOC_JOBQUEUE_IOCTL_START	0x80

Why this offset?

> +
> +#define VIDIOC_JOBQUEUE_INIT		_IOW('|', VIDIOC_JOBQUEUE_IOCTL_START + 0x00, struct v4l2_jobqueue_init)
> +#define VIDIOC_JOBQUEUE_QJOB		_IO('|', VIDIOC_JOBQUEUE_IOCTL_START + 0x01)
> +#define VIDIOC_JOBQUEUE_DQJOB		_IO('|', VIDIOC_JOBQUEUE_IOCTL_START + 0x02)
> +#define VIDIOC_JOBQUEUE_EXPORT_JOB	_IOR('|', VIDIOC_JOBQUEUE_IOCTL_START + 0x03, struct v4l2_jobqueue_job)
> +#define VIDIOC_JOBQUEUE_IMPORT_JOB	_IOW('|', VIDIOC_JOBQUEUE_IOCTL_START + 0x03, struct v4l2_jobqueue_job)
> +
> +#endif
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 45cf7359822c..7f43e97cf461 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1591,6 +1591,8 @@ struct v4l2_ext_controls {
>  #define V4L2_CTRL_MAX_DIMS	  (4)
>  #define V4L2_CTRL_WHICH_CUR_VAL   0
>  #define V4L2_CTRL_WHICH_DEF_VAL   0x0f000000
> +#define V4L2_CTRL_WHICH_CURJOB_VAL   0x0e000000
> +#define V4L2_CTRL_WHICH_DEQJOB_VAL   0x0d000000
>  
>  enum v4l2_ctrl_type {
>  	V4L2_CTRL_TYPE_INTEGER	     = 1,
> 

Regards,

	Hans