> From: Hans Verkuil [mailto:[email protected]]
> Sent: Wednesday, April 27, 2022 2:38 PM
> To: Ming Qian <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>; Aisheng Dong
> <[email protected]>; [email protected];
> [email protected]; [email protected]
> 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 <[email protected]>
> > ---
> > 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.
I'll make a v2 patch that place the whole if-else under 'if (!*num_planes) {'
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];
On 27/04/2022 09:01, Ming Qian wrote:
>> From: Hans Verkuil [mailto:[email protected]]
>> Sent: Wednesday, April 27, 2022 2:38 PM
>> To: Ming Qian <[email protected]>; [email protected];
>> [email protected]
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; dl-linux-imx <[email protected]>; Aisheng Dong
>> <[email protected]>; [email protected];
>> [email protected]; [email protected]
>> 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 <[email protected]>
>>> ---
>>> 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];
>
> From: Hans Verkuil [mailto:[email protected]]
> Sent: Wednesday, April 27, 2022 3:25 PM
> To: Ming Qian <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>; Aisheng Dong
> <[email protected]>; [email protected];
> [email protected]; [email protected]
> 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:[email protected]]
> >> Sent: Wednesday, April 27, 2022 2:38 PM
> >> To: Ming Qian <[email protected]>; [email protected];
> >> [email protected]
> >> Cc: [email protected]; [email protected];
> >> [email protected]; [email protected]; dl-linux-imx
> >> <[email protected]>; Aisheng Dong <[email protected]>;
> >> [email protected]; [email protected];
> >> [email protected]
> >> 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 <[email protected]>
> >>> ---
> >>> 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);
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];
> >
On 27/04/2022 11:31, Ming Qian wrote:
>> From: Hans Verkuil [mailto:[email protected]]
>> Sent: Wednesday, April 27, 2022 3:25 PM
>> To: Ming Qian <[email protected]>; [email protected];
>> [email protected]
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; dl-linux-imx <[email protected]>; Aisheng Dong
>> <[email protected]>; [email protected];
>> [email protected]; [email protected]
>> 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:[email protected]]
>>>> Sent: Wednesday, April 27, 2022 2:38 PM
>>>> To: Ming Qian <[email protected]>; [email protected];
>>>> [email protected]
>>>> Cc: [email protected]; [email protected];
>>>> [email protected]; [email protected]; dl-linux-imx
>>>> <[email protected]>; Aisheng Dong <[email protected]>;
>>>> [email protected]; [email protected];
>>>> [email protected]
>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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];
>>>
>