| Message ID | 20231211133251.150999-1-benjamin.gaignard@collabora.com |
|---|---|
| Headers | show |
| Series | Clean up min_buffers_needed misusages | expand |
Hi Benjamin, On 11/12/2023 14:32, Benjamin Gaignard wrote: > Rename min_buffers_needed into min_queued_buffers and update > the documentation about it. I merged this patch, but not the others. I also dropped one functional change: <snip> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 40d89f29fa33..8912dff5bde3 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -865,7 +865,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > /* > * Make sure the requested values and current defaults are sane. > */ > - num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); > + num_buffers = max_t(unsigned int, *count, q->min_queued_buffers + 1); > num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers); > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > /* That "+ 1" didn't really belong here, since everything else was just renaming a field. Such a patch shouldn't make any other changes. There were also three more occurrences of min_buffers_needed (one in a comment, two in a vivid function argument), and I renamed those as well. 'git grep min_buffers_needed' now no longer shows any hits. I decided not to take the other patches, I think it is best if you rebase and repost the series on top of staging and in the new year we'll continue with it. I did not feel that I had enough time to really review the remaining patches. However, it is nice to have this large rename patch merged. It touches on a lot of files, so it is annoying to have to carry that around. And now was a good moment to merge it. Regards, Hans
Le 13/12/2023 à 17:39, Hans Verkuil a écrit : > Hi Benjamin, > > On 11/12/2023 14:32, Benjamin Gaignard wrote: >> Rename min_buffers_needed into min_queued_buffers and update >> the documentation about it. > I merged this patch, but not the others. I also dropped one functional > change: > > <snip> > >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >> index 40d89f29fa33..8912dff5bde3 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-core.c >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >> @@ -865,7 +865,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >> /* >> * Make sure the requested values and current defaults are sane. >> */ >> - num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); >> + num_buffers = max_t(unsigned int, *count, q->min_queued_buffers + 1); >> num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers); >> memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); >> /* > That "+ 1" didn't really belong here, since everything else was just renaming a > field. Such a patch shouldn't make any other changes. > > There were also three more occurrences of min_buffers_needed (one in a comment, > two in a vivid function argument), and I renamed those as well. > > 'git grep min_buffers_needed' now no longer shows any hits. > > I decided not to take the other patches, I think it is best if you rebase > and repost the series on top of staging and in the new year we'll continue with > it. I did not feel that I had enough time to really review the remaining patches. Do you want me to re-post only the two missing patches or should I add the patches for delete buffers feature since it is the ultimate goal of this ? Regards, Benjamin > > However, it is nice to have this large rename patch merged. It touches on a lot > of files, so it is annoying to have to carry that around. And now was a good > moment to merge it. > > Regards, > > Hans >
Hi Benjamin, On Mon, Dec 11, 2023 at 02:32:49PM +0100, Benjamin Gaignard wrote: > Rename min_buffers_needed into min_queued_buffers and update > the documentation about it. > Sorry for being late to the party. I think this is generally a good idea, thanks for doing this! Just one comment inline. [snip] > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 40d89f29fa33..8912dff5bde3 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -865,7 +865,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > /* > * Make sure the requested values and current defaults are sane. > */ > - num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); > + num_buffers = max_t(unsigned int, *count, q->min_queued_buffers + 1); Where does this + 1 come from here? Agreed with Hans that this change deserves its own patch with a proper explanation in its description. > num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers); > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > /* > @@ -917,7 +917,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > * There is no point in continuing if we can't allocate the minimum > * number of buffers needed by this vb2_queue. > */ > - if (allocated_buffers < q->min_buffers_needed) > + if (allocated_buffers < q->min_queued_buffers) > ret = -ENOMEM; > > /* > @@ -1653,7 +1653,7 @@ EXPORT_SYMBOL_GPL(vb2_core_prepare_buf); > * @q: videobuf2 queue > * > * Attempt to start streaming. When this function is called there must be > - * at least q->min_buffers_needed buffers queued up (i.e. the minimum > + * at least q->min_queued_buffers queued up (i.e. the minimum > * number of buffers required for the DMA engine to function). If the > * @start_streaming op fails it is supposed to return all the driver-owned > * buffers back to vb2 in state QUEUED. Check if that happened and if > @@ -1846,7 +1846,7 @@ int vb2_core_qbuf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb, > * then we can finally call start_streaming(). > */ > if (q->streaming && !q->start_streaming_called && > - q->queued_count >= q->min_buffers_needed) { > + q->queued_count >= q->min_queued_buffers) { > ret = vb2_start_streaming(q); > if (ret) { > /* > @@ -2210,9 +2210,9 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type) > return -EINVAL; > } > > - if (q_num_bufs < q->min_buffers_needed) { > - dprintk(q, 1, "need at least %u allocated buffers\n", > - q->min_buffers_needed); > + if (q_num_bufs < q->min_queued_buffers) { > + dprintk(q, 1, "need at least %u queued buffers\n", Note that the value being checked here is the number of allocated buffers, not queued buffers. So basically we're enforcing here that at the time STREAMON is called, there is enough buffers allocated to queue the minimum number of buffers to start streaming, but then later down we're not actually enforcing that they are queued - if not, the queue start_streaming operation is deferred until enough buffers are queued. The question is: Do we really want this to be an error? Or should we just be consistent with the allocated-but-not-queued case and let the application allocate more buffers later using CREATE_BUFS? (Another question: How does an application know how many buffers to allocate for STREAMON to work?) That said, this doesn't really affect the correctness of the patch itself. Just some additional oddity in the current implementation that we could simplify in the future. > + q->min_queued_buffers); > return -EINVAL; > } > [snip] > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 5557d78b6f20..f9a00b6c3c46 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -546,10 +546,13 @@ struct vb2_buf_ops { > * @gfp_flags: additional gfp flags used when allocating the buffers. > * Typically this is 0, but it may be e.g. %GFP_DMA or %__GFP_DMA32 > * to force the buffer allocation to a specific memory zone. > - * @min_buffers_needed: the minimum number of buffers needed before > + * @min_queued_buffers: the minimum number of queued buffers needed before > * @start_streaming can be called. Used when a DMA engine > * cannot be started unless at least this number of buffers > * have been queued into the driver. > + * VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1 > + * buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not > + * modify the requested buffer count. Same here, this + 1 needs a proper explanation. Also, I don't like this API inconsistency where REQBUFS guarantees the right number of buffers, but CREATE_BUFS doesn't. In fact, would an application have a way to know how many buffers to allocate if it simply wants to use CREATE_BUFS? (It's generally related to the oddity that I pointed out above. Maybe we should let the allocation code only handle allocation constraints and not care about STREAMON constraints?) [snip] Best regards, Tomasz
Le 26/12/2023 à 09:23, Tomasz Figa a écrit : > Hi Benjamin, > > On Mon, Dec 11, 2023 at 02:32:49PM +0100, Benjamin Gaignard wrote: >> Rename min_buffers_needed into min_queued_buffers and update >> the documentation about it. >> > Sorry for being late to the party. I think this is generally a good idea, > thanks for doing this! Just one comment inline. > > [snip] >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >> index 40d89f29fa33..8912dff5bde3 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-core.c >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >> @@ -865,7 +865,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >> /* >> * Make sure the requested values and current defaults are sane. >> */ >> - num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); >> + num_buffers = max_t(unsigned int, *count, q->min_queued_buffers + 1); > Where does this + 1 come from here? > Agreed with Hans that this change deserves its own patch with a proper > explanation in its description. Hans have merged this patch without this line. Since I still aiming to add delete buffers feature I have include this change in this series: https://www.spinics.net/lists/linux-media/msg246289.html Regards, Benjamin > >> num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers); >> memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); >> /* >> @@ -917,7 +917,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >> * There is no point in continuing if we can't allocate the minimum >> * number of buffers needed by this vb2_queue. >> */ >> - if (allocated_buffers < q->min_buffers_needed) >> + if (allocated_buffers < q->min_queued_buffers) >> ret = -ENOMEM; >> >> /* >> @@ -1653,7 +1653,7 @@ EXPORT_SYMBOL_GPL(vb2_core_prepare_buf); >> * @q: videobuf2 queue >> * >> * Attempt to start streaming. When this function is called there must be >> - * at least q->min_buffers_needed buffers queued up (i.e. the minimum >> + * at least q->min_queued_buffers queued up (i.e. the minimum >> * number of buffers required for the DMA engine to function). If the >> * @start_streaming op fails it is supposed to return all the driver-owned >> * buffers back to vb2 in state QUEUED. Check if that happened and if >> @@ -1846,7 +1846,7 @@ int vb2_core_qbuf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb, >> * then we can finally call start_streaming(). >> */ >> if (q->streaming && !q->start_streaming_called && >> - q->queued_count >= q->min_buffers_needed) { >> + q->queued_count >= q->min_queued_buffers) { >> ret = vb2_start_streaming(q); >> if (ret) { >> /* >> @@ -2210,9 +2210,9 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type) >> return -EINVAL; >> } >> >> - if (q_num_bufs < q->min_buffers_needed) { >> - dprintk(q, 1, "need at least %u allocated buffers\n", >> - q->min_buffers_needed); >> + if (q_num_bufs < q->min_queued_buffers) { >> + dprintk(q, 1, "need at least %u queued buffers\n", > Note that the value being checked here is the number of allocated buffers, > not queued buffers. So basically we're enforcing here that at the time > STREAMON is called, there is enough buffers allocated to queue the minimum > number of buffers to start streaming, but then later down we're not > actually enforcing that they are queued - if not, the queue start_streaming > operation is deferred until enough buffers are queued. > > The question is: Do we really want this to be an error? Or should we just > be consistent with the allocated-but-not-queued case and let the > application allocate more buffers later using CREATE_BUFS? > (Another question: How does an application know how many buffers to > allocate for STREAMON to work?) > > That said, this doesn't really affect the correctness of the patch itself. > Just some additional oddity in the current implementation that we could > simplify in the future. > >> + q->min_queued_buffers); >> return -EINVAL; >> } >> > [snip] >> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h >> index 5557d78b6f20..f9a00b6c3c46 100644 >> --- a/include/media/videobuf2-core.h >> +++ b/include/media/videobuf2-core.h >> @@ -546,10 +546,13 @@ struct vb2_buf_ops { >> * @gfp_flags: additional gfp flags used when allocating the buffers. >> * Typically this is 0, but it may be e.g. %GFP_DMA or %__GFP_DMA32 >> * to force the buffer allocation to a specific memory zone. >> - * @min_buffers_needed: the minimum number of buffers needed before >> + * @min_queued_buffers: the minimum number of queued buffers needed before >> * @start_streaming can be called. Used when a DMA engine >> * cannot be started unless at least this number of buffers >> * have been queued into the driver. >> + * VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1 >> + * buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not >> + * modify the requested buffer count. > Same here, this + 1 needs a proper explanation. > Also, I don't like this API inconsistency where REQBUFS guarantees the > right number of buffers, but CREATE_BUFS doesn't. In fact, would an > application have a way to know how many buffers to allocate if it simply > wants to use CREATE_BUFS? > > (It's generally related to the oddity that I pointed out above. Maybe we > should let the allocation code only handle allocation constraints and not > care about STREAMON constraints?) > > [snip] > > Best regards, > Tomasz