[PATCH 0/3] omap3isp trivial enhancements

[PATCH 0/3] omap3isp trivial enhancements

[Lad Prabhakar]

From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>

Hi Laurent/Sakari,

This patch series is intended to use the helpers provided
by the v4l-core.
Please note I have just compile tested them.

Lad, Prabhakar (3):
  media: omap3isp: ispvideo: drop setting of vb2 buffer state to
    VB2_BUF_STATE_ACTIVE
  media: omap3isp: ispvideo: drop driver specific isp_video_fh
  media: omap3isp: ispvideo: use vb2_fop_mmap/poll

 drivers/media/platform/omap3isp/ispvideo.c | 153 +++++++++--------------------
 drivers/media/platform/omap3isp/ispvideo.h |  13 +--
 2 files changed, 50 insertions(+), 116 deletions(-)

-- 
2.1.0

[PATCH 1/3] media: omap3isp: ispvideo: drop setting of vb2 buffer state to VB2_BUF_STATE_ACTIVE

[Lad Prabhakar]

From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>

There isn't a need to assign the state of vb2_buffer to active
as this is already done by the core.

Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
 drivers/media/platform/omap3isp/ispvideo.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 3fe9047..837018d 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -524,7 +524,6 @@ struct isp_buffer *omap3isp_video_buffer_next(struct isp_video *video)
 
 	buf = list_first_entry(&video->dmaqueue, struct isp_buffer,
 			       irqlist);
-	buf->vb.state = VB2_BUF_STATE_ACTIVE;
 
 	spin_unlock_irqrestore(&video->irqlock, flags);
 
-- 
2.1.0

Re: [PATCH 1/3] media: omap3isp: ispvideo: drop setting of vb2 buffer state to VB2_BUF_STATE_ACTIVE

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Monday 23 February 2015 20:19:31 Lad Prabhakar wrote:
> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> 
> There isn't a need to assign the state of vb2_buffer to active
> as this is already done by the core.
> 
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and queued to my tree. I still have to test the patch, I'll report any issue I 
can find.

> ---
>  drivers/media/platform/omap3isp/ispvideo.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index 3fe9047..837018d 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -524,7 +524,6 @@ struct isp_buffer *omap3isp_video_buffer_next(struct
> isp_video *video)
> 
>  	buf = list_first_entry(&video->dmaqueue, struct isp_buffer,
>  			       irqlist);
> -	buf->vb.state = VB2_BUF_STATE_ACTIVE;
> 
>  	spin_unlock_irqrestore(&video->irqlock, flags);

-- 
Regards,

Laurent Pinchart

[PATCH 2/3] media: omap3isp: ispvideo: drop driver specific isp_video_fh

[Lad Prabhakar]

From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>

this patch drops driver specific isp_video_fh, as this
can be handled by core.

Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
 drivers/media/platform/omap3isp/ispvideo.c | 128 +++++++++++------------------
 drivers/media/platform/omap3isp/ispvideo.h |  13 +--
 2 files changed, 49 insertions(+), 92 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 837018d..b648176 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -294,22 +294,22 @@ __isp_video_get_format(struct isp_video *video, struct v4l2_format *format)
 }
 
 static int
