lkml.org 
[lkml]   [2023]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v5 02/10] media: videobuf2: Access vb2_queue bufs array through helper functions
    From

    Le 31/08/2023 à 12:22, Hans Verkuil a écrit :
    > On 31/08/2023 09:54, Benjamin Gaignard wrote:
    >> Le 30/08/2023 à 18:36, Hans Verkuil a écrit :
    >>> On 30/08/2023 18:24, Benjamin Gaignard wrote:
    >>>> Le 30/08/2023 à 15:23, Hans Verkuil a écrit :
    >>>>> On 24/08/2023 11:21, Benjamin Gaignard wrote:
    >>>>>> The first step before changing how vb2 buffers are stored into queue
    >>>>>> is to avoid direct access to bufs arrays.
    >>>>>>
    >>>>>> This patch adds 2 helpers functions to add and remove vb2 buffers
    >>>>>> from a queue. With these 2 and vb2_get_buffer(), bufs field of
    >>>>>> struct vb2_queue becomes like a private member of the structure.
    >>>>>>
    >>>>>> After each call to vb2_get_buffer() we need to be sure that we get
    >>>>>> a valid pointer so check the return value of all of them.
    >>>>>>
    >>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
    >>>>>>
    >>>>>> # Conflicts:
    >>>>>> #    drivers/media/common/videobuf2/videobuf2-core.c
    >>>>>> ---
    >>>>>>    .../media/common/videobuf2/videobuf2-core.c   | 203 ++++++++++++++----
    >>>>>>    .../media/common/videobuf2/videobuf2-v4l2.c   |  28 ++-
    >>>>>>    drivers/media/platform/amphion/vpu_dbg.c      |  22 +-
    >>>>>>    .../platform/mediatek/jpeg/mtk_jpeg_core.c    |   6 +-
    >>>>>>    .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c |   2 +-
    >>>>>>    drivers/media/platform/st/sti/hva/hva-v4l2.c  |   4 +
    >>>>>>    drivers/media/test-drivers/visl/visl-dec.c    |  28 ++-
    >>>>>>    .../staging/media/atomisp/pci/atomisp_ioctl.c |   2 +-
    >>>>>>    8 files changed, 230 insertions(+), 65 deletions(-)
    >>>>>>
    >>>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
    >>>>>> index e06905533ef4..8aa13591b782 100644
    >>>>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
    >>>>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
    >>>>>> @@ -403,6 +403,37 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
    >>>>>>            vb->skip_cache_sync_on_finish = 1;
    >>>>>>    }
    >>>>>>    +/**
    >>>>>> + * vb2_queue_add_buffer() - add a buffer to a queue
    >>>>>> + * @q:    pointer to &struct vb2_queue with videobuf2 queue.
    >>>>>> + * @vb:    pointer to &struct vb2_buffer to be added to the queue.
    >>>>>> + * @index: index where add vb2_buffer in the queue
    >>>>>> + */
    >>>>>> +static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, int index)
    >>>>>> +{
    >>>>>> +    if (index < VB2_MAX_FRAME && !q->bufs[index]) {
    >>>>>> +        q->bufs[index] = vb;
    >>>>>> +        vb->index = index;
    >>>>>> +        vb->vb2_queue = q;
    >>>>>> +        return true;
    >>>>>> +    }
    >>>>>> +
    >>>>>> +    return false;
    >>>>>> +}
    >>>>>> +
    >>>>>> +/**
    >>>>>> + * vb2_queue_remove_buffer() - remove a buffer from a queue
    >>>>>> + * @q:    pointer to &struct vb2_queue with videobuf2 queue.
    >>>>>> + * @vb:    pointer to &struct vb2_buffer to be removed from the queue.
    >>>>>> + */
    >>>>>> +static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
    >>>>>> +{
    >>>>>> +    if (vb->index < VB2_MAX_FRAME) {
    >>>>>> +        q->bufs[vb->index] = NULL;
    >>>>>> +        vb->vb2_queue = NULL;
    >>>>>> +    }
    >>>>>> +}
    >>>>>> +
    >>>>>>    /*
    >>>>>>     * __vb2_queue_alloc() - allocate vb2 buffer structures and (for MMAP type)
    >>>>>>     * video buffer memory for all buffers/planes on the queue and initializes the
    >>>>>> @@ -431,9 +462,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
    >>>>>>            }
    >>>>>>              vb->state = VB2_BUF_STATE_DEQUEUED;
    >>>>>> -        vb->vb2_queue = q;
    >>>>>>            vb->num_planes = num_planes;
    >>>>>> -        vb->index = q->num_buffers + buffer;
    >>>>>>            vb->type = q->type;
    >>>>>>            vb->memory = memory;
    >>>>>>            init_buffer_cache_hints(q, vb);
    >>>>>> @@ -443,7 +472,11 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
    >>>>>>            }
    >>>>>>            call_void_bufop(q, init_buffer, vb);
    >>>>>>    -        q->bufs[vb->index] = vb;
    >>>>>> +        if (!vb2_queue_add_buffer(q, vb, q->num_buffers + buffer)) {
    >>>>>> +            dprintk(q, 1, "failed adding buffer %d to queue\n", buffer);
    >>>>>> +            kfree(vb);
    >>>>>> +            break;
    >>>>>> +        }
    >>>>>>              /* Allocate video buffer memory for the MMAP type */
    >>>>>>            if (memory == VB2_MEMORY_MMAP) {
    >>>>>> @@ -451,7 +484,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
    >>>>>>                if (ret) {
    >>>>>>                    dprintk(q, 1, "failed allocating memory for buffer %d\n",
    >>>>>>                        buffer);
    >>>>>> -                q->bufs[vb->index] = NULL;
    >>>>>> +                vb2_queue_remove_buffer(q, vb);
    >>>>>>                    kfree(vb);
    >>>>>>                    break;
    >>>>>>                }
    >>>>>> @@ -466,7 +499,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
    >>>>>>                    dprintk(q, 1, "buffer %d %p initialization failed\n",
    >>>>>>                        buffer, vb);
    >>>>>>                    __vb2_buf_mem_free(vb);
    >>>>>> -                q->bufs[vb->index] = NULL;
    >>>>>> +                vb2_queue_remove_buffer(q, vb);
    >>>>>>                    kfree(vb);
    >>>>>>                    break;
    >>>>>>                }
    >>>>>> @@ -489,7 +522,7 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers)
    >>>>>>          for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
    >>>>>>             ++buffer) {
    >>>>>> -        vb = q->bufs[buffer];
    >>>>>> +        vb = vb2_get_buffer(q, buffer);
    >>>>>>            if (!vb)
    >>>>>>                continue;
    >>>>>>    @@ -517,7 +550,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
    >>>>>>        /* Call driver-provided cleanup function for each buffer, if provided */
    >>>>>>        for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
    >>>>>>             ++buffer) {
    >>>>>> -        struct vb2_buffer *vb = q->bufs[buffer];
    >>>>>> +        struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
    >>>>>>              if (vb && vb->planes[0].mem_priv)
    >>>>>>                call_void_vb_qop(vb, buf_cleanup, vb);
    >>>>>> @@ -557,15 +590,20 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
    >>>>>>            q->cnt_unprepare_streaming = 0;
    >>>>>>        }
    >>>>>>        for (buffer = 0; buffer < q->num_buffers; ++buffer) {
    >>>>>> -        struct vb2_buffer *vb = q->bufs[buffer];
    >>>>>> -        bool unbalanced = vb->cnt_mem_alloc != vb->cnt_mem_put ||
    >>>>>> -                  vb->cnt_mem_prepare != vb->cnt_mem_finish ||
    >>>>>> -                  vb->cnt_mem_get_userptr != vb->cnt_mem_put_userptr ||
    >>>>>> -                  vb->cnt_mem_attach_dmabuf != vb->cnt_mem_detach_dmabuf ||
    >>>>>> -                  vb->cnt_mem_map_dmabuf != vb->cnt_mem_unmap_dmabuf ||
    >>>>>> -                  vb->cnt_buf_queue != vb->cnt_buf_done ||
    >>>>>> -                  vb->cnt_buf_prepare != vb->cnt_buf_finish ||
    >>>>>> -                  vb->cnt_buf_init != vb->cnt_buf_cleanup;
    >>>>>> +        struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
    >>>>>> +        bool unbalanced;
    >>>>>> +
    >>>>>> +        if (!vb)
    >>>>>> +            continue;
    >>>>>> +
    >>>>>> +        unbalanced = vb->cnt_mem_alloc != vb->cnt_mem_put ||
    >>>>>> +                 vb->cnt_mem_prepare != vb->cnt_mem_finish ||
    >>>>>> +                 vb->cnt_mem_get_userptr != vb->cnt_mem_put_userptr ||
    >>>>>> +                 vb->cnt_mem_attach_dmabuf != vb->cnt_mem_detach_dmabuf ||
    >>>>>> +                 vb->cnt_mem_map_dmabuf != vb->cnt_mem_unmap_dmabuf ||
    >>>>>> +                 vb->cnt_buf_queue != vb->cnt_buf_done ||
    >>>>>> +                 vb->cnt_buf_prepare != vb->cnt_buf_finish ||
    >>>>>> +                 vb->cnt_buf_init != vb->cnt_buf_cleanup;
    >>>>>>              if (unbalanced || debug) {
    >>>>> I think we should drop the '|| debug' part. It is already annoying today to see these
    >>>>> messages when the debug parameter is > 0, and now the number of buffers is still
    >>>>> fairly small. But if we allow a lot more buffers, then this will really spam the
    >>>>> kernel log.
    >>>>>
    >>>>> I think this should be dropped, and we only report unbalanced buffers.
    >>>>>
    >>>>> And another optimization is to only report the unbalanced counters. Right now
    >>>>> it reports all counters, but it is again too much spamming of the kernel log.
    >>>>>
    >>>>> I think this change can be done as a separate patch before this patch.
    >>>>> That way it can be picked up separately from the other changes in this series.
    >>>>>
    >>>>>>                pr_info("   counters for queue %p, buffer %d:%s\n",
    >>>>>> @@ -597,8 +635,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
    >>>>>>        /* Free vb2 buffers */
    >>>>>>        for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
    >>>>>>             ++buffer) {
    >>>>>> -        kfree(q->bufs[buffer]);
    >>>>>> -        q->bufs[buffer] = NULL;
    >>>>>> +        struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
    >>>>>> +
    >>>>>> +        if (!vb)
    >>>>>> +            continue;
    >>>>>> +
    >>>>>> +        vb2_queue_remove_buffer(q, vb);
    >>>>>> +        kfree(vb);
    >>>>>>        }
    >>>>>>          q->num_buffers -= buffers;
    >>>>>> @@ -634,7 +677,12 @@ static bool __buffers_in_use(struct vb2_queue *q)
    >>>>>>    {
    >>>>>>        unsigned int buffer;
    >>>>>>        for (buffer = 0; buffer < q->num_buffers; ++buffer) {
    >>>>>> -        if (vb2_buffer_in_use(q, q->bufs[buffer]))
    >>>>>> +        struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
    >>>>>> +
    >>>>>> +        if (!vb)
    >>>>>> +            continue;
    >>>>>> +
    >>>>>> +        if (vb2_buffer_in_use(q, vb))
    >>>>>>                return true;
    >>>>>>        }
    >>>>>>        return false;
    >>>>>> @@ -642,7 +690,10 @@ static bool __buffers_in_use(struct vb2_queue *q)
    >>>>>>      void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
    >>>>>>    {
    >>>>>> -    call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
    >>>>>> +    struct vb2_buffer *vb = vb2_get_buffer(q, index);
    >>>>>> +
    >>>>>> +    if (vb)
    >>>>>> +        call_void_bufop(q, fill_user_buffer, vb, pb);
    >>>>> I think that rather than passing the index (that then has to be verified)
    >>>>> it is better to pass the vb2_buffer pointer directly and leave it up to
    >>>>> the caller to do the index verification.
    >>>>>
    >>>>> Another option is to drop this function altogether and let the called
    >>>>> call the fill_user_buffer function. Either works for me.
    >>>>>
    >>>>>>    }
    >>>>>>    EXPORT_SYMBOL_GPL(vb2_core_querybuf);
    >>>>>>    @@ -1553,7 +1604,13 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
    >>>>> Here too it is better to pass the vb2_buffer pointer instead of an index.
    >>>>>
    >>>>> This function assumes that the index is valid, so the called actually does the
    >>>>> validation. Passing the vb pointer instead of the index makes more sense
    >>>>> in this new situation.
    >>>>>
    >>>>> This is also true for two other core functions: vb2_core_qbuf and vb2_core_expbuf.
    >>>>>
    >>>>>>        struct vb2_buffer *vb;
    >>>>>>        int ret;
    >>>>>>    -    vb = q->bufs[index];
    >>>>>> +    vb = vb2_get_buffer(q, index);
    >>>>>> +
    >>>>>> +    if (!vb) {
    >>>>>> +        dprintk(q, 1, "can't find the requested buffer\n");
    >>>>>> +        return -EINVAL;
    >>>>>> +    }
    >>>>> Changing that avoids having to add this check, so it simplifies the code.
    >>>>>
    >>>>> I think that this change can be done in a separate patch before this one.
    >>>>>
    >>>>> It makes sense to apply that regardless of the remainder of this series.
    >>>>>
    >>>>>> +
    >>>>>>        if (vb->state != VB2_BUF_STATE_DEQUEUED) {
    >>>>>>            dprintk(q, 1, "invalid buffer state %s\n",
    >>>>>>                vb2_state_name(vb->state));
    >>>>>> @@ -1624,7 +1681,11 @@ static int vb2_start_streaming(struct vb2_queue *q)
    >>>>>>             * correctly return them to vb2.
    >>>>>>             */
    >>>>>>            for (i = 0; i < q->num_buffers; ++i) {
    >>>>>> -            vb = q->bufs[i];
    >>>>>> +            vb = vb2_get_buffer(q, i);
    >>>>>> +
    >>>>>> +            if (!vb)
    >>>>>> +                continue;
    >>>>>> +
    >>>>>>                if (vb->state == VB2_BUF_STATE_ACTIVE)
    >>>>>>                    vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED);
    >>>>>>            }
    >>>>>> @@ -1652,7 +1713,12 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
    >>>>>>            return -EIO;
    >>>>>>        }
    >>>>>>    -    vb = q->bufs[index];
    >>>>>> +    vb = vb2_get_buffer(q, index);
    >>>>>> +
    >>>>>> +    if (!vb) {
    >>>>>> +        dprintk(q, 1, "can't find the requested buffer\n");
    >>>>>> +        return -EINVAL;
    >>>>>> +    }
    >>>>>>          if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
    >>>>>>            q->requires_requests) {
    >>>>>> @@ -2028,12 +2094,18 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
    >>>>>>         * to vb2 in stop_streaming().
    >>>>>>         */
    >>>>>>        if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
    >>>>>> -        for (i = 0; i < q->num_buffers; ++i)
    >>>>>> -            if (q->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
    >>>>>> +        for (i = 0; i < q->num_buffers; ++i) {
    >>>>>> +            struct vb2_buffer *vb = vb2_get_buffer(q, i);
    >>>>>> +
    >>>>>> +            if (!vb)
    >>>>>> +                continue;
    >>>>>> +
    >>>>>> +            if (vb->state == VB2_BUF_STATE_ACTIVE) {
    >>>>>>                    pr_warn("driver bug: stop_streaming operation is leaving buf %p in active state\n",
    >>>>>> -                    q->bufs[i]);
    >>>>>> -                vb2_buffer_done(q->bufs[i], VB2_BUF_STATE_ERROR);
    >>>>>> +                    vb);
    >>>>>> +                vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
    >>>>>>                }
    >>>>>> +        }
    >>>>>>            /* Must be zero now */
    >>>>>>            WARN_ON(atomic_read(&q->owned_by_drv_count));
    >>>>>>        }
    >>>>>> @@ -2067,9 +2139,14 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
    >>>>>>         * be changed, so we can't move the buf_finish() to __vb2_dqbuf().
    >>>>>>         */
    >>>>>>        for (i = 0; i < q->num_buffers; ++i) {
    >>>>>> -        struct vb2_buffer *vb = q->bufs[i];
    >>>>>> -        struct media_request *req = vb->req_obj.req;
    >>>>>> +        struct vb2_buffer *vb;
    >>>>>> +        struct media_request *req;
    >>>>>> +
    >>>>>> +        vb = vb2_get_buffer(q, i);
    >>>>>> +        if (!vb)
    >>>>>> +            continue;
    >>>>>>    +        req = vb->req_obj.req;
    >>>>>>            /*
    >>>>>>             * If a request is associated with this buffer, then
    >>>>>>             * call buf_request_cancel() to give the driver to complete()
    >>>>>> @@ -2219,7 +2296,10 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
    >>>>>>        buffer = (off >> (PLANE_INDEX_SHIFT + PAGE_SHIFT)) & MAX_BUFFERS;
    >>>>>>        plane = (off >> PAGE_SHIFT) & PLANE_INDEX_MASK;
    >>>>>>    -    vb = q->bufs[buffer];
    >>>>>> +    vb = vb2_get_buffer(q, buffer);
    >>>>>> +    if (!vb)
    >>>>>> +        return -EINVAL;
    >>>>>> +
    >>>>>>        if (vb->planes[plane].m.offset == off) {
    >>>>>>            *_buffer = buffer;
    >>>>>>            *_plane = plane;
    >>>>>> @@ -2262,7 +2342,12 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
    >>>>>>            return -EINVAL;
    >>>>>>        }
    >>>>>>    -    vb = q->bufs[index];
    >>>>>> +    vb = vb2_get_buffer(q, index);
    >>>>>> +
    >>>>>> +    if (!vb) {
    >>>>>> +        dprintk(q, 1, "can't find the requested buffer\n");
    >>>>>> +        return -EINVAL;
    >>>>>> +    }
    >>>>>>          if (plane >= vb->num_planes) {
    >>>>>>            dprintk(q, 1, "buffer plane out of range\n");
    >>>>>> @@ -2339,7 +2424,13 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
    >>>>>>        if (ret)
    >>>>>>            goto unlock;
    >>>>>>    -    vb = q->bufs[buffer];
    >>>>>> +    vb = vb2_get_buffer(q, buffer);
    >>>>>> +
    >>>>>> +    if (!vb) {
    >>>>>> +        dprintk(q, 1, "can't find the requested buffer\n");
    >>>>>> +        ret = -EINVAL;
    >>>>>> +        goto unlock;
    >>>>>> +    }
    >>>>>>          /*
    >>>>>>         * MMAP requires page_aligned buffers.
    >>>>>> @@ -2396,7 +2487,12 @@ unsigned long vb2_get_unmapped_area(struct vb2_queue *q,
    >>>>>>        if (ret)
    >>>>>>            goto unlock;
    >>>>>>    -    vb = q->bufs[buffer];
    >>>>>> +    vb = vb2_get_buffer(q, buffer);
    >>>>>> +    if (!vb) {
    >>>>>> +        dprintk(q, 1, "can't find the requested buffer\n");
    >>>>>> +        ret = -EINVAL;
    >>>>>> +        goto unlock;
    >>>>>> +    }
    >>>>>>          vaddr = vb2_plane_vaddr(vb, plane);
    >>>>>>        mutex_unlock(&q->mmap_lock);
    >>>>>> @@ -2625,6 +2721,7 @@ struct vb2_fileio_data {
    >>>>>>    static int __vb2_init_fileio(struct vb2_queue *q, int read)
    >>>>>>    {
    >>>>>>        struct vb2_fileio_data *fileio;
    >>>>>> +    struct vb2_buffer *vb;
    >>>>>>        int i, ret;
    >>>>>>        unsigned int count = 0;
    >>>>>>    @@ -2679,7 +2776,13 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
    >>>>>>         * Check if plane_count is correct
    >>>>>>         * (multiplane buffers are not supported).
    >>>>>>         */
    >>>>>> -    if (q->bufs[0]->num_planes != 1) {
    >>>>>> +    vb = vb2_get_buffer(q, 0);
    >>>>>> +    if (!vb) {
    >>>>>> +        ret = -EBUSY;
    >>>>>> +        goto err_reqbufs;
    >>>>>> +    }
    >>>>> This cannot happen. These fileio helper functions implement the read() support
    >>>>> and all the buffer allocation happens here. Userspace can never add or delete
    >>>>> buffers later, so there will never be holes. It is safe to assume that
    >>>>> vb2_get_buffer(q, i) will always return a valid vb pointer for i in the range
    >>>>> of 0 - q->num_buffers-1.
    >>>>>
    >>>>> Perhaps add a comment to that effect, but otherwise you can drop the checks.
    >>>>>
    >>>>>> +
    >>>>>> +    if (vb->num_planes != 1) {
    >>>>>>            ret = -EBUSY;
    >>>>>>            goto err_reqbufs;
    >>>>>>        }
    >>>>>> @@ -2688,12 +2791,17 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
    >>>>>>         * Get kernel address of each buffer.
    >>>>>>         */
    >>>>>>        for (i = 0; i < q->num_buffers; i++) {
    >>>>>> -        fileio->bufs[i].vaddr = vb2_plane_vaddr(q->bufs[i], 0);
    >>>>>> +        vb = vb2_get_buffer(q, i);
    >>>>>> +
    >>>>>> +        if (!vb)
    >>>>>> +            continue;
    >>>>>> +
    >>>>>> +        fileio->bufs[i].vaddr = vb2_plane_vaddr(vb, 0);
    >>>>>>            if (fileio->bufs[i].vaddr == NULL) {
    >>>>>>                ret = -EINVAL;
    >>>>>>                goto err_reqbufs;
    >>>>>>            }
    >>>>>> -        fileio->bufs[i].size = vb2_plane_size(q->bufs[i], 0);
    >>>>>> +        fileio->bufs[i].size = vb2_plane_size(vb, 0);
    >>>>>>        }
    >>>>>>          /*
    >>>>>> @@ -2821,15 +2929,18 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
    >>>>>>              fileio->cur_index = index;
    >>>>>>            buf = &fileio->bufs[index];
    >>>>>> -        b = q->bufs[index];
    >>>>>> +        b = vb2_get_buffer(q, index);
    >>>>>> +
    >>>>>> +        if (!b)
    >>>>>> +            return -EINVAL;
    >>>>>>              /*
    >>>>>>             * Get number of bytes filled by the driver
    >>>>>>             */
    >>>>>>            buf->pos = 0;
    >>>>>>            buf->queued = 0;
    >>>>>> -        buf->size = read ? vb2_get_plane_payload(q->bufs[index], 0)
    >>>>>> -                 : vb2_plane_size(q->bufs[index], 0);
    >>>>>> +        buf->size = read ? vb2_get_plane_payload(b, 0)
    >>>>>> +                 : vb2_plane_size(b, 0);
    >>>>>>            /* Compensate for data_offset on read in the multiplanar case. */
    >>>>>>            if (is_multiplanar && read &&
    >>>>>>                    b->planes[0].data_offset < buf->size) {
    >>>>>> @@ -2872,8 +2983,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
    >>>>>>         * Queue next buffer if required.
    >>>>>>         */
    >>>>>>        if (buf->pos == buf->size || (!read && fileio->write_immediately)) {
    >>>>>> -        struct vb2_buffer *b = q->bufs[index];
    >>>>>> +        struct vb2_buffer *b = vb2_get_buffer(q, index);
    >>>>>>    +        if (!b) {
    >>>>>> +            dprintk(q, 1, "can't find the requested buffer\n");
    >>>>>> +            return -EINVAL;
    >>>>>> +        }
    >>>>>>            /*
    >>>>>>             * Check if this is the last buffer to read.
    >>>>>>             */
    >>>>>> @@ -2899,7 +3014,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
    >>>>>>             */
    >>>>>>            buf->pos = 0;
    >>>>>>            buf->queued = 1;
    >>>>>> -        buf->size = vb2_plane_size(q->bufs[index], 0);
    >>>>>> +        buf->size = vb2_plane_size(b, 0);
    >>>>>>            fileio->q_count += 1;
    >>>>>>            /*
    >>>>>>             * If we are queuing up buffers for the first time, then
    >>>>>> @@ -2970,7 +3085,9 @@ static int vb2_thread(void *data)
    >>>>>>             * Call vb2_dqbuf to get buffer back.
    >>>>>>             */
    >>>>>>            if (prequeue) {
    >>>>>> -            vb = q->bufs[index++];
    >>>>>> +            vb = vb2_get_buffer(q, index++);
    >>>>>> +            if (!vb)
    >>>>>> +                continue;
    >>>>>>                prequeue--;
    >>>>>>            } else {
    >>>>>>                call_void_qop(q, wait_finish, q);
    >>>>>> @@ -2979,7 +3096,7 @@ static int vb2_thread(void *data)
    >>>>>>                call_void_qop(q, wait_prepare, q);
    >>>>>>                dprintk(q, 5, "file io: vb2_dqbuf result: %d\n", ret);
    >>>>>>                if (!ret)
    >>>>>> -                vb = q->bufs[index];
    >>>>>> +                vb = vb2_get_buffer(q, index);
    >>>>>>            }
    >>>>>>            if (ret || threadio->stop)
    >>>>>>                break;
    >>>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
    >>>>>> index c7a54d82a55e..724135d41f7f 100644
    >>>>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
    >>>>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
    >>>>>> @@ -383,8 +383,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
    >>>>>>            return -EINVAL;
    >>>>>>        }
    >>>>>>    -    if (q->bufs[b->index] == NULL) {
    >>>>>> -        /* Should never happen */
    >>>>>> +    if (!vb2_get_buffer(q, b->index)) {
    >>>>>>            dprintk(q, 1, "%s: buffer is NULL\n", opname);
    >>>>> How about:
    >>>>>
    >>>>>             dprintk(q, 1, "%s: buffer %u was deleted\n", opname, b->index);
    >>>>>
    >>>>> although perhaps that change is more appropriate in patch 09/10?
    >>>>>
    >>>>> Regardless, once it is possible to delete buffers, then this message should be
    >>>>> adjusted accordingly.
    >>>>>
    >>>>>>            return -EINVAL;
    >>>>>>        }
    >>>>>> @@ -394,7 +393,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
    >>>>>>            return -EINVAL;
    >>>>>>        }
    >>>>>>    -    vb = q->bufs[b->index];
    >>>>>> +    vb = vb2_get_buffer(q, b->index);
    >>>>> This can be moved up to the 'if (!vb2_get_buffer(q, b->index)) {' check above.
    >>>>> That avoids calling vb2_get_buffer twice.
    >>>>>
    >>>>>>        vbuf = to_vb2_v4l2_buffer(vb);
    >>>>>>        ret = __verify_planes_array(vb, b);
    >>>>>>        if (ret)
    >>>>>> @@ -628,11 +627,18 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
    >>>>>>    struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp)
    >>>>>>    {
    >>>>>>        unsigned int i;
    >>>>>> +    struct vb2_buffer *vb2;
    >>>>>>    -    for (i = 0; i < q->num_buffers; i++)
    >>>>>> -        if (q->bufs[i]->copied_timestamp &&
    >>>>>> -            q->bufs[i]->timestamp == timestamp)
    >>>>>> -            return vb2_get_buffer(q, i);
    >>>>> Perhaps add a comment here that this loop doesn't scale if there
    >>>>> is a really large number of buffers and something more efficient
    >>>>> will have to be found in that case.
    >>>>>
    >>>>>> +    for (i = 0; i < q->num_buffers; i++) {
    >>>>>> +        vb2 = vb2_get_buffer(q, i);
    >>>>>> +
    >>>>>> +        if (!vb2)
    >>>>>> +            continue;
    >>>>>> +
    >>>>>> +        if (vb2->copied_timestamp &&
    >>>>>> +            vb2->timestamp == timestamp)
    >>>>>> +            return vb2;
    >>>>>> +    }
    >>>>>>        return NULL;
    >>>>>>    }
    >>>>>>    EXPORT_SYMBOL_GPL(vb2_find_buffer);
    >>>>>> @@ -664,7 +670,13 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
    >>>>>>            dprintk(q, 1, "buffer index out of range\n");
    >>>>>>            return -EINVAL;
    >>>>>>        }
    >>>>>> -    vb = q->bufs[b->index];
    >>>>>> +    vb = vb2_get_buffer(q, b->index);
    >>>>>> +
    >>>>>> +    if (!vb) {
    >>>>>> +        dprintk(q, 1, "can't find the requested buffer\n");
    >>>>>> +        return -EINVAL;
    >>>>>> +    }
    >>>>>> +
    >>>>>>        ret = __verify_planes_array(vb, b);
    >>>>>>        if (!ret)
    >>>>>>            vb2_core_querybuf(q, b->index, b);
    >>>>>> diff --git a/drivers/media/platform/amphion/vpu_dbg.c b/drivers/media/platform/amphion/vpu_dbg.c
    >>>>>> index 982c2c777484..a462d6fe4ea9 100644
    >>>>>> --- a/drivers/media/platform/amphion/vpu_dbg.c
    >>>>>> +++ b/drivers/media/platform/amphion/vpu_dbg.c
    >>>>>> @@ -140,11 +140,18 @@ static int vpu_dbg_instance(struct seq_file *s, void *data)
    >>>>>>          vq = v4l2_m2m_get_src_vq(inst->fh.m2m_ctx);
    >>>>>>        for (i = 0; i < vq->num_buffers; i++) {
    >>>>>> -        struct vb2_buffer *vb = vq->bufs[i];
    >>>>>> -        struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
    >>>>>> +        struct vb2_buffer *vb;
    >>>>>> +        struct vb2_v4l2_buffer *vbuf;
    >>>>>> +
    >>>>>> +        vb = vb2_get_buffer(vq, i);
    >>>>>> +        if (!vb)
    >>>>>> +            continue;
    >>>>>>              if (vb->state == VB2_BUF_STATE_DEQUEUED)
    >>>>>>                continue;
    >>>>>> +
    >>>>>> +        vbuf = to_vb2_v4l2_buffer(vb);
    >>>>>> +
    >>>>>>            num = scnprintf(str, sizeof(str),
    >>>>>>                    "output [%2d] state = %10s, %8s\n",
    >>>>>>                    i, vb2_stat_name[vb->state],
    >>>>>> @@ -155,11 +162,18 @@ static int vpu_dbg_instance(struct seq_file *s, void *data)
    >>>>>>          vq = v4l2_m2m_get_dst_vq(inst->fh.m2m_ctx);
    >>>>>>        for (i = 0; i < vq->num_buffers; i++) {
    >>>>>> -        struct vb2_buffer *vb = vq->bufs[i];
    >>>>>> -        struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
    >>>>>> +        struct vb2_buffer *vb;
    >>>>>> +        struct vb2_v4l2_buffer *vbuf;
    >>>>>> +
    >>>>>> +        vb = vb2_get_buffer(vq, i);
    >>>>>> +        if (!vb)
    >>>>>> +            continue;
    >>>>>>              if (vb->state == VB2_BUF_STATE_DEQUEUED)
    >>>>>>                continue;
    >>>>>> +
    >>>>>> +        vbuf = to_vb2_v4l2_buffer(vb);
    >>>>>> +
    >>>>>>            num = scnprintf(str, sizeof(str),
    >>>>>>                    "capture[%2d] state = %10s, %8s\n",
    >>>>>>                    i, vb2_stat_name[vb->state],
    >>>>> This can be a separate patch, right? It doesn't depend on any core changes.
    >>>>>
    >>>>> And this can also be applied before this patch.
    >>>> Hans, I would like to clarify this comment (and the following Ditto).
    >>>> Are you against use vb2_get_buffer() outside core ?
    >>>> or testing vb2_get_buffer() result ?
    >>>> The goal of this patch was to remove all access like vq->bufs[i] and to make
    >>>> sure that vb buffer are always valid.
    >>> Sorry for the confusion. I meant that AFAICS each of these driver changes can be
    >>> done in the separate patch and that those separate patches can be applied before
    >>> this patch. I.e., they are independent.
    >>>
    >>> I always prefer specific driver changes to be done as separate patches rather
    >>> than one patch modifying a lot of drivers in one go. That is not always possible,
    >>> of course, but in this case I think it is fine, unless I missed something.
    >> All changes in the drivers use vb2_get_buffer() which is introduced in this patch
    >> so I can't do them before.
    >> If you want I can make a patch per driver but after this patch.
    > ??? vb2_get_buffer() has been defined in include/media/videobuf2-core.h for a long
    > time (2019 to be precise).

    Yes I have made a mistake, sorry for the noise.

    >
    > Regards,
    >
    > Hans
    >
    >> Regards,
    >> Benjamin
    >>
    >>> Regards,
    >>>
    >>>     Hans
    >>>
    >>>> Regards,
    >>>> Benjamin
    >>>>
    >>>>>> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
    >>>>>> index 621038aab116..62910a1b8a98 100644
    >>>>>> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
    >>>>>> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
    >>>>>> @@ -603,7 +603,11 @@ static int mtk_jpeg_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
    >>>>>>            return -EINVAL;
    >>>>>>        }
    >>>>>>    -    vb = vq->bufs[buf->index];
    >>>>>> +    vb = vb2_get_buffer(vq, buf->index);
    >>>>>> +    if (!vb) {
    >>>>>> +        dev_err(ctx->jpeg->dev, "buffer not found\n");
    >>>>>> +        return -EINVAL;
    >>>>>> +    }
    >>>>>>        jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(vb);
    >>>>>>        jpeg_src_buf->bs_size = buf->m.planes[0].bytesused;
    >>>>>>
    >>>>> Ditto.
    >>>>>
    >>>>>> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
    >>>>>> index e393e3e668f8..3d2ae0e1b5b6 100644
    >>>>>> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
    >>>>>> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
    >>>>>> @@ -1696,7 +1696,7 @@ static int vdec_vp9_slice_setup_core_buffer(struct vdec_vp9_slice_instance *inst
    >>>>>>          /* update internal buffer's width/height */
    >>>>>>        for (i = 0; i < vq->num_buffers; i++) {
    >>>>>> -        if (vb == vq->bufs[i]) {
    >>>>>> +        if (vb == vb2_get_buffer(vq, i)) {
    >>>>>>                instance->dpb[i].width = w;
    >>>>>>                instance->dpb[i].height = h;
    >>>>>>                break;
    >>>>> Ditto.
    >>>>>
    >>>>>> diff --git a/drivers/media/platform/st/sti/hva/hva-v4l2.c b/drivers/media/platform/st/sti/hva/hva-v4l2.c
    >>>>>> index 3a848ca32a0e..326be09bdb55 100644
    >>>>>> --- a/drivers/media/platform/st/sti/hva/hva-v4l2.c
    >>>>>> +++ b/drivers/media/platform/st/sti/hva/hva-v4l2.c
    >>>>>> @@ -577,6 +577,10 @@ static int hva_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
    >>>>>>            }
    >>>>>>              vb2_buf = vb2_get_buffer(vq, buf->index);
    >>>>>> +        if (!vb2_buf) {
    >>>>>> +            dev_dbg(dev, "%s buffer index %d not found\n", ctx->name, buf->index);
    >>>>>> +            return -EINVAL;
    >>>>>> +        }
    >>>>>>            stream = to_hva_stream(to_vb2_v4l2_buffer(vb2_buf));
    >>>>>>            stream->bytesused = buf->bytesused;
    >>>>>>        }
    >>>>> Ditto.
    >>>>>
    >>>>>> diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
    >>>>>> index 318d675e5668..ba20ea998d19 100644
    >>>>>> --- a/drivers/media/test-drivers/visl/visl-dec.c
    >>>>>> +++ b/drivers/media/test-drivers/visl/visl-dec.c
    >>>>>> @@ -290,13 +290,20 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
    >>>>>>        for (i = 0; i < out_q->num_buffers; i++) {
    >>>>>>            char entry[] = "index: %u, state: %s, request_fd: %d, ";
    >>>>>>            u32 old_len = len;
    >>>>>> -        char *q_status = visl_get_vb2_state(out_q->bufs[i]->state);
    >>>>>> +        struct vb2_buffer *vb2;
    >>>>>> +        char *q_status;
    >>>>>> +
    >>>>>> +        vb2 = vb2_get_buffer(out_q, i);
    >>>>>> +        if (!vb2)
    >>>>>> +            continue;
    >>>>>> +
    >>>>>> +        q_status = visl_get_vb2_state(vb2->state);
    >>>>>>              len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
    >>>>>>                     entry, i, q_status,
    >>>>>> -                 to_vb2_v4l2_buffer(out_q->bufs[i])->request_fd);
    >>>>>> +                 to_vb2_v4l2_buffer(vb2)->request_fd);
    >>>>>>    -        len += visl_fill_bytesused(to_vb2_v4l2_buffer(out_q->bufs[i]),
    >>>>>> +        len += visl_fill_bytesused(to_vb2_v4l2_buffer(vb2),
    >>>>>>                           &buf[len],
    >>>>>>                           TPG_STR_BUF_SZ - len);
    >>>>>>    @@ -342,13 +349,20 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
    >>>>>>        len = 0;
    >>>>>>        for (i = 0; i < cap_q->num_buffers; i++) {
    >>>>>>            u32 old_len = len;
    >>>>>> -        char *q_status = visl_get_vb2_state(cap_q->bufs[i]->state);
    >>>>>> +        struct vb2_buffer *vb2;
    >>>>>> +        char *q_status;
    >>>>>> +
    >>>>>> +        vb2 = vb2_get_buffer(cap_q, i);
    >>>>>> +        if (!vb2)
    >>>>>> +            continue;
    >>>>>> +
    >>>>>> +        q_status = visl_get_vb2_state(vb2->state);
    >>>>>>              len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
    >>>>>>                     "index: %u, status: %s, timestamp: %llu, is_held: %d",
    >>>>>> -                 cap_q->bufs[i]->index, q_status,
    >>>>>> -                 cap_q->bufs[i]->timestamp,
    >>>>>> -                 to_vb2_v4l2_buffer(cap_q->bufs[i])->is_held);
    >>>>>> +                 vb2->index, q_status,
    >>>>>> +                 vb2->timestamp,
    >>>>>> +                 to_vb2_v4l2_buffer(vb2)->is_held);
    >>>>>>              tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
    >>>>>>            frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
    >>>>> Ditto.
    >>>>>
    >>>>>> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
    >>>>>> index d2174156573a..4b65c69fa60d 100644
    >>>>>> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
    >>>>>> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
    >>>>>> @@ -1061,7 +1061,7 @@ static int atomisp_dqbuf_wrapper(struct file *file, void *fh, struct v4l2_buffer
    >>>>>>        if (ret)
    >>>>>>            return ret;
    >>>>>>    -    vb = pipe->vb_queue.bufs[buf->index];
    >>>>>> +    vb = vb2_get_buffer(&pipe->vb_queue, buf->index);
    >>>>>>        frame = vb_to_frame(vb);
    >>>>>>          buf->reserved = asd->frame_status[buf->index];
    >>>>> Ditto.
    >>>>>
    >>>>> Background: I think it is really useful to merge a lot of the groundwork early
    >>>>> on, where possible. It simplifies the remainder of the patch series.
    >>>>>
    >>>>> Regards,
    >>>>>
    >>>>>      Hans
    >>>>>
    >

    \
     
     \ /
      Last update: 2023-08-31 14:09    [W:5.195 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site