[libcamera-devel] [PATCH v3] libcamera: pipeline: simple: Support camera sensors that contain an ISP

[Laurent Pinchart]

Hi Jacopo,

On Thu, Mar 04, 2021 at 07:38:35PM +0100, Jacopo Mondi wrote:
> On Wed, Mar 03, 2021 at 04:56:09PM +0200, Laurent Pinchart wrote:
> > Camera sensors can include an ISP. For instance, the AP1302 external ISP
> > can be connected to up to two raw camera sensors, and the combination of
> > the sensors and ISP is considered as a (smart) camera sensor from
> > libcamera's point of view.
> >
> > The CameraSensor class has limited support for this already. Extend the
> > simple pipeline handler to support such sensors, by using the media
> > entity corresponding to the ISP instead of the raw camera sensor's
> > entity.
> >
> > We don't need to handle the case where an entity in the SoC would expose
> > the MEDIA_ENT_F_PROC_VIDEO_ISP function, as pipeline containing an ISP
> > would have a dedicated pipeline handler.
> >
> > The implementation is limited as it won't support other multi-entity
> > camera sensors (such as CCS). While this would be worth supporting, we
> > don't have a test platform with a CCS-compatible sensor at this point,
> > so let's not over-engineer the solution. Extending support to CCS (and
> > possibly other sensor topologies) will likely involve helpers that can
> > be used by other pipeline handlers (such as generic graph walk helpers
> > for instance) and extensions to the CameraSensor class.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > Changes since v2:
> >
> > - Simplify link handling as all links can be enabled
> >
> > Changes since v1:
> >
> > - Fix link handling
> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 77 ++++++++++++++++++++----
> >  1 file changed, 64 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 7fba495cdc14..2e09d330c0e1 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -244,6 +244,8 @@ private:
> >  			PipelineHandler::cameraData(camera));
> >  	}
> >
> > +	std::vector<MediaEntity *> locateSensors();
> > +
> >  	void bufferReady(FrameBuffer *buffer);
> >  	void converterInputDone(FrameBuffer *buffer);
> >  	void converterOutputDone(FrameBuffer *buffer);
> > @@ -868,6 +870,67 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
> >   * Match and Setup
> >   */
> >
> > +std::vector<MediaEntity *> SimplePipelineHandler::locateSensors()
> > +{
> > +	std::vector<MediaEntity *> entities;
> > +
> > +	/*
> > +	 * Gather all the camera sensor entities based on the function they
> > +	 * expose.
> > +	 */
> > +	for (MediaEntity *entity : media_->entities()) {
> > +		if (entity->function() == MEDIA_ENT_F_CAM_SENSOR)
> > +			entities.push_back(entity);
> > +	}
> > +
> > +	if (entities.empty())
> > +		return {};
> > +
> > +	/*
> > +	 * Sensors can be made of multiple entities. For instance, a raw sensor
> > +	 * can be connected to an ISP, and the combination of both should be
> > +	 * treated as one sensor. To support this, as a crude heuristic, check
> > +	 * the downstream entity from the camera sensor, and if it is an ISP,
> > +	 * use it instead of the sensor.
> > +	 */
> > +	std::vector<MediaEntity *> sensors;
> > +
> > +	for (MediaEntity *entity : entities) {
> > +		/*
> > +		 * Locate the downstream entity by following the first link
> > +		 * from a source pad.
> > +		 */
> > +		const MediaLink *link = nullptr;
> > +
> > +		for (const MediaPad *pad : entity->pads()) {
> > +			if ((pad->flags() & MEDIA_PAD_FL_SOURCE) &&
> > +			    !pad->links().empty()) {
> > +				link = pad->links()[0];
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (!link)
> > +			continue;
> > +
> > +		MediaEntity *remote = link->sink()->entity();
> > +		if (remote->function() == MEDIA_ENT_F_PROC_VIDEO_ISP)
> > +			sensors.push_back(remote);
> > +		else
> > +			sensors.push_back(entity);
> > +	}
> > +
> > +	/*
> > +	 * Remove duplicates, in case multiple sensors are connected to the
> > +	 * same ISP.
> > +	 */
> > +	std::sort(sensors.begin(), sensors.end());
> > +	auto last = std::unique(sensors.begin(), sensors.end());
> > +	sensors.erase(last, sensors.end());
> 
> Mmmm, this seems a solution that supports a very specific topology,
> with a bit of assumptions, like using the first SOURCE pad you find
> from the sensor.
> 
> I'm not sure the last cleanup is helpful, if multiple sensor are
> connected to the same ISP as  part of the same camera device seems
> quite a specific use case. The will the ISP entity will be added twice, yes
> but do you have examples of such devices ?

This is specifically done to support the AP1302 ISP with two sensors. In
that case, the sensors have to be identical, the ISP synchronizes them,
and stitches the images together side by side (in a single VC/DT over
CSI-2). The combination of those three subdevs is handled by the simple
pipeline handler as a single sensor that produces an image twice as wide
as a single sensor.

> Anyway, I don't know very well the simple pipeline handler to really
> comment on the use cases it covers, but I wonder how long this change
> will stay here before we need  to introduce a SmartCameraSensor class,
> that takes the ISP entity and walks its sinks to find the image sensor
> connected there.

Possibly not very long, I'm not entirely sure yet :-) I think it will
definitely evolve, I'm just not sure in which direction exactly.

> The change is however good
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > +
> > +	return sensors;
> > +}
> > +
> >  bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> >  {
> >  	const SimplePipelineInfo *info = nullptr;
> > @@ -891,19 +954,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> >  	}
> >
> >  	/* Locate the sensors. */
> > -	std::vector<MediaEntity *> sensors;
> > -
> > -	for (MediaEntity *entity : media_->entities()) {
> > -		switch (entity->function()) {
> > -		case MEDIA_ENT_F_CAM_SENSOR:
> > -			sensors.push_back(entity);
> > -			break;
> > -
> > -		default:
> > -			break;
> > -		}
> > -	}
> > -
> > +	std::vector<MediaEntity *> sensors = locateSensors();
> >  	if (sensors.empty()) {
> >  		LOG(SimplePipeline, Error) << "No sensor found";
> >  		return false;

-- 
Regards,

Laurent Pinchart