| Message ID | 20211022115537.2964533-6-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this patch. On Fri, 22 Oct 2021 at 12:55, Naushir Patuck <naush@raspberrypi.com> wrote: > > 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> > --- > .../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 fc190e39732a..5aaf24436f27 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -994,7 +994,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 */ > @@ -1015,9 +1014,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")); > @@ -1027,7 +1033,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); > @@ -1055,6 +1060,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 > -- > 2.25.1 > I was just wondering, is there any implication for our "embedded data present" CamHelper functions? Can we now just tell, or do we still need them? Other than this: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David
Hi David, Thank you for your feedback. On Fri, 22 Oct 2021 at 14:18, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for this patch. > > On Fri, 22 Oct 2021 at 12:55, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > 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> > > --- > > .../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 fc190e39732a..5aaf24436f27 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -994,7 +994,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 */ > > @@ -1015,9 +1014,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")); > > @@ -1027,7 +1033,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); > > @@ -1055,6 +1060,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 > > -- > > 2.25.1 > > > > I was just wondering, is there any implication for our "embedded data > present" CamHelper functions? Can we now just tell, or do we still > need them? > It does make CamHelper::SensorEmbeddedDataPresent() a bit redundant. I currently just use it to verify that both kernel and userland agree, but perhaps I should remove that member function entirely? Regards, Naush > > Other than this: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks! > > David >
Hi Naush On Fri, 22 Oct 2021 at 14:31, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi David, > > Thank you for your feedback. > > On Fri, 22 Oct 2021 at 14:18, David Plowman <david.plowman@raspberrypi.com> wrote: >> >> Hi Naush >> >> Thanks for this patch. >> >> On Fri, 22 Oct 2021 at 12:55, Naushir Patuck <naush@raspberrypi.com> wrote: >> > >> > 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> >> > --- >> > .../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 fc190e39732a..5aaf24436f27 100644 >> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> > @@ -994,7 +994,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 */ >> > @@ -1015,9 +1014,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")); >> > @@ -1027,7 +1033,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); >> > @@ -1055,6 +1060,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 >> > -- >> > 2.25.1 >> > >> >> I was just wondering, is there any implication for our "embedded data >> present" CamHelper functions? Can we now just tell, or do we still >> need them? > > > It does make CamHelper::SensorEmbeddedDataPresent() a bit redundant. > I currently just use it to verify that both kernel and userland agree, but perhaps > I should remove that member function entirely? Personally I'd vote for removal, though I don't feel very strongly if anyone disagrees. The less "boilerplate" in there that you can get wrong, the better. But we do just need to consider the CamHelperImx219 where the embedded data is technically available but we choose to ignore it. Will that still work? David > > Regards, > Naush > >> >> >> Other than this: >> >> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> >> >> Thanks! >> >> David
Hi David, On Fri, 22 Oct 2021 at 14:49, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > On Fri, 22 Oct 2021 at 14:31, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > Hi David, > > > > Thank you for your feedback. > > > > On Fri, 22 Oct 2021 at 14:18, David Plowman < > david.plowman@raspberrypi.com> wrote: > >> > >> Hi Naush > >> > >> Thanks for this patch. > >> > >> On Fri, 22 Oct 2021 at 12:55, Naushir Patuck <naush@raspberrypi.com> > wrote: > >> > > >> > 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> > >> > --- > >> > .../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 fc190e39732a..5aaf24436f27 100644 > >> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >> > @@ -994,7 +994,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 */ > >> > @@ -1015,9 +1014,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")); > >> > @@ -1027,7 +1033,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); > >> > @@ -1055,6 +1060,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 > >> > -- > >> > 2.25.1 > >> > > >> > >> I was just wondering, is there any implication for our "embedded data > >> present" CamHelper functions? Can we now just tell, or do we still > >> need them? > > > > > > It does make CamHelper::SensorEmbeddedDataPresent() a bit redundant. > > I currently just use it to verify that both kernel and userland agree, > but perhaps > > I should remove that member function entirely? > > Personally I'd vote for removal, though I don't feel very strongly if > anyone disagrees. The less "boilerplate" in there that you can get > wrong, the better. > > But we do just need to consider the CamHelperImx219 where the embedded > data is technically available but we choose to ignore it. Will that > still work? > Good point. With this change as-is, we will safely ignore the imx219 embedded data. If we were to remove CamHelper::SensorEmbeddedDataPresent(), the only way to avoid using it would be to report correctly through the sensor driver that embedded data is missing. Perhaps we are not ready to remove it just yet... Naush > > David > > > > > Regards, > > Naush > > > >> > >> > >> Other than this: > >> > >> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > >> > >> Thanks! > >> > >> David >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index fc190e39732a..5aaf24436f27 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -994,7 +994,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 */ @@ -1015,9 +1014,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")); @@ -1027,7 +1033,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); @@ -1055,6 +1060,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> --- .../pipeline/raspberrypi/raspberrypi.cpp | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)