On Thu, Jan 18, 2024 at 8:47 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>
> On 1/18/24 12:14, Tomasz Figa wrote:
> > Hi Hans,
> >
> > On Tue, Jan 16, 2024 at 11:05 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
> >>
> >> Several functions implementing VIDIOC_REQBUFS and _CREATE_BUFS all use almost
> >> the same code to fill in the flags and capability fields. Refactor this into a
> >> new vb2_set_flags_and_caps() function that replaces the old fill_buf_caps()
> >> and validate_memory_flags() functions.
> >>
> >> This also fixes a bug where vb2_ioctl_create_bufs() would not set the
> >> V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS cap and also not fill in the
> >> max_num_buffers field.
> >>
> >> Fixes: d055a76c0065 ("media: core: Report the maximum possible number of buffers for the queue")
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> >> ---
> >> .../media/common/videobuf2/videobuf2-v4l2.c | 55 +++++++++----------
> >> 1 file changed, 26 insertions(+), 29 deletions(-)
> >>
> >
> > Thanks for the patch! Please check my comments inline.
> >
> >> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> >> index 54d572c3b515..c575198e8354 100644
> >> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> >> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> >> @@ -671,8 +671,20 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >> }
> >> EXPORT_SYMBOL(vb2_querybuf);
> >>
> >> -static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
> >> +static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
> >> + u32 *flags, u32 *caps, u32 *max_num_bufs)
> >> {
> >> + if (!q->allow_cache_hints || memory != V4L2_MEMORY_MMAP) {
> >> + /*
> >> + * This needs to clear V4L2_MEMORY_FLAG_NON_COHERENT only,
> >> + * but in order to avoid bugs we zero out all bits.
> >> + */
> >> + *flags = 0;
> >> + } else {
> >> + /* Clear all unknown flags. */
> >> + *flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
> >> + }
> >> +
> >> *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
> >> if (q->io_modes & VB2_MMAP)
> >> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
> >> @@ -686,21 +698,9 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
> >> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS;
> >> if (q->supports_requests)
> >> *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
> >> -}
> >> -
> >> -static void validate_memory_flags(struct vb2_queue *q,
> >> - int memory,
> >> - u32 *flags)
> >> -{
> >> - if (!q->allow_cache_hints || memory != V4L2_MEMORY_MMAP) {
> >> - /*
> >> - * This needs to clear V4L2_MEMORY_FLAG_NON_COHERENT only,
> >> - * but in order to avoid bugs we zero out all bits.
> >> - */
> >> - *flags = 0;
> >> - } else {
> >> - /* Clear all unknown flags. */
> >> - *flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
> >> + if (max_num_bufs) {
> >> + *max_num_bufs = q->max_num_buffers;
> >> + *caps |= V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS;
> >> }
> >> }
> >>
> >> @@ -709,8 +709,8 @@ int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
> >> int ret = vb2_verify_memory_type(q, req->memory, req->type);
> >> u32 flags = req->flags;
> >
> > I think we have a bug here, we check the memory type, but just let the
> > code continue even if it's invalid.
>
> It should be harmless. The old code did the same, BTW.
>
Yeah, I've double checked it and there is nothing that depends on the
valid memory type and the error code is not overwritten anywhere on
the way, so the code at the bottom takes it into account correctly.
That said, it's kind of confusing and I can imagine someone adding
some code in the middle which could easily break any of the two
assumptions and the problem may not even be visible in the diff
context.
> >
> >>
> >> - fill_buf_caps(q, &req->capabilities);
> >> - validate_memory_flags(q, req->memory, &flags);
> >> + vb2_set_flags_and_caps(q, req->memory, &flags,
> >> + &req->capabilities, NULL);
> >> req->flags = flags;
> >> return ret ? ret : vb2_core_reqbufs(q, req->memory,
> >> req->flags, &req->count);
> >> @@ -751,11 +751,9 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
> >> int ret = vb2_verify_memory_type(q, create->memory, f->type);
> >> unsigned i;
> >>
> >
> > Ditto.
> >
> >> - fill_buf_caps(q, &create->capabilities);
> >> - validate_memory_flags(q, create->memory, &create->flags);
> >> create->index = vb2_get_num_buffers(q);
> >> - create->max_num_buffers = q->max_num_buffers;
> >> - create->capabilities |= V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS;
> >> + vb2_set_flags_and_caps(q, create->memory, &create->flags,
> >> + &create->capabilities, &create->max_num_buffers);
> >> if (create->count == 0)
> >> return ret != -EBUSY ? ret : 0;
> >>
> >> @@ -1006,8 +1004,8 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv,
> >> int res = vb2_verify_memory_type(vdev->queue, p->memory, p->type);
> >> u32 flags = p->flags;
> >>
> >> - fill_buf_caps(vdev->queue, &p->capabilities);
> >> - validate_memory_flags(vdev->queue, p->memory, &flags);
> >> + vb2_set_flags_and_caps(vdev->queue, p->memory, &flags,
> >> + &p->capabilities, NULL);
> >
> > Could we just call vb2_reqbufs() instead of vb2_core_reqbufs() and
> > avoid all the duplicate checks?
> >
> >> p->flags = flags;
> >> if (res)
> >> return res;
> >> @@ -1026,12 +1024,11 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
> >> struct v4l2_create_buffers *p)
> >> {
> >> struct video_device *vdev = video_devdata(file);
> >> - int res = vb2_verify_memory_type(vdev->queue, p->memory,
> >> - p->format.type);
> >> + 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);
> >> + p->index = vb2_get_num_buffers(vdev->queue);
> >> + vb2_set_flags_and_caps(vdev->queue, p->memory, &p->flags,
> >> + &p->capabilities, &p->max_num_buffers);
> >
> > This function in fact already calls vb2_create_bufs() which includes
> > all the checks above, except the one for vb2_queue_is_bus() that
> > continues down below, so maybe we can just remove them?
>
> Same answer for both reqbufs and create_bufs: it has to do with the
> order in which checks are done.
>
> When using vb2_ioctl_reqbufs you want to first check vb2_verify_memory_type()
> and return if that's wrong. Next you check if the queue is busy and return
> -EBUSY. Only then do you call vb2_core_reqbufs() (or vb2_create_bufs()).
Does the order really matter here?
>
> Note that this is a fixes patch (that should have been in the subject, sorry
> for that), and I don't want to make more changes than strictly necessary.
>
In such a situation, I'd personally avoid the refactoring part, just
do the minimum change necessary to fix the issue and perform the
refactoring in a follow up patch.
That said, since this is really just a change for code that got
recently merged into next, I don't have a strong opinion.
I'm okay with leaving this patch as is +/- the dropping the change
around vb2_get_num_buffers() that got included in the latest version
of Benjamin's patch:
Acked-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> What really should happen is that the last users of vb2_reqbufs and
> vb2_create_bufs are converted to use vb2_ioctl_reqbufs/create_bufs. But
> that's another story.
That wouldn't be possible for m2m devices (and the m2m helpers),
because they have multiple queues per video node, while those
vb2_ioctl* helpers assume vdev->queue is the only queue.
Best regards,
Tomasz
>
> Regards,
>
> Hans
>
> >
> > Best regards,
> > Tomasz
> >
> >> /*
> >> * 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.
> >> --
> >> 2.42.0
> >>
> >>
>
