| Message ID | 218877d9-5f95-42ab-8bbf-2325cb31ed73@xs4all.nl (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: omap3isp: drop wait_prepare/finish callbacks | expand |
Hi Hans, Thank you for the patch. On Tue, Jan 28, 2025 at 04:08:18PM +0100, Hans Verkuil wrote: > Since commit 88785982a19d ("media: vb2: use lock if wait_prepare/finish > are NULL") it is no longer needed to set the wait_prepare/finish > vb2_ops callbacks as long as the lock field in vb2_queue is set. > > Set the queue lock to &video->queue_lock, which makes it possible to drop > the wait_prepare/finish callbacks. > > This simplifies the code and this is a step towards the goal of deleting > these callbacks. > > Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> > --- > drivers/media/platform/ti/omap3isp/ispvideo.c | 19 +------------------ > 1 file changed, 1 insertion(+), 18 deletions(-) > > diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c > index 5c9aa80023fd..78e30298c7ad 100644 > --- a/drivers/media/platform/ti/omap3isp/ispvideo.c > +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c > @@ -480,29 +480,11 @@ static int isp_video_start_streaming(struct vb2_queue *queue, > return 0; > } > > -static void omap3isp_wait_prepare(struct vb2_queue *vq) > -{ > - struct isp_video_fh *vfh = vb2_get_drv_priv(vq); > - struct isp_video *video = vfh->video; > - > - mutex_unlock(&video->queue_lock); > -} > - > -static void omap3isp_wait_finish(struct vb2_queue *vq) > -{ > - struct isp_video_fh *vfh = vb2_get_drv_priv(vq); > - struct isp_video *video = vfh->video; > - > - mutex_lock(&video->queue_lock); > -} > - > static const struct vb2_ops isp_video_queue_ops = { > .queue_setup = isp_video_queue_setup, > .buf_prepare = isp_video_buffer_prepare, > .buf_queue = isp_video_buffer_queue, > .start_streaming = isp_video_start_streaming, > - .wait_prepare = omap3isp_wait_prepare, > - .wait_finish = omap3isp_wait_finish, > }; > > /* > @@ -1338,6 +1320,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; This is goind to deadlock at least when calling VIDIOC_DQBUF in blocking mode. > > ret = vb2_queue_init(&handle->queue); > if (ret < 0) {
On 28/01/2025 16:40, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Tue, Jan 28, 2025 at 04:08:18PM +0100, Hans Verkuil wrote: >> Since commit 88785982a19d ("media: vb2: use lock if wait_prepare/finish >> are NULL") it is no longer needed to set the wait_prepare/finish >> vb2_ops callbacks as long as the lock field in vb2_queue is set. >> >> Set the queue lock to &video->queue_lock, which makes it possible to drop >> the wait_prepare/finish callbacks. >> >> This simplifies the code and this is a step towards the goal of deleting >> these callbacks. >> >> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> >> --- >> drivers/media/platform/ti/omap3isp/ispvideo.c | 19 +------------------ >> 1 file changed, 1 insertion(+), 18 deletions(-) >> >> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c >> index 5c9aa80023fd..78e30298c7ad 100644 >> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c >> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c >> @@ -480,29 +480,11 @@ static int isp_video_start_streaming(struct vb2_queue *queue, >> return 0; >> } >> >> -static void omap3isp_wait_prepare(struct vb2_queue *vq) >> -{ >> - struct isp_video_fh *vfh = vb2_get_drv_priv(vq); >> - struct isp_video *video = vfh->video; >> - >> - mutex_unlock(&video->queue_lock); >> -} >> - >> -static void omap3isp_wait_finish(struct vb2_queue *vq) >> -{ >> - struct isp_video_fh *vfh = vb2_get_drv_priv(vq); >> - struct isp_video *video = vfh->video; >> - >> - mutex_lock(&video->queue_lock); >> -} >> - >> static const struct vb2_ops isp_video_queue_ops = { >> .queue_setup = isp_video_queue_setup, >> .buf_prepare = isp_video_buffer_prepare, >> .buf_queue = isp_video_buffer_queue, >> .start_streaming = isp_video_start_streaming, >> - .wait_prepare = omap3isp_wait_prepare, >> - .wait_finish = omap3isp_wait_finish, >> }; >> >> /* >> @@ -1338,6 +1320,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; > > This is goind to deadlock at least when calling VIDIOC_DQBUF in blocking > mode. Yeah, you are right. I think I will need to test this on real hardware. That will take some time. Regards, Hans > >> >> ret = vb2_queue_init(&handle->queue); >> if (ret < 0) { >
diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c index 5c9aa80023fd..78e30298c7ad 100644 --- a/drivers/media/platform/ti/omap3isp/ispvideo.c +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c @@ -480,29 +480,11 @@ static int isp_video_start_streaming(struct vb2_queue *queue, return 0; } -static void omap3isp_wait_prepare(struct vb2_queue *vq) -{ - struct isp_video_fh *vfh = vb2_get_drv_priv(vq); - struct isp_video *video = vfh->video; - - mutex_unlock(&video->queue_lock); -} - -static void omap3isp_wait_finish(struct vb2_queue *vq) -{ - struct isp_video_fh *vfh = vb2_get_drv_priv(vq); - struct isp_video *video = vfh->video; - - mutex_lock(&video->queue_lock); -} - static const struct vb2_ops isp_video_queue_ops = { .queue_setup = isp_video_queue_setup, .buf_prepare = isp_video_buffer_prepare, .buf_queue = isp_video_buffer_queue, .start_streaming = isp_video_start_streaming, - .wait_prepare = omap3isp_wait_prepare, - .wait_finish = omap3isp_wait_finish, }; /* @@ -1338,6 +1320,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) {
Since commit 88785982a19d ("media: vb2: use lock if wait_prepare/finish are NULL") it is no longer needed to set the wait_prepare/finish vb2_ops callbacks as long as the lock field in vb2_queue is set. Set the queue lock to &video->queue_lock, which makes it possible to drop the wait_prepare/finish callbacks. This simplifies the code and this is a step towards the goal of deleting these callbacks. Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> --- drivers/media/platform/ti/omap3isp/ispvideo.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)