| Message ID | 20230705121056.37017-2-benjamin.gaignard@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add DELETE_BUF ioctl | expand |
Hi Benjamin, On 05/07/2023 14:10, 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> > --- > .../media/common/videobuf2/videobuf2-core.c | 215 ++++++++++++++---- > .../media/common/videobuf2/videobuf2-v4l2.c | 28 ++- > drivers/media/platform/amphion/vpu_dbg.c | 22 +- > .../platform/mediatek/jpeg/mtk_jpeg_core.c | 6 +- > .../vcodec/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, 240 insertions(+), 67 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index cf6727d9c81f..42fd3984c2bc 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -359,8 +359,16 @@ static void __setup_offsets(struct vb2_buffer *vb) > unsigned long off = 0; > > if (vb->index) { > - struct vb2_buffer *prev = q->bufs[vb->index - 1]; > - struct vb2_plane *p = &prev->planes[prev->num_planes - 1]; > + struct vb2_buffer *prev; > + struct vb2_plane *p; > + > + prev = vb2_get_buffer(q, vb->index - 1); > + if (!prev) { > + dprintk(q, 1, "can't find the requested buffer\n"); > + return; > + } > + > + p = &prev->planes[prev->num_planes - 1]; > > off = PAGE_ALIGN(p->m.offset + p->length); This does not work. Before this patch there should be a patch that changes the way the offset is determined since it affects this functionality. __setup_offsets() fills in the m.offset field for each plane in the buffer. In the current code it starts with 0 for the first buffer, adds the length for each plane to it, and for the next buffer it continues doing that. So the offset for a plane is the total length of all preceding buffers and planes. In addition, bit 30 of the offset field is used to signal if it is a capture or output buffer in the v4l2-mem2mem framework (see the DST_QUEUE_OFF_BASE define). In addition, the offset has to be page aligned, so the lower 12 bits are 0. That leaves only 32-1-12 = 19 bits for the offset. Since we allow 8 planes per buffer, that means that we can't have more than 65536 buffers given a 32 bit offset. I think the way m.offset is calculated should change drastically: it should instead be something like this: m.offset = ((vb->index << 3) | plane) << PAGE_SHIFT; and DST_QUEUE_OFF_BASE should use bit 31 instead of 30. Parsing the offset would be much simpler as well, since you can just decode the index and plane numbers directly rather than using the cumbersome __find_plane_by_offset() function. The currently mechanism will fail completely once you can delete buffers, and this proposal will simplify and improve the code, so I think it should be done first. Regards, Hans > } > @@ -397,6 +405,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 > @@ -425,9 +464,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); > @@ -437,7 +474,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) { > @@ -445,7 +486,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; > } > @@ -460,7 +501,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; > } > @@ -483,7 +524,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; > > @@ -511,7 +552,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); > @@ -551,15 +592,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", > @@ -591,8 +637,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; > @@ -628,7 +679,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; > @@ -636,7 +692,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); > > @@ -1547,7 +1606,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)); > @@ -1618,7 +1683,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); > } > @@ -1646,7 +1715,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) { > @@ -2022,12 +2096,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)); > } > @@ -2061,9 +2141,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() > @@ -2215,7 +2300,10 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off, > * return its buffer and plane numbers. > */ > for (buffer = 0; buffer < q->num_buffers; ++buffer) { > - vb = q->bufs[buffer]; > + vb = vb2_get_buffer(q, buffer); > + > + if (!vb) > + continue; > > for (plane = 0; plane < vb->num_planes; ++plane) { > if (vb->planes[plane].m.offset == off) { > @@ -2262,7 +2350,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 +2432,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 +2495,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 +2729,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 +2784,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 +2799,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 +2937,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 +2991,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 +3022,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 +3093,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 +3104,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 44b830ae01d8..e2c371c2fde5 100644 > --- a/drivers/media/platform/amphion/vpu_dbg.c > +++ b/drivers/media/platform/amphion/vpu_dbg.c > @@ -133,11 +133,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], > @@ -148,11 +155,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 0051f372a66c..ea37069ba355 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/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c > index cf16cf2807f0..6532a69f1fa8 100644 > --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c > +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c > @@ -1701,7 +1701,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 384f31fc66c5..8df301ab41ad 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c > +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c > @@ -1077,7 +1077,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];
On 21/08/2023 16:24, Hans Verkuil wrote: > Hi Benjamin, > > On 05/07/2023 14:10, 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> >> --- >> .../media/common/videobuf2/videobuf2-core.c | 215 ++++++++++++++---- >> .../media/common/videobuf2/videobuf2-v4l2.c | 28 ++- >> drivers/media/platform/amphion/vpu_dbg.c | 22 +- >> .../platform/mediatek/jpeg/mtk_jpeg_core.c | 6 +- >> .../vcodec/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, 240 insertions(+), 67 deletions(-) >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >> index cf6727d9c81f..42fd3984c2bc 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-core.c >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >> @@ -359,8 +359,16 @@ static void __setup_offsets(struct vb2_buffer *vb) >> unsigned long off = 0; >> >> if (vb->index) { >> - struct vb2_buffer *prev = q->bufs[vb->index - 1]; >> - struct vb2_plane *p = &prev->planes[prev->num_planes - 1]; >> + struct vb2_buffer *prev; >> + struct vb2_plane *p; >> + >> + prev = vb2_get_buffer(q, vb->index - 1); >> + if (!prev) { >> + dprintk(q, 1, "can't find the requested buffer\n"); >> + return; >> + } >> + >> + p = &prev->planes[prev->num_planes - 1]; >> >> off = PAGE_ALIGN(p->m.offset + p->length); > > This does not work. Before this patch there should be a patch that changes > the way the offset is determined since it affects this functionality. > > __setup_offsets() fills in the m.offset field for each plane in the buffer. > In the current code it starts with 0 for the first buffer, adds the length > for each plane to it, and for the next buffer it continues doing that. So > the offset for a plane is the total length of all preceding buffers and planes. > > In addition, bit 30 of the offset field is used to signal if it is a capture > or output buffer in the v4l2-mem2mem framework (see the DST_QUEUE_OFF_BASE > define). > > In addition, the offset has to be page aligned, so the lower 12 bits are 0. > That leaves only 32-1-12 = 19 bits for the offset. > > Since we allow 8 planes per buffer, that means that we can't have more than > 65536 buffers given a 32 bit offset. > > I think the way m.offset is calculated should change drastically: it should > instead be something like this: > > m.offset = ((vb->index << 3) | plane) << PAGE_SHIFT; > > and DST_QUEUE_OFF_BASE should use bit 31 instead of 30. > > Parsing the offset would be much simpler as well, since you can just decode > the index and plane numbers directly rather than using the cumbersome > __find_plane_by_offset() function. > > The currently mechanism will fail completely once you can delete buffers, > and this proposal will simplify and improve the code, so I think it should > be done first. I could have saved myself some work if I read the other patches first. I see that patch 3/10 addresses this. But for the v5, just move that patch to the beginning of the series. I would also suggest changing DST_QUEUE_OFF_BASE to use bit 31 to allow for 16 bits instead of 15 for the buffer index. Regards, Hans > > Regards, > > Hans > >> } >> @@ -397,6 +405,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 >> @@ -425,9 +464,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); >> @@ -437,7 +474,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) { >> @@ -445,7 +486,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; >> } >> @@ -460,7 +501,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; >> } >> @@ -483,7 +524,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; >> >> @@ -511,7 +552,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); >> @@ -551,15 +592,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", >> @@ -591,8 +637,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; >> @@ -628,7 +679,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; >> @@ -636,7 +692,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); >> >> @@ -1547,7 +1606,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)); >> @@ -1618,7 +1683,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); >> } >> @@ -1646,7 +1715,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) { >> @@ -2022,12 +2096,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)); >> } >> @@ -2061,9 +2141,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() >> @@ -2215,7 +2300,10 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off, >> * return its buffer and plane numbers. >> */ >> for (buffer = 0; buffer < q->num_buffers; ++buffer) { >> - vb = q->bufs[buffer]; >> + vb = vb2_get_buffer(q, buffer); >> + >> + if (!vb) >> + continue; >> >> for (plane = 0; plane < vb->num_planes; ++plane) { >> if (vb->planes[plane].m.offset == off) { >> @@ -2262,7 +2350,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 +2432,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 +2495,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 +2729,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 +2784,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 +2799,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 +2937,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 +2991,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 +3022,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 +3093,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 +3104,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 44b830ae01d8..e2c371c2fde5 100644 >> --- a/drivers/media/platform/amphion/vpu_dbg.c >> +++ b/drivers/media/platform/amphion/vpu_dbg.c >> @@ -133,11 +133,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], >> @@ -148,11 +155,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 0051f372a66c..ea37069ba355 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/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c >> index cf16cf2807f0..6532a69f1fa8 100644 >> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c >> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c >> @@ -1701,7 +1701,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 384f31fc66c5..8df301ab41ad 100644 >> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c >> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c >> @@ -1077,7 +1077,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]; >
Le 21/08/2023 à 16:31, Hans Verkuil a écrit : > On 21/08/2023 16:24, Hans Verkuil wrote: >> Hi Benjamin, >> >> On 05/07/2023 14:10, 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> >>> --- >>> .../media/common/videobuf2/videobuf2-core.c | 215 ++++++++++++++---- >>> .../media/common/videobuf2/videobuf2-v4l2.c | 28 ++- >>> drivers/media/platform/amphion/vpu_dbg.c | 22 +- >>> .../platform/mediatek/jpeg/mtk_jpeg_core.c | 6 +- >>> .../vcodec/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, 240 insertions(+), 67 deletions(-) >>> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >>> index cf6727d9c81f..42fd3984c2bc 100644 >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >>> @@ -359,8 +359,16 @@ static void __setup_offsets(struct vb2_buffer *vb) >>> unsigned long off = 0; >>> >>> if (vb->index) { >>> - struct vb2_buffer *prev = q->bufs[vb->index - 1]; >>> - struct vb2_plane *p = &prev->planes[prev->num_planes - 1]; >>> + struct vb2_buffer *prev; >>> + struct vb2_plane *p; >>> + >>> + prev = vb2_get_buffer(q, vb->index - 1); >>> + if (!prev) { >>> + dprintk(q, 1, "can't find the requested buffer\n"); >>> + return; >>> + } >>> + >>> + p = &prev->planes[prev->num_planes - 1]; >>> >>> off = PAGE_ALIGN(p->m.offset + p->length); >> This does not work. Before this patch there should be a patch that changes >> the way the offset is determined since it affects this functionality. >> >> __setup_offsets() fills in the m.offset field for each plane in the buffer. >> In the current code it starts with 0 for the first buffer, adds the length >> for each plane to it, and for the next buffer it continues doing that. So >> the offset for a plane is the total length of all preceding buffers and planes. >> >> In addition, bit 30 of the offset field is used to signal if it is a capture >> or output buffer in the v4l2-mem2mem framework (see the DST_QUEUE_OFF_BASE >> define). >> >> In addition, the offset has to be page aligned, so the lower 12 bits are 0. >> That leaves only 32-1-12 = 19 bits for the offset. >> >> Since we allow 8 planes per buffer, that means that we can't have more than >> 65536 buffers given a 32 bit offset. >> >> I think the way m.offset is calculated should change drastically: it should >> instead be something like this: >> >> m.offset = ((vb->index << 3) | plane) << PAGE_SHIFT; >> >> and DST_QUEUE_OFF_BASE should use bit 31 instead of 30. >> >> Parsing the offset would be much simpler as well, since you can just decode >> the index and plane numbers directly rather than using the cumbersome >> __find_plane_by_offset() function. >> >> The currently mechanism will fail completely once you can delete buffers, >> and this proposal will simplify and improve the code, so I think it should >> be done first. > I could have saved myself some work if I read the other patches first. > > I see that patch 3/10 addresses this. But for the v5, just move that patch > to the beginning of the series. I will try to do that but these patches hit the same pieces of code so changing the order isn't that simple. > > I would also suggest changing DST_QUEUE_OFF_BASE to use bit 31 to allow for > 16 bits instead of 15 for the buffer index. DST_QUEUE_OFF_BASE is used with PAGE_SHIFT in v4l2-mem2mem and s5p_mfc in mmap/unmap functions. I'm bit afraid to change that. With my change each queue can store up to 0xffff (65535) buffers. Sound large enough for me ;-) Benjamin > > Regards, > > Hans > >> Regards, >> >> Hans >> >>> } >>> @@ -397,6 +405,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 >>> @@ -425,9 +464,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); >>> @@ -437,7 +474,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) { >>> @@ -445,7 +486,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; >>> } >>> @@ -460,7 +501,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; >>> } >>> @@ -483,7 +524,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; >>> >>> @@ -511,7 +552,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); >>> @@ -551,15 +592,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", >>> @@ -591,8 +637,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; >>> @@ -628,7 +679,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; >>> @@ -636,7 +692,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); >>> >>> @@ -1547,7 +1606,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)); >>> @@ -1618,7 +1683,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); >>> } >>> @@ -1646,7 +1715,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) { >>> @@ -2022,12 +2096,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)); >>> } >>> @@ -2061,9 +2141,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() >>> @@ -2215,7 +2300,10 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off, >>> * return its buffer and plane numbers. >>> */ >>> for (buffer = 0; buffer < q->num_buffers; ++buffer) { >>> - vb = q->bufs[buffer]; >>> + vb = vb2_get_buffer(q, buffer); >>> + >>> + if (!vb) >>> + continue; >>> >>> for (plane = 0; plane < vb->num_planes; ++plane) { >>> if (vb->planes[plane].m.offset == off) { >>> @@ -2262,7 +2350,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 +2432,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 +2495,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 +2729,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 +2784,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 +2799,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 +2937,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 +2991,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 +3022,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 +3093,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 +3104,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 44b830ae01d8..e2c371c2fde5 100644 >>> --- a/drivers/media/platform/amphion/vpu_dbg.c >>> +++ b/drivers/media/platform/amphion/vpu_dbg.c >>> @@ -133,11 +133,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], >>> @@ -148,11 +155,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 0051f372a66c..ea37069ba355 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/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c >>> index cf16cf2807f0..6532a69f1fa8 100644 >>> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c >>> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c >>> @@ -1701,7 +1701,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 384f31fc66c5..8df301ab41ad 100644 >>> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c >>> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c >>> @@ -1077,7 +1077,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]; >
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index cf6727d9c81f..42fd3984c2bc 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -359,8 +359,16 @@ static void __setup_offsets(struct vb2_buffer *vb) unsigned long off = 0; if (vb->index) { - struct vb2_buffer *prev = q->bufs[vb->index - 1]; - struct vb2_plane *p = &prev->planes[prev->num_planes - 1]; + struct vb2_buffer *prev; + struct vb2_plane *p; + + prev = vb2_get_buffer(q, vb->index - 1); + if (!prev) { + dprintk(q, 1, "can't find the requested buffer\n"); + return; + } + + p = &prev->planes[prev->num_planes - 1]; off = PAGE_ALIGN(p->m.offset + p->length); } @@ -397,6 +405,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 @@ -425,9 +464,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); @@ -437,7 +474,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) { @@ -445,7 +486,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; } @@ -460,7 +501,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; } @@ -483,7 +524,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; @@ -511,7 +552,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); @@ -551,15 +592,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", @@ -591,8 +637,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; @@ -628,7 +679,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; @@ -636,7 +692,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); @@ -1547,7 +1606,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)); @@ -1618,7 +1683,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); } @@ -1646,7 +1715,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) { @@ -2022,12 +2096,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)); } @@ -2061,9 +2141,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() @@ -2215,7 +2300,10 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off, * return its buffer and plane numbers. */ for (buffer = 0; buffer < q->num_buffers; ++buffer) { - vb = q->bufs[buffer]; + vb = vb2_get_buffer(q, buffer); + + if (!vb) + continue; for (plane = 0; plane < vb->num_planes; ++plane) { if (vb->planes[plane].m.offset == off) { @@ -2262,7 +2350,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 +2432,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 +2495,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 +2729,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 +2784,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 +2799,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 +2937,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 +2991,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 +3022,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 +3093,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 +3104,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 44b830ae01d8..e2c371c2fde5 100644 --- a/drivers/media/platform/amphion/vpu_dbg.c +++ b/drivers/media/platform/amphion/vpu_dbg.c @@ -133,11 +133,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], @@ -148,11 +155,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 0051f372a66c..ea37069ba355 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/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c index cf16cf2807f0..6532a69f1fa8 100644 --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c @@ -1701,7 +1701,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 384f31fc66c5..8df301ab41ad 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c @@ -1077,7 +1077,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> --- .../media/common/videobuf2/videobuf2-core.c | 215 ++++++++++++++---- .../media/common/videobuf2/videobuf2-v4l2.c | 28 ++- drivers/media/platform/amphion/vpu_dbg.c | 22 +- .../platform/mediatek/jpeg/mtk_jpeg_core.c | 6 +- .../vcodec/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, 240 insertions(+), 67 deletions(-)