On 20/02/2025 09:10, 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?
I did find my BeagleBoard, but didn't have time yet to work on this.
Regards,
Hans
>
> Regards,
>
> Hans
>
