diff mbox

[RFC,1/7] V4L: Add mem2mem ioctl and file operation helpers

Message ID 1379076986-10446-2-git-send-email-s.nawrocki@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Sept. 13, 2013, 12:56 p.m. UTC 
This patch adds ioctl helpers to the V4L2 mem-to-mem API, so we
can avoid several ioctl handlers in the mem-to-mem video node
drivers that are simply a pass-through to the v4l2_m2m_* calls.
These helpers will only be useful for drivers that use same mutex
for both OUTPUT and CAPTURE queue, which is the case for all
currently in tree v4l2 m2m drivers.
In order to use the helpers the driver are required to use
struct v4l2_fh.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyugmin Park <kyungmin.park@samsung.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c |  110 ++++++++++++++++++++++++++++++++
 include/media/v4l2-fh.h                |    4 ++
 include/media/v4l2-mem2mem.h           |   22 +++++++
 3 files changed, 136 insertions(+)

Comments

Philipp Zabel Sept. 13, 2013, 1:13 p.m. UTC | #1 
Hi Sylwester,

Am Freitag, den 13.09.2013, 14:56 +0200 schrieb Sylwester Nawrocki:
> This patch adds ioctl helpers to the V4L2 mem-to-mem API, so we
> can avoid several ioctl handlers in the mem-to-mem video node
> drivers that are simply a pass-through to the v4l2_m2m_* calls.
> These helpers will only be useful for drivers that use same mutex
> for both OUTPUT and CAPTURE queue, which is the case for all
> currently in tree v4l2 m2m drivers.
> In order to use the helpers the driver are required to use
> struct v4l2_fh.

this looks good to me.

> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyugmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c |  110 ++++++++++++++++++++++++++++++++
>  include/media/v4l2-fh.h                |    4 ++
>  include/media/v4l2-mem2mem.h           |   22 +++++++
>  3 files changed, 136 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 7c43712..dddad5b 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -544,6 +544,8 @@ unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
>  
>  	if (m2m_ctx->m2m_dev->m2m_ops->unlock)
>  		m2m_ctx->m2m_dev->m2m_ops->unlock(m2m_ctx->priv);
> +	else if (m2m_ctx->q_lock)
> +		mutex_unlock(m2m_ctx->q_lock);
>  
>  	if (list_empty(&src_q->done_list))
>  		poll_wait(file, &src_q->done_wq, wait);
> @@ -552,6 +554,8 @@ unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
>  
>  	if (m2m_ctx->m2m_dev->m2m_ops->lock)
>  		m2m_ctx->m2m_dev->m2m_ops->lock(m2m_ctx->priv);
> +	else if (m2m_ctx->q_lock)
> +		mutex_lock(m2m_ctx->q_lock);
>  
>  	spin_lock_irqsave(&src_q->done_lock, flags);
>  	if (!list_empty(&src_q->done_list))
> @@ -679,6 +683,13 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4l2_m2m_dev *m2m_dev,
>  
>  	if (ret)
>  		goto err;
> +	/*
> +	 * If both queues use same mutex assign it as the common buffer
> +	 * queues lock to the m2m context. This lock is used in the
> +	 * v4l2_m2m_ioctl_* helpers.
> +	 */
> +	if (out_q_ctx->q.lock == cap_q_ctx->q.lock)
> +		m2m_ctx->q_lock = out_q_ctx->q.lock;
>  
>  	return m2m_ctx;
>  err:
> @@ -726,3 +737,102 @@ void v4l2_m2m_buf_queue(struct v4l2_m2m_ctx *m2m_ctx, struct vb2_buffer *vb)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_buf_queue);
>  
> +/* Videobuf2 ioctl helpers */
> +
> +int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
> +				struct v4l2_requestbuffers *rb)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_reqbufs(file, fh->m2m_ctx, rb);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_reqbufs);
> +
> +int v4l2_m2m_ioctl_querybuf(struct file *file, void *priv,
> +				struct v4l2_buffer *buf)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_querybuf(file, fh->m2m_ctx, buf);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_querybuf);
> +
> +int v4l2_m2m_ioctl_qbuf(struct file *file, void *priv,
> +				struct v4l2_buffer *buf)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_qbuf(file, fh->m2m_ctx, buf);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_qbuf);
> +
> +int v4l2_m2m_ioctl_dqbuf(struct file *file, void *priv,
> +				struct v4l2_buffer *buf)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_dqbuf(file, fh->m2m_ctx, buf);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_dqbuf);
> +

