Messages in this thread | | | Date | Thu, 31 Aug 2023 14:06:42 +0200 | Subject | Re: [PATCH v5 02/10] media: videobuf2: Access vb2_queue bufs array through helper functions | From | Benjamin Gaignard <> |
| |
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 >>>>> >
| |