[libcamera-devel,v3,8/9] pipeline: raspberrypi: Account for a missing Unicam embedded data node
diff mbox series

Message ID 20211027092803.3671096-9-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Conversion to media controller
Related show

Commit Message

Naushir Patuck Oct. 27, 2021, 9:28 a.m. UTC 
The unicam driver no longer registers 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 usage in these cases.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp         | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

Kieran Bingham Oct. 27, 2021, 12:23 p.m. UTC | #1 
Quoting Naushir Patuck (2021-10-27 10:28:02)
> The unicam driver no longer registers 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 usage in these cases.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 e01359b20fd9..52521857b61c 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1015,7 +1015,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 */
> @@ -1036,9 +1035,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"));
> @@ -1048,7 +1054,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);
> @@ -1076,6 +1081,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;

Should the data->unicam_[Unicam::Embedded] stream/ signal be
disconnected here? (if they were created) or will they just be fine
sitting idle?

I presume they'll be fine and keep otherwise quiet on the pipeline, and
get cleaned up at the end anyway.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +       }
> +
>         /*
>          * 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
>
Naushir Patuck Oct. 27, 2021, 12:40 p.m. UTC | #2 
Hi Kieran,

Thank you for your feedback,

On Wed, 27 Oct 2021 at 13:23, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Quoting Naushir Patuck (2021-10-27 10:28:02)
> > The unicam driver no longer registers 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 usage in these cases.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 e01359b20fd9..52521857b61c 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1015,7 +1015,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 */
> > @@ -1036,9 +1035,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"));
> > @@ -1048,7 +1054,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);
> > @@ -1076,6 +1081,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;
>
> Should the data->unicam_[Unicam::Embedded] stream/ signal be
> disconnected here? (if they were created) or will they just be fine
> sitting idle?
>
> I presume they'll be fine and keep otherwise quiet on the pipeline, and
> get cleaned up at the end anyway.
>

It shouldn't matter as it will never be triggered, but I will add a
disconnect() call
in these cases.

Regards,
Naush


>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> > +       }
> > +
> >         /*
> >          * 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
> >
>
Laurent Pinchart Oct. 27, 2021, 2:23 p.m. UTC | #3 
On Wed, Oct 27, 2021 at 01:40:53PM +0100, Naushir Patuck wrote:
> On Wed, 27 Oct 2021 at 13:23, Kieran Bingham  wrote:
> 
> > Quoting Naushir Patuck (2021-10-27 10:28:02)
> > > The unicam driver no longer registers 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 usage in these cases.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 e01359b20fd9..52521857b61c 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1015,7 +1015,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 */
> > > @@ -1036,9 +1035,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"));
> > > @@ -1048,7 +1054,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);
> > > @@ -1076,6 +1081,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;
> >
> > Should the data->unicam_[Unicam::Embedded] stream/ signal be
> > disconnected here? (if they were created) or will they just be fine
> > sitting idle?
> >
> > I presume they'll be fine and keep otherwise quiet on the pipeline, and
> > get cleaned up at the end anyway.
> 
> It shouldn't matter as it will never be triggered, but I will add a disconnect() call
> in these cases.

If it's never triggered we can keep it connected. If it happens to get
triggered anyway, the event will never be handled, and the event loop
will always wake up immediately, so we'll be in trouble.

> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > > +       }
> > > +
> > >         /*
> > >          * 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

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index e01359b20fd9..52521857b61c 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1015,7 +1015,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 */
@@ -1036,9 +1035,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"));
@@ -1048,7 +1054,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);
@@ -1076,6 +1081,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