| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 23 Oct 2017 17:35:31 +0900
From: Alexandre Courbot <acourbot@...omium.org>
To: Hans Verkuil <hverkuil@...all.nl>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Pawel Osciak <pawel@...iak.com>,
Marek Szyprowski <m.szyprowski@...sung.com>,
Tomasz Figa <tfiga@...omium.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Gustavo Padovan <gustavo.padovan@...labora.com>,
Linux Media Mailing List <linux-media@...r.kernel.org>,
linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 2/9] [media] v4l2-core: add core jobs API support
Hi Hans,
On Mon, Oct 16, 2017 at 7:01 PM, Hans Verkuil <hverkuil@...all.nl> wrote:
>> +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.
Indeed!
>> 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.
Regarding the number of queued frames, frames end up being queued into
the VB2 queue even if they are not passed to the driver, so I guess
the driver can still peek at forward frames through that mechanism.
Same can be done for the list of jobs themselves with the state change
listener.
> 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.
Mmm, but wouldn't that kind of defeat the idea of jobs as a single
unit of work if they needed to be grouped before being processed? I
need to think a bit more about that. Maybe we could pass the buffers
to the drivers prior to their job being scheduled, but I like the idea
of having VB2 managing the buffer's flow as it is easy to screw the
flow otherwise.
>> 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?
IIRC this is to avoid collision with other V4L2 ioctls that start at
offset 0. I have not thought this thoroughly however, just wanted to
have something that works for experimenting.
Thanks,
Alex.
Powered by blists - more mailing lists