Here I'm missing one 

+int v4l2_m2m_ioctl_create_bufs(struct file *file, void *priv,
+			       struct v4l2_create_buffers *create)
+{
+	struct v4l2_fh *fh = file->private_data;
+	return v4l2_m2m_create_bufs(file, fh->m2m_ctx, create);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_create_bufs);

> +int v4l2_m2m_ioctl_expbuf(struct file *file, void *priv,
> +				struct v4l2_exportbuffer *eb)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_expbuf(file, fh->m2m_ctx, eb);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_expbuf);
> +
> +int v4l2_m2m_ioctl_streamon(struct file *file, void *priv,
> +				enum v4l2_buf_type type)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_streamon(file, fh->m2m_ctx, type);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_streamon);
> +
> +int v4l2_m2m_ioctl_streamoff(struct file *file, void *priv,
> +				enum v4l2_buf_type type)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_streamoff(file, fh->m2m_ctx, type);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_streamoff);
> +
> +/*
> + * v4l2_file_operations helpers. It is assumed here same lock is used
> + * for the output and the capture buffer queue.
> + */
> +
> +int v4l2_m2m_fop_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	struct v4l2_m2m_ctx *m2m_ctx = fh->m2m_ctx;
> +	int ret;
> +
> +	if (m2m_ctx->q_lock && mutex_lock_interruptible(m2m_ctx->q_lock))
> +		return -ERESTARTSYS;
> +
> +	ret = v4l2_m2m_mmap(file, m2m_ctx, vma);
> +
> +	if (m2m_ctx->q_lock)
> +		mutex_unlock(m2m_ctx->q_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_fop_mmap);
> +
> +unsigned int v4l2_m2m_fop_poll(struct file *file, poll_table *wait)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	struct v4l2_m2m_ctx *m2m_ctx = fh->m2m_ctx;
> +	unsigned int ret;
> +
> +	if (m2m_ctx->q_lock)
> +		mutex_lock(m2m_ctx->q_lock);
> +
> +	ret = v4l2_m2m_poll(file, m2m_ctx, wait);
> +
> +	if (m2m_ctx->q_lock)
> +		mutex_unlock(m2m_ctx->q_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_fop_poll);
> +
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> index a62ee18..d942f79 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -43,6 +43,10 @@ struct v4l2_fh {
>  	struct list_head	available; /* Dequeueable event */
>  	unsigned int		navailable;
>  	u32			sequence;
> +
> +#if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
> +	struct v4l2_m2m_ctx	*m2m_ctx;
> +#endif
>  };
>  
>  /*
> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> index 44542a2..2a0e489 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -64,6 +64,9 @@ struct v4l2_m2m_queue_ctx {
>  };
>  
>  struct v4l2_m2m_ctx {
> +	/* optional cap/out vb2 queues lock */
> +	struct mutex			*q_lock;
> +
>  /* private: internal use only */
>  	struct v4l2_m2m_dev		*m2m_dev;
>  
> @@ -229,5 +232,24 @@ static inline void *v4l2_m2m_dst_buf_remove(struct v4l2_m2m_ctx *m2m_ctx)
>  	return v4l2_m2m_buf_remove(&m2m_ctx->cap_q_ctx);
>  }
>  
> +/* v4l2 ioctl helpers */
> +
> +int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
> +				struct v4l2_requestbuffers *rb);
> +int v4l2_m2m_ioctl_querybuf(struct file *file, void *fh,
> +				struct v4l2_buffer *buf);
> +int v4l2_m2m_ioctl_qbuf(struct file *file, void *fh,
> +				struct v4l2_buffer *buf);
> +int v4l2_m2m_ioctl_dqbuf(struct file *file, void *fh,
> +				struct v4l2_buffer *buf);

