[v2,03/10] libcamera: mali-c55: Acquire and plumb in 3a params and stats

20634 diff mbox series

Acquire the mali-c55 3a stats and parameters video devices during
::match() and plumb them in.

For this commit we simply allocate and release buffers for the
statistics and parameters. Statistics buffers are queue and dequeued
from the stats video device but their contents are for now untouched.

Acked-by: Nayden Kanchev  <nayden.kanchev@arm.com>
Co-developed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v2:

	- Links between the ISP subdevice and the statistics and parameters
	  video devices are now enabled (previously they were immutable)

 src/libcamera/pipeline/mali-c55/mali-c55.cpp | 174 ++++++++++++++++++-
 1 file changed, 171 insertions(+), 3 deletions(-)

Quoting Daniel Scally (2024-07-09 15:49:43)
> Acquire the mali-c55 3a stats and parameters video devices during
> ::match() and plumb them in.
> 
> For this commit we simply allocate and release buffers for the
> statistics and parameters. Statistics buffers are queue and dequeued
> from the stats video device but their contents are for now untouched.
> 
> Acked-by: Nayden Kanchev  <nayden.kanchev@arm.com>
> Co-developed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v2:
> 
>         - Links between the ISP subdevice and the statistics and parameters
>           video devices are now enabled (previously they were immutable)
> 
>  src/libcamera/pipeline/mali-c55/mali-c55.cpp | 174 ++++++++++++++++++-
>  1 file changed, 171 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> index 916b1d30..1e5674fc 100644
> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> @@ -12,6 +12,7 @@
>  #include <set>
>  #include <string>
>  
> +#include <linux/mali-c55-config.h>
>  #include <linux/media-bus-format.h>
>  #include <linux/media.h>
>  
> @@ -26,6 +27,7 @@
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/device_enumerator.h"
> +#include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
> @@ -510,6 +512,8 @@ public:
>  
>         int exportFrameBuffers(Camera *camera, Stream *stream,
>                                std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> +       int allocateBuffers(Camera *camera);
> +       void freeBuffers(Camera *camera);
>  
>         int start(Camera *camera, const ControlList *controls) override;
>         void stopDevice(Camera *camera) override;
> @@ -517,6 +521,7 @@ public:
>         int queueRequestDevice(Camera *camera, Request *request) override;
>  
>         void bufferReady(FrameBuffer *buffer);
> +       void statsBufferReady(FrameBuffer *buffer);
>  
>         bool match(DeviceEnumerator *enumerator) override;
>  
> @@ -576,6 +581,14 @@ private:
>  
>         MediaDevice *media_;
>         std::unique_ptr<V4L2Subdevice> isp_;
> +       std::unique_ptr<V4L2VideoDevice> stats_;
> +       std::unique_ptr<V4L2VideoDevice> params_;
> +
> +       std::vector<std::unique_ptr<FrameBuffer>> statsBuffers_;
> +       std::queue<FrameBuffer *> availableStatsBuffers_;
> +
> +       std::vector<std::unique_ptr<FrameBuffer>> paramsBuffers_;
> +       std::queue<FrameBuffer *> availableParamsBuffers_;
>  
>         std::array<MaliC55Pipe, MaliC55NumPipes> pipes_;
>  
> @@ -835,6 +848,16 @@ int PipelineHandlerMaliC55::configure(Camera *camera,
>                         return ret;
>         }
>  
> +       V4L2DeviceFormat statsFormat;
> +       ret = stats_->getFormat(&statsFormat);
> +       if (ret)
> +               return ret;
> +
> +       if (statsFormat.planes[0].size != sizeof(struct mali_c55_stats_buffer)) {
> +               LOG(MaliC55, Error) << "3a stats buffer size invalid";
> +               return -EINVAL;
> +       }
> +
>         /*
>          * Propagate the format to the ISP sink pad and configure the input
>          * crop rectangle (no crop at the moment).
> @@ -896,6 +919,27 @@ int PipelineHandlerMaliC55::configure(Camera *camera,
>                 pipe->stream = stream;
>         }
>  
> +       /*
> +        * Enable the media link between the ISP subdevice and the statistics
> +        * video device.
> +        */
> +       const MediaEntity *ispEntity = isp_->entity();
> +       ret = ispEntity->getPadByIndex(3)->links()[0]->setEnabled(true);

Are the pad indexes defined by the ABI from the kernel now? I wonder if
we should put the indexes into an enum or something.

Or I'm envisioning better ways to represent the links between two
entities - perhaps a later incarnation of the MediaPipeline or otherwise
might handle point to point links a bit more descriptively without
encoding pad numbers...

Still, that doesn't block here I don't think.


> +       if (ret) {
> +               LOG(MaliC55, Error) << "Couldn't enable statistics link";
> +               return ret;
> +       }
> +
> +       /*
> +        * Enable the media link between the ISP subdevice and the parameters
> +        * video device.
> +        */
> +       ret = ispEntity->getPadByIndex(4)->links()[0]->setEnabled(true);
> +       if (ret) {
> +               LOG(MaliC55, Error) << "Couldn't enable parameters link";
> +               return ret;
> +       }
> +

Are we now always enabling 'all' output links? Maybe we should just
iterate them instead of open coding each one ... but maybe it's also
just fine as it is as we directly report if there's an issue. Perhaps
something we could do to reduce code later when we know it's all working
well.

>         data->updateControls();
>  
>         return 0;
> @@ -910,27 +954,110 @@ int PipelineHandlerMaliC55::exportFrameBuffers(Camera *camera, Stream *stream,
>         return pipe->cap->exportBuffers(count, buffers);
>  }
>  
> -int PipelineHandlerMaliC55::start([[maybe_unused]] Camera *camera, [[maybe_unused]] const ControlList *controls)
> +void PipelineHandlerMaliC55::freeBuffers([[maybe_unused]] Camera *camera)
>  {
> +       while (!availableStatsBuffers_.empty())
> +               availableStatsBuffers_.pop();
> +       while (!availableParamsBuffers_.empty())
> +               availableParamsBuffers_.pop();
> +
> +       statsBuffers_.clear();
> +       paramsBuffers_.clear();
> +
> +       if (stats_->releaseBuffers())
> +               LOG(MaliC55, Error) << "Failed to release stats buffers";
> +
> +       if (params_->releaseBuffers())
> +               LOG(MaliC55, Error) << "Failed to release stats buffers";
> +
> +       return;
> +}
> +
> +int PipelineHandlerMaliC55::allocateBuffers(Camera *camera)
> +{
> +       MaliC55CameraData *data = cameraData(camera);
> +       unsigned int bufferCount;
> +       int ret;
> +
> +       bufferCount = std::max({
> +               data->frStream_.configuration().bufferCount,
> +               data->dsStream_.configuration().bufferCount,
> +       });
> +
> +       ret = stats_->allocateBuffers(bufferCount, &statsBuffers_);
> +       if (ret < 0)
> +               return ret;
> +
> +       for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_)
> +               availableStatsBuffers_.push(buffer.get());
> +
> +       ret = params_->allocateBuffers(bufferCount, &paramsBuffers_);
> +       if (ret < 0)
> +               return ret;
> +
> +       for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_)
> +               availableParamsBuffers_.push(buffer.get());
> +
> +       return 0;
> +}
> +
> +int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
> +{
> +       int ret;
> +
> +       ret = allocateBuffers(camera);
> +       if (ret)
> +               return ret;
> +
>         for (MaliC55Pipe &pipe : pipes_) {
>                 if (!pipe.stream)
>                         continue;
>  
>                 Stream *stream = pipe.stream;
>  
> -               int ret = pipe.cap->importBuffers(stream->configuration().bufferCount);
> +               ret = pipe.cap->importBuffers(stream->configuration().bufferCount);
>                 if (ret) {
>                         LOG(MaliC55, Error) << "Failed to import buffers";
> +                       freeBuffers(camera);
>                         return ret;
>                 }
>  
>                 ret = pipe.cap->streamOn();
>                 if (ret) {
>                         LOG(MaliC55, Error) << "Failed to start stream";
> +                       freeBuffers(camera);
>                         return ret;
>                 }
>         }
>  
> +       ret = stats_->streamOn();
> +       if (ret) {
> +               LOG(MaliC55, Error) << "Failed to start stats stream";
> +
> +               for (MaliC55Pipe &pipe : pipes_) {
> +                       if (pipe.stream)
> +                               pipe.cap->streamOff();
> +               }
> +
> +               freeBuffers(camera);
> +               return ret;
> +       }
> +
> +       ret = params_->streamOn();
> +       if (ret) {
> +               LOG(MaliC55, Error) << "Failed to start params stream";
> +
> +               stats_->streamOff();
> +
> +               for (MaliC55Pipe &pipe : pipes_) {
> +                       if (pipe.stream)
> +                               pipe.cap->streamOff();
> +               }
> +
> +               freeBuffers(camera);
> +               return ret;
> +       }
> +

Laurent merged a series that lets us add cleanup actions. There's a lot
of repeated cleanup in the error paths above that could probably make
good use of that.

>         return 0;
>  }
>  
> @@ -943,6 +1070,10 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera)
>                 pipe.cap->streamOff();
>                 pipe.cap->releaseBuffers();
>         }
> +
> +       stats_->streamOff();
> +       params_->streamOff();
> +       freeBuffers(camera);
>  }
>  
>  void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,
> @@ -1043,8 +1174,24 @@ void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,
>  
>  int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)
>  {
> +       FrameBuffer *statsBuffer;
>         int ret;
>  
> +       if (availableStatsBuffers_.empty()) {
> +               LOG(MaliC55, Error) << "Stats buffer underrun";
> +               return -ENOENT;
> +       }

I wonder what happens if we simulate this happening. Putting in a

 if (request->sequence % 100 == 0) {
        LOG(MaliC55, Error) << "FAKE Stats buffer underrun";
 	return -ENOENT;
 }

would be a good way to test that the pipeline handler can
continue/recover here.



Anyway, just some feedback - but I don't think any of it blocks so:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +
> +       statsBuffer = availableStatsBuffers_.front();
> +       availableStatsBuffers_.pop();
> +
> +       /*
> +        * We need to associate the Request to this buffer even though it's a
> +        * purely internal one because we will need to use request->sequence()
> +        * later.
> +        */
> +       statsBuffer->_d()->setRequest(request);
> +
>         for (auto &[stream, buffer] : request->buffers()) {
>                 MaliC55Pipe *pipe = pipeFromStream(cameraData(camera), stream);
>  
> @@ -1062,6 +1209,10 @@ int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)
>          */
>         applyScalerCrop(camera, request->controls());
>  
> +       ret = stats_->queueBuffer(statsBuffer);
> +       if (ret)
> +               return ret;
> +
>         return 0;
>  }
>  
> @@ -1073,6 +1224,11 @@ void PipelineHandlerMaliC55::bufferReady(FrameBuffer *buffer)
>                 completeRequest(request);
>  }
>  
> +void PipelineHandlerMaliC55::statsBufferReady(FrameBuffer *buffer)
> +{
> +       availableStatsBuffers_.push(buffer);
> +}
> +
>  void PipelineHandlerMaliC55::registerMaliCamera(std::unique_ptr<MaliC55CameraData> data,
>                                                 const std::string &name)
>  {
> @@ -1150,7 +1306,7 @@ bool PipelineHandlerMaliC55::match(DeviceEnumerator *enumerator)
>         const MediaPad *ispSink;
>  
>         /*
> -        * We search for just the ISP subdevice and the full resolution pipe.
> +        * We search for just the always-available elements of the media graph.
>          * The TPG and the downscale pipe are both optional blocks and may not
>          * be fitted.
>          */
> @@ -1158,6 +1314,8 @@ bool PipelineHandlerMaliC55::match(DeviceEnumerator *enumerator)
>         dm.add("mali-c55 isp");
>         dm.add("mali-c55 resizer fr");
>         dm.add("mali-c55 fr");
> +       dm.add("mali-c55 3a stats");
> +       dm.add("mali-c55 3a params");
>  
>         media_ = acquireMediaDevice(enumerator, dm);
>         if (!media_)
> @@ -1167,6 +1325,14 @@ bool PipelineHandlerMaliC55::match(DeviceEnumerator *enumerator)
>         if (isp_->open() < 0)
>                 return false;
>  
> +       stats_ = V4L2VideoDevice::fromEntityName(media_, "mali-c55 3a stats");
> +       if (stats_->open() < 0)
> +               return false;
> +
> +       params_ = V4L2VideoDevice::fromEntityName(media_, "mali-c55 3a params");
> +       if (params_->open() < 0)
> +               return false;
> +
>         MaliC55Pipe *frPipe = &pipes_[MaliC55FR];
>         frPipe->resizer = V4L2Subdevice::fromEntityName(media_, "mali-c55 resizer fr");
>         if (frPipe->resizer->open() < 0)
> @@ -1195,6 +1361,8 @@ bool PipelineHandlerMaliC55::match(DeviceEnumerator *enumerator)
>                 dsPipe->cap->bufferReady.connect(this, &PipelineHandlerMaliC55::bufferReady);
>         }
>  
> +       stats_->bufferReady.connect(this, &PipelineHandlerMaliC55::statsBufferReady);
> +
>         ispSink = isp_->entity()->getPadByIndex(0);
>         if (!ispSink || ispSink->links().empty()) {
>                 LOG(MaliC55, Error) << "ISP sink pad error";
> -- 
> 2.34.1
>