-isp_video_check_format(struct isp_video *video, struct isp_video_fh *vfh)
+isp_video_check_format(struct isp_video *video)
 {
 	struct v4l2_format format;
 	int ret;
 
-	memcpy(&format, &vfh->format, sizeof(format));
+	memcpy(&format, &video->format, sizeof(format));
 	ret = __isp_video_get_format(video, &format);
 	if (ret < 0)
 		return ret;
 
-	if (vfh->format.fmt.pix.pixelformat != format.fmt.pix.pixelformat ||
-	    vfh->format.fmt.pix.height != format.fmt.pix.height ||
-	    vfh->format.fmt.pix.width != format.fmt.pix.width ||
-	    vfh->format.fmt.pix.bytesperline != format.fmt.pix.bytesperline ||
-	    vfh->format.fmt.pix.sizeimage != format.fmt.pix.sizeimage ||
-	    vfh->format.fmt.pix.field != format.fmt.pix.field)
+	if (video->format.fmt.pix.pixelformat != format.fmt.pix.pixelformat ||
+	    video->format.fmt.pix.height != format.fmt.pix.height ||
+	    video->format.fmt.pix.width != format.fmt.pix.width ||
+	    video->format.fmt.pix.bytesperline != format.fmt.pix.bytesperline ||
+	    video->format.fmt.pix.sizeimage != format.fmt.pix.sizeimage ||
+	    video->format.fmt.pix.field != format.fmt.pix.field)
 		return -EINVAL;
 
 	return 0;
@@ -324,12 +324,11 @@ static int isp_video_queue_setup(struct vb2_queue *queue,
 				 unsigned int *count, unsigned int *num_planes,
 				 unsigned int sizes[], void *alloc_ctxs[])
 {
-	struct isp_video_fh *vfh = vb2_get_drv_priv(queue);
-	struct isp_video *video = vfh->video;
+	struct isp_video *video = vb2_get_drv_priv(queue);
 
 	*num_planes = 1;
 
-	sizes[0] = vfh->format.fmt.pix.sizeimage;
+	sizes[0] = video->format.fmt.pix.sizeimage;
 	if (sizes[0] == 0)
 		return -EINVAL;
 
@@ -342,9 +341,8 @@ static int isp_video_queue_setup(struct vb2_queue *queue,
 
 static int isp_video_buffer_prepare(struct vb2_buffer *buf)
 {
-	struct isp_video_fh *vfh = vb2_get_drv_priv(buf->vb2_queue);
+	struct isp_video *video = vb2_get_drv_priv(buf->vb2_queue);
 	struct isp_buffer *buffer = to_isp_buffer(buf);
-	struct isp_video *video = vfh->video;
 	dma_addr_t addr;
 
 	/* Refuse to prepare the buffer is the video node has registered an
@@ -363,7 +361,7 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
 		return -EINVAL;
 	}
 
-	vb2_set_plane_payload(&buffer->vb, 0, vfh->format.fmt.pix.sizeimage);
+	vb2_set_plane_payload(&buffer->vb, 0, video->format.fmt.pix.sizeimage);
 	buffer->dma = addr;
 
 	return 0;
@@ -380,9 +378,8 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
  */
 static void isp_video_buffer_queue(struct vb2_buffer *buf)
 {
-	struct isp_video_fh *vfh = vb2_get_drv_priv(buf->vb2_queue);
+	struct isp_video *video = vb2_get_drv_priv(buf->vb2_queue);
 	struct isp_buffer *buffer = to_isp_buffer(buf);
-	struct isp_video *video = vfh->video;
 	struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
 	enum isp_pipeline_state state;
 	unsigned long flags;
@@ -573,7 +570,7 @@ void omap3isp_video_resume(struct isp_video *video, int continuous)
 
 	if (continuous && video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		mutex_lock(&video->queue_lock);
-		vb2_discard_done(video->queue);
+		vb2_discard_done(&video->queue);
 		mutex_unlock(&video->queue_lock);
 	}
 
@@ -615,14 +612,13 @@ isp_video_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
 static int
 isp_video_get_format(struct file *file, void *fh, struct v4l2_format *format)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(fh);
 	struct isp_video *video = video_drvdata(file);
 
 	if (format->type != video->type)
 		return -EINVAL;
 
 	mutex_lock(&video->mutex);
-	*format = vfh->format;
+	*format = video->format;
 	mutex_unlock(&video->mutex);
 
 	return 0;
@@ -631,7 +627,6 @@ isp_video_get_format(struct file *file, void *fh, struct v4l2_format *format)
 static int
 isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(fh);
 	struct isp_video *video = video_drvdata(file);
 	struct v4l2_mbus_framefmt fmt;
 
@@ -680,7 +675,7 @@ isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
 	isp_video_mbus_to_pix(video, &fmt, &format->fmt.pix);
 
 	mutex_lock(&video->mutex);
-	vfh->format = *format;
+	video->format = *format;
 	mutex_unlock(&video->mutex);
 
 	return 0;
@@ -787,7 +782,6 @@ isp_video_set_crop(struct file *file, void *fh, const struct v4l2_crop *crop)
 static int
 isp_video_get_param(struct file *file, void *fh, struct v4l2_streamparm *a)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(fh);
 	struct isp_video *video = video_drvdata(file);
 
 	if (video->type != V4L2_BUF_TYPE_VIDEO_OUTPUT ||
@@ -797,7 +791,7 @@ isp_video_get_param(struct file *file, void *fh, struct v4l2_streamparm *a)
 	memset(a, 0, sizeof(*a));
 	a->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
 	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
-	a->parm.output.timeperframe = vfh->timeperframe;
+	a->parm.output.timeperframe = video->timeperframe;
 
 	return 0;
 }
@@ -805,7 +799,6 @@ isp_video_get_param(struct file *file, void *fh, struct v4l2_streamparm *a)
 static int
 isp_video_set_param(struct file *file, void *fh, struct v4l2_streamparm *a)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(fh);
 	struct isp_video *video = video_drvdata(file);
 
 	if (video->type != V4L2_BUF_TYPE_VIDEO_OUTPUT ||
@@ -815,7 +808,7 @@ isp_video_set_param(struct file *file, void *fh, struct v4l2_streamparm *a)
 	if (a->parm.output.timeperframe.denominator == 0)
 		a->parm.output.timeperframe.denominator = 1;
 
-	vfh->timeperframe = a->parm.output.timeperframe;
+	video->timeperframe = a->parm.output.timeperframe;
 
 	return 0;
 }
@@ -823,12 +816,11 @@ isp_video_set_param(struct file *file, void *fh, struct v4l2_streamparm *a)
 static int
 isp_video_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *rb)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(fh);
 	struct isp_video *video = video_drvdata(file);
 	int ret;
 
 	mutex_lock(&video->queue_lock);
-	ret = vb2_reqbufs(&vfh->queue, rb);
+	ret = vb2_reqbufs(&video->queue, rb);
 	mutex_unlock(&video->queue_lock);
 
 	return ret;
@@ -837,12 +829,11 @@ isp_video_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *rb)
 static int
 isp_video_querybuf(struct file *file, void *fh, struct v4l2_buffer *b)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(fh);
 	struct isp_video *video = video_drvdata(file);
 	int ret;
 
 	mutex_lock(&video->queue_lock);
-	ret = vb2_querybuf(&vfh->queue, b);
+	ret = vb2_querybuf(&video->queue, b);
 	mutex_unlock(&video->queue_lock);
 
 	return ret;
@@ -851,12 +842,11 @@ isp_video_querybuf(struct file *file, void *fh, struct v4l2_buffer *b)
 static int
 isp_video_qbuf(struct file *file, void *fh, struct v4l2_buffer *b)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(fh);
 	struct isp_video *video = video_drvdata(file);
 	int ret;
 
 	mutex_lock(&video->queue_lock);
-	ret = vb2_qbuf(&vfh->queue, b);
+	ret = vb2_qbuf(&video->queue, b);
 	mutex_unlock(&video->queue_lock);
 
 	return ret;
@@ -865,12 +855,11 @@ isp_video_qbuf(struct file *file, void *fh, struct v4l2_buffer *b)
 static int
 isp_video_dqbuf(struct file *file, void *fh, struct v4l2_buffer *b)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(fh);
 	struct isp_video *video = video_drvdata(file);
 	int ret;
 
 	mutex_lock(&video->queue_lock);
-	ret = vb2_dqbuf(&vfh->queue, b, file->f_flags & O_NONBLOCK);
+	ret = vb2_dqbuf(&video->queue, b, file->f_flags & O_NONBLOCK);
 	mutex_unlock(&video->queue_lock);
 
 	return ret;
@@ -1001,7 +990,6 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
 static int
 isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(fh);
 	struct isp_video *video = video_drvdata(file);
 	enum isp_pipeline_state state;
 	struct isp_pipeline *pipe;
@@ -1033,12 +1021,12 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	/* Verify that the currently configured format matches the output of
 	 * the connected subdev.
 	 */
-	ret = isp_video_check_format(video, vfh);
+	ret = isp_video_check_format(video);
 	if (ret < 0)
 		goto err_check_format;
 
 	video->bpl_padding = ret;
-	video->bpl_value = vfh->format.fmt.pix.bytesperline;
+	video->bpl_value = video->format.fmt.pix.bytesperline;
 
 	ret = isp_video_get_graph_data(video, pipe);
 	if (ret < 0)
@@ -1065,15 +1053,14 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	 * support the request limit.
 	 */
 	if (video->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
-		pipe->max_timeperframe = vfh->timeperframe;
+		pipe->max_timeperframe = video->timeperframe;
 
-	video->queue = &vfh->queue;
 	INIT_LIST_HEAD(&video->dmaqueue);
 	atomic_set(&pipe->frame_number, -1);
-	pipe->field = vfh->format.fmt.pix.field;
+	pipe->field = video->format.fmt.pix.field;
 
 	mutex_lock(&video->queue_lock);
-	ret = vb2_streamon(&vfh->queue, type);
+	ret = vb2_streamon(&video->queue, type);
 	mutex_unlock(&video->queue_lock);
 	if (ret < 0)
 		goto err_check_format;
@@ -1098,7 +1085,7 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 
 err_set_stream:
 	mutex_lock(&video->queue_lock);
-	vb2_streamoff(&vfh->queue, type);
+	vb2_streamoff(&video->queue, type);
 	mutex_unlock(&video->queue_lock);
 err_check_format:
 	media_entity_pipeline_stop(&video->video.entity);
@@ -1113,7 +1100,6 @@ err_pipeline_start:
 	 * free-running sensor.
 	 */
 	INIT_LIST_HEAD(&video->dmaqueue);
-	video->queue = NULL;
 
 	mutex_unlock(&video->stream_lock);
 	return ret;
@@ -1122,7 +1108,6 @@ err_pipeline_start:
 static int
 isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(fh);
 	struct isp_video *video = video_drvdata(file);
 	struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
 	enum isp_pipeline_state state;
@@ -1136,7 +1121,7 @@ isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 
 	/* Make sure we're not streaming yet. */
 	mutex_lock(&video->queue_lock);
-	streaming = vb2_is_streaming(&vfh->queue);
+	streaming = vb2_is_streaming(&video->queue);
 	mutex_unlock(&video->queue_lock);
 
 	if (!streaming)
@@ -1159,9 +1144,8 @@ isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 	omap3isp_video_cancel_stream(video);
 
 	mutex_lock(&video->queue_lock);
-	vb2_streamoff(&vfh->queue, type);
+	vb2_streamoff(&video->queue, type);
 	mutex_unlock(&video->queue_lock);
-	video->queue = NULL;
 	video->error = false;
 
 	if (video->isp->pdata->set_constraints)
@@ -1230,16 +1214,12 @@ static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
 static int isp_video_open(struct file *file)
 {
 	struct isp_video *video = video_drvdata(file);
-	struct isp_video_fh *handle;
 	struct vb2_queue *queue;
-	int ret = 0;
-
-	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
-	if (handle == NULL)
-		return -ENOMEM;
+	int ret;
 
-	v4l2_fh_init(&handle->vfh, &video->video);
-	v4l2_fh_add(&handle->vfh);
+	ret = v4l2_fh_open(file);
+	if (ret)
+		return ret;
 
 	/* If this is the first user, initialise the pipeline. */
 	if (omap3isp_get(video->isp) == NULL) {
@@ -1253,70 +1233,57 @@ static int isp_video_open(struct file *file)
 		goto done;
 	}
 
-	queue = &handle->queue;
+	queue = &video->queue;
 	queue->type = video->type;
 	queue->io_modes = VB2_MMAP | VB2_USERPTR;
-	queue->drv_priv = handle;
+	queue->drv_priv = video;
 	queue->ops = &isp_video_queue_ops;
 	queue->mem_ops = &vb2_dma_contig_memops;
 	queue->buf_struct_size = sizeof(struct isp_buffer);
 	queue->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
-	ret = vb2_queue_init(&handle->queue);
+	ret = vb2_queue_init(queue);
 	if (ret < 0) {
 		omap3isp_put(video->isp);
 		goto done;
 	}
 
-	memset(&handle->format, 0, sizeof(handle->format));
-	handle->format.type = video->type;
-	handle->timeperframe.denominator = 1;
-
-	handle->video = video;
-	file->private_data = &handle->vfh;
+	memset(&video->format, 0, sizeof(video->format));
+	video->format.type = video->type;
+	video->timeperframe.denominator = 1;
 
 done:
-	if (ret < 0) {
-		v4l2_fh_del(&handle->vfh);
-		kfree(handle);
-	}
-
 	return ret;
 }
 
 static int isp_video_release(struct file *file)
 {
 	struct isp_video *video = video_drvdata(file);
-	struct v4l2_fh *vfh = file->private_data;
-	struct isp_video_fh *handle = to_isp_video_fh(vfh);
+	int ret;
 
 	/* Disable streaming and free the buffers queue resources. */
-	isp_video_streamoff(file, vfh, video->type);
+	isp_video_streamoff(file, NULL, video->type);
 
 	mutex_lock(&video->queue_lock);
-	vb2_queue_release(&handle->queue);
+	vb2_queue_release(&video->queue);
 	mutex_unlock(&video->queue_lock);
 
 	omap3isp_pipeline_pm_use(&video->video.entity, 0);
 
-	/* Release the file handle. */
-	v4l2_fh_del(vfh);
-	kfree(handle);
-	file->private_data = NULL;
+	ret = _vb2_fop_release(file, NULL);
 
 	omap3isp_put(video->isp);
 
-	return 0;
+	return ret;
 }
 
 static unsigned int isp_video_poll(struct file *file, poll_table *wait)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(file->private_data);
 	struct isp_video *video = video_drvdata(file);
 	int ret;
 
 	mutex_lock(&video->queue_lock);
-	ret = vb2_poll(&vfh->queue, file, wait);
+	ret = vb2_poll(&video->queue, file, wait);
 	mutex_unlock(&video->queue_lock);
 
 	return ret;
@@ -1324,12 +1291,11 @@ static unsigned int isp_video_poll(struct file *file, poll_table *wait)
 
 static int isp_video_mmap(struct file *file, struct vm_area_struct *vma)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(file->private_data);
 	struct isp_video *video = video_drvdata(file);
 	int ret;
 
 	mutex_lock(&video->queue_lock);
-	ret = vb2_mmap(&vfh->queue, vma);
+	ret = vb2_mmap(&video->queue, vma);
 	mutex_unlock(&video->queue_lock);
 
 	return ret;
diff --git a/drivers/media/platform/omap3isp/ispvideo.h b/drivers/media/platform/omap3isp/ispvideo.h
index 4071dd7..d960bbd 100644
--- a/drivers/media/platform/omap3isp/ispvideo.h
+++ b/drivers/media/platform/omap3isp/ispvideo.h
@@ -172,28 +172,19 @@ struct isp_video {
 
 	/* Video buffers queue */
 	void *alloc_ctx;
-	struct vb2_queue *queue;
+	struct vb2_queue queue;
 	struct mutex queue_lock;	/* protects the queue */
 	spinlock_t irqlock;		/* protects dmaqueue */
 	struct list_head dmaqueue;
 	enum isp_video_dmaqueue_flags dmaqueue_flags;
 
 	const struct isp_video_operations *ops;
-};
-
-#define to_isp_video(vdev)	container_of(vdev, struct isp_video, video)
 
-struct isp_video_fh {
-	struct v4l2_fh vfh;
-	struct isp_video *video;
-	struct vb2_queue queue;
 	struct v4l2_format format;
 	struct v4l2_fract timeperframe;
 };
 
-#define to_isp_video_fh(fh)	container_of(fh, struct isp_video_fh, vfh)
-#define isp_video_queue_to_isp_video_fh(q) \
-				container_of(q, struct isp_video_fh, queue)
+#define to_isp_video(vdev)	container_of(vdev, struct isp_video, video)
 
 int omap3isp_video_init(struct isp_video *video, const char *name);
 void omap3isp_video_cleanup(struct isp_video *video);
-- 
2.1.0

Re: [PATCH 2/3] media: omap3isp: ispvideo: drop driver specific isp_video_fh

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Monday 23 February 2015 20:19:32 Lad Prabhakar wrote:
> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> 
> this patch drops driver specific isp_video_fh, as this
> can be handled by core.

I'm afraid it's not that simple.

The omap3isp driver stores video queues per file handle for a reason. This was 
design to permit creating a high-resolution still image capture queue and 
prepare buffers ahead of time, to avoid the large delay due to cache 
management as prepare time when taking the snapshot.

Now this use case has been partially solved by VIDIOC_CREATE_BUFS, but we're 
still missing a VIDIOC_DESTROY_BUFS to make it work completely. That needs to 
be solved first.

> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> ---
>  drivers/media/platform/omap3isp/ispvideo.c | 128  ++++++++++---------------
>  drivers/media/platform/omap3isp/ispvideo.h |  13 +--
>  2 files changed, 49 insertions(+), 92 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] media: omap3isp: ispvideo: drop driver specific isp_video_fh

[Lad, Prabhakar]

Hi Laurent,

Thanks for the review.

On Tue, Feb 24, 2015 at 12:35 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Monday 23 February 2015 20:19:32 Lad Prabhakar wrote:
>> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
>>
>> this patch drops driver specific isp_video_fh, as this
>> can be handled by core.
>
> I'm afraid it's not that simple.
>
> The omap3isp driver stores video queues per file handle for a reason. This was
> design to permit creating a high-resolution still image capture queue and
> prepare buffers ahead of time, to avoid the large delay due to cache
> management as prepare time when taking the snapshot.
>
Ah I see the reason.

> Now this use case has been partially solved by VIDIOC_CREATE_BUFS, but we're
> still missing a VIDIOC_DESTROY_BUFS to make it work completely. That needs to
> be solved first.
>
I haven't used the VIDIOC_CREATE_BUFS ioctl so far in any of the apps
so cant comment
much on this.
But isn't that obvious we need VIDIOC_DESTROY_BUFS or is there any backdoor
to destroy them that I am missing ?

Regards,
--Prabhakar Lad

>> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>> ---
>>  drivers/media/platform/omap3isp/ispvideo.c | 128  ++++++++++---------------
>>  drivers/media/platform/omap3isp/ispvideo.h |  13 +--
>>  2 files changed, 49 insertions(+), 92 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH 2/3] media: omap3isp: ispvideo: drop driver specific isp_video_fh

[Laurent Pinchart]

Hi Prabhakar,

On Tuesday 24 February 2015 08:04:40 Lad, Prabhakar wrote:
> On Tue, Feb 24, 2015 at 12:35 AM, Laurent Pinchart wrote:
> > On Monday 23 February 2015 20:19:32 Lad Prabhakar wrote:
> >> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> >> 
> >> this patch drops driver specific isp_video_fh, as this
> >> can be handled by core.
> > 
> > I'm afraid it's not that simple.
> > 
> > The omap3isp driver stores video queues per file handle for a reason. This
> > was design to permit creating a high-resolution still image capture queue
> > and prepare buffers ahead of time, to avoid the large delay due to cache
> > management as prepare time when taking the snapshot.
> 
> Ah I see the reason.
> 
> > Now this use case has been partially solved by VIDIOC_CREATE_BUFS, but
> > we're still missing a VIDIOC_DESTROY_BUFS to make it work completely.
> > That needs to be solved first.
> 
> I haven't used the VIDIOC_CREATE_BUFS ioctl so far in any of the apps
> so cant comment much on this.
> But isn't that obvious we need VIDIOC_DESTROY_BUFS or is there any backdoor
> to destroy them that I am missing ?

You can destroy buffers allocated with CREATE_BUFS using REQBUFS(0), but you 
can't destroy them individually without a new ioctl.

> >> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> >> ---
> >> 
> >>  drivers/media/platform/omap3isp/ispvideo.c | 128 +++++++++--------------
> >>  drivers/media/platform/omap3isp/ispvideo.h |  13 +--
> >>  2 files changed, 49 http://vger.kernel.org/majordomo-info.html

-- 
Regards,

Laurent Pinchart

[PATCH 3/3] media: omap3isp: ispvideo: use vb2_fop_mmap/poll

[Lad Prabhakar]

From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>

No need to reinvent the wheel. Just use the already existing
functions provided v4l-core.

Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
 drivers/media/platform/omap3isp/ispvideo.c | 30 ++++--------------------------
 1 file changed, 4 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index b648176..5dd5ffc 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1277,37 +1277,13 @@ static int isp_video_release(struct file *file)
 	return ret;
 }
 
-static unsigned int isp_video_poll(struct file *file, poll_table *wait)
-{
-	struct isp_video *video = video_drvdata(file);
-	int ret;
-
-	mutex_lock(&video->queue_lock);
-	ret = vb2_poll(&video->queue, file, wait);
-	mutex_unlock(&video->queue_lock);
-
-	return ret;
-}
-
-static int isp_video_mmap(struct file *file, struct vm_area_struct *vma)
-{
-	struct isp_video *video = video_drvdata(file);
-	int ret;
-
-	mutex_lock(&video->queue_lock);
-	ret = vb2_mmap(&video->queue, vma);
-	mutex_unlock(&video->queue_lock);
-
-	return ret;
-}
-
 static struct v4l2_file_operations isp_video_fops = {
 	.owner = THIS_MODULE,
 	.unlocked_ioctl = video_ioctl2,
 	.open = isp_video_open,
 	.release = isp_video_release,
-	.poll = isp_video_poll,
-	.mmap = isp_video_mmap,
+	.poll = vb2_fop_poll,
+	.mmap = vb2_fop_mmap,
 };
 
 /* -----------------------------------------------------------------------------
@@ -1389,6 +1365,8 @@ int omap3isp_video_register(struct isp_video *video, struct v4l2_device *vdev)
 
 	video->video.v4l2_dev = vdev;
 
+	/* queue isnt initalized */
+	video->video.queue = &video->queue;
 	ret = video_register_device(&video->video, VFL_TYPE_GRABBER, -1);
 	if (ret < 0)
 		dev_err(video->isp->dev,
-- 
2.1.0

Re: [PATCH 3/3] media: omap3isp: ispvideo: use vb2_fop_mmap/poll

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Monday 23 February 2015 20:19:33 Lad Prabhakar wrote:
> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> 
> No need to reinvent the wheel. Just use the already existing
> functions provided v4l-core.
> 
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> ---
>  drivers/media/platform/omap3isp/ispvideo.c | 30  ++++----------------------
>  1 file changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index b648176..5dd5ffc 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1277,37 +1277,13 @@ static int isp_video_release(struct file *file)
>  	return ret;
>  }
> 
> -static unsigned int isp_video_poll(struct file *file, poll_table *wait)
> -{
> -	struct isp_video *video = video_drvdata(file);
> -	int ret;
> -
> -	mutex_lock(&video->queue_lock);
> -	ret = vb2_poll(&video->queue, file, wait);
> -	mutex_unlock(&video->queue_lock);
> -
> -	return ret;
> -}

This depends on patch 2/3, which can't be accepted as-is for now.

> -static int isp_video_mmap(struct file *file, struct vm_area_struct *vma)
> -{
> -	struct isp_video *video = video_drvdata(file);
> -	int ret;
> -
> -	mutex_lock(&video->queue_lock);
> -	ret = vb2_mmap(&video->queue, vma);
> -	mutex_unlock(&video->queue_lock);
> -
> -	return ret;
> -}

This should be good but has the side effect of removing locking in 
isp_video_mmap(). Now, I think that's the right thing to do, but it should be 
done in a separate patch first with a proper explanation. I can do so, or you 
can submit an additional patch.

>  static struct v4l2_file_operations isp_video_fops = {
>  	.owner = THIS_MODULE,
>  	.unlocked_ioctl = video_ioctl2,
>  	.open = isp_video_open,
>  	.release = isp_video_release,
> -	.poll = isp_video_poll,
> -	.mmap = isp_video_mmap,
> +	.poll = vb2_fop_poll,
> +	.mmap = vb2_fop_mmap,
>  };
> 
>  /* ------------------------------------------------------------------------
>  @@ -1389,6 +1365,8 @@ int omap3isp_video_register(struct isp_video
> *video, struct v4l2_device *vdev)
> 
>  	video->video.v4l2_dev = vdev;
> 
> +	/* queue isnt initalized */
> +	video->video.queue = &video->queue;
>  	ret = video_register_device(&video->video, VFL_TYPE_GRABBER, -1);
>  	if (ret < 0)
>  		dev_err(video->isp->dev,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] media: omap3isp: ispvideo: use vb2_fop_mmap/poll

[Lad, Prabhakar]

Hi Laurent,

Thanks for the review.

On Tue, Feb 24, 2015 at 12:23 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Monday 23 February 2015 20:19:33 Lad Prabhakar wrote:
>> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
>>
>> No need to reinvent the wheel. Just use the already existing
>> functions provided v4l-core.
>>
>> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>> ---
>>  drivers/media/platform/omap3isp/ispvideo.c | 30  ++++----------------------
>>  1 file changed, 4 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
>> b/drivers/media/platform/omap3isp/ispvideo.c index b648176..5dd5ffc 100644
>> --- a/drivers/media/platform/omap3isp/ispvideo.c
>> +++ b/drivers/media/platform/omap3isp/ispvideo.c
>> @@ -1277,37 +1277,13 @@ static int isp_video_release(struct file *file)
>>       return ret;
>>  }
>>
>> -static unsigned int isp_video_poll(struct file *file, poll_table *wait)
>> -{
>> -     struct isp_video *video = video_drvdata(file);
>> -     int ret;
>> -
>> -     mutex_lock(&video->queue_lock);
>> -     ret = vb2_poll(&video->queue, file, wait);
>> -     mutex_unlock(&video->queue_lock);
>> -
>> -     return ret;
>> -}
>
> This depends on patch 2/3, which can't be accepted as-is for now.
>
>> -static int isp_video_mmap(struct file *file, struct vm_area_struct *vma)
>> -{
>> -     struct isp_video *video = video_drvdata(file);
>> -     int ret;
>> -
>> -     mutex_lock(&video->queue_lock);
>> -     ret = vb2_mmap(&video->queue, vma);
>> -     mutex_unlock(&video->queue_lock);
>> -
>> -     return ret;
>> -}
>
> This should be good but has the side effect of removing locking in
> isp_video_mmap(). Now, I think that's the right thing to do, but it should be
> done in a separate patch first with a proper explanation. I can do so, or you
> can submit an additional patch.
>
I am fine you can go ahead posting the patch.