and a

+int v4l2_m2m_ioctl_create_bufs(struct file *file, void *fh,
+			       struct v4l2_create_buffers *create);

> +int v4l2_m2m_ioctl_expbuf(struct file *file, void *fh,
> +				struct v4l2_exportbuffer *eb);
> +int v4l2_m2m_ioctl_streamon(struct file *file, void *fh,
> +				enum v4l2_buf_type type);
> +int v4l2_m2m_ioctl_streamoff(struct file *file, void *fh,
> +				enum v4l2_buf_type type);
> +int v4l2_m2m_fop_mmap(struct file *file, struct vm_area_struct *vma);
> +unsigned int v4l2_m2m_fop_poll(struct file *file, poll_table *wait);
> +
>  #endif /* _MEDIA_V4L2_MEM2MEM_H */
>  

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki Sept. 15, 2013, 9:11 p.m. UTC | #2 
Hi Philipp,

On 09/13/2013 03:13 PM, Philipp Zabel wrote:
> Am Freitag, den 13.09.2013, 14:56 +0200 schrieb Sylwester Nawrocki:
>> This patch adds ioctl helpers to the V4L2 mem-to-mem API, so we
>> can avoid several ioctl handlers in the mem-to-mem video node
>> drivers that are simply a pass-through to the v4l2_m2m_* calls.
>> These helpers will only be useful for drivers that use same mutex
>> for both OUTPUT and CAPTURE queue, which is the case for all
>> currently in tree v4l2 m2m drivers.
>> In order to use the helpers the driver are required to use
>> struct v4l2_fh.
>
> this looks good to me.

Thank you for the review.

>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> index 7c43712..dddad5b 100644
>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
[...]
>> +/* Videobuf2 ioctl helpers */
>> +
>> +int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
>> +				struct v4l2_requestbuffers *rb)
>> +{
>> +	struct v4l2_fh *fh = file->private_data;
>> +	return v4l2_m2m_reqbufs(file, fh->m2m_ctx, rb);
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_reqbufs);
>> +
>> +int v4l2_m2m_ioctl_querybuf(struct file *file, void *priv,
>> +				struct v4l2_buffer *buf)
>> +{
>> +	struct v4l2_fh *fh = file->private_data;
>> +	return v4l2_m2m_querybuf(file, fh->m2m_ctx, buf);
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_querybuf);
>> +
>> +int v4l2_m2m_ioctl_qbuf(struct file *file, void *priv,
>> +				struct v4l2_buffer *buf)
>> +{
>> +	struct v4l2_fh *fh = file->private_data;
>> +	return v4l2_m2m_qbuf(file, fh->m2m_ctx, buf);
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_qbuf);
>> +
>> +int v4l2_m2m_ioctl_dqbuf(struct file *file, void *priv,
>> +				struct v4l2_buffer *buf)
>> +{
>> +	struct v4l2_fh *fh = file->private_data;
>> +	return v4l2_m2m_dqbuf(file, fh->m2m_ctx, buf);
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_dqbuf);
>> +
>
> Here I'm missing one
>
> +int v4l2_m2m_ioctl_create_bufs(struct file *file, void *priv,
> +			       struct v4l2_create_buffers *create)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_create_bufs(file, fh->m2m_ctx, create);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_create_bufs);

OK, I'll add that in the next iteration. I thought I would need to
add v4l2_m2m_ioctl_prepare_buf() similarly, but it's not necessary,
since vidioc_create_buf() calls directly to videobuf2, so that's
even simpler.

