[libcamera-devel,3/9] libcamera: pipeline: raspberrypi: Add some debug logging
diff mbox series

Message ID 20200713084727.232422-4-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Zero-copy RAW stream work
Related show

Commit Message

Naushir Patuck July 13, 2020, 8:47 a.m. UTC 
No functional changes, only added some more trace points.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Niklas Söderlund July 13, 2020, 11:39 a.m. UTC | #1 
Hi Naushir,

Thanks for your work.

On 2020-07-13 09:47:22 +0100, Naushir Patuck wrote:
> No functional changes, only added some more trace points.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  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 8b6f578f..65c8557d 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1175,6 +1175,11 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
>  	if (state_ == State::Stopped)
>  		return;
>  
> +	LOG(RPI, Debug) << "Stream ISP Input buffer complete"
> +			<< ", buffer id " << buffer->cookie()

This worries me a bit. For buffers coming from the application the 
cookie is controlled by the application. This means that pipeline 
handlers can't set the cookie for any buffer it receives in a Request.  
Maybe we need to create something in libcamera core to allow 
pipelines/IPA to have an internal cookie value for buffers?

Or maybe I'm misunderstanding the code and cookies are only used for 
buffers that are not seen by the applications at all (such as ISP 
statistics and parameters)?

> +			<< ", timestamp: " << buffer->metadata().timestamp;
> +
> +	/* The ISP input buffer gets re-queued into Unicam. */
>  	handleStreamBuffer(buffer, &unicam_[Unicam::Image]);
>  	handleState();
>  }
> @@ -1339,6 +1344,8 @@ void RPiCameraData::checkRequestCompleted()
>  		pipe_->completeRequest(camera_, request);
>  		requestQueue_.pop_front();
>  		requestCompleted = true;
> +
> +		LOG(RPI, Debug) << "Request is complete";
>  	}
>  
>  	/*
> @@ -1477,7 +1484,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;
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Naushir Patuck July 13, 2020, 1:05 p.m. UTC | #2 
Hi Niklas,


On Mon, 13 Jul 2020 at 12:39, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
>
> Hi Naushir,
>
> Thanks for your work.
>
> On 2020-07-13 09:47:22 +0100, Naushir Patuck wrote:
> > No functional changes, only added some more trace points.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  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 8b6f578f..65c8557d 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1175,6 +1175,11 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
> >       if (state_ == State::Stopped)
> >               return;
> >
> > +     LOG(RPI, Debug) << "Stream ISP Input buffer complete"
> > +                     << ", buffer id " << buffer->cookie()
>
> This worries me a bit. For buffers coming from the application the
> cookie is controlled by the application. This means that pipeline
> handlers can't set the cookie for any buffer it receives in a Request.
> Maybe we need to create something in libcamera core to allow
> pipelines/IPA to have an internal cookie value for buffers?
>

Actually I did not realise that the application was meant to use the
cookie.  I thought it was a pipeline handler thing to identify
buffers.

> Or maybe I'm misunderstanding the code and cookies are only used for
> buffers that are not seen by the applications at all (such as ISP
> statistics and parameters)?

This is correct, it was only used for internal buffers, but I relied
on it for this change as well.  This is not a problem, I use it
currently to identify buffers passed to/from the IPA, and I can use a
simple linear index to do the same.

>
> > +                     << ", timestamp: " << buffer->metadata().timestamp;
> > +
> > +     /* The ISP input buffer gets re-queued into Unicam. */
> >       handleStreamBuffer(buffer, &unicam_[Unicam::Image]);
> >       handleState();
> >  }
> > @@ -1339,6 +1344,8 @@ void RPiCameraData::checkRequestCompleted()
> >               pipe_->completeRequest(camera_, request);
> >               requestQueue_.pop_front();
> >               requestCompleted = true;
> > +
> > +             LOG(RPI, Debug) << "Request is complete";
> >       }
> >
> >       /*
> > @@ -1477,7 +1484,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;
> > --
> > 2.25.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

Regards,
Naush

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 8b6f578f..65c8557d 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1175,6 +1175,11 @@  void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
 	if (state_ == State::Stopped)
 		return;
 
+	LOG(RPI, Debug) << "Stream ISP Input buffer complete"
+			<< ", buffer id " << buffer->cookie()
+			<< ", timestamp: " << buffer->metadata().timestamp;
+
+	/* The ISP input buffer gets re-queued into Unicam. */
 	handleStreamBuffer(buffer, &unicam_[Unicam::Image]);
 	handleState();
 }
@@ -1339,6 +1344,8 @@  void RPiCameraData::checkRequestCompleted()
 		pipe_->completeRequest(camera_, request);
 		requestQueue_.pop_front();
 		requestCompleted = true;
+
+		LOG(RPI, Debug) << "Request is complete";
 	}
 
 	/*
@@ -1477,7 +1484,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;