diff mbox series

[v4,3/4] media: videobuf2-core: reverse the iteration order in __vb2_buf_dmabuf_put

Message ID 20240614073702.316810-4-yunkec@chromium.org (mailing list archive)
State Superseded
Delegated to: Hans Verkuil
Headers
Series media: videobuf2-core: attach once if multiple planes share the same dbuf |

Commit Message

Yunke Cao June 14, 2024, 7:37 a.m. UTC 
  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(-)

Comments

Hans Verkuil Aug. 12, 2024, 7:09 a.m. UTC | #1 
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 mbox series

Patch

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]);
 }