[libcamera-devel] [PATCH/RFC] libcamera: camera_sensor: Accept entities exposing the ISP function

[Niklas Söderlund]

Hi Laurent,

On 2021-02-09 00:33:36 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Mon, Feb 08, 2021 at 09:16:01PM +0100, Niklas Söderlund wrote:
> > On 2021-02-08 03:39:03 +0200, Laurent Pinchart wrote:
> > > Camera sensors can include an ISP, which may be reported as a separate
> > > entity from the pixel array in the media graph.
> > > 
> > > Support such sensors by accepting MEDIA_ENT_F_PROC_VIDEO_ISP as a valid
> > > entity type. This allows using sensors that can be fully (or at least
> > > meaningfully) configured through the ISP's source pad only. Sensors that
> > > require further configuration, on the ISP sink pad and/or on the pixel
> > > array's source pad, will require further extension to the CameraSensor
> > > class.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  src/libcamera/camera_sensor.cpp | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > This patch depends on the new MEDIA_ENT_F_PROC_VIDEO_ISP entity
> > > function, which will be available in v5.12-rc1. I'll update the kernel
> > > headers when that kernel will be released, in a few weeks from now.
> > 
> > Do I understand things correctly that such sensors would primarily be 
> > used with the simple pipeline handler?
> 
> That's correct, it's the primary target.
> 
> > How would this work if someones 
> > wires up a sensor with an ISP to a pipeline with an IPA? Is the inline 
> > ISP close to the sensor always "passive" and controlled by user-space or 
> > can we create a tug-of-war between the sensor inline ISP and a pipeline 
> > IPA/ISP?
> 
> If someone wired a "smart" sensor to an SoC with an ISP, we would likely
> need to disable one of the two ISPs. Such a design could occur with the
> aim of using the camera sensor in raw mode with the SoC-side ISP (if the
> camera sensor supports raw capture in addition of YUV), or to use the
> sensor-side ISP only. This isn't a situation we support today, and we
> would need to further extend the CameraSensor class, as well as the
> pipeline handlers.
> 
> Note that this patch in itself will not cause the CameraSensor class to
> incorrectly try to handle the SoC-side ISP for a raw sensor connected
> directly to an SoC ISP entity, as it's the pipeline handler that decides
> which entity to pass to the CameraSensor class.

That is cool, just wanted to check I understood the situation. As you 
point out to support this we will need to extend CameraSensor and doing 
so based on the function is a nice way to do that, in the future. For 
now I think this patch is doing the right thing to expand the scope of 
things we support.

> 
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index 59834ffcdd94..8158a84b63db 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -198,7 +198,12 @@ int CameraSensor::init()
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	if (entity_->function() != MEDIA_ENT_F_CAM_SENSOR) {
> > > +	switch (entity_->function()) {
> > > +	case MEDIA_ENT_F_CAM_SENSOR:
> > > +	case MEDIA_ENT_F_PROC_VIDEO_ISP:
> > > +		break;
> > > +
> > 
> > nit: I would remove this blank line.
> 
> I like the blank line :-) But I can drop it too.

Then by all means keep it ;-)

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> 
> > > +	default:
> > >  		LOG(CameraSensor, Error)
> > >  			<< "Invalid sensor function "
> > >  			<< utils::hex(entity_->function());
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund