Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220706182657.210650-1-ezequiel@vanguardiasur.com.ar> <20220706182657.210650-9-ezequiel@vanguardiasur.com.ar>
In-Reply-To: <20220706182657.210650-9-ezequiel@vanguardiasur.com.ar>
From:   Tomasz Figa <tfiga@chromium.org>
Date:   Fri, 8 Jul 2022 13:45:35 +0900
Message-ID: <CAAFQd5Cxn+HVun+H66-RvUh6SUqnSbug0vpmFFanmrGywfqHeg@mail.gmail.com>
Subject: Re: [PATCH 8/8] videobuf2: Remove vb2_find_timestamp()
To:     Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Cc:     linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Marek Szyprowski <m.szyprowski@samsung.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Ezequiel,

On Thu, Jul 7, 2022 at 3:27 AM Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> Now that we've transitioned all users to vb2_find_buffer API,
> remove the unused vb2_find_timestamp().
>
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 12 ++++-----
>  include/media/videobuf2-v4l2.h                | 26 +------------------
>  2 files changed, 7 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 075d24ebf44c..a9696442dfba 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -625,18 +625,18 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
>         .copy_timestamp         = __copy_timestamp,
>  };
>
> -int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
> -                      unsigned int start_idx)
> +struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp)
>  {
>         unsigned int i;
>
> -       for (i = start_idx; i < q->num_buffers; i++)
> +       for (i = 0; i < q->num_buffers; i++)
>                 if (q->bufs[i]->copied_timestamp &&
>                     q->bufs[i]->timestamp == timestamp)
> -                       return i;
> -       return -1;
> +                       return vb2_get_buffer(q, i);
> +
> +       return NULL;
>  }
> -EXPORT_SYMBOL_GPL(vb2_find_timestamp);
> +EXPORT_SYMBOL_GPL(vb2_find_buffer);
>
>  /*
>   * vb2_querybuf() - query video buffer information
> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> index 7f9ae5b39b78..5a845887850b 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -62,22 +62,6 @@ struct vb2_v4l2_buffer {
>  #define to_vb2_v4l2_buffer(vb) \
>         container_of(vb, struct vb2_v4l2_buffer, vb2_buf)
>
> -/**
> - * vb2_find_timestamp() - Find buffer with given timestamp in the queue
> - *
> - * @q:         pointer to &struct vb2_queue with videobuf2 queue.
> - * @timestamp: the timestamp to find.
> - * @start_idx: the start index (usually 0) in the buffer array to start
> - *             searching from. Note that there may be multiple buffers
> - *             with the same timestamp value, so you can restart the search
> - *             by setting @start_idx to the previously found index + 1.
> - *
> - * Returns the buffer index of the buffer with the given @timestamp, or
> - * -1 if no buffer with @timestamp was found.
> - */
> -int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
> -                      unsigned int start_idx);
> -
>  /**
>   * vb2_find_buffer() - Find a buffer with given timestamp
>   *
> @@ -86,15 +70,7 @@ int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
>   *
>   * Returns the buffer with the given @timestamp, or NULL if not found.
>   */
> -static inline struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q,
> -                                                u64 timestamp)
> -{
> -       int index = vb2_find_timestamp(q, timestamp, 0);
> -
> -       if (index < 0)
> -               return NULL;
> -       return vb2_get_buffer(q, index);
> -}
> +struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp);

Was there any specific reason to add it as an inline initially rather
than just having it close to the final shape from the very beginning?
Sorry for being picky, but I find it more difficult to review this
way.

Best regards,
Tomasz

>
>  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
>
> --
> 2.34.3
>