vb2_create_bufs() takes care of setting V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS
flag and vb2_queue max_num_buffers field so call it instead duplicate the
same code in vb2_ioctl_create_bufs(). Testing p->count after calling
vb2_create_bufs() is safe since the function also check it.
Fixes: d055a76c0065 ("media: core: Report the maximum possible number of buffers for the queue")
Signed-off-by: Benjamin Gaignard <[email protected]>
---
.../media/common/videobuf2/videobuf2-v4l2.c | 23 ++++++++++---------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 54d572c3b515..bf304a7234a0 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -1029,23 +1029,24 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
int res = vb2_verify_memory_type(vdev->queue, p->memory,
p->format.type);
- p->index = vdev->queue->num_buffers;
- fill_buf_caps(vdev->queue, &p->capabilities);
- validate_memory_flags(vdev->queue, p->memory, &p->flags);
+ if (vb2_queue_is_busy(vdev->queue, file)) {
+ res = -EBUSY;
+ goto failed;
+ }
+
+ res = vb2_create_bufs(vdev->queue, p);
+ if (res == 0)
+ vdev->queue->owner = file->private_data;
+
+failed:
/*
* If count == 0, then just check if memory and type are valid.
- * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0.
+ * Any -EBUSY result from vb2_verify_memory_type() or
+ * vb2_queue_is_busy() can be mapped to 0.
*/
if (p->count == 0)
return res != -EBUSY ? res : 0;
- if (res)
- return res;
- if (vb2_queue_is_busy(vdev->queue, file))
- return -EBUSY;
- res = vb2_create_bufs(vdev->queue, p);
- if (res == 0)
- vdev->queue->owner = file->private_data;
return res;
}
EXPORT_SYMBOL_GPL(vb2_ioctl_create_bufs);
--
2.40.1
Use vb2_get_num_buffers() to avoid using queue num_buffers field directly.
This allows us to change how the number of buffers is computed in the
future.
Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
Signed-off-by: Benjamin Gaignard <[email protected]>
---
drivers/media/common/videobuf2/videobuf2-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 41a832dd1426..b6bf8f232f48 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -989,7 +989,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
bool no_previous_buffers = !q_num_bufs;
int ret = 0;
- if (q->num_buffers == q->max_num_buffers) {
+ if (q_num_bufs == q->max_num_buffers) {
dprintk(q, 1, "maximum number of buffers already allocated\n");
return -ENOBUFS;
}
--
2.40.1
On 15/01/2024 18:08, Benjamin Gaignard wrote:
> Use vb2_get_num_buffers() to avoid using queue num_buffers field directly.
> This allows us to change how the number of buffers is computed in the
> future.
>
> Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
> Signed-off-by: Benjamin Gaignard <[email protected]>
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 41a832dd1426..b6bf8f232f48 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -989,7 +989,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> bool no_previous_buffers = !q_num_bufs;
> int ret = 0;
>
> - if (q->num_buffers == q->max_num_buffers) {
> + if (q_num_bufs == q->max_num_buffers) {
> dprintk(q, 1, "maximum number of buffers already allocated\n");
> return -ENOBUFS;
> }
There is another case in vb2_ioctl_create_bufs() where num_buffers is accessed directly.
Can you fix that one as well?
Regards,
Hans
Le 16/01/2024 à 10:21, Hans Verkuil a écrit :
> On 15/01/2024 18:08, Benjamin Gaignard wrote:
>> Use vb2_get_num_buffers() to avoid using queue num_buffers field directly.
>> This allows us to change how the number of buffers is computed in the
>> future.
>>
>> Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
>> Signed-off-by: Benjamin Gaignard <[email protected]>
>> ---
>> drivers/media/common/videobuf2/videobuf2-core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 41a832dd1426..b6bf8f232f48 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -989,7 +989,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>> bool no_previous_buffers = !q_num_bufs;
>> int ret = 0;
>>
>> - if (q->num_buffers == q->max_num_buffers) {
>> + if (q_num_bufs == q->max_num_buffers) {
>> dprintk(q, 1, "maximum number of buffers already allocated\n");
>> return -ENOBUFS;
>> }
> There is another case in vb2_ioctl_create_bufs() where num_buffers is accessed directly.
> Can you fix that one as well?
It is removed by the patch I have send just after this one:
"media: core: Refactor vb2_ioctl_create_bufs() to always set capabilities flags"
Regards,
Benjamin
>
> Regards,
>
> Hans
>
On 15/01/2024 18:08, Benjamin Gaignard wrote:
> vb2_create_bufs() takes care of setting V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS
> flag and vb2_queue max_num_buffers field so call it instead duplicate the
> same code in vb2_ioctl_create_bufs(). Testing p->count after calling
> vb2_create_bufs() is safe since the function also check it.
>
> Fixes: d055a76c0065 ("media: core: Report the maximum possible number of buffers for the queue")
> Signed-off-by: Benjamin Gaignard <[email protected]>
> ---
> .../media/common/videobuf2/videobuf2-v4l2.c | 23 ++++++++++---------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 54d572c3b515..bf304a7234a0 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -1029,23 +1029,24 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
> int res = vb2_verify_memory_type(vdev->queue, p->memory,
> p->format.type);
>
> - p->index = vdev->queue->num_buffers;
> - fill_buf_caps(vdev->queue, &p->capabilities);
> - validate_memory_flags(vdev->queue, p->memory, &p->flags);
> + if (vb2_queue_is_busy(vdev->queue, file)) {
> + res = -EBUSY;
> + goto failed;
> + }
> +
> + res = vb2_create_bufs(vdev->queue, p);
> + if (res == 0)
> + vdev->queue->owner = file->private_data;
This would set owner when called with p->count == 0, but that's
not what you want.
I decided to make an attempt at this myself, I'll post a patch.
Regards,
Hans
> +
> +failed:
> /*
> * If count == 0, then just check if memory and type are valid.
> - * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0.
> + * Any -EBUSY result from vb2_verify_memory_type() or
> + * vb2_queue_is_busy() can be mapped to 0.
> */
> if (p->count == 0)
> return res != -EBUSY ? res : 0;
> - if (res)
> - return res;
> - if (vb2_queue_is_busy(vdev->queue, file))
> - return -EBUSY;
>
> - res = vb2_create_bufs(vdev->queue, p);
> - if (res == 0)
> - vdev->queue->owner = file->private_data;
> return res;
> }
> EXPORT_SYMBOL_GPL(vb2_ioctl_create_bufs);
On Tue, Jan 16, 2024 at 6:32 PM Benjamin Gaignard
<[email protected]> wrote:
>
>
> Le 16/01/2024 à 10:21, Hans Verkuil a écrit :
> > On 15/01/2024 18:08, Benjamin Gaignard wrote:
> >> Use vb2_get_num_buffers() to avoid using queue num_buffers field directly.
> >> This allows us to change how the number of buffers is computed in the
> >> future.
> >>
> >> Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
> >> Signed-off-by: Benjamin Gaignard <[email protected]>
> >> ---
> >> drivers/media/common/videobuf2/videobuf2-core.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> >> index 41a832dd1426..b6bf8f232f48 100644
> >> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> >> @@ -989,7 +989,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> >> bool no_previous_buffers = !q_num_bufs;
> >> int ret = 0;
> >>
> >> - if (q->num_buffers == q->max_num_buffers) {
> >> + if (q_num_bufs == q->max_num_buffers) {
> >> dprintk(q, 1, "maximum number of buffers already allocated\n");
> >> return -ENOBUFS;
> >> }
> > There is another case in vb2_ioctl_create_bufs() where num_buffers is accessed directly.
> > Can you fix that one as well?
>
> It is removed by the patch I have send just after this one:
> "media: core: Refactor vb2_ioctl_create_bufs() to always set capabilities flags"
I'd prefer that to be also included in this fix, so that it's all
logically grouped together. Actually Hans also ended up including that
change in his patch, without the commit message mentioning it.
Best regards,
Tomasz
On 1/18/24 12:22, Tomasz Figa wrote:
> On Tue, Jan 16, 2024 at 6:32 PM Benjamin Gaignard
> <[email protected]> wrote:
>>
>>
>> Le 16/01/2024 à 10:21, Hans Verkuil a écrit :
>>> On 15/01/2024 18:08, Benjamin Gaignard wrote:
>>>> Use vb2_get_num_buffers() to avoid using queue num_buffers field directly.
>>>> This allows us to change how the number of buffers is computed in the
>>>> future.
>>>>
>>>> Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
>>>> Signed-off-by: Benjamin Gaignard <[email protected]>
>>>> ---
>>>> drivers/media/common/videobuf2/videobuf2-core.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>>>> index 41a832dd1426..b6bf8f232f48 100644
>>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>>> @@ -989,7 +989,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>>> bool no_previous_buffers = !q_num_bufs;
>>>> int ret = 0;
>>>>
>>>> - if (q->num_buffers == q->max_num_buffers) {
>>>> + if (q_num_bufs == q->max_num_buffers) {
>>>> dprintk(q, 1, "maximum number of buffers already allocated\n");
>>>> return -ENOBUFS;
>>>> }
>>> There is another case in vb2_ioctl_create_bufs() where num_buffers is accessed directly.
>>> Can you fix that one as well?
>>
>> It is removed by the patch I have send just after this one:
>> "media: core: Refactor vb2_ioctl_create_bufs() to always set capabilities flags"
>
> I'd prefer that to be also included in this fix, so that it's all
> logically grouped together. Actually Hans also ended up including that
> change in his patch, without the commit message mentioning it.
Yeah, it's borderline. But I think it is better if this patch updates both
places, and then I'll make a v2 for my patch on top.
Regards,
Hans
>
> Best regards,
> Tomasz