On Thu, Feb 20, 2025 at 10:40:54AM +0000, Sakari Ailus wrote:
> 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.
I asked in
https://lore.kernel.org/ksummit/20250219150553.GD15114@xxxxxxxxxxxxxxxxxxxxxxxxxx/
if any R4L (rust for Linux) developers could help with the C side, and
offered to learn enough rust to review rust adaptation patches for V4L2
in return. That would be one concrete way to build trust and show that
rust in the kernel would help reducing maintainer overload.
--
Regards,
Laurent Pinchart
