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@xxxxxxxxx>
> >>> ---
> >>> 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.
--
Regards,
Sakari Ailus
