| Message ID | 20230914133323.198857-46-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add DELETE_BUF ioctl | expand |
Hi Benjamin, kernel test robot noticed the following build errors: [auto build test ERROR on linus/master] [also build test ERROR on v6.6-rc1] [cannot apply to next-20230914] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Rework-offset-cookie-encoding-pattern/20230914-221757 base: linus/master patch link: https://lore.kernel.org/r/20230914133323.198857-46-benjamin.gaignard%40collabora.com patch subject: [PATCH v7 45/49] media: core: Add bitmap manage bufs array entries config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230915/202309150835.kxjWQyEU-lkp@intel.com/config) compiler: sparc64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230915/202309150835.kxjWQyEU-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202309150835.kxjWQyEU-lkp@intel.com/ All errors (new ones prefixed by >>): samples/v4l/v4l2-pci-skeleton.c: In function 'queue_setup': >> samples/v4l/v4l2-pci-skeleton.c:170:15: error: 'struct vb2_queue' has no member named 'num_buffers' 170 | if (vq->num_buffers + *nbuffers < 3) | ^~ samples/v4l/v4l2-pci-skeleton.c:171:35: error: 'struct vb2_queue' has no member named 'num_buffers' 171 | *nbuffers = 3 - vq->num_buffers; | ^~ -- drivers/input/touchscreen/sur40.c: In function 'sur40_queue_setup': >> drivers/input/touchscreen/sur40.c:851:14: error: 'struct vb2_queue' has no member named 'num_buffers' 851 | if (q->num_buffers + *nbuffers < 3) | ^~ drivers/input/touchscreen/sur40.c:852:34: error: 'struct vb2_queue' has no member named 'num_buffers' 852 | *nbuffers = 3 - q->num_buffers; | ^~ vim +170 samples/v4l/v4l2-pci-skeleton.c 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 145 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 146 /* 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 147 * Setup the constraints of the queue: besides setting the number of planes 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 148 * per buffer and the size and allocation context of each plane, it also 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 149 * checks if sufficient buffers have been allocated. Usually 3 is a good 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 150 * minimum number: many DMA engines need a minimum of 2 buffers in the 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 151 * queue and you need to have another available for userspace processing. 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 152 */ df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28 153 static int queue_setup(struct vb2_queue *vq, 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 154 unsigned int *nbuffers, unsigned int *nplanes, 36c0f8b32c4bd4 samples/v4l/v4l2-pci-skeleton.c Hans Verkuil 2016-04-15 155 unsigned int sizes[], struct device *alloc_devs[]) 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 156 { 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 157 struct skeleton *skel = vb2_get_drv_priv(vq); 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 158 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 159 skel->field = skel->format.field; 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 160 if (skel->field == V4L2_FIELD_ALTERNATE) { 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 161 /* 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 162 * You cannot use read() with FIELD_ALTERNATE since the field 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 163 * information (TOP/BOTTOM) cannot be passed back to the user. 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 164 */ 3130a28a1568b1 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-23 165 if (vb2_fileio_is_active(vq)) 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 166 return -EINVAL; 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 167 skel->field = V4L2_FIELD_TOP; 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 168 } 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 169 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 @170 if (vq->num_buffers + *nbuffers < 3) 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 171 *nbuffers = 3 - vq->num_buffers; 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 172 df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28 173 if (*nplanes) df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28 174 return sizes[0] < skel->format.sizeimage ? -EINVAL : 0; 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 175 *nplanes = 1; df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28 176 sizes[0] = skel->format.sizeimage; 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 177 return 0; 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 178 } 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 179
Le 15/09/2023 à 02:47, kernel test robot a écrit : > Hi Benjamin, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on linus/master] > [also build test ERROR on v6.6-rc1] > [cannot apply to next-20230914] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Rework-offset-cookie-encoding-pattern/20230914-221757 > base: linus/master > patch link: https://lore.kernel.org/r/20230914133323.198857-46-benjamin.gaignard%40collabora.com > patch subject: [PATCH v7 45/49] media: core: Add bitmap manage bufs array entries > config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230915/202309150835.kxjWQyEU-lkp@intel.com/config) > compiler: sparc64-linux-gcc (GCC) 13.2.0 > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230915/202309150835.kxjWQyEU-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202309150835.kxjWQyEU-lkp@intel.com/ > > All errors (new ones prefixed by >>): > > samples/v4l/v4l2-pci-skeleton.c: In function 'queue_setup': >>> samples/v4l/v4l2-pci-skeleton.c:170:15: error: 'struct vb2_queue' has no member named 'num_buffers' > 170 | if (vq->num_buffers + *nbuffers < 3) > | ^~ > samples/v4l/v4l2-pci-skeleton.c:171:35: error: 'struct vb2_queue' has no member named 'num_buffers' > 171 | *nbuffers = 3 - vq->num_buffers; > | ^~ > -- > drivers/input/touchscreen/sur40.c: In function 'sur40_queue_setup': >>> drivers/input/touchscreen/sur40.c:851:14: error: 'struct vb2_queue' has no member named 'num_buffers' > 851 | if (q->num_buffers + *nbuffers < 3) > | ^~ > drivers/input/touchscreen/sur40.c:852:34: error: 'struct vb2_queue' has no member named 'num_buffers' > 852 | *nbuffers = 3 - q->num_buffers; > | ^~ > > > vim +170 samples/v4l/v4l2-pci-skeleton.c v4l2 drivers outside drivers/media or drivers/staging/media direct directories. I have miss them. I will fix that in v8. Regards, Benjamin > > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 145 > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 146 /* > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 147 * Setup the constraints of the queue: besides setting the number of planes > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 148 * per buffer and the size and allocation context of each plane, it also > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 149 * checks if sufficient buffers have been allocated. Usually 3 is a good > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 150 * minimum number: many DMA engines need a minimum of 2 buffers in the > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 151 * queue and you need to have another available for userspace processing. > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 152 */ > df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28 153 static int queue_setup(struct vb2_queue *vq, > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 154 unsigned int *nbuffers, unsigned int *nplanes, > 36c0f8b32c4bd4 samples/v4l/v4l2-pci-skeleton.c Hans Verkuil 2016-04-15 155 unsigned int sizes[], struct device *alloc_devs[]) > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 156 { > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 157 struct skeleton *skel = vb2_get_drv_priv(vq); > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 158 > 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 159 skel->field = skel->format.field; > 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 160 if (skel->field == V4L2_FIELD_ALTERNATE) { > 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 161 /* > 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 162 * You cannot use read() with FIELD_ALTERNATE since the field > 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 163 * information (TOP/BOTTOM) cannot be passed back to the user. > 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 164 */ > 3130a28a1568b1 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-23 165 if (vb2_fileio_is_active(vq)) > 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 166 return -EINVAL; > 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 167 skel->field = V4L2_FIELD_TOP; > 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 168 } > 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11 169 > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 @170 if (vq->num_buffers + *nbuffers < 3) > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 171 *nbuffers = 3 - vq->num_buffers; > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 172 > df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28 173 if (*nplanes) > df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28 174 return sizes[0] < skel->format.sizeimage ? -EINVAL : 0; > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 175 *nplanes = 1; > df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28 176 sizes[0] = skel->format.sizeimage; > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 177 return 0; > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 178 } > 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 179 >
On 14/09/2023 15:33, Benjamin Gaignard wrote: > Add a bitmap field to know which of bufs array entries are > used or not. > Remove no more used num_buffers field from queue structure. > Use bitmap_find_next_zero_area() to find the first possible > range when creating new buffers to fill the gaps. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > .../media/common/videobuf2/videobuf2-core.c | 55 +++++++++++++++---- > include/media/videobuf2-core.h | 9 ++- > 2 files changed, 51 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index a4c2fae8705d..c5d4a388331b 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -411,10 +411,11 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb) > */ > static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index) > { > - if (index < q->max_allowed_buffers && !q->bufs[index]) { > + if (index < q->max_allowed_buffers && !test_bit(index, q->bufs_map)) { I think bufs_bitmap would be a better name. > q->bufs[index] = vb; > vb->index = index; > vb->vb2_queue = q; > + set_bit(index, q->bufs_map); > return true; > } > > @@ -428,9 +429,10 @@ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns > */ > static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb) > { > - if (vb->index < q->max_allowed_buffers) { > + if (vb->index < q->max_allowed_buffers && test_bit(vb->index, q->bufs_map)) { As mentioned in past reviews, I think these tests can be dropped, it makes no sense that these ever fail. > q->bufs[vb->index] = NULL; > vb->vb2_queue = NULL; > + clear_bit(vb->index, q->bufs_map); > } > } > > @@ -451,11 +453,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, > unsigned long first_index; > int ret; > > - /* Ensure that q->num_buffers+num_buffers is below q->max_allowed_buffers */ > + /* Ensure that the number of already queue + num_buffers is below q->max_allowed_buffers */ Hmm, how about: /* Ensure that vb2_get_num_buffers(q) + num_buffers is no more than q->max_allowed_buffers */ > num_buffers = min_t(unsigned int, num_buffers, > q->max_allowed_buffers - vb2_get_num_buffers(q)); > > - first_index = vb2_get_num_buffers(q); > + first_index = bitmap_find_next_zero_area(q->bufs_map, q->max_allowed_buffers, > + 0, num_buffers, 0); > > if (first_index >= q->max_allowed_buffers) > return 0; > @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) > > struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, unsigned int index) > { > - if (index < q->num_buffers) > + if (!q->bufs_map || !q->bufs) > + return NULL; I don't think this can ever happen. > + > + if (index >= q->max_allowed_buffers) > + return NULL; > + > + if (test_bit(index, q->bufs_map)) > return q->bufs[index]; > return NULL; > } > @@ -683,7 +692,10 @@ EXPORT_SYMBOL_GPL(vb2_get_buffer); > > unsigned int vb2_get_num_buffers(struct vb2_queue *q) > { > - return q->num_buffers; > + if (!q->bufs_map) > + return 0; Ditto. > + > + return bitmap_weight(q->bufs_map, q->max_allowed_buffers); > } > EXPORT_SYMBOL_GPL(vb2_get_num_buffers); > > @@ -899,6 +911,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); > if (!q->bufs) > ret = -ENOMEM; > + > + if (!q->bufs_map) > + q->bufs_map = bitmap_zalloc(q->max_allowed_buffers, GFP_KERNEL); > + if (!q->bufs_map) { > + ret = -ENOMEM; > + kfree(q->bufs); > + q->bufs = NULL; > + } > q->memory = memory; > mutex_unlock(&q->mmap_lock); > if (ret) > @@ -968,7 +988,6 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > } > > mutex_lock(&q->mmap_lock); > - q->num_buffers = allocated_buffers; > > if (ret < 0) { > /* > @@ -995,6 +1014,10 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > mutex_lock(&q->mmap_lock); > q->memory = VB2_MEMORY_UNKNOWN; > mutex_unlock(&q->mmap_lock); > + kfree(q->bufs); > + q->bufs = NULL; > + bitmap_free(q->bufs_map); > + q->bufs_map = NULL; > return ret; > } > EXPORT_SYMBOL_GPL(vb2_core_reqbufs); > @@ -1031,9 +1054,19 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > q->memory = memory; > if (!q->bufs) > q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); > - if (!q->bufs) > + if (!q->bufs) { > + ret = -ENOMEM; > + goto unlock; > + } > + if (!q->bufs_map) > + q->bufs_map = bitmap_zalloc(q->max_allowed_buffers, GFP_KERNEL); > + if (!q->bufs_map) { > ret = -ENOMEM; > + kfree(q->bufs); > + q->bufs = NULL; > + } > mutex_unlock(&q->mmap_lock); > +unlock: > if (ret) > return ret; > q->waiting_for_buffers = !q->is_output; > @@ -1095,7 +1128,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > } > > mutex_lock(&q->mmap_lock); > - q->num_buffers += allocated_buffers; > > if (ret < 0) { > /* > @@ -2588,6 +2620,9 @@ void vb2_core_queue_release(struct vb2_queue *q) > __vb2_queue_free(q, q->max_allowed_buffers); > kfree(q->bufs); > q->bufs = NULL; > + bitmap_free(q->bufs_map); > + q->bufs_map = NULL; > + > mutex_unlock(&q->mmap_lock); > } > EXPORT_SYMBOL_GPL(vb2_core_queue_release); > @@ -2944,7 +2979,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_ > * Check if we need to dequeue the buffer. > */ > index = fileio->cur_index; > - if (index >= q->num_buffers) { > + if (!test_bit(index, q->bufs_map)) { > struct vb2_buffer *b; > > /* > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 19c93d8eb7c8..734437236cc4 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -557,7 +557,7 @@ struct vb2_buf_ops { > * @memory: current memory type used > * @dma_dir: DMA mapping direction. > * @bufs: videobuf2 buffer structures > - * @num_buffers: number of allocated/used buffers > + * @bufs_map: bitmap to manage bufs entries. > * @max_allowed_buffers: upper limit of number of allocated/used buffers > * @queued_list: list of buffers currently queued from userspace > * @queued_count: number of buffers queued and ready for streaming. > @@ -621,7 +621,7 @@ struct vb2_queue { > unsigned int memory; > enum dma_data_direction dma_dir; > struct vb2_buffer **bufs; > - unsigned int num_buffers; > + unsigned long *bufs_map; > unsigned int max_allowed_buffers; > > struct list_head queued_list; > @@ -1151,7 +1151,10 @@ static inline bool vb2_fileio_is_active(struct vb2_queue *q) > */ > static inline bool vb2_is_busy(struct vb2_queue *q) > { > - return (q->num_buffers > 0); > + if (!q->bufs_map) > + return false; I don't think this can happen. > + > + return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0); How about: return vb2_get_num_buffers(q) > 0; > } > > /** Regards, Hans
Le 19/09/2023 à 17:00, Hans Verkuil a écrit : > On 14/09/2023 15:33, Benjamin Gaignard wrote: >> Add a bitmap field to know which of bufs array entries are >> used or not. >> Remove no more used num_buffers field from queue structure. >> Use bitmap_find_next_zero_area() to find the first possible >> range when creating new buffers to fill the gaps. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> .../media/common/videobuf2/videobuf2-core.c | 55 +++++++++++++++---- >> include/media/videobuf2-core.h | 9 ++- >> 2 files changed, 51 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >> index a4c2fae8705d..c5d4a388331b 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-core.c >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >> @@ -411,10 +411,11 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb) >> */ >> static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index) >> { >> - if (index < q->max_allowed_buffers && !q->bufs[index]) { >> + if (index < q->max_allowed_buffers && !test_bit(index, q->bufs_map)) { > I think bufs_bitmap would be a better name. Ok I will change it > >> q->bufs[index] = vb; >> vb->index = index; >> vb->vb2_queue = q; >> + set_bit(index, q->bufs_map); >> return true; >> } >> >> @@ -428,9 +429,10 @@ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns >> */ >> static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb) >> { >> - if (vb->index < q->max_allowed_buffers) { >> + if (vb->index < q->max_allowed_buffers && test_bit(vb->index, q->bufs_map)) { > As mentioned in past reviews, I think these tests can be dropped, it makes no > sense that these ever fail. I will drop them. > >> q->bufs[vb->index] = NULL; >> vb->vb2_queue = NULL; >> + clear_bit(vb->index, q->bufs_map); >> } >> } >> >> @@ -451,11 +453,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, >> unsigned long first_index; >> int ret; >> >> - /* Ensure that q->num_buffers+num_buffers is below q->max_allowed_buffers */ >> + /* Ensure that the number of already queue + num_buffers is below q->max_allowed_buffers */ > Hmm, how about: > > /* Ensure that vb2_get_num_buffers(q) + num_buffers is no more than q->max_allowed_buffers */ sure > >> num_buffers = min_t(unsigned int, num_buffers, >> q->max_allowed_buffers - vb2_get_num_buffers(q)); >> >> - first_index = vb2_get_num_buffers(q); >> + first_index = bitmap_find_next_zero_area(q->bufs_map, q->max_allowed_buffers, >> + 0, num_buffers, 0); >> >> if (first_index >= q->max_allowed_buffers) >> return 0; >> @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) >> >> struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, unsigned int index) >> { >> - if (index < q->num_buffers) >> + if (!q->bufs_map || !q->bufs) >> + return NULL; > I don't think this can ever happen. I got kernel crash without them. I will keep them. > >> + >> + if (index >= q->max_allowed_buffers) >> + return NULL; >> + >> + if (test_bit(index, q->bufs_map)) >> return q->bufs[index]; >> return NULL; >> } >> @@ -683,7 +692,10 @@ EXPORT_SYMBOL_GPL(vb2_get_buffer); >> >> unsigned int vb2_get_num_buffers(struct vb2_queue *q) >> { >> - return q->num_buffers; >> + if (!q->bufs_map) >> + return 0; > Ditto. > >> + >> + return bitmap_weight(q->bufs_map, q->max_allowed_buffers); >> } >> EXPORT_SYMBOL_GPL(vb2_get_num_buffers); >> >> @@ -899,6 +911,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >> q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); >> if (!q->bufs) >> ret = -ENOMEM; >> + >> + if (!q->bufs_map) >> + q->bufs_map = bitmap_zalloc(q->max_allowed_buffers, GFP_KERNEL); >> + if (!q->bufs_map) { >> + ret = -ENOMEM; >> + kfree(q->bufs); >> + q->bufs = NULL; >> + } >> q->memory = memory; >> mutex_unlock(&q->mmap_lock); >> if (ret) >> @@ -968,7 +988,6 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >> } >> >> mutex_lock(&q->mmap_lock); >> - q->num_buffers = allocated_buffers; >> >> if (ret < 0) { >> /* >> @@ -995,6 +1014,10 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >> mutex_lock(&q->mmap_lock); >> q->memory = VB2_MEMORY_UNKNOWN; >> mutex_unlock(&q->mmap_lock); >> + kfree(q->bufs); >> + q->bufs = NULL; >> + bitmap_free(q->bufs_map); >> + q->bufs_map = NULL; >> return ret; >> } >> EXPORT_SYMBOL_GPL(vb2_core_reqbufs); >> @@ -1031,9 +1054,19 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, >> q->memory = memory; >> if (!q->bufs) >> q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); >> - if (!q->bufs) >> + if (!q->bufs) { >> + ret = -ENOMEM; >> + goto unlock; >> + } >> + if (!q->bufs_map) >> + q->bufs_map = bitmap_zalloc(q->max_allowed_buffers, GFP_KERNEL); >> + if (!q->bufs_map) { >> ret = -ENOMEM; >> + kfree(q->bufs); >> + q->bufs = NULL; >> + } >> mutex_unlock(&q->mmap_lock); >> +unlock: >> if (ret) >> return ret; >> q->waiting_for_buffers = !q->is_output; >> @@ -1095,7 +1128,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, >> } >> >> mutex_lock(&q->mmap_lock); >> - q->num_buffers += allocated_buffers; >> >> if (ret < 0) { >> /* >> @@ -2588,6 +2620,9 @@ void vb2_core_queue_release(struct vb2_queue *q) >> __vb2_queue_free(q, q->max_allowed_buffers); >> kfree(q->bufs); >> q->bufs = NULL; >> + bitmap_free(q->bufs_map); >> + q->bufs_map = NULL; >> + >> mutex_unlock(&q->mmap_lock); >> } >> EXPORT_SYMBOL_GPL(vb2_core_queue_release); >> @@ -2944,7 +2979,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_ >> * Check if we need to dequeue the buffer. >> */ >> index = fileio->cur_index; >> - if (index >= q->num_buffers) { >> + if (!test_bit(index, q->bufs_map)) { >> struct vb2_buffer *b; >> >> /* >> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h >> index 19c93d8eb7c8..734437236cc4 100644 >> --- a/include/media/videobuf2-core.h >> +++ b/include/media/videobuf2-core.h >> @@ -557,7 +557,7 @@ struct vb2_buf_ops { >> * @memory: current memory type used >> * @dma_dir: DMA mapping direction. >> * @bufs: videobuf2 buffer structures >> - * @num_buffers: number of allocated/used buffers >> + * @bufs_map: bitmap to manage bufs entries. >> * @max_allowed_buffers: upper limit of number of allocated/used buffers >> * @queued_list: list of buffers currently queued from userspace >> * @queued_count: number of buffers queued and ready for streaming. >> @@ -621,7 +621,7 @@ struct vb2_queue { >> unsigned int memory; >> enum dma_data_direction dma_dir; >> struct vb2_buffer **bufs; >> - unsigned int num_buffers; >> + unsigned long *bufs_map; >> unsigned int max_allowed_buffers; >> >> struct list_head queued_list; >> @@ -1151,7 +1151,10 @@ static inline bool vb2_fileio_is_active(struct vb2_queue *q) >> */ >> static inline bool vb2_is_busy(struct vb2_queue *q) >> { >> - return (q->num_buffers > 0); >> + if (!q->bufs_map) >> + return false; > I don't think this can happen. > >> + >> + return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0); > How about: > > return vb2_get_num_buffers(q) > 0; vb2_get_num_buffers is defined in videobuf2-core.c, I'm not sure that an inline function could depend of a module function. Regards, Benjamin > >> } >> >> /** > Regards, > > Hans >
On 20/09/2023 16:30, Benjamin Gaignard wrote: > <snip> >>> num_buffers = min_t(unsigned int, num_buffers, >>> q->max_allowed_buffers - vb2_get_num_buffers(q)); >>> - first_index = vb2_get_num_buffers(q); >>> + first_index = bitmap_find_next_zero_area(q->bufs_map, q->max_allowed_buffers, >>> + 0, num_buffers, 0); >>> if (first_index >= q->max_allowed_buffers) >>> return 0; >>> @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) >>> struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, unsigned int index) >>> { >>> - if (index < q->num_buffers) >>> + if (!q->bufs_map || !q->bufs) >>> + return NULL; >> I don't think this can ever happen. > > I got kernel crash without them. > I will keep them. What is the backtrace? How can this happen? It feels wrong that this can be called with a vb2_queue that apparently is not properly initialized. >>> + >>> + return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0); >> How about: >> >> return vb2_get_num_buffers(q) > 0; > > vb2_get_num_buffers is defined in videobuf2-core.c, I'm not sure that > an inline function could depend of a module function. Not a problem. E.g. v4l2-ctrls.h is full of such static inlines. Regards, Hans
Le 20/09/2023 à 16:56, Hans Verkuil a écrit : > On 20/09/2023 16:30, Benjamin Gaignard wrote: > <snip> > >>>> num_buffers = min_t(unsigned int, num_buffers, >>>> q->max_allowed_buffers - vb2_get_num_buffers(q)); >>>> - first_index = vb2_get_num_buffers(q); >>>> + first_index = bitmap_find_next_zero_area(q->bufs_map, q->max_allowed_buffers, >>>> + 0, num_buffers, 0); >>>> if (first_index >= q->max_allowed_buffers) >>>> return 0; >>>> @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) >>>> struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, unsigned int index) >>>> { >>>> - if (index < q->num_buffers) >>>> + if (!q->bufs_map || !q->bufs) >>>> + return NULL; >>> I don't think this can ever happen. >> I got kernel crash without them. >> I will keep them. > What is the backtrace? How can this happen? It feels wrong that this can be > called with a vb2_queue that apparently is not properly initialized. I will add backtrace when doing test on v8 > > >>>> + >>>> + return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0); >>> How about: >>> >>> return vb2_get_num_buffers(q) > 0; >> vb2_get_num_buffers is defined in videobuf2-core.c, I'm not sure that >> an inline function could depend of a module function. > Not a problem. E.g. v4l2-ctrls.h is full of such static inlines. I will change vb2_get_num_buffers() to inline function that solve the problem too. > > Regards, > > Hans >
Le 20/09/2023 à 16:56, Hans Verkuil a écrit : > On 20/09/2023 16:30, Benjamin Gaignard wrote: > <snip> > >>>> num_buffers = min_t(unsigned int, num_buffers, >>>> q->max_allowed_buffers - vb2_get_num_buffers(q)); >>>> - first_index = vb2_get_num_buffers(q); >>>> + first_index = bitmap_find_next_zero_area(q->bufs_map, q->max_allowed_buffers, >>>> + 0, num_buffers, 0); >>>> if (first_index >= q->max_allowed_buffers) >>>> return 0; >>>> @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) >>>> struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, unsigned int index) >>>> { >>>> - if (index < q->num_buffers) >>>> + if (!q->bufs_map || !q->bufs) >>>> + return NULL; >>> I don't think this can ever happen. >> I got kernel crash without them. >> I will keep them. > What is the backtrace? How can this happen? It feels wrong that this can be > called with a vb2_queue that apparently is not properly initialized. I have this log when adding dump_stack() in vb2_get_buffer() if !q->bufs_bitmap: [ 18.924627] Call trace: [ 18.927090] dump_backtrace+0x94/0xec [ 18.930787] show_stack+0x18/0x24 [ 18.934137] dump_stack_lvl+0x48/0x60 [ 18.937833] dump_stack+0x18/0x24 [ 18.941166] __vb2_queue_cancel+0x23c/0x2f0 [ 18.945365] vb2_core_queue_release+0x24/0x6c [ 18.949740] vb2_queue_release+0x10/0x1c [ 18.953677] v4l2_m2m_ctx_release+0x20/0x40 [ 18.957892] hantro_release+0x20/0x54 [ 18.961584] v4l2_release+0x74/0xec [ 18.965110] __fput+0xb4/0x274 [ 18.968205] __fput_sync+0x50/0x5c [ 18.971626] __arm64_sys_close+0x38/0x7c [ 18.975562] invoke_syscall+0x48/0x114 [ 18.979329] el0_svc_common.constprop.0+0xc0/0xe0 [ 18.984068] do_el0_svc+0x1c/0x28 [ 18.987402] el0_svc+0x40/0xe8 [ 18.990470] el0t_64_sync_handler+0x100/0x12c [ 18.994842] el0t_64_sync+0x190/0x194 This happen at boot time when hantro driver is open and close without other actions. > >>>> + >>>> + return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0); >>> How about: >>> >>> return vb2_get_num_buffers(q) > 0; >> vb2_get_num_buffers is defined in videobuf2-core.c, I'm not sure that >> an inline function could depend of a module function. > Not a problem. E.g. v4l2-ctrls.h is full of such static inlines. > > Regards, > > Hans >
On 21/09/2023 11:28, Benjamin Gaignard wrote: > > Le 20/09/2023 à 16:56, Hans Verkuil a écrit : >> On 20/09/2023 16:30, Benjamin Gaignard wrote: >> <snip> >> >>>>> num_buffers = min_t(unsigned int, num_buffers, >>>>> q->max_allowed_buffers - vb2_get_num_buffers(q)); >>>>> - first_index = vb2_get_num_buffers(q); >>>>> + first_index = bitmap_find_next_zero_area(q->bufs_map, q->max_allowed_buffers, >>>>> + 0, num_buffers, 0); >>>>> if (first_index >= q->max_allowed_buffers) >>>>> return 0; >>>>> @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) >>>>> struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, unsigned int index) >>>>> { >>>>> - if (index < q->num_buffers) >>>>> + if (!q->bufs_map || !q->bufs) >>>>> + return NULL; >>>> I don't think this can ever happen. >>> I got kernel crash without them. >>> I will keep them. >> What is the backtrace? How can this happen? It feels wrong that this can be >> called with a vb2_queue that apparently is not properly initialized. > > I have this log when adding dump_stack() in vb2_get_buffer() if !q->bufs_bitmap: > > [ 18.924627] Call trace: > [ 18.927090] dump_backtrace+0x94/0xec > [ 18.930787] show_stack+0x18/0x24 > [ 18.934137] dump_stack_lvl+0x48/0x60 > [ 18.937833] dump_stack+0x18/0x24 > [ 18.941166] __vb2_queue_cancel+0x23c/0x2f0 > [ 18.945365] vb2_core_queue_release+0x24/0x6c > [ 18.949740] vb2_queue_release+0x10/0x1c > [ 18.953677] v4l2_m2m_ctx_release+0x20/0x40 > [ 18.957892] hantro_release+0x20/0x54 > [ 18.961584] v4l2_release+0x74/0xec > [ 18.965110] __fput+0xb4/0x274 > [ 18.968205] __fput_sync+0x50/0x5c > [ 18.971626] __arm64_sys_close+0x38/0x7c > [ 18.975562] invoke_syscall+0x48/0x114 > [ 18.979329] el0_svc_common.constprop.0+0xc0/0xe0 > [ 18.984068] do_el0_svc+0x1c/0x28 > [ 18.987402] el0_svc+0x40/0xe8 > [ 18.990470] el0t_64_sync_handler+0x100/0x12c > [ 18.994842] el0t_64_sync+0x190/0x194 > > This happen at boot time when hantro driver is open and close without other actions. Ah, now I see the problem. q->bufs and q->bufs_map are allocated in vb2_core_create_bufs and vb2_core_reqbufs, but they should be allocated in vb2_queue_init: that's the counterpart of vb2_core_queue_release. With that change you shouldn't have to check for q->bufs/bufs_map anymore. Regards, Hans > >> >>>>> + >>>>> + return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0); >>>> How about: >>>> >>>> return vb2_get_num_buffers(q) > 0; >>> vb2_get_num_buffers is defined in videobuf2-core.c, I'm not sure that >>> an inline function could depend of a module function. >> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines. >> >> Regards, >> >> Hans >>
Le 21/09/2023 à 12:24, Hans Verkuil a écrit : > On 21/09/2023 11:28, Benjamin Gaignard wrote: >> Le 20/09/2023 à 16:56, Hans Verkuil a écrit : >>> On 20/09/2023 16:30, Benjamin Gaignard wrote: >>> <snip> >>> >>>>>> num_buffers = min_t(unsigned int, num_buffers, >>>>>> q->max_allowed_buffers - vb2_get_num_buffers(q)); >>>>>> - first_index = vb2_get_num_buffers(q); >>>>>> + first_index = bitmap_find_next_zero_area(q->bufs_map, q->max_allowed_buffers, >>>>>> + 0, num_buffers, 0); >>>>>> if (first_index >= q->max_allowed_buffers) >>>>>> return 0; >>>>>> @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) >>>>>> struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, unsigned int index) >>>>>> { >>>>>> - if (index < q->num_buffers) >>>>>> + if (!q->bufs_map || !q->bufs) >>>>>> + return NULL; >>>>> I don't think this can ever happen. >>>> I got kernel crash without them. >>>> I will keep them. >>> What is the backtrace? How can this happen? It feels wrong that this can be >>> called with a vb2_queue that apparently is not properly initialized. >> I have this log when adding dump_stack() in vb2_get_buffer() if !q->bufs_bitmap: >> >> [ 18.924627] Call trace: >> [ 18.927090] dump_backtrace+0x94/0xec >> [ 18.930787] show_stack+0x18/0x24 >> [ 18.934137] dump_stack_lvl+0x48/0x60 >> [ 18.937833] dump_stack+0x18/0x24 >> [ 18.941166] __vb2_queue_cancel+0x23c/0x2f0 >> [ 18.945365] vb2_core_queue_release+0x24/0x6c >> [ 18.949740] vb2_queue_release+0x10/0x1c >> [ 18.953677] v4l2_m2m_ctx_release+0x20/0x40 >> [ 18.957892] hantro_release+0x20/0x54 >> [ 18.961584] v4l2_release+0x74/0xec >> [ 18.965110] __fput+0xb4/0x274 >> [ 18.968205] __fput_sync+0x50/0x5c >> [ 18.971626] __arm64_sys_close+0x38/0x7c >> [ 18.975562] invoke_syscall+0x48/0x114 >> [ 18.979329] el0_svc_common.constprop.0+0xc0/0xe0 >> [ 18.984068] do_el0_svc+0x1c/0x28 >> [ 18.987402] el0_svc+0x40/0xe8 >> [ 18.990470] el0t_64_sync_handler+0x100/0x12c >> [ 18.994842] el0t_64_sync+0x190/0x194 >> >> This happen at boot time when hantro driver is open and close without other actions. > Ah, now I see the problem. q->bufs and q->bufs_map are allocated in > vb2_core_create_bufs and vb2_core_reqbufs, but they should be allocated > in vb2_queue_init: that's the counterpart of vb2_core_queue_release. > > With that change you shouldn't have to check for q->bufs/bufs_map anymore. It is a better solution but even like this vb2_core_queue_release() is called at least 2 times on the same vivid queue and without testing q->bufs_bitmap makes kernel crash. > > Regards, > > Hans > >> >>>>>> + >>>>>> + return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0); >>>>> How about: >>>>> >>>>> return vb2_get_num_buffers(q) > 0; >>>> vb2_get_num_buffers is defined in videobuf2-core.c, I'm not sure that >>>> an inline function could depend of a module function. >>> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines. >>> >>> Regards, >>> >>> Hans >>> >
On 21/09/2023 14:05, Benjamin Gaignard wrote: > > Le 21/09/2023 à 12:24, Hans Verkuil a écrit : >> On 21/09/2023 11:28, Benjamin Gaignard wrote: >>> Le 20/09/2023 à 16:56, Hans Verkuil a écrit : >>>> On 20/09/2023 16:30, Benjamin Gaignard wrote: >>>> <snip> >>>> >>>>>>> num_buffers = min_t(unsigned int, num_buffers, >>>>>>> q->max_allowed_buffers - vb2_get_num_buffers(q)); >>>>>>> - first_index = vb2_get_num_buffers(q); >>>>>>> + first_index = bitmap_find_next_zero_area(q->bufs_map, q->max_allowed_buffers, >>>>>>> + 0, num_buffers, 0); >>>>>>> if (first_index >= q->max_allowed_buffers) >>>>>>> return 0; >>>>>>> @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) >>>>>>> struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, unsigned int index) >>>>>>> { >>>>>>> - if (index < q->num_buffers) >>>>>>> + if (!q->bufs_map || !q->bufs) >>>>>>> + return NULL; >>>>>> I don't think this can ever happen. >>>>> I got kernel crash without them. >>>>> I will keep them. >>>> What is the backtrace? How can this happen? It feels wrong that this can be >>>> called with a vb2_queue that apparently is not properly initialized. >>> I have this log when adding dump_stack() in vb2_get_buffer() if !q->bufs_bitmap: >>> >>> [ 18.924627] Call trace: >>> [ 18.927090] dump_backtrace+0x94/0xec >>> [ 18.930787] show_stack+0x18/0x24 >>> [ 18.934137] dump_stack_lvl+0x48/0x60 >>> [ 18.937833] dump_stack+0x18/0x24 >>> [ 18.941166] __vb2_queue_cancel+0x23c/0x2f0 >>> [ 18.945365] vb2_core_queue_release+0x24/0x6c >>> [ 18.949740] vb2_queue_release+0x10/0x1c >>> [ 18.953677] v4l2_m2m_ctx_release+0x20/0x40 >>> [ 18.957892] hantro_release+0x20/0x54 >>> [ 18.961584] v4l2_release+0x74/0xec >>> [ 18.965110] __fput+0xb4/0x274 >>> [ 18.968205] __fput_sync+0x50/0x5c >>> [ 18.971626] __arm64_sys_close+0x38/0x7c >>> [ 18.975562] invoke_syscall+0x48/0x114 >>> [ 18.979329] el0_svc_common.constprop.0+0xc0/0xe0 >>> [ 18.984068] do_el0_svc+0x1c/0x28 >>> [ 18.987402] el0_svc+0x40/0xe8 >>> [ 18.990470] el0t_64_sync_handler+0x100/0x12c >>> [ 18.994842] el0t_64_sync+0x190/0x194 >>> >>> This happen at boot time when hantro driver is open and close without other actions. >> Ah, now I see the problem. q->bufs and q->bufs_map are allocated in >> vb2_core_create_bufs and vb2_core_reqbufs, but they should be allocated >> in vb2_queue_init: that's the counterpart of vb2_core_queue_release. >> >> With that change you shouldn't have to check for q->bufs/bufs_map anymore. > > It is a better solution but even like this vb2_core_queue_release() is called > at least 2 times on the same vivid queue and without testing q->bufs_bitmap > makes kernel crash. Do you have a stacktrace for that? Perhaps vb2_core_queue_release should check for q->bufs/q->bufs_map and return if those are NULL. But it could also be a bug that it is called twice, it just was never noticed because it was harmless before. Regards, Hans > >> >> Regards, >> >> Hans >> >>> >>>>>>> + >>>>>>> + return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0); >>>>>> How about: >>>>>> >>>>>> return vb2_get_num_buffers(q) > 0; >>>>> vb2_get_num_buffers is defined in videobuf2-core.c, I'm not sure that >>>>> an inline function could depend of a module function. >>>> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines. >>>> >>>> Regards, >>>> >>>> Hans >>>> >>
Le 21/09/2023 à 14:13, Hans Verkuil a écrit : > On 21/09/2023 14:05, Benjamin Gaignard wrote: >> Le 21/09/2023 à 12:24, Hans Verkuil a écrit : >>> On 21/09/2023 11:28, Benjamin Gaignard wrote: >>>> Le 20/09/2023 à 16:56, Hans Verkuil a écrit : >>>>> On 20/09/2023 16:30, Benjamin Gaignard wrote: >>>>> <snip> >>>>> >>>>>>>> num_buffers = min_t(unsigned int, num_buffers, >>>>>>>> q->max_allowed_buffers - vb2_get_num_buffers(q)); >>>>>>>> - first_index = vb2_get_num_buffers(q); >>>>>>>> + first_index = bitmap_find_next_zero_area(q->bufs_map, q->max_allowed_buffers, >>>>>>>> + 0, num_buffers, 0); >>>>>>>> if (first_index >= q->max_allowed_buffers) >>>>>>>> return 0; >>>>>>>> @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) >>>>>>>> struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, unsigned int index) >>>>>>>> { >>>>>>>> - if (index < q->num_buffers) >>>>>>>> + if (!q->bufs_map || !q->bufs) >>>>>>>> + return NULL; >>>>>>> I don't think this can ever happen. >>>>>> I got kernel crash without them. >>>>>> I will keep them. >>>>> What is the backtrace? How can this happen? It feels wrong that this can be >>>>> called with a vb2_queue that apparently is not properly initialized. >>>> I have this log when adding dump_stack() in vb2_get_buffer() if !q->bufs_bitmap: >>>> >>>> [ 18.924627] Call trace: >>>> [ 18.927090] dump_backtrace+0x94/0xec >>>> [ 18.930787] show_stack+0x18/0x24 >>>> [ 18.934137] dump_stack_lvl+0x48/0x60 >>>> [ 18.937833] dump_stack+0x18/0x24 >>>> [ 18.941166] __vb2_queue_cancel+0x23c/0x2f0 >>>> [ 18.945365] vb2_core_queue_release+0x24/0x6c >>>> [ 18.949740] vb2_queue_release+0x10/0x1c >>>> [ 18.953677] v4l2_m2m_ctx_release+0x20/0x40 >>>> [ 18.957892] hantro_release+0x20/0x54 >>>> [ 18.961584] v4l2_release+0x74/0xec >>>> [ 18.965110] __fput+0xb4/0x274 >>>> [ 18.968205] __fput_sync+0x50/0x5c >>>> [ 18.971626] __arm64_sys_close+0x38/0x7c >>>> [ 18.975562] invoke_syscall+0x48/0x114 >>>> [ 18.979329] el0_svc_common.constprop.0+0xc0/0xe0 >>>> [ 18.984068] do_el0_svc+0x1c/0x28 >>>> [ 18.987402] el0_svc+0x40/0xe8 >>>> [ 18.990470] el0t_64_sync_handler+0x100/0x12c >>>> [ 18.994842] el0t_64_sync+0x190/0x194 >>>> >>>> This happen at boot time when hantro driver is open and close without other actions. >>> Ah, now I see the problem. q->bufs and q->bufs_map are allocated in >>> vb2_core_create_bufs and vb2_core_reqbufs, but they should be allocated >>> in vb2_queue_init: that's the counterpart of vb2_core_queue_release. >>> >>> With that change you shouldn't have to check for q->bufs/bufs_map anymore. >> It is a better solution but even like this vb2_core_queue_release() is called >> at least 2 times on the same vivid queue and without testing q->bufs_bitmap >> makes kernel crash. > Do you have a stacktrace for that? Perhaps vb2_core_queue_release should check > for q->bufs/q->bufs_map and return if those are NULL. But it could also be a > bug that it is called twice, it just was never noticed because it was harmless > before. I have added some printk to log that when running test-media on vivid: [ 130.497426] vb2_core_queue_init queue cap-0000000050d195ab allocate q->bufs 00000000dc2c15ed and q->bufs_bitmap 000000008173fc5a ... [ 130.733967] vb2_core_queue_release queue cap-0000000050d195ab release q->bufs and q->bufs_bitmap [ 133.866345] vb2_get_buffer queue cap-0000000050d195ab q->bufs_bitmap is NULL [ 133.873454] CPU: 1 PID: 321 Comm: v4l2-ctl Not tainted 6.6.0-rc1+ #542 [ 133.879997] Hardware name: NXP i.MX8MQ EVK (DT) [ 133.884536] Call trace: [ 133.886988] dump_backtrace+0x94/0xec [ 133.890673] show_stack+0x18/0x24 [ 133.894002] dump_stack_lvl+0x48/0x60 [ 133.897681] dump_stack+0x18/0x24 [ 133.901009] __vb2_queue_cancel+0x250/0x31c [ 133.905209] vb2_core_queue_release+0x24/0x88 [ 133.909580] _vb2_fop_release+0xb0/0xbc [ 133.913428] vb2_fop_release+0x2c/0x58 [ 133.917187] vivid_fop_release+0x80/0x388 [vivid] [ 133.921948] v4l2_release+0x74/0xec [ 133.925452] __fput+0xb4/0x274 [ 133.928520] __fput_sync+0x50/0x5c [ 133.931934] __arm64_sys_close+0x38/0x7c [ 133.935868] invoke_syscall+0x48/0x114 [ 133.939630] el0_svc_common.constprop.0+0x40/0xe0 [ 133.944349] do_el0_svc+0x1c/0x28 [ 133.947677] el0_svc+0x40/0xe8 [ 133.950741] el0t_64_sync_handler+0x100/0x12c [ 133.955109] el0t_64_sync+0x190/0x194 and later I have a call to reqbufs on the same queue without call to vb2_core_queue_init before [ 58.696812] __vb2_queue_alloc queue cap- 0000000050d195abq->bufs_bitmap is NULL [ 58.704148] CPU: 1 PID: 319 Comm: v4l2-compliance Not tainted 6.6.0-rc1+ #544 [ 58.711291] Hardware name: NXP i.MX8MQ EVK (DT) [ 58.715826] Call trace: [ 58.718274] dump_backtrace+0x94/0xec [ 58.721951] show_stack+0x18/0x24 [ 58.725274] dump_stack_lvl+0x48/0x60 [ 58.728946] dump_stack+0x18/0x24 [ 58.732268] __vb2_queue_alloc+0x4a8/0x50c [ 58.736374] vb2_core_reqbufs+0x274/0x46c [ 58.740391] vb2_ioctl_reqbufs+0xb0/0xe8 [ 58.744320] vidioc_reqbufs+0x50/0x64 [vivid] [ 58.748717] v4l_reqbufs+0x50/0x64 [ 58.752125] __video_do_ioctl+0x164/0x3c8 [ 58.756140] video_usercopy+0x200/0x668 [ 58.759982] video_ioctl2+0x18/0x28 [ 58.763475] v4l2_ioctl+0x40/0x60 [ 58.766798] __arm64_sys_ioctl+0xac/0xf0 [ 58.770730] invoke_syscall+0x48/0x114 [ 58.774487] el0_svc_common.constprop.0+0x40/0xe0 [ 58.779199] do_el0_svc+0x1c/0x28 [ 58.782520] el0_svc+0x40/0xe8 [ 58.785580] el0t_64_sync_handler+0x100/0x12c [ 58.789942] el0t_64_sync+0x190/0x194 > > Regards, > > Hans > >>> Regards, >>> >>> Hans >>> >>>> >>>>>>>> + >>>>>>>> + return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0); >>>>>>> How about: >>>>>>> >>>>>>> return vb2_get_num_buffers(q) > 0; >>>>>> vb2_get_num_buffers is defined in videobuf2-core.c, I'm not sure that >>>>>> an inline function could depend of a module function. >>>>> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines. >>>>> >>>>> Regards, >>>>> >>>>> Hans >>>>> >
Le 21/09/2023 à 14:46, Benjamin Gaignard a écrit : > > Le 21/09/2023 à 14:13, Hans Verkuil a écrit : >> On 21/09/2023 14:05, Benjamin Gaignard wrote: >>> Le 21/09/2023 à 12:24, Hans Verkuil a écrit : >>>> On 21/09/2023 11:28, Benjamin Gaignard wrote: >>>>> Le 20/09/2023 à 16:56, Hans Verkuil a écrit : >>>>>> On 20/09/2023 16:30, Benjamin Gaignard wrote: >>>>>> <snip> >>>>>> >>>>>>>>> num_buffers = min_t(unsigned int, num_buffers, >>>>>>>>> q->max_allowed_buffers - >>>>>>>>> vb2_get_num_buffers(q)); >>>>>>>>> - first_index = vb2_get_num_buffers(q); >>>>>>>>> + first_index = bitmap_find_next_zero_area(q->bufs_map, >>>>>>>>> q->max_allowed_buffers, >>>>>>>>> + 0, num_buffers, 0); >>>>>>>>> if (first_index >= q->max_allowed_buffers) >>>>>>>>> return 0; >>>>>>>>> @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct >>>>>>>>> vb2_queue *q, unsigned int buffers) >>>>>>>>> struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, >>>>>>>>> unsigned int index) >>>>>>>>> { >>>>>>>>> - if (index < q->num_buffers) >>>>>>>>> + if (!q->bufs_map || !q->bufs) >>>>>>>>> + return NULL; >>>>>>>> I don't think this can ever happen. >>>>>>> I got kernel crash without them. >>>>>>> I will keep them. >>>>>> What is the backtrace? How can this happen? It feels wrong that >>>>>> this can be >>>>>> called with a vb2_queue that apparently is not properly initialized. >>>>> I have this log when adding dump_stack() in vb2_get_buffer() if >>>>> !q->bufs_bitmap: >>>>> >>>>> [ 18.924627] Call trace: >>>>> [ 18.927090] dump_backtrace+0x94/0xec >>>>> [ 18.930787] show_stack+0x18/0x24 >>>>> [ 18.934137] dump_stack_lvl+0x48/0x60 >>>>> [ 18.937833] dump_stack+0x18/0x24 >>>>> [ 18.941166] __vb2_queue_cancel+0x23c/0x2f0 >>>>> [ 18.945365] vb2_core_queue_release+0x24/0x6c >>>>> [ 18.949740] vb2_queue_release+0x10/0x1c >>>>> [ 18.953677] v4l2_m2m_ctx_release+0x20/0x40 >>>>> [ 18.957892] hantro_release+0x20/0x54 >>>>> [ 18.961584] v4l2_release+0x74/0xec >>>>> [ 18.965110] __fput+0xb4/0x274 >>>>> [ 18.968205] __fput_sync+0x50/0x5c >>>>> [ 18.971626] __arm64_sys_close+0x38/0x7c >>>>> [ 18.975562] invoke_syscall+0x48/0x114 >>>>> [ 18.979329] el0_svc_common.constprop.0+0xc0/0xe0 >>>>> [ 18.984068] do_el0_svc+0x1c/0x28 >>>>> [ 18.987402] el0_svc+0x40/0xe8 >>>>> [ 18.990470] el0t_64_sync_handler+0x100/0x12c >>>>> [ 18.994842] el0t_64_sync+0x190/0x194 >>>>> >>>>> This happen at boot time when hantro driver is open and close >>>>> without other actions. >>>> Ah, now I see the problem. q->bufs and q->bufs_map are allocated in >>>> vb2_core_create_bufs and vb2_core_reqbufs, but they should be >>>> allocated >>>> in vb2_queue_init: that's the counterpart of vb2_core_queue_release. Hans, I think we are doing loops in your comment :-) https://patchwork.kernel.org/comment/25496456/ Regards, Benjamin >>>> >>>> With that change you shouldn't have to check for q->bufs/bufs_map >>>> anymore. >>> It is a better solution but even like this vb2_core_queue_release() >>> is called >>> at least 2 times on the same vivid queue and without testing >>> q->bufs_bitmap >>> makes kernel crash. >> Do you have a stacktrace for that? Perhaps vb2_core_queue_release >> should check >> for q->bufs/q->bufs_map and return if those are NULL. But it could >> also be a >> bug that it is called twice, it just was never noticed because it was >> harmless >> before. > > I have added some printk to log that when running test-media on vivid: > > [ 130.497426] vb2_core_queue_init queue cap-0000000050d195ab allocate > q->bufs 00000000dc2c15ed and q->bufs_bitmap 000000008173fc5a > ... > [ 130.733967] vb2_core_queue_release queue cap-0000000050d195ab > release q->bufs and q->bufs_bitmap > [ 133.866345] vb2_get_buffer queue cap-0000000050d195ab > q->bufs_bitmap is NULL > [ 133.873454] CPU: 1 PID: 321 Comm: v4l2-ctl Not tainted 6.6.0-rc1+ #542 > [ 133.879997] Hardware name: NXP i.MX8MQ EVK (DT) > [ 133.884536] Call trace: > [ 133.886988] dump_backtrace+0x94/0xec > [ 133.890673] show_stack+0x18/0x24 > [ 133.894002] dump_stack_lvl+0x48/0x60 > [ 133.897681] dump_stack+0x18/0x24 > [ 133.901009] __vb2_queue_cancel+0x250/0x31c > [ 133.905209] vb2_core_queue_release+0x24/0x88 > [ 133.909580] _vb2_fop_release+0xb0/0xbc > [ 133.913428] vb2_fop_release+0x2c/0x58 > [ 133.917187] vivid_fop_release+0x80/0x388 [vivid] > [ 133.921948] v4l2_release+0x74/0xec > [ 133.925452] __fput+0xb4/0x274 > [ 133.928520] __fput_sync+0x50/0x5c > [ 133.931934] __arm64_sys_close+0x38/0x7c > [ 133.935868] invoke_syscall+0x48/0x114 > [ 133.939630] el0_svc_common.constprop.0+0x40/0xe0 > [ 133.944349] do_el0_svc+0x1c/0x28 > [ 133.947677] el0_svc+0x40/0xe8 > [ 133.950741] el0t_64_sync_handler+0x100/0x12c > [ 133.955109] el0t_64_sync+0x190/0x194 > > and later I have a call to reqbufs on the same queue without call to > vb2_core_queue_init before > > [ 58.696812] __vb2_queue_alloc queue cap- > 0000000050d195abq->bufs_bitmap is NULL > [ 58.704148] CPU: 1 PID: 319 Comm: v4l2-compliance Not tainted > 6.6.0-rc1+ #544 > [ 58.711291] Hardware name: NXP i.MX8MQ EVK (DT) > [ 58.715826] Call trace: > [ 58.718274] dump_backtrace+0x94/0xec > [ 58.721951] show_stack+0x18/0x24 > [ 58.725274] dump_stack_lvl+0x48/0x60 > [ 58.728946] dump_stack+0x18/0x24 > [ 58.732268] __vb2_queue_alloc+0x4a8/0x50c > [ 58.736374] vb2_core_reqbufs+0x274/0x46c > [ 58.740391] vb2_ioctl_reqbufs+0xb0/0xe8 > [ 58.744320] vidioc_reqbufs+0x50/0x64 [vivid] > [ 58.748717] v4l_reqbufs+0x50/0x64 > [ 58.752125] __video_do_ioctl+0x164/0x3c8 > [ 58.756140] video_usercopy+0x200/0x668 > [ 58.759982] video_ioctl2+0x18/0x28 > [ 58.763475] v4l2_ioctl+0x40/0x60 > [ 58.766798] __arm64_sys_ioctl+0xac/0xf0 > [ 58.770730] invoke_syscall+0x48/0x114 > [ 58.774487] el0_svc_common.constprop.0+0x40/0xe0 > [ 58.779199] do_el0_svc+0x1c/0x28 > [ 58.782520] el0_svc+0x40/0xe8 > [ 58.785580] el0t_64_sync_handler+0x100/0x12c > [ 58.789942] el0t_64_sync+0x190/0x194 > >> >> Regards, >> >> Hans >> >>>> Regards, >>>> >>>> Hans >>>> >>>>>>>>> + >>>>>>>>> + return (bitmap_weight(q->bufs_map, >>>>>>>>> q->max_allowed_buffers) > 0); >>>>>>>> How about: >>>>>>>> >>>>>>>> return vb2_get_num_buffers(q) > 0; >>>>>>> vb2_get_num_buffers is defined in videobuf2-core.c, I'm not sure >>>>>>> that >>>>>>> an inline function could depend of a module function. >>>>>> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines. >>>>>> >>>>>> Regards, >>>>>> >>>>>> Hans >>>>>> >>
On 21/09/2023 14:46, Benjamin Gaignard wrote: > > Le 21/09/2023 à 14:13, Hans Verkuil a écrit : >> On 21/09/2023 14:05, Benjamin Gaignard wrote: >>> Le 21/09/2023 à 12:24, Hans Verkuil a écrit : >>>> On 21/09/2023 11:28, Benjamin Gaignard wrote: >>>>> Le 20/09/2023 à 16:56, Hans Verkuil a écrit : >>>>>> On 20/09/2023 16:30, Benjamin Gaignard wrote: >>>>>> <snip> >>>>>> >>>>>>>>> num_buffers = min_t(unsigned int, num_buffers, >>>>>>>>> q->max_allowed_buffers - vb2_get_num_buffers(q)); >>>>>>>>> - first_index = vb2_get_num_buffers(q); >>>>>>>>> + first_index = bitmap_find_next_zero_area(q->bufs_map, q->max_allowed_buffers, >>>>>>>>> + 0, num_buffers, 0); >>>>>>>>> if (first_index >= q->max_allowed_buffers) >>>>>>>>> return 0; >>>>>>>>> @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) >>>>>>>>> struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, unsigned int index) >>>>>>>>> { >>>>>>>>> - if (index < q->num_buffers) >>>>>>>>> + if (!q->bufs_map || !q->bufs) >>>>>>>>> + return NULL; >>>>>>>> I don't think this can ever happen. >>>>>>> I got kernel crash without them. >>>>>>> I will keep them. >>>>>> What is the backtrace? How can this happen? It feels wrong that this can be >>>>>> called with a vb2_queue that apparently is not properly initialized. >>>>> I have this log when adding dump_stack() in vb2_get_buffer() if !q->bufs_bitmap: >>>>> >>>>> [ 18.924627] Call trace: >>>>> [ 18.927090] dump_backtrace+0x94/0xec >>>>> [ 18.930787] show_stack+0x18/0x24 >>>>> [ 18.934137] dump_stack_lvl+0x48/0x60 >>>>> [ 18.937833] dump_stack+0x18/0x24 >>>>> [ 18.941166] __vb2_queue_cancel+0x23c/0x2f0 >>>>> [ 18.945365] vb2_core_queue_release+0x24/0x6c >>>>> [ 18.949740] vb2_queue_release+0x10/0x1c >>>>> [ 18.953677] v4l2_m2m_ctx_release+0x20/0x40 >>>>> [ 18.957892] hantro_release+0x20/0x54 >>>>> [ 18.961584] v4l2_release+0x74/0xec >>>>> [ 18.965110] __fput+0xb4/0x274 >>>>> [ 18.968205] __fput_sync+0x50/0x5c >>>>> [ 18.971626] __arm64_sys_close+0x38/0x7c >>>>> [ 18.975562] invoke_syscall+0x48/0x114 >>>>> [ 18.979329] el0_svc_common.constprop.0+0xc0/0xe0 >>>>> [ 18.984068] do_el0_svc+0x1c/0x28 >>>>> [ 18.987402] el0_svc+0x40/0xe8 >>>>> [ 18.990470] el0t_64_sync_handler+0x100/0x12c >>>>> [ 18.994842] el0t_64_sync+0x190/0x194 >>>>> >>>>> This happen at boot time when hantro driver is open and close without other actions. >>>> Ah, now I see the problem. q->bufs and q->bufs_map are allocated in >>>> vb2_core_create_bufs and vb2_core_reqbufs, but they should be allocated >>>> in vb2_queue_init: that's the counterpart of vb2_core_queue_release. >>>> >>>> With that change you shouldn't have to check for q->bufs/bufs_map anymore. >>> It is a better solution but even like this vb2_core_queue_release() is called >>> at least 2 times on the same vivid queue and without testing q->bufs_bitmap >>> makes kernel crash. >> Do you have a stacktrace for that? Perhaps vb2_core_queue_release should check >> for q->bufs/q->bufs_map and return if those are NULL. But it could also be a >> bug that it is called twice, it just was never noticed because it was harmless >> before. > > I have added some printk to log that when running test-media on vivid: > > [ 130.497426] vb2_core_queue_init queue cap-0000000050d195ab allocate q->bufs 00000000dc2c15ed and q->bufs_bitmap 000000008173fc5a > ... > [ 130.733967] vb2_core_queue_release queue cap-0000000050d195ab release q->bufs and q->bufs_bitmap > [ 133.866345] vb2_get_buffer queue cap-0000000050d195ab q->bufs_bitmap is NULL > [ 133.873454] CPU: 1 PID: 321 Comm: v4l2-ctl Not tainted 6.6.0-rc1+ #542 > [ 133.879997] Hardware name: NXP i.MX8MQ EVK (DT) > [ 133.884536] Call trace: > [ 133.886988] dump_backtrace+0x94/0xec > [ 133.890673] show_stack+0x18/0x24 > [ 133.894002] dump_stack_lvl+0x48/0x60 > [ 133.897681] dump_stack+0x18/0x24 > [ 133.901009] __vb2_queue_cancel+0x250/0x31c > [ 133.905209] vb2_core_queue_release+0x24/0x88 > [ 133.909580] _vb2_fop_release+0xb0/0xbc > [ 133.913428] vb2_fop_release+0x2c/0x58 > [ 133.917187] vivid_fop_release+0x80/0x388 [vivid] > [ 133.921948] v4l2_release+0x74/0xec > [ 133.925452] __fput+0xb4/0x274 > [ 133.928520] __fput_sync+0x50/0x5c > [ 133.931934] __arm64_sys_close+0x38/0x7c > [ 133.935868] invoke_syscall+0x48/0x114 > [ 133.939630] el0_svc_common.constprop.0+0x40/0xe0 > [ 133.944349] do_el0_svc+0x1c/0x28 > [ 133.947677] el0_svc+0x40/0xe8 > [ 133.950741] el0t_64_sync_handler+0x100/0x12c > [ 133.955109] el0t_64_sync+0x190/0x194 > > and later I have a call to reqbufs on the same queue without call to vb2_core_queue_init before > > [ 58.696812] __vb2_queue_alloc queue cap- 0000000050d195abq->bufs_bitmap is NULL > [ 58.704148] CPU: 1 PID: 319 Comm: v4l2-compliance Not tainted 6.6.0-rc1+ #544 > [ 58.711291] Hardware name: NXP i.MX8MQ EVK (DT) > [ 58.715826] Call trace: > [ 58.718274] dump_backtrace+0x94/0xec > [ 58.721951] show_stack+0x18/0x24 > [ 58.725274] dump_stack_lvl+0x48/0x60 > [ 58.728946] dump_stack+0x18/0x24 > [ 58.732268] __vb2_queue_alloc+0x4a8/0x50c > [ 58.736374] vb2_core_reqbufs+0x274/0x46c > [ 58.740391] vb2_ioctl_reqbufs+0xb0/0xe8 > [ 58.744320] vidioc_reqbufs+0x50/0x64 [vivid] > [ 58.748717] v4l_reqbufs+0x50/0x64 > [ 58.752125] __video_do_ioctl+0x164/0x3c8 > [ 58.756140] video_usercopy+0x200/0x668 > [ 58.759982] video_ioctl2+0x18/0x28 > [ 58.763475] v4l2_ioctl+0x40/0x60 > [ 58.766798] __arm64_sys_ioctl+0xac/0xf0 > [ 58.770730] invoke_syscall+0x48/0x114 > [ 58.774487] el0_svc_common.constprop.0+0x40/0xe0 > [ 58.779199] do_el0_svc+0x1c/0x28 > [ 58.782520] el0_svc+0x40/0xe8 > [ 58.785580] el0t_64_sync_handler+0x100/0x12c > [ 58.789942] el0t_64_sync+0x190/0x194 Argh, I see what is happening. The root cause is that vb2_core_queue_release is actually not a true counterpart to vb2_core_queue_init. The '_release' part refers to when a file handle is released, and not to releasing resources allocated in queue_init. The queue_init function never actually allocated any resources, so there was never a reason to make a counterpart to that, but now that bites us. Changing this would be a huge amount of work, and it is not worth the effort, IMHO. But at least we shouldn't have to test for both bufs and bufs_map, they are either both set or both NULL. Just test one of the two. The vb2_core_queue_init() function documentation in the header should perhaps be more clear about the fact that this function does not allocate any resources, and that there is no cleanup counterpart. It is what got me confused... Regards, Hans > >> >> Regards, >> >> Hans >> >>>> Regards, >>>> >>>> Hans >>>> >>>>> >>>>>>>>> + >>>>>>>>> + return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0); >>>>>>>> How about: >>>>>>>> >>>>>>>> return vb2_get_num_buffers(q) > 0; >>>>>>> vb2_get_num_buffers is defined in videobuf2-core.c, I'm not sure that >>>>>>> an inline function could depend of a module function. >>>>>> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines. >>>>>> >>>>>> Regards, >>>>>> >>>>>> Hans >>>>>> >>
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index a4c2fae8705d..c5d4a388331b 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -411,10 +411,11 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb) */ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index) { - if (index < q->max_allowed_buffers && !q->bufs[index]) { + if (index < q->max_allowed_buffers && !test_bit(index, q->bufs_map)) { q->bufs[index] = vb; vb->index = index; vb->vb2_queue = q; + set_bit(index, q->bufs_map); return true; } @@ -428,9 +429,10 @@ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns */ static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb) { - if (vb->index < q->max_allowed_buffers) { + if (vb->index < q->max_allowed_buffers && test_bit(vb->index, q->bufs_map)) { q->bufs[vb->index] = NULL; vb->vb2_queue = NULL; + clear_bit(vb->index, q->bufs_map); } } @@ -451,11 +453,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, unsigned long first_index; int ret; - /* Ensure that q->num_buffers+num_buffers is below q->max_allowed_buffers */ + /* Ensure that the number of already queue + num_buffers is below q->max_allowed_buffers */ num_buffers = min_t(unsigned int, num_buffers, q->max_allowed_buffers - vb2_get_num_buffers(q)); - first_index = vb2_get_num_buffers(q); + first_index = bitmap_find_next_zero_area(q->bufs_map, q->max_allowed_buffers, + 0, num_buffers, 0); if (first_index >= q->max_allowed_buffers) return 0; @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, unsigned int index) { - if (index < q->num_buffers) + if (!q->bufs_map || !q->bufs) + return NULL; + + if (index >= q->max_allowed_buffers) + return NULL; + + if (test_bit(index, q->bufs_map)) return q->bufs[index]; return NULL; } @@ -683,7 +692,10 @@ EXPORT_SYMBOL_GPL(vb2_get_buffer); unsigned int vb2_get_num_buffers(struct vb2_queue *q) { - return q->num_buffers; + if (!q->bufs_map) + return 0; + + return bitmap_weight(q->bufs_map, q->max_allowed_buffers); } EXPORT_SYMBOL_GPL(vb2_get_num_buffers); @@ -899,6 +911,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); if (!q->bufs) ret = -ENOMEM; + + if (!q->bufs_map) + q->bufs_map = bitmap_zalloc(q->max_allowed_buffers, GFP_KERNEL); + if (!q->bufs_map) { + ret = -ENOMEM; + kfree(q->bufs); + q->bufs = NULL; + } q->memory = memory; mutex_unlock(&q->mmap_lock); if (ret) @@ -968,7 +988,6 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, } mutex_lock(&q->mmap_lock); - q->num_buffers = allocated_buffers; if (ret < 0) { /* @@ -995,6 +1014,10 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, mutex_lock(&q->mmap_lock); q->memory = VB2_MEMORY_UNKNOWN; mutex_unlock(&q->mmap_lock); + kfree(q->bufs); + q->bufs = NULL; + bitmap_free(q->bufs_map); + q->bufs_map = NULL; return ret; } EXPORT_SYMBOL_GPL(vb2_core_reqbufs); @@ -1031,9 +1054,19 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, q->memory = memory; if (!q->bufs) q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); - if (!q->bufs) + if (!q->bufs) { + ret = -ENOMEM; + goto unlock; + } + if (!q->bufs_map) + q->bufs_map = bitmap_zalloc(q->max_allowed_buffers, GFP_KERNEL); + if (!q->bufs_map) { ret = -ENOMEM; + kfree(q->bufs); + q->bufs = NULL; + } mutex_unlock(&q->mmap_lock); +unlock: if (ret) return ret; q->waiting_for_buffers = !q->is_output; @@ -1095,7 +1128,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, } mutex_lock(&q->mmap_lock); - q->num_buffers += allocated_buffers; if (ret < 0) { /* @@ -2588,6 +2620,9 @@ void vb2_core_queue_release(struct vb2_queue *q) __vb2_queue_free(q, q->max_allowed_buffers); kfree(q->bufs); q->bufs = NULL; + bitmap_free(q->bufs_map); + q->bufs_map = NULL; + mutex_unlock(&q->mmap_lock); } EXPORT_SYMBOL_GPL(vb2_core_queue_release); @@ -2944,7 +2979,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_ * Check if we need to dequeue the buffer. */ index = fileio->cur_index; - if (index >= q->num_buffers) { + if (!test_bit(index, q->bufs_map)) { struct vb2_buffer *b; /* diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 19c93d8eb7c8..734437236cc4 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -557,7 +557,7 @@ struct vb2_buf_ops { * @memory: current memory type used * @dma_dir: DMA mapping direction. * @bufs: videobuf2 buffer structures - * @num_buffers: number of allocated/used buffers + * @bufs_map: bitmap to manage bufs entries. * @max_allowed_buffers: upper limit of number of allocated/used buffers * @queued_list: list of buffers currently queued from userspace * @queued_count: number of buffers queued and ready for streaming. @@ -621,7 +621,7 @@ struct vb2_queue { unsigned int memory; enum dma_data_direction dma_dir; struct vb2_buffer **bufs; - unsigned int num_buffers; + unsigned long *bufs_map; unsigned int max_allowed_buffers; struct list_head queued_list; @@ -1151,7 +1151,10 @@ static inline bool vb2_fileio_is_active(struct vb2_queue *q) */ static inline bool vb2_is_busy(struct vb2_queue *q) { - return (q->num_buffers > 0); + if (!q->bufs_map) + return false; + + return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0); } /**
Add a bitmap field to know which of bufs array entries are used or not. Remove no more used num_buffers field from queue structure. Use bitmap_find_next_zero_area() to find the first possible range when creating new buffers to fill the gaps. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- .../media/common/videobuf2/videobuf2-core.c | 55 +++++++++++++++---- include/media/videobuf2-core.h | 9 ++- 2 files changed, 51 insertions(+), 13 deletions(-)