| Message ID | 20211022143907.3089419-6-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Fri, Oct 22, 2021 at 03:39:06PM +0100, Naushir Patuck wrote: > The unicam driver no longer regesters an embedded data node if the sensor does s/regesters/registers/ > not provide this stream. Account for this in the pipeline handler match routine > by not assuming it is always present. > > Add a warning if Unicam and the CamHelper do not agree on the presense of sensor > embedded data, and disable its useage in these cases. s/useage/usage/ > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Review-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 2b70b877e70a..c5e9607c7d95 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -984,7 +984,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > DeviceMatch unicam("unicam"); > DeviceMatch isp("bcm2835-isp"); > > - unicam.add("unicam-embedded"); > unicam.add("unicam-image"); > > isp.add("bcm2835-isp0-output0"); /* Input */ > @@ -1005,9 +1004,16 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > return false; > > /* Locate and open the unicam video streams. */ > - data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded")); > data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image")); > > + /* An embedded data node will not be present if the sensor does not support it. */ > + MediaEntity *embeddedEntity = unicam_->getEntityByName("unicam-embedded"); > + if (embeddedEntity) { > + data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", embeddedEntity); > + data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), > + &RPiCameraData::unicamBufferDequeue); It would be nice to keep signal connection uniform, but that's no big deal. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + } > + > /* Tag the ISP input stream as an import stream. */ > data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true); > data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1")); > @@ -1017,7 +1023,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > /* Wire up all the buffer connections. */ > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted); > data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); > - data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); > data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue); > data->isp_[Isp::Output0].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); > data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); > @@ -1045,6 +1050,11 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > return false; > } > > + if (sensorConfig.sensorMetadata ^ !!embeddedEntity) { > + LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!"; > + sensorConfig.sensorMetadata = false; > + } > + > /* > * Open all Unicam and ISP streams. The exception is the embedded data > * stream, which only gets opened below if the IPA reports that the sensor
Hi Laurent, Thank you for your feedback. On Mon, 25 Oct 2021 at 18:04, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Fri, Oct 22, 2021 at 03:39:06PM +0100, Naushir Patuck wrote: > > The unicam driver no longer regesters an embedded data node if the > sensor does > > s/regesters/registers/ > > > not provide this stream. Account for this in the pipeline handler match > routine > > by not assuming it is always present. > > > > Add a warning if Unicam and the CamHelper do not agree on the presense > of sensor > > embedded data, and disable its useage in these cases. > > s/useage/usage/ > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Review-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 16 +++++++++++++--- > > 1 file changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 2b70b877e70a..c5e9607c7d95 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -984,7 +984,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator > *enumerator) > > DeviceMatch unicam("unicam"); > > DeviceMatch isp("bcm2835-isp"); > > > > - unicam.add("unicam-embedded"); > > unicam.add("unicam-image"); > > > > isp.add("bcm2835-isp0-output0"); /* Input */ > > @@ -1005,9 +1004,16 @@ bool PipelineHandlerRPi::match(DeviceEnumerator > *enumerator) > > return false; > > > > /* Locate and open the unicam video streams. */ > > - data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", > unicam_->getEntityByName("unicam-embedded")); > > data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", > unicam_->getEntityByName("unicam-image")); > > > > + /* An embedded data node will not be present if the sensor does > not support it. */ > > + MediaEntity *embeddedEntity = > unicam_->getEntityByName("unicam-embedded"); > > + if (embeddedEntity) { > > + data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam > Embedded", embeddedEntity); > > + > data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), > > + > &RPiCameraData::unicamBufferDequeue); > > It would be nice to keep signal connection uniform, but that's no big > deal. > Not sure I understand here. If the embedded node is absent in cases where the sensor does not support it, data->unicam_[Unicam::Embedded].dev() will not be valid, so cannot connect to anything? Regards, Naush > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + } > > + > > /* Tag the ISP input stream as an import stream. */ > > data->isp_[Isp::Input] = RPi::Stream("ISP Input", > isp_->getEntityByName("bcm2835-isp0-output0"), true); > > data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", > isp_->getEntityByName("bcm2835-isp0-capture1")); > > @@ -1017,7 +1023,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator > *enumerator) > > /* Wire up all the buffer connections. */ > > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), > &RPiCameraData::frameStarted); > > > data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), > &RPiCameraData::unicamBufferDequeue); > > - > data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), > &RPiCameraData::unicamBufferDequeue); > > data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), > &RPiCameraData::ispInputDequeue); > > data->isp_[Isp::Output0].dev()->bufferReady.connect(data.get(), > &RPiCameraData::ispOutputDequeue); > > data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), > &RPiCameraData::ispOutputDequeue); > > @@ -1045,6 +1050,11 @@ bool PipelineHandlerRPi::match(DeviceEnumerator > *enumerator) > > return false; > > } > > > > + if (sensorConfig.sensorMetadata ^ !!embeddedEntity) { > > + LOG(RPI, Warning) << "Mismatch between Unicam and > CamHelper for embedded data usage!"; > > + sensorConfig.sensorMetadata = false; > > + } > > + > > /* > > * Open all Unicam and ISP streams. The exception is the embedded > data > > * stream, which only gets opened below if the IPA reports that > the sensor > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Tue, Oct 26, 2021 at 08:53:17AM +0100, Naushir Patuck wrote: > On Mon, 25 Oct 2021 at 18:04, Laurent Pinchart wrote: > > On Fri, Oct 22, 2021 at 03:39:06PM +0100, Naushir Patuck wrote: > > > The unicam driver no longer regesters an embedded data node if the sensor does > > > > s/regesters/registers/ > > > > > not provide this stream. Account for this in the pipeline handler match routine > > > by not assuming it is always present. > > > > > > Add a warning if Unicam and the CamHelper do not agree on the presense of sensor > > > embedded data, and disable its useage in these cases. > > > > s/useage/usage/ > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Review-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 16 +++++++++++++--- > > > 1 file changed, 13 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 2b70b877e70a..c5e9607c7d95 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -984,7 +984,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > > DeviceMatch unicam("unicam"); > > > DeviceMatch isp("bcm2835-isp"); > > > > > > - unicam.add("unicam-embedded"); > > > unicam.add("unicam-image"); > > > > > > isp.add("bcm2835-isp0-output0"); /* Input */ > > > @@ -1005,9 +1004,16 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > > return false; > > > > > > /* Locate and open the unicam video streams. */ > > > - data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded")); > > > data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image")); > > > > > > + /* An embedded data node will not be present if the sensor does not support it. */ > > > + MediaEntity *embeddedEntity = unicam_->getEntityByName("unicam-embedded"); > > > + if (embeddedEntity) { > > > + data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", embeddedEntity); > > > + data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), > > > + &RPiCameraData::unicamBufferDequeue); > > > > It would be nice to keep signal connection uniform, but that's no big > > deal. > > Not sure I understand here. If the embedded node is absent in cases where the sensor > does not support it, data->unicam_[Unicam::Embedded].dev() will not be valid, so cannot > connect to anything? I meant that it would have been nice to keep the signal connection with all the other ones below, but it's really not a big deal, just an OCD-triggered comment :-) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > + } > > > + > > > /* Tag the ISP input stream as an import stream. */ > > > data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true); > > > data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1")); > > > @@ -1017,7 +1023,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > > /* Wire up all the buffer connections. */ > > > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted); > > > data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); > > > - data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); > > > data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue); > > > data->isp_[Isp::Output0].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); > > > data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); > > > @@ -1045,6 +1050,11 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > > return false; > > > } > > > > > > + if (sensorConfig.sensorMetadata ^ !!embeddedEntity) { > > > + LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!"; > > > + sensorConfig.sensorMetadata = false; > > > + } > > > + > > > /* > > > * Open all Unicam and ISP streams. The exception is the embedded data > > > * stream, which only gets opened below if the IPA reports that the sensor
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 2b70b877e70a..c5e9607c7d95 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -984,7 +984,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) DeviceMatch unicam("unicam"); DeviceMatch isp("bcm2835-isp"); - unicam.add("unicam-embedded"); unicam.add("unicam-image"); isp.add("bcm2835-isp0-output0"); /* Input */ @@ -1005,9 +1004,16 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) return false; /* Locate and open the unicam video streams. */ - data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded")); data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image")); + /* An embedded data node will not be present if the sensor does not support it. */ + MediaEntity *embeddedEntity = unicam_->getEntityByName("unicam-embedded"); + if (embeddedEntity) { + data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", embeddedEntity); + data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), + &RPiCameraData::unicamBufferDequeue); + } + /* Tag the ISP input stream as an import stream. */ data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true); data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1")); @@ -1017,7 +1023,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) /* Wire up all the buffer connections. */ data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted); data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); - data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue); data->isp_[Isp::Output0].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); @@ -1045,6 +1050,11 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) return false; } + if (sensorConfig.sensorMetadata ^ !!embeddedEntity) { + LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!"; + sensorConfig.sensorMetadata = false; + } + /* * Open all Unicam and ISP streams. The exception is the embedded data * stream, which only gets opened below if the IPA reports that the sensor
The unicam driver no longer regesters an embedded data node if the sensor does not provide this stream. Account for this in the pipeline handler match routine by not assuming it is always present. Add a warning if Unicam and the CamHelper do not agree on the presense of sensor embedded data, and disable its useage in these cases. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Review-by: David Plowman <david.plowman@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)