Cheers,
--Prabhakar Lad

>>  static struct v4l2_file_operations isp_video_fops = {
>>       .owner = THIS_MODULE,
>>       .unlocked_ioctl = video_ioctl2,
>>       .open = isp_video_open,
>>       .release = isp_video_release,
>> -     .poll = isp_video_poll,
>> -     .mmap = isp_video_mmap,
>> +     .poll = vb2_fop_poll,
>> +     .mmap = vb2_fop_mmap,
>>  };
>>
>>  /* ------------------------------------------------------------------------
>>  @@ -1389,6 +1365,8 @@ int omap3isp_video_register(struct isp_video
>> *video, struct v4l2_device *vdev)
>>
>>       video->video.v4l2_dev = vdev;
>>
>> +     /* queue isnt initalized */
>> +     video->video.queue = &video->queue;
>>       ret = video_register_device(&video->video, VFL_TYPE_GRABBER, -1);
>>       if (ret < 0)
>>               dev_err(video->isp->dev,
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH 3/3] media: omap3isp: ispvideo: use vb2_fop_mmap/poll

[Laurent Pinchart]

Hi Prabhakar,

On Tuesday 24 February 2015 02:23:26 Laurent Pinchart wrote:
> On Monday 23 February 2015 20:19:33 Lad Prabhakar wrote:
> > From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> > 
> > No need to reinvent the wheel. Just use the already existing
> > functions provided v4l-core.
> > 
> > Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> > ---
> > 
> >  drivers/media/platform/omap3isp/ispvideo.c | 30 ++++---------------------
> >  1 file changed, 4 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> > b/drivers/media/platform/omap3isp/ispvideo.c index b648176..5dd5ffc 100644
> > --- a/drivers/media/platform/omap3isp/ispvideo.c
> > +++ b/drivers/media/platform/omap3isp/ispvideo.c
> > @@ -1277,37 +1277,13 @@ static int isp_video_release(struct file *file)
> >  	return ret;
> >  }
> > 
> > -static unsigned int isp_video_poll(struct file *file, poll_table *wait)
> > -{
> > -	struct isp_video *video = video_drvdata(file);
> > -	int ret;
> > -
> > -	mutex_lock(&video->queue_lock);
> > -	ret = vb2_poll(&video->queue, file, wait);
> > -	mutex_unlock(&video->queue_lock);
> > -
> > -	return ret;
> > -}
> 
> This depends on patch 2/3, which can't be accepted as-is for now.
> 
> > -static int isp_video_mmap(struct file *file, struct vm_area_struct *vma)
> > -{
> > -	struct isp_video *video = video_drvdata(file);
> > -	int ret;
> > -
> > -	mutex_lock(&video->queue_lock);
> > -	ret = vb2_mmap(&video->queue, vma);
> > -	mutex_unlock(&video->queue_lock);
> > -
> > -	return ret;
> > -}
> 
> This should be good but has the side effect of removing locking in
> isp_video_mmap(). Now, I think that's the right thing to do, but it should
> be done in a separate patch first with a proper explanation. I can do so,
> or you can submit an additional patch.

After testing the change I realized it also depends on patch 2/3, as video-
>queue is only set at streamon time at the moment. I'm thus dropping the patch 
from my queue.

> >  static struct v4l2_file_operations isp_video_fops = {
> >  	.owner = THIS_MODULE,
> >  	.unlocked_ioctl = video_ioctl2,
> >  	.open = isp_video_open,
> >  	.release = isp_video_release,
> > -	.poll = isp_video_poll,
> > -	.mmap = isp_video_mmap,
> > +	.poll = vb2_fop_poll,
> > +	.mmap = vb2_fop_mmap,
> >  };
> >  
> >  /* ----------------------------------------------------------------------
> >  @@ -1389,6 +1365,8 @@ int omap3isp_video_register(struct isp_video
> > *video, struct v4l2_device *vdev)
> > 
> >  	video->video.v4l2_dev = vdev;
> > 
> > +	/* queue isnt initalized */
> > +	video->video.queue = &video->queue;
> >  	ret = video_register_device(&video->video, VFL_TYPE_GRABBER, -1);
> >  	if (ret < 0)
> >  		dev_err(video->isp->dev,

-- 
Regards,

Laurent Pinchart