[libcamera-devel] [PATCH v4 2/3] pipeline: raspberrypi: Add a Unicam dequeue timeout

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Wed, Apr 06, 2022 at 08:58:58AM +0100, Naushir Patuck wrote:
> From: Naushir Patuck via libcamera-devel <libcamera-devel at lists.libcamera.org>

Here too.

> Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and
> connect the timeout signal to a slot in the pipeline handler. This slot will
> log an error message informing the user of a possible hardware stall.
> 
> The timeout is calculated as 2x the maximum frame length possible for a given
> mode, returned by the IPA.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom           |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp               |  2 ++
>  .../pipeline/raspberrypi/raspberrypi.cpp          | 15 +++++++++++++++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index acd3cafe6c91..5a228b75cd2f 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -41,6 +41,7 @@ struct IPAConfig {
>  struct StartConfig {
>  	libcamera.ControlList controls;
>  	int32 dropFrameCount;
> +	uint32 maxSensorFrameLengthMs;
>  };
>  
>  interface IPARPiInterface {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 1bf4e270b062..89767a9db471 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf
>  	}
>  
>  	startConfig->dropFrameCount = dropFrameCount_;
> +	const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;
> +	startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get<std::milli>();
>  
>  	firstStart_ = false;
>  	lastRunTimestamp_ = 0;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 8fd79be67988..98c463e39548 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -45,6 +45,8 @@
>  #include "dma_heaps.h"
>  #include "rpi_stream.h"
>  
> +using namespace std::chrono_literals;
> +
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(RPI)
> @@ -208,6 +210,7 @@ public:
>  	void setIspControls(const ControlList &controls);
>  	void setDelayedControls(const ControlList &controls);
>  	void setSensorControls(ControlList &controls);
> +	void unicamTimeout();
>  
>  	/* bufferComplete signal handlers. */
>  	void unicamBufferDequeue(FrameBuffer *buffer);
> @@ -1050,6 +1053,10 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>  		}
>  	}
>  
> +	/* Set the dequeue timeout to 2x the maximum possible frame duration. */
> +	utils::duration timeout(2 * startConfig.maxSensorFrameLengthMs * 1ms);

This is a utils::duration, but setDequeueTimeout takes a
utils::Duration (what have we done... ?). It will compile and I think do
the right thing thanks to implicit conversion, but it would be better to
use utils::Duration.

I could fix this locally too, but I'd feel better if you could test it,
would you mind sending a v5 that would also take the comments on 1/3
into account ?

> +	data->unicam_[Unicam::Image].dev()->setDequeueTimeout(timeout);
> +
>  	return 0;
>  }
>  
> @@ -1192,6 +1199,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
>  	data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3);
>  
>  	/* Wire up all the buffer connections. */
> +	data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout);
>  	data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);
>  	data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);
>  	data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue);
> @@ -1773,6 +1781,13 @@ void RPiCameraData::setSensorControls(ControlList &controls)
>  	sensor_->setControls(&controls);
>  }
>  
> +void RPiCameraData::unicamTimeout()
> +{
> +	LOG(RPI, Error) << "Unicam has timed out!";
> +	LOG(RPI, Error) << "Please check that your camera sensor connector is attached securely.";
> +	LOG(RPI, Error) << "Alternatively, try another cable and/or sensor.";
> +}
> +
>  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  {
>  	RPi::Stream *stream = nullptr;

-- 
Regards,

Laurent Pinchart