--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Verkuil Sept. 30, 2013, 9:41 a.m. UTC | #3 
On 09/13/2013 02:56 PM, Sylwester Nawrocki wrote:
> This patch adds ioctl helpers to the V4L2 mem-to-mem API, so we
> can avoid several ioctl handlers in the mem-to-mem video node
> drivers that are simply a pass-through to the v4l2_m2m_* calls.
> These helpers will only be useful for drivers that use same mutex
> for both OUTPUT and CAPTURE queue, which is the case for all
> currently in tree v4l2 m2m drivers.
> In order to use the helpers the driver are required to use
> struct v4l2_fh.

Looks good! I have one small comment below that you might want to address,
although it isn't blocking.

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyugmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c |  110 ++++++++++++++++++++++++++++++++
>  include/media/v4l2-fh.h                |    4 ++
>  include/media/v4l2-mem2mem.h           |   22 +++++++
>  3 files changed, 136 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 7c43712..dddad5b 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -544,6 +544,8 @@ unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
>  
>  	if (m2m_ctx->m2m_dev->m2m_ops->unlock)
>  		m2m_ctx->m2m_dev->m2m_ops->unlock(m2m_ctx->priv);
> +	else if (m2m_ctx->q_lock)
> +		mutex_unlock(m2m_ctx->q_lock);
>  
>  	if (list_empty(&src_q->done_list))
>  		poll_wait(file, &src_q->done_wq, wait);
> @@ -552,6 +554,8 @@ unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
>  
>  	if (m2m_ctx->m2m_dev->m2m_ops->lock)
>  		m2m_ctx->m2m_dev->m2m_ops->lock(m2m_ctx->priv);
> +	else if (m2m_ctx->q_lock)
> +		mutex_lock(m2m_ctx->q_lock);
>  
>  	spin_lock_irqsave(&src_q->done_lock, flags);
>  	if (!list_empty(&src_q->done_list))
> @@ -679,6 +683,13 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4l2_m2m_dev *m2m_dev,
>  
>  	if (ret)
>  		goto err;
> +	/*
> +	 * If both queues use same mutex assign it as the common buffer
> +	 * queues lock to the m2m context. This lock is used in the
> +	 * v4l2_m2m_ioctl_* helpers.
> +	 */
> +	if (out_q_ctx->q.lock == cap_q_ctx->q.lock)
> +		m2m_ctx->q_lock = out_q_ctx->q.lock;
>  
>  	return m2m_ctx;
>  err:
> @@ -726,3 +737,102 @@ void v4l2_m2m_buf_queue(struct v4l2_m2m_ctx *m2m_ctx, struct vb2_buffer *vb)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_buf_queue);
>  
> +/* Videobuf2 ioctl helpers */
> +
> +int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
> +				struct v4l2_requestbuffers *rb)
> +{
> +	struct v4l2_fh *fh = file->private_data;

I prefer an empty line after the variable declaration. Ditto below.

> +	return v4l2_m2m_reqbufs(file, fh->m2m_ctx, rb);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_reqbufs);
> +
> +int v4l2_m2m_ioctl_querybuf(struct file *file, void *priv,
> +				struct v4l2_buffer *buf)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_querybuf(file, fh->m2m_ctx, buf);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_querybuf);
> +
> +int v4l2_m2m_ioctl_qbuf(struct file *file, void *priv,
> +				struct v4l2_buffer *buf)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_qbuf(file, fh->m2m_ctx, buf);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_qbuf);
> +
> +int v4l2_m2m_ioctl_dqbuf(struct file *file, void *priv,
> +				struct v4l2_buffer *buf)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_dqbuf(file, fh->m2m_ctx, buf);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_dqbuf);
> +
> +int v4l2_m2m_ioctl_expbuf(struct file *file, void *priv,
> +				struct v4l2_exportbuffer *eb)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_expbuf(file, fh->m2m_ctx, eb);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_expbuf);
> +
> +int v4l2_m2m_ioctl_streamon(struct file *file, void *priv,
> +				enum v4l2_buf_type type)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_streamon(file, fh->m2m_ctx, type);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_streamon);
> +
> +int v4l2_m2m_ioctl_streamoff(struct file *file, void *priv,
> +				enum v4l2_buf_type type)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	return v4l2_m2m_streamoff(file, fh->m2m_ctx, type);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_streamoff);
> +
> +/*
> + * v4l2_file_operations helpers. It is assumed here same lock is used
> + * for the output and the capture buffer queue.
> + */
> +
> +int v4l2_m2m_fop_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	struct v4l2_m2m_ctx *m2m_ctx = fh->m2m_ctx;
> +	int ret;
> +
> +	if (m2m_ctx->q_lock && mutex_lock_interruptible(m2m_ctx->q_lock))
> +		return -ERESTARTSYS;
> +
> +	ret = v4l2_m2m_mmap(file, m2m_ctx, vma);
> +
> +	if (m2m_ctx->q_lock)
> +		mutex_unlock(m2m_ctx->q_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_fop_mmap);
> +
> +unsigned int v4l2_m2m_fop_poll(struct file *file, poll_table *wait)
> +{
> +	struct v4l2_fh *fh = file->private_data;
> +	struct v4l2_m2m_ctx *m2m_ctx = fh->m2m_ctx;
> +	unsigned int ret;
> +
> +	if (m2m_ctx->q_lock)
> +		mutex_lock(m2m_ctx->q_lock);
> +
> +	ret = v4l2_m2m_poll(file, m2m_ctx, wait);
> +
> +	if (m2m_ctx->q_lock)
> +		mutex_unlock(m2m_ctx->q_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_fop_poll);
> +
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> index a62ee18..d942f79 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -43,6 +43,10 @@ struct v4l2_fh {
>  	struct list_head	available; /* Dequeueable event */
>  	unsigned int		navailable;
>  	u32			sequence;
> +
> +#if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
> +	struct v4l2_m2m_ctx	*m2m_ctx;
> +#endif
>  };
>  
>  /*
> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> index 44542a2..2a0e489 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -64,6 +64,9 @@ struct v4l2_m2m_queue_ctx {
>  };
>  
>  struct v4l2_m2m_ctx {
> +	/* optional cap/out vb2 queues lock */
> +	struct mutex			*q_lock;
> +
>  /* private: internal use only */
>  	struct v4l2_m2m_dev		*m2m_dev;
>  
> @@ -229,5 +232,24 @@ static inline void *v4l2_m2m_dst_buf_remove(struct v4l2_m2m_ctx *m2m_ctx)
>  	return v4l2_m2m_buf_remove(&m2m_ctx->cap_q_ctx);
>  }
>  
> +/* v4l2 ioctl helpers */
> +
> +int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
> +				struct v4l2_requestbuffers *rb);
> +int v4l2_m2m_ioctl_querybuf(struct file *file, void *fh,
> +				struct v4l2_buffer *buf);
> +int v4l2_m2m_ioctl_qbuf(struct file *file, void *fh,
> +				struct v4l2_buffer *buf);
> +int v4l2_m2m_ioctl_dqbuf(struct file *file, void *fh,
> +				struct v4l2_buffer *buf);
> +int v4l2_m2m_ioctl_expbuf(struct file *file, void *fh,
> +				struct v4l2_exportbuffer *eb);
> +int v4l2_m2m_ioctl_streamon(struct file *file, void *fh,
> +				enum v4l2_buf_type type);
> +int v4l2_m2m_ioctl_streamoff(struct file *file, void *fh,
> +				enum v4l2_buf_type type);
> +int v4l2_m2m_fop_mmap(struct file *file, struct vm_area_struct *vma);
> +unsigned int v4l2_m2m_fop_poll(struct file *file, poll_table *wait);
> +
>  #endif /* _MEDIA_V4L2_MEM2MEM_H */
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki Oct. 12, 2013, 11:02 a.m. UTC | #4 
On 09/30/2013 11:41 AM, Hans Verkuil wrote:
> On 09/13/2013 02:56 PM, Sylwester Nawrocki wrote:
>> This patch adds ioctl helpers to the V4L2 mem-to-mem API, so we
>> can avoid several ioctl handlers in the mem-to-mem video node
>> drivers that are simply a pass-through to the v4l2_m2m_* calls.
>> These helpers will only be useful for drivers that use same mutex
>> for both OUTPUT and CAPTURE queue, which is the case for all
>> currently in tree v4l2 m2m drivers.
>> In order to use the helpers the driver are required to use
>> struct v4l2_fh.
>
> Looks good! I have one small comment below that you might want to address,
> although it isn't blocking.
>
> Acked-by: Hans Verkuil<hans.verkuil@cisco.com>
>
> Regards,
>
> 	Hans
>
>> +/* Videobuf2 ioctl helpers */
>> +
>> +int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
>> +				struct v4l2_requestbuffers *rb)
>> +{
>> +	struct v4l2_fh *fh = file->private_data;
>
> I prefer an empty line after the variable declaration. Ditto below.

Thank you for the review. All right, I will include this change in next
iteration. Even though my feeling is that such two-liner functions look
better without an empty line and readability is still maintained.

--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 7c43712..dddad5b 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -544,6 +544,8 @@  unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 
 	if (m2m_ctx->m2m_dev->m2m_ops->unlock)
 		m2m_ctx->m2m_dev->m2m_ops->unlock(m2m_ctx->priv);
+	else if (m2m_ctx->q_lock)
+		mutex_unlock(m2m_ctx->q_lock);
 
 	if (list_empty(&src_q->done_list))
 		poll_wait(file, &src_q->done_wq, wait);
@@ -552,6 +554,8 @@  unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 
 	if (m2m_ctx->m2m_dev->m2m_ops->lock)
 		m2m_ctx->m2m_dev->m2m_ops->lock(m2m_ctx->priv);
+	else if (m2m_ctx->q_lock)
+		mutex_lock(m2m_ctx->q_lock);
 
 	spin_lock_irqsave(&src_q->done_lock, flags);
 	if (!list_empty(&src_q->done_list))
@@ -679,6 +683,13 @@  struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4l2_m2m_dev *m2m_dev,
 
 	if (ret)
 		goto err;
+	/*
+	 * If both queues use same mutex assign it as the common buffer
+	 * queues lock to the m2m context. This lock is used in the
+	 * v4l2_m2m_ioctl_* helpers.
+	 */
+	if (out_q_ctx->q.lock == cap_q_ctx->q.lock)
+		m2m_ctx->q_lock = out_q_ctx->q.lock;
 
 	return m2m_ctx;
 err:
@@ -726,3 +737,102 @@  void v4l2_m2m_buf_queue(struct v4l2_m2m_ctx *m2m_ctx, struct vb2_buffer *vb)
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_buf_queue);
 
