Hi Hans,
On Thu, Feb 20, 2025 at 09:10:39AM +0100, Hans Verkuil wrote:
> 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@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.
> >
>
> 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?
Absolutely! :-)
Btw. if you happen to have free time, reworking the Media device lifetime
management set to your liking could be an option to solve that problem. :-)
With the Rust support knocking, we should really get the lifetime
management issues resolved. Still that set is just the beginning of it,
much more work will be needed.
--
Regards,
Sakari Ailus
