| Message ID | 218877d9-5f95-42ab-8bbf-2325cb31ed73@xs4all.nl |
|---|---|
| State | New |
| Headers | show |
| Series | media: omap3isp: drop wait_prepare/finish callbacks | expand |
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) { >
On 29/01/2025 15:21, Hans Verkuil wrote: > 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. I still haven't found my Beagle Board. I found the box for the Beagle Board, but not the board itself :-( When I'm back in the Netherlands I'll dig around some more to see if it is there, but if I can't find it, are you or someone else from Ideas on Board willing to test patches from me? This driver is the last remaining user of these wait_prepare/finish helpers, so I'd really like to get this fixed. Regards, Hans > > That will take some time. > > Regards, > > Hans > >> >>> >>> ret = vb2_queue_init(&handle->queue); >>> if (ret < 0) { >> >
On 20/02/2025 08:58, Sakari Ailus wrote: > Hi Hans, > > On Thu, Feb 20, 2025 at 08:48:32AM +0100, Hans Verkuil wrote: >> On 29/01/2025 15:21, Hans Verkuil wrote: >>> 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. >> >> I still haven't found my Beagle Board. I found the box for the Beagle Board, >> but not the board itself :-( >> >> When I'm back in the Netherlands I'll dig around some more to see if it is >> there, but if I can't find it, are you or someone else from Ideas on Board >> willing to test patches from me? >> >> This driver is the last remaining user of these wait_prepare/finish helpers, >> so I'd really like to get this fixed. > > If you have a patch, I can test it, presuming omap3isp will work on N900. > :-) I haven't tested it for a while. > > omap3isp is a bit special as the video buffer queues are specific to file > handles but I'm not sure it matters here. > That shouldn't matter. Thank you for the offer, I'll keep you in reserve if I really can't find my Beagle Board. I've got the freakin' box, so the board must be somewhere, right? Regards, Hans
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(-)