<div dir="ltr"><div dir="ltr">Hi David,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 22 Oct 2021 at 14:49, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
On Fri, 22 Oct 2021 at 14:31, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Hi David,<br>
><br>
> Thank you for your feedback.<br>
><br>
> On Fri, 22 Oct 2021 at 14:18, David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>> wrote:<br>
>><br>
>> Hi Naush<br>
>><br>
>> Thanks for this patch.<br>
>><br>
>> On Fri, 22 Oct 2021 at 12:55, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
>> ><br>
>> > The unicam driver no longer regesters an embedded data node if the sensor does<br>
>> > not provide this stream. Account for this in the pipeline handler match routine<br>
>> > by not assuming it is always present.<br>
>> ><br>
>> > Add a warning if Unicam and the CamHelper do not agree on the presense of sensor<br>
>> > embedded data, and disable its useage in these cases.<br>
>> ><br>
>> > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
>> > ---<br>
>> > .../pipeline/raspberrypi/raspberrypi.cpp | 16 +++++++++++++---<br>
>> > 1 file changed, 13 insertions(+), 3 deletions(-)<br>
>> ><br>
>> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
>> > index fc190e39732a..5aaf24436f27 100644<br>
>> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
>> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
>> > @@ -994,7 +994,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)<br>
>> > DeviceMatch unicam("unicam");<br>
>> > DeviceMatch isp("bcm2835-isp");<br>
>> ><br>
>> > - unicam.add("unicam-embedded");<br>
>> > unicam.add("unicam-image");<br>
>> ><br>
>> > isp.add("bcm2835-isp0-output0"); /* Input */<br>
>> > @@ -1015,9 +1014,16 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)<br>
>> > return false;<br>
>> ><br>
>> > /* Locate and open the unicam video streams. */<br>
>> > - data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));<br>
>> > data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image"));<br>
>> ><br>
>> > + /* An embedded data node will not be present if the sensor does not support it. */<br>
>> > + MediaEntity *embeddedEntity = unicam_->getEntityByName("unicam-embedded");<br>
>> > + if (embeddedEntity) {<br>
>> > + data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", embeddedEntity);<br>
>> > + data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),<br>
>> > + &RPiCameraData::unicamBufferDequeue);<br>
>> > + }<br>
>> > +<br>
>> > /* Tag the ISP input stream as an import stream. */<br>
>> > data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);<br>
>> > data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));<br>
>> > @@ -1027,7 +1033,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)<br>
>> > /* Wire up all the buffer connections. */<br>
>> > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);<br>
>> > data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);<br>
>> > - data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);<br>
>> > data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue);<br>
>> > data->isp_[Isp::Output0].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);<br>
>> > data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);<br>
>> > @@ -1055,6 +1060,11 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)<br>
>> > return false;<br>
>> > }<br>
>> ><br>
>> > + if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {<br>
>> > + LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!";<br>
>> > + sensorConfig.sensorMetadata = false;<br>
>> > + }<br>
>> > +<br>
>> > /*<br>
>> > * Open all Unicam and ISP streams. The exception is the embedded data<br>
>> > * stream, which only gets opened below if the IPA reports that the sensor<br>
>> > --<br>
>> > 2.25.1<br>
>> ><br>
>><br>
>> I was just wondering, is there any implication for our "embedded data<br>
>> present" CamHelper functions? Can we now just tell, or do we still<br>
>> need them?<br>
><br>
><br>
> It does make CamHelper::SensorEmbeddedDataPresent() a bit redundant.<br>
> I currently just use it to verify that both kernel and userland agree, but perhaps<br>
> I should remove that member function entirely?<br>
<br>
Personally I'd vote for removal, though I don't feel very strongly if<br>
anyone disagrees. The less "boilerplate" in there that you can get<br>
wrong, the better.<br>
<br>
But we do just need to consider the CamHelperImx219 where the embedded<br>
data is technically available but we choose to ignore it. Will that<br>
still work?<br></blockquote><div><br></div><div>Good point. With this change as-is, we will safely ignore the imx219 embedded</div><div>data. If we were to remove CamHelper::SensorEmbeddedDataPresent(), the</div><div>only way to avoid using it would be to report correctly through the sensor driver</div><div>that embedded data is missing.</div><div><br></div><div>Perhaps we are not ready to remove it just yet...</div><div><br></div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
David<br>
<br>
><br>
> Regards,<br>
> Naush<br>
><br>
>><br>
>><br>
>> Other than this:<br>
>><br>
>> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
>><br>
>> Thanks!<br>
>><br>
>> David<br>
</blockquote></div></div>