[EXT] Re: [PATCH] media: amphion: ensure the buffer count is not less than min_buffer

[Hans Verkuil]

On 27/04/2022 11:31, Ming Qian wrote:
>> From: Hans Verkuil [mailto:hverkuil-cisco at xs4all.nl]
>> Sent: Wednesday, April 27, 2022 3:25 PM
>> To: Ming Qian <ming.qian at nxp.com>; mchehab at kernel.org;
>> shawnguo at kernel.org
>> Cc: robh+dt at kernel.org; s.hauer at pengutronix.de; kernel at pengutronix.de;
>> festevam at gmail.com; dl-linux-imx <linux-imx at nxp.com>; Aisheng Dong
>> <aisheng.dong at nxp.com>; linux-media at vger.kernel.org;
>> linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org
>> Subject: Re: [EXT] Re: [PATCH] media: amphion: ensure the buffer count is not
>> less than min_buffer
>>
>> Caution: EXT Email
>>
>> On 27/04/2022 09:01, Ming Qian wrote:
>>>> From: Hans Verkuil [mailto:hverkuil-cisco at xs4all.nl]
>>>> Sent: Wednesday, April 27, 2022 2:38 PM
>>>> To: Ming Qian <ming.qian at nxp.com>; mchehab at kernel.org;
>>>> shawnguo at kernel.org
>>>> Cc: robh+dt at kernel.org; s.hauer at pengutronix.de;
>>>> kernel at pengutronix.de; festevam at gmail.com; dl-linux-imx
>>>> <linux-imx at nxp.com>; Aisheng Dong <aisheng.dong at nxp.com>;
>>>> linux-media at vger.kernel.org; linux-kernel at vger.kernel.org;
>>>> linux-arm-kernel at lists.infradead.org
>>>> Subject: [EXT] Re: [PATCH] media: amphion: ensure the buffer count is
>>>> not less than min_buffer
>>>>
>>>> Caution: EXT Email
>>>>
>>>> Hi Ming Qian,
>>>>
>>>> On 22/03/2022 09:28, Ming Qian wrote:
>>>>> the output buffer count should >= min_buffer_out the capture buffer
>>>>> count should >= min_buffer_cap
>>>>>
>>>>> Signed-off-by: Ming Qian <ming.qian at nxp.com>
>>>>> ---
>>>>>  drivers/media/platform/amphion/vpu_v4l2.c | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/platform/amphion/vpu_v4l2.c
>>>>> b/drivers/media/platform/amphion/vpu_v4l2.c
>>>>> index cbf3315605a9..72a0544f4da3 100644
>>>>> --- a/drivers/media/platform/amphion/vpu_v4l2.c
>>>>> +++ b/drivers/media/platform/amphion/vpu_v4l2.c
>>>>> @@ -355,6 +355,10 @@ static int vpu_vb2_queue_setup(struct
>> vb2_queue
>>>> *vq,
>>>>>               return 0;
>>>>>       }
>>>>>
>>>>> +     if (V4L2_TYPE_IS_OUTPUT(vq->type))
>>>>> +             *buf_count = max_t(unsigned int, *buf_count,
>>>> inst->min_buffer_out);
>>>>> +     else
>>>>> +             *buf_count = max_t(unsigned int, *buf_count,
>>>>> + inst->min_buffer_cap);
>>>>
>>>> I noticed that min_buffer_out/cap is set to 2, but min_buffers_needed
>>>> is set to 1. Wouldn't it make more sense to set min_buffers_needed to
>>>> 2 as well?
>>>>
>>>> If you do that, then the vb2 core will already take care of ensuring
>>>> that the buf_count is adjusted.
>>>>
>>>> If you *do* have to do this manually, then you need to place the
>>>> whole if-else under 'if (!*num_planes) {', otherwise it will mess up
>>>> the VIDIOC_CREATE_BUFS ioctl. See the queue_setup in
>>>> include/media/videobuf2-core.h documentation for the sordid details.
>>>>
>>>> Regards,
>>>>
>>>>         Hans
>>>>
>>>
>>> Hi Hans,
>>>     I want to make the vpu start when 1 frames is queued, so I set the
>> min_buffers_needed to 1.
>>> And the min_buffer_cap may be changed when a source change event is
>> triggered. So in most case, it will be larger than 2.
>>
>> Ah, I only grepped for min_buffer_out, not _cap, so I missed that that one isn't
>> constant.
>>
>>>     I'll make a v2 patch that place the whole if-else under 'if (!*num_planes)
>> {'
>>
> Hi Hans,
>     I send a v2 patch. 
> 	But I think the v1 is OK, as the full code has already guaranteed the condition ` if (!*num_planes)`,
> 
> 	if (*plane_count) {
> 		... ...
> 		return 0;
>     }
>  	if (V4L2_TYPE_IS_OUTPUT(vq->type))
> 		*buf_count = max_t(unsigned int, *buf_count, inst->min_buffer_out);
> 	else
> 		*buf_count = max_t(unsigned int, *buf_count, inst->min_buffer_cap);
> 

You are correct, it wasn't visible in the patch that the *buf_count adjustments
only happened if *plane_count == 0.

I'm going back to v1.

Sorry for the confusion!

	Hans

> Ming
> 
>> Great, thank you!
>>
>>         Hans
>>
>>>     Thanks for your reminder
>>>
>>> Ming
>>>
>>>>>       *plane_count = cur_fmt->num_planes;
>>>>>       for (i = 0; i < cur_fmt->num_planes; i++)
>>>>>               psize[i] = cur_fmt->sizeimage[i];
>>>
>