On 27/04/2022 11:31, Ming Qian wrote: >> From: Hans Verkuil [mailto:hverkuil-cisco@xxxxxxxxx] >> Sent: Wednesday, April 27, 2022 3:25 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: 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@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) >> {' >> > 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]; >>> >
