[libcamera-devel] [PATCH v4 3/9] libcamera: pipeline: raspberrypi: Add some debug logging

[Naushir Patuck]

Hi Laurent,

Thank you for the review.

On Wed, 22 Jul 2020 at 15:58, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Jul 20, 2020 at 10:13:05AM +0100, Naushir Patuck wrote:
> > No functional changes, only added some more trace points.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index d3639ce1..35f14dda 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1160,6 +1160,11 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
> >       if (state_ == State::Stopped)
> >               return;
> >
> > +     LOG(RPI, Debug) << "Stream ISP Input buffer complete"
>
> Did you mean s/Stream ISP/ISP/ ?

I did mean to use the stream qualifier, in order to be consistent with
the other log points in ispOutputDequeue() and unicamBufferDequeue().

>
> > +                     << ", buffer id " << buffer->cookie()
>
> Maybe s/id/cookie/ ?

Possibly, but see patch 9/9.  We stop using buffer->cookie() entirely,
so id will be the right term eventually :-)

>
> > +                     << ", timestamp: " << buffer->metadata().timestamp;
> > +
> > +     /* The ISP input buffer gets re-queued into Unicam. */
> >       handleStreamBuffer(buffer, &unicam_[Unicam::Image]);
> >       handleState();
> >  }
> > @@ -1324,6 +1329,8 @@ void RPiCameraData::checkRequestCompleted()
> >               pipe_->completeRequest(camera_, request);
> >               requestQueue_.pop_front();
> >               requestCompleted = true;
> > +
> > +             LOG(RPI, Debug) << "Request is complete";
>
> Would it make sense to add this to Request::complete() instead ? And
> maybe print the request cookie ?

Sure, I can add a log point in Request::complete() to replace this one.

>
> >       }
> >
> >       /*
> > @@ -1462,7 +1469,7 @@ FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t t
> >               if (b->metadata().timestamp < timestamp) {
> >                       q.pop();
> >                       dev->queueBuffer(b);
> > -                     LOG(RPI, Error) << "Dropping input frame!";
> > +                     LOG(RPI, Warning) << "Dropping input frame!";
> >               } else if (b->metadata().timestamp == timestamp) {
> >                       /* The calling function will pop the item from the queue. */
> >                       return b;
>
> --
> Regards,
>
> Laurent Pinchart

Regards,
Naush