| Message ID | 20230824092133.39510-3-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add DELETE_BUF ioctl | expand |
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. > 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
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. 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 >
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. 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 >>
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. 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 >>>
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). 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 >>>>
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 >>>>> >
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. That will be be in v6 > > 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. I will reuse your words and put that in v6 > >> + >> + 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. I will update the same in the patch introducing DELETE_BUFS > >> 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. ok > >> 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. You will have it in v6 > >> + 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. > >> 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 >
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) { 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); } EXPORT_SYMBOL_GPL(vb2_core_querybuf); @@ -1553,7 +1604,13 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb) 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; + } + 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; + } + + 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); 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); 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); + 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], 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; 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; 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; } 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]); 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];
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(-)