| Message ID | 20231127165454.166373-4-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Clean up queue_setup()/min_buffers_needed (ab)use | expand |
On 27/11/2023 17:54, Benjamin Gaignard wrote: > 'min_buffers_needed' is suppose to be used to indicate the number > of buffers needed by DMA engine to start streaming. > cx231xx driver doesn't use DMA engine and just want to specify > the minimum number of buffers to allocate when calling VIDIOC_REQBUFS. > That 'min_reqbufs_allocation' field purpose so use it. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > drivers/media/usb/cx231xx/cx231xx-417.c | 2 +- > drivers/media/usb/cx231xx/cx231xx-video.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/usb/cx231xx/cx231xx-417.c b/drivers/media/usb/cx231xx/cx231xx-417.c > index 45973fe690b2..66043ed50c8e 100644 > --- a/drivers/media/usb/cx231xx/cx231xx-417.c > +++ b/drivers/media/usb/cx231xx/cx231xx-417.c > @@ -1782,7 +1782,7 @@ int cx231xx_417_register(struct cx231xx *dev) > q->ops = &cx231xx_video_qops; > q->mem_ops = &vb2_vmalloc_memops; > q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > - q->min_buffers_needed = 1; > + q->min_reqbufs_allocation = 1; There is no point setting min_reqbufs_allocation to 1: you can't allocate less than 1 buffer after all. It is different in that respect from min_buffers_needed: you can call VIDIOC_STREAMON (and thus start_streaming) without any buffers queued. This also suggests a better name for min_buffers_needed: min_queued_buffers So 'min_queued_buffers' buffers have to be queued before start_streaming can be called. The old min_buffers_needed mixed up the two requirements of the minimum number of buffers to allocate in REQBUFS and the minimum number of buffers that need to be queued before you can start streaming. Separating these two meanings should make things easier to understand. The only relationship between the two is that min_reqbufs_allocation > min_queued_buffers, otherwise you would end up in a state where the driver would just cycle buffers and never be able to return a buffer to userspace to process. Regards, Hans > q->lock = &dev->lock; > err = vb2_queue_init(q); > if (err) > diff --git a/drivers/media/usb/cx231xx/cx231xx-video.c b/drivers/media/usb/cx231xx/cx231xx-video.c > index c8eb4222319d..df572c466bfb 100644 > --- a/drivers/media/usb/cx231xx/cx231xx-video.c > +++ b/drivers/media/usb/cx231xx/cx231xx-video.c > @@ -1811,7 +1811,7 @@ int cx231xx_register_analog_devices(struct cx231xx *dev) > q->ops = &cx231xx_video_qops; > q->mem_ops = &vb2_vmalloc_memops; > q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > - q->min_buffers_needed = 1; > + q->min_reqbufs_allocation = 1; > q->lock = &dev->lock; > ret = vb2_queue_init(q); > if (ret) > @@ -1871,7 +1871,7 @@ int cx231xx_register_analog_devices(struct cx231xx *dev) > q->ops = &cx231xx_vbi_qops; > q->mem_ops = &vb2_vmalloc_memops; > q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > - q->min_buffers_needed = 1; > + q->min_reqbufs_allocation = 1; > q->lock = &dev->lock; > ret = vb2_queue_init(q); > if (ret)
Le 28/11/2023 à 11:18, Hans Verkuil a écrit : > On 27/11/2023 17:54, Benjamin Gaignard wrote: >> 'min_buffers_needed' is suppose to be used to indicate the number >> of buffers needed by DMA engine to start streaming. >> cx231xx driver doesn't use DMA engine and just want to specify >> the minimum number of buffers to allocate when calling VIDIOC_REQBUFS. >> That 'min_reqbufs_allocation' field purpose so use it. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> drivers/media/usb/cx231xx/cx231xx-417.c | 2 +- >> drivers/media/usb/cx231xx/cx231xx-video.c | 4 ++-- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/usb/cx231xx/cx231xx-417.c b/drivers/media/usb/cx231xx/cx231xx-417.c >> index 45973fe690b2..66043ed50c8e 100644 >> --- a/drivers/media/usb/cx231xx/cx231xx-417.c >> +++ b/drivers/media/usb/cx231xx/cx231xx-417.c >> @@ -1782,7 +1782,7 @@ int cx231xx_417_register(struct cx231xx *dev) >> q->ops = &cx231xx_video_qops; >> q->mem_ops = &vb2_vmalloc_memops; >> q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; >> - q->min_buffers_needed = 1; >> + q->min_reqbufs_allocation = 1; > There is no point setting min_reqbufs_allocation to 1: you can't allocate > less than 1 buffer after all. Does that mean I should just remove this line ? > > It is different in that respect from min_buffers_needed: you can call > VIDIOC_STREAMON (and thus start_streaming) without any buffers queued. > > This also suggests a better name for min_buffers_needed: min_queued_buffers > > So 'min_queued_buffers' buffers have to be queued before start_streaming can > be called. Ok I will change that in V2 Regards, Benjamin > > The old min_buffers_needed mixed up the two requirements of the minimum > number of buffers to allocate in REQBUFS and the minimum number of buffers > that need to be queued before you can start streaming. Separating these two > meanings should make things easier to understand. > > The only relationship between the two is that min_reqbufs_allocation > > min_queued_buffers, otherwise you would end up in a state where the > driver would just cycle buffers and never be able to return a buffer > to userspace to process. > > Regards, > > Hans > >> q->lock = &dev->lock; >> err = vb2_queue_init(q); >> if (err) >> diff --git a/drivers/media/usb/cx231xx/cx231xx-video.c b/drivers/media/usb/cx231xx/cx231xx-video.c >> index c8eb4222319d..df572c466bfb 100644 >> --- a/drivers/media/usb/cx231xx/cx231xx-video.c >> +++ b/drivers/media/usb/cx231xx/cx231xx-video.c >> @@ -1811,7 +1811,7 @@ int cx231xx_register_analog_devices(struct cx231xx *dev) >> q->ops = &cx231xx_video_qops; >> q->mem_ops = &vb2_vmalloc_memops; >> q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; >> - q->min_buffers_needed = 1; >> + q->min_reqbufs_allocation = 1; >> q->lock = &dev->lock; >> ret = vb2_queue_init(q); >> if (ret) >> @@ -1871,7 +1871,7 @@ int cx231xx_register_analog_devices(struct cx231xx *dev) >> q->ops = &cx231xx_vbi_qops; >> q->mem_ops = &vb2_vmalloc_memops; >> q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; >> - q->min_buffers_needed = 1; >> + q->min_reqbufs_allocation = 1; >> q->lock = &dev->lock; >> ret = vb2_queue_init(q); >> if (ret) >
diff --git a/drivers/media/usb/cx231xx/cx231xx-417.c b/drivers/media/usb/cx231xx/cx231xx-417.c index 45973fe690b2..66043ed50c8e 100644 --- a/drivers/media/usb/cx231xx/cx231xx-417.c +++ b/drivers/media/usb/cx231xx/cx231xx-417.c @@ -1782,7 +1782,7 @@ int cx231xx_417_register(struct cx231xx *dev) q->ops = &cx231xx_video_qops; q->mem_ops = &vb2_vmalloc_memops; q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; - q->min_buffers_needed = 1; + q->min_reqbufs_allocation = 1; q->lock = &dev->lock; err = vb2_queue_init(q); if (err) diff --git a/drivers/media/usb/cx231xx/cx231xx-video.c b/drivers/media/usb/cx231xx/cx231xx-video.c index c8eb4222319d..df572c466bfb 100644 --- a/drivers/media/usb/cx231xx/cx231xx-video.c +++ b/drivers/media/usb/cx231xx/cx231xx-video.c @@ -1811,7 +1811,7 @@ int cx231xx_register_analog_devices(struct cx231xx *dev) q->ops = &cx231xx_video_qops; q->mem_ops = &vb2_vmalloc_memops; q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; - q->min_buffers_needed = 1; + q->min_reqbufs_allocation = 1; q->lock = &dev->lock; ret = vb2_queue_init(q); if (ret) @@ -1871,7 +1871,7 @@ int cx231xx_register_analog_devices(struct cx231xx *dev) q->ops = &cx231xx_vbi_qops; q->mem_ops = &vb2_vmalloc_memops; q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; - q->min_buffers_needed = 1; + q->min_reqbufs_allocation = 1; q->lock = &dev->lock; ret = vb2_queue_init(q); if (ret)
'min_buffers_needed' is suppose to be used to indicate the number of buffers needed by DMA engine to start streaming. cx231xx driver doesn't use DMA engine and just want to specify the minimum number of buffers to allocate when calling VIDIOC_REQBUFS. That 'min_reqbufs_allocation' field purpose so use it. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- drivers/media/usb/cx231xx/cx231xx-417.c | 2 +- drivers/media/usb/cx231xx/cx231xx-video.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)