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

[Hans Verkuil]

On 27/04/2022 09:01, Ming Qian wrote:
>> From: Hans Verkuil [mailto:hverkuil-cisco@xxxxxxxxx]
>> Sent: Wednesday, April 27, 2022 2:38 PM
>> To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx;
>> shawnguo@xxxxxxxxxx
>> Cc: robh+dt@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
>> festevam@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Aisheng Dong
>> <aisheng.dong@xxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
>> linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> 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@xxxxxxx>
>>> ---
>>>  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) {'

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];
>