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?
Regards,
Hans
