diff mbox series

media: omap3isp: drop wait_prepare/finish callbacks

Message ID 218877d9-5f95-42ab-8bbf-2325cb31ed73@xs4all.nl
State New
Headers show
Series media: omap3isp: drop wait_prepare/finish callbacks | expand

Commit Message

Hans Verkuil Jan. 28, 2025, 3:08 p.m. UTC
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@xs4all.nl>
---
 drivers/media/platform/ti/omap3isp/ispvideo.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

Comments

Hans Verkuil Jan. 29, 2025, 2:21 p.m. UTC | #1
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@xs4all.nl>
>> ---
>>  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.

That will take some time.

Regards,

	Hans

> 
>>
>>  	ret = vb2_queue_init(&handle->queue);
>>  	if (ret < 0) {
>
Hans Verkuil Feb. 20, 2025, 7:48 a.m. UTC | #2
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@xs4all.nl>
>>> ---
>>>  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) {
>>
>
Hans Verkuil Feb. 20, 2025, 8:10 a.m. UTC | #3
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@xs4all.nl>
>>>>> ---
>>>>>  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
diff mbox series

Patch

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;

 	ret = vb2_queue_init(&handle->queue);
 	if (ret < 0) {