+/* Videobuf2 ioctl helpers */
+
+int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
+				struct v4l2_requestbuffers *rb)
+{
+	struct v4l2_fh *fh = file->private_data;
+	return v4l2_m2m_reqbufs(file, fh->m2m_ctx, rb);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_reqbufs);
+
+int v4l2_m2m_ioctl_querybuf(struct file *file, void *priv,
+				struct v4l2_buffer *buf)
+{
+	struct v4l2_fh *fh = file->private_data;
+	return v4l2_m2m_querybuf(file, fh->m2m_ctx, buf);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_querybuf);
+
+int v4l2_m2m_ioctl_qbuf(struct file *file, void *priv,
+				struct v4l2_buffer *buf)
+{
+	struct v4l2_fh *fh = file->private_data;
+	return v4l2_m2m_qbuf(file, fh->m2m_ctx, buf);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_qbuf);
+
+int v4l2_m2m_ioctl_dqbuf(struct file *file, void *priv,
+				struct v4l2_buffer *buf)
+{
+	struct v4l2_fh *fh = file->private_data;
+	return v4l2_m2m_dqbuf(file, fh->m2m_ctx, buf);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_dqbuf);
+
+int v4l2_m2m_ioctl_expbuf(struct file *file, void *priv,
+				struct v4l2_exportbuffer *eb)
+{
+	struct v4l2_fh *fh = file->private_data;
+	return v4l2_m2m_expbuf(file, fh->m2m_ctx, eb);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_expbuf);
+
+int v4l2_m2m_ioctl_streamon(struct file *file, void *priv,
+				enum v4l2_buf_type type)
+{
+	struct v4l2_fh *fh = file->private_data;
+	return v4l2_m2m_streamon(file, fh->m2m_ctx, type);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_streamon);
+
+int v4l2_m2m_ioctl_streamoff(struct file *file, void *priv,
+				enum v4l2_buf_type type)
+{
+	struct v4l2_fh *fh = file->private_data;
+	return v4l2_m2m_streamoff(file, fh->m2m_ctx, type);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_streamoff);
+
+/*
+ * v4l2_file_operations helpers. It is assumed here same lock is used
+ * for the output and the capture buffer queue.
+ */
+
+int v4l2_m2m_fop_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct v4l2_fh *fh = file->private_data;
+	struct v4l2_m2m_ctx *m2m_ctx = fh->m2m_ctx;
+	int ret;
+
+	if (m2m_ctx->q_lock && mutex_lock_interruptible(m2m_ctx->q_lock))
+		return -ERESTARTSYS;
+
+	ret = v4l2_m2m_mmap(file, m2m_ctx, vma);
+
+	if (m2m_ctx->q_lock)
+		mutex_unlock(m2m_ctx->q_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_fop_mmap);
+
+unsigned int v4l2_m2m_fop_poll(struct file *file, poll_table *wait)
+{
+	struct v4l2_fh *fh = file->private_data;
+	struct v4l2_m2m_ctx *m2m_ctx = fh->m2m_ctx;
+	unsigned int ret;
+
+	if (m2m_ctx->q_lock)
+		mutex_lock(m2m_ctx->q_lock);
+
+	ret = v4l2_m2m_poll(file, m2m_ctx, wait);
+
+	if (m2m_ctx->q_lock)
+		mutex_unlock(m2m_ctx->q_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_fop_poll);
+
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index a62ee18..d942f79 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -43,6 +43,10 @@  struct v4l2_fh {
 	struct list_head	available; /* Dequeueable event */
 	unsigned int		navailable;
 	u32			sequence;
+
+#if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
+	struct v4l2_m2m_ctx	*m2m_ctx;
+#endif
 };
 
 /*
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 44542a2..2a0e489 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -64,6 +64,9 @@  struct v4l2_m2m_queue_ctx {
 };
 
 struct v4l2_m2m_ctx {
+	/* optional cap/out vb2 queues lock */
+	struct mutex			*q_lock;
+
 /* private: internal use only */
 	struct v4l2_m2m_dev		*m2m_dev;
 
@@ -229,5 +232,24 @@  static inline void *v4l2_m2m_dst_buf_remove(struct v4l2_m2m_ctx *m2m_ctx)
 	return v4l2_m2m_buf_remove(&m2m_ctx->cap_q_ctx);
 }
 
+/* v4l2 ioctl helpers */
+
+int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
+				struct v4l2_requestbuffers *rb);
+int v4l2_m2m_ioctl_querybuf(struct file *file, void *fh,
+				struct v4l2_buffer *buf);
+int v4l2_m2m_ioctl_qbuf(struct file *file, void *fh,
+				struct v4l2_buffer *buf);
+int v4l2_m2m_ioctl_dqbuf(struct file *file, void *fh,
+				struct v4l2_buffer *buf);
+int v4l2_m2m_ioctl_expbuf(struct file *file, void *fh,
+				struct v4l2_exportbuffer *eb);
+int v4l2_m2m_ioctl_streamon(struct file *file, void *fh,
+				enum v4l2_buf_type type);
+int v4l2_m2m_ioctl_streamoff(struct file *file, void *fh,
+				enum v4l2_buf_type type);
+int v4l2_m2m_fop_mmap(struct file *file, struct vm_area_struct *vma);
+unsigned int v4l2_m2m_fop_poll(struct file *file, poll_table *wait);
+
 #endif /* _MEDIA_V4L2_MEM2MEM_H */