On Mon, 2018-07-02 at 19:49 +0300, Laurent Pinchart wrote:
> 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 <ezequ...@collabora.com>
> > ---
> > 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.
>
AFAICS, vb2_discard_done() simply iterates over a list, and it's
protected by a spinlock. It's already thread-safe.
> > 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 ?
>
Hm, I don't remember getting warnings.
> > - 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 ?
>
Yes, I can see that now. It will only cause problems if the subdev
calls, then try to take the queue lock, either directly or by re-
entering into the driver.
I don't have access to this hardware, so I do not have other answers.
> > 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.
>
>
Right.
