Hi Ezequiel,
(CC'ing Sakari)
Thank you for the patch.
On Friday, 22 June 2018 06:53:58 EEST Ezequiel Garcia wrote:
> vb2_queue locks is now mandatory. Add it, remove driver ad-hoc locks,
> and implement wait_{prepare, finish}.
>
> Also, remove stream_lock mutex. Since the ioctls operations
> are now protected by the queue mutex, the stream_lock mutex is
> not needed.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> ---
> drivers/media/platform/omap3isp/ispvideo.c | 65 ++++------------------
> drivers/media/platform/omap3isp/ispvideo.h | 1 -
> 2 files changed, 11 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index
> 9d228eac24ea..f835aeb9ddc5 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -496,6 +496,8 @@ static const struct vb2_ops isp_video_queue_ops = {
> .buf_prepare = isp_video_buffer_prepare,
> .buf_queue = isp_video_buffer_queue,
> .start_streaming = isp_video_start_streaming,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> };
>
> /*
> @@ -628,11 +630,8 @@ void omap3isp_video_resume(struct isp_video *video, int
> continuous) {
> struct isp_buffer *buf = NULL;
>
> - if (continuous && video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> - mutex_lock(&video->queue_lock);
> + if (continuous && video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> vb2_discard_done(video->queue);
> - mutex_unlock(&video->queue_lock);
> - }
Why can locking be removed here ? As far as I know the lock isn't taken by the
caller. It would help to explain the rationale in the commit message.
> if (!list_empty(&video->dmaqueue)) {
> buf = list_first_entry(&video->dmaqueue,
> @@ -909,13 +908,8 @@ 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);
The video variable isn't used anymore. Didn't gcc warn you ?
> - int ret;
> -
> - mutex_lock(&video->queue_lock);
> - ret = vb2_reqbufs(&vfh->queue, rb);
> - mutex_unlock(&video->queue_lock);
>
> - return ret;
> + return vb2_reqbufs(&vfh->queue, rb);
> }
>
> static int
> @@ -923,13 +917,8 @@ 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);
Same here and in other functions below.
> - int ret;
>
> - mutex_lock(&video->queue_lock);
> - ret = vb2_querybuf(&vfh->queue, b);
> - mutex_unlock(&video->queue_lock);
> -
> - return ret;
> + return vb2_querybuf(&vfh->queue, b);
> }
>
> static int
> @@ -937,13 +926,8 @@ 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);
> - mutex_unlock(&video->queue_lock);
> -
> - return ret;
> + return vb2_qbuf(&vfh->queue, b);
> }
>
> static int
> @@ -951,13 +935,8 @@ 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);
> - mutex_unlock(&video->queue_lock);
> -
> - return ret;
> + return vb2_dqbuf(&vfh->queue, b, file->f_flags & O_NONBLOCK);
> }
>
> static int isp_video_check_external_subdevs(struct isp_video *video,
> @@ -1096,8 +1075,6 @@ isp_video_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type) if (type != video->type)
> return -EINVAL;
>
> - mutex_lock(&video->stream_lock);
> -
> /* Start streaming on the pipeline. No link touching an entity in the
> * pipeline can be activated or deactivated once streaming is started.
> */
> @@ -1106,7 +1083,7 @@ isp_video_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type)
>
> ret = media_entity_enum_init(&pipe->ent_enum, &video->isp->media_dev);
> if (ret)
> - goto err_enum_init;
> + return ret;
>
> /* TODO: Implement PM QoS */
> pipe->l3_ick = clk_get_rate(video->isp->clock[ISP_CLK_L3_ICK]);
> @@ -1158,14 +1135,10 @@ isp_video_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type) atomic_set(&pipe->frame_number, -1);
> pipe->field = vfh->format.fmt.pix.field;
>
> - mutex_lock(&video->queue_lock);
> ret = vb2_streamon(&vfh->queue, type);
> - mutex_unlock(&video->queue_lock);
> if (ret < 0)
> goto err_check_format;
>
> - mutex_unlock(&video->stream_lock);
> -
> return 0;
>
> err_check_format:
> @@ -1184,9 +1157,6 @@ isp_video_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type)
>
> media_entity_enum_cleanup(&pipe->ent_enum);
>
> -err_enum_init:
> - mutex_unlock(&video->stream_lock);
> -
> return ret;
> }
>
> @@ -1203,15 +1173,10 @@ isp_video_streamoff(struct file *file, void *fh,
> enum v4l2_buf_type type) if (type != video->type)
> return -EINVAL;
>
> - mutex_lock(&video->stream_lock);
> -
> /* Make sure we're not streaming yet. */
> - mutex_lock(&video->queue_lock);
> streaming = vb2_is_streaming(&vfh->queue);
> - mutex_unlock(&video->queue_lock);
> -
> if (!streaming)
> - goto done;
> + return 0;
>
> /* Update the pipeline state. */
> if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> @@ -1229,19 +1194,13 @@ isp_video_streamoff(struct file *file, void *fh,
> enum v4l2_buf_type type)
> omap3isp_pipeline_set_stream(pipe, ISP_PIPELINE_STREAM_STOPPED);
With this patch, the omap3isp_pipeline_set_stream() function will wait for the
pipeline to stop with the queue lock held. This means that all concurrent
access to buffer-related functions (and in particular DQBUF and and poll())
will potentially wait for much longer than they do now. Could this be a
problem ? Could waiting for the pipeline to stop with the lock held also cause
other problems than that ?
> omap3isp_video_cancel_stream(video);
>
> - mutex_lock(&video->queue_lock);
> vb2_streamoff(&vfh->queue, type);
> - mutex_unlock(&video->queue_lock);
> video->queue = NULL;
> video->error = false;
>
> /* TODO: Implement PM QoS */
> media_pipeline_stop(&video->video.entity);
> -
> media_entity_enum_cleanup(&pipe->ent_enum);
> -
> -done:
> - mutex_unlock(&video->stream_lock);
> return 0;
> }
>
> @@ -1333,6 +1292,7 @@ static int isp_video_open(struct file *file)
> queue->buf_struct_size = sizeof(struct isp_buffer);
> queue->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> queue->dev = video->isp->dev;
> + queue->lock = &video->queue_lock;
>
> ret = vb2_queue_init(&handle->queue);
> if (ret < 0) {
> @@ -1363,10 +1323,9 @@ static int isp_video_release(struct file *file)
> struct v4l2_fh *vfh = file->private_data;
> struct isp_video_fh *handle = to_isp_video_fh(vfh);
>
> + mutex_lock(&video->queue_lock);
> /* Disable streaming and free the buffers queue resources. */
> isp_video_streamoff(file, vfh, video->type);
> -
> - mutex_lock(&video->queue_lock);
> vb2_queue_release(&handle->queue);
> mutex_unlock(&video->queue_lock);
>
> @@ -1449,7 +1408,6 @@ int omap3isp_video_init(struct isp_video *video, const
> char *name) atomic_set(&video->active, 0);
>
> spin_lock_init(&video->pipe.lock);
> - mutex_init(&video->stream_lock);
> mutex_init(&video->queue_lock);
> spin_lock_init(&video->irqlock);
>
> @@ -1474,7 +1432,6 @@ void omap3isp_video_cleanup(struct isp_video *video)
> {
> media_entity_cleanup(&video->video.entity);
> mutex_destroy(&video->queue_lock);
> - mutex_destroy(&video->stream_lock);
> mutex_destroy(&video->mutex);
> }
>
> diff --git a/drivers/media/platform/omap3isp/ispvideo.h
> b/drivers/media/platform/omap3isp/ispvideo.h index
> f6a2082b4a0a..5a8fba85e0eb 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.h
> +++ b/drivers/media/platform/omap3isp/ispvideo.h
> @@ -167,7 +167,6 @@ struct isp_video {
>
> /* Pipeline state */
> struct isp_pipeline pipe;
> - struct mutex stream_lock; /* pipeline and stream states */
The queue_lock now covers more than just the queue, please update the comment
to list the fields that are protected by the mutex.
> bool error;
>
> /* Video buffers queue */
--
Regards,
Laurent Pinchart
