| Message ID | 20210208013903.30915-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. 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@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? 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? > > 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. > + default: > LOG(CameraSensor, Error) > << "Invalid sensor function " > << utils::hex(entity_->function()); > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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. > > 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. > > + default: > > LOG(CameraSensor, Error) > > << "Invalid sensor function " > > << utils::hex(entity_->function());
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@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@ragnatech.se> > > > > + default: > > > LOG(CameraSensor, Error) > > > << "Invalid sensor function " > > > << utils::hex(entity_->function()); > > -- > Regards, > > Laurent Pinchart
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; + + default: LOG(CameraSensor, Error) << "Invalid sensor function " << utils::hex(entity_->function());
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@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.