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@xxxxxxxxxxxx>
> ---
> 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
