| Message ID | 20220322082859.9834-1-ming.qian@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: amphion: ensure the buffer count is not less than min_buffer | expand |
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@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 > *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:hverkuil-cisco@xs4all.nl] > Sent: Wednesday, April 27, 2022 2:38 PM > To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; > shawnguo@kernel.org > Cc: robh+dt@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de; > festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Aisheng Dong > <aisheng.dong@nxp.com>; linux-media@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-arm-kernel@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@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. 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:hverkuil-cisco@xs4all.nl] >> Sent: Wednesday, April 27, 2022 2:38 PM >> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; >> shawnguo@kernel.org >> Cc: robh+dt@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de; >> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Aisheng Dong >> <aisheng.dong@nxp.com>; linux-media@vger.kernel.org; >> linux-kernel@vger.kernel.org; linux-arm-kernel@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@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) {' 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:hverkuil-cisco@xs4all.nl] > Sent: Wednesday, April 27, 2022 3:25 PM > To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; > shawnguo@kernel.org > Cc: robh+dt@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de; > festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Aisheng Dong > <aisheng.dong@nxp.com>; linux-media@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-arm-kernel@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@xs4all.nl] > >> Sent: Wednesday, April 27, 2022 2:38 PM > >> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; > >> shawnguo@kernel.org > >> Cc: robh+dt@kernel.org; s.hauer@pengutronix.de; > >> kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx > >> <linux-imx@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>; > >> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; > >> linux-arm-kernel@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@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); 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:hverkuil-cisco@xs4all.nl] >> Sent: Wednesday, April 27, 2022 3:25 PM >> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; >> shawnguo@kernel.org >> Cc: robh+dt@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de; >> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Aisheng Dong >> <aisheng.dong@nxp.com>; linux-media@vger.kernel.org; >> linux-kernel@vger.kernel.org; linux-arm-kernel@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@xs4all.nl] >>>> Sent: Wednesday, April 27, 2022 2:38 PM >>>> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; >>>> shawnguo@kernel.org >>>> Cc: robh+dt@kernel.org; s.hauer@pengutronix.de; >>>> kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx >>>> <linux-imx@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>; >>>> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; >>>> linux-arm-kernel@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@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]; >>> >
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); *plane_count = cur_fmt->num_planes; for (i = 0; i < cur_fmt->num_planes; i++) psize[i] = cur_fmt->sizeimage[i];
the output buffer count should >= min_buffer_out the capture buffer count should >= min_buffer_cap Signed-off-by: Ming Qian <ming.qian@nxp.com> --- drivers/media/platform/amphion/vpu_v4l2.c | 4 ++++ 1 file changed, 4 insertions(+)