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.
Regards,
Hans
>
> That will take some time.
>
> Regards,
>
> Hans
>
>>
>>>
>>> ret = vb2_queue_init(&handle->queue);
>>> if (ret < 0) {
>>
>
