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;
Date:   Fri, 8 Jul 2022 08:15:05 -0300
From:   Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
To:     Tomasz Figa <tfiga@chromium.org>
Cc:     linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: [PATCH 8/8] videobuf2: Remove vb2_find_timestamp()
Message-ID: <YsgRuahdB/65K88a@eze-laptop>
References: <20220706182657.210650-1-ezequiel@vanguardiasur.com.ar>
 <20220706182657.210650-9-ezequiel@vanguardiasur.com.ar>
 <CAAFQd5Cxn+HVun+H66-RvUh6SUqnSbug0vpmFFanmrGywfqHeg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAAFQd5Cxn+HVun+H66-RvUh6SUqnSbug0vpmFFanmrGywfqHeg@mail.gmail.com>
Precedence: bulk

Hi Tomasz,

On Fri, Jul 08, 2022 at 01:45:35PM +0900, Tomasz Figa wrote:
> 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.
> 

Yeah, that makes sense, I'll re-work it for the next iteration.

Thanks,
Ezequiel

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