| Message ID | 20240614073702.316810-4-yunkec@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: videobuf2-core: attach once if multiple planes share the same dbuf | expand |
Hi Yunke, So this series looks fine to me, but this patch needs a better commit log, explaining *why* this is done. And there should also be a comment before the for loop explaining the same thing. I understand that the order is changed to prepare for the next patch and to ensure that any duplicated dmabufs are 'put' first to avoid potential invalid mem_priv pointers, but that is not actually stated anywhere. On 14/06/2024 09:37, Yunke Cao wrote: > Release the planes from num_planes - 1 to 0. > > Signed-off-by: Yunke Cao <yunkec@chromium.org> > --- > v3: > - Change local variable to an integer to make the code cleaner. > --- > drivers/media/common/videobuf2/videobuf2-core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index a4fbc7a57ee0..cbc8928f0418 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -324,9 +324,9 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p) > */ > static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb) > { > - unsigned int plane; > + int plane; > > - for (plane = 0; plane < vb->num_planes; ++plane) So here I need a comment explaining the reason for the reversed order and to prevent future devs from changing it. > + for (plane = vb->num_planes - 1; plane >= 0; --plane) > __vb2_plane_dmabuf_put(vb, &vb->planes[plane]); > } > You can post just a v4.1 for this patch, or post a v5, up to you. Regards, Hans
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index a4fbc7a57ee0..cbc8928f0418 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -324,9 +324,9 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p) */ static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb) { - unsigned int plane; + int plane; - for (plane = 0; plane < vb->num_planes; ++plane) + for (plane = vb->num_planes - 1; plane >= 0; --plane) __vb2_plane_dmabuf_put(vb, &vb->planes[plane]); }
Release the planes from num_planes - 1 to 0. Signed-off-by: Yunke Cao <yunkec@chromium.org> --- v3: - Change local variable to an integer to make the code cleaner. --- drivers/media/common/videobuf2/videobuf2-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)