| Message ID | 20210218170126.2060783-4-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. s/enabled/enable/ in the subject ? On Thu, Feb 18, 2021 at 05:01:25PM +0000, Naushir Patuck wrote: > The pipeline handler would enable and use the Unicam embedded data stream > even if the sensor did not support it. This was to allow a means to > pass exposure and gain values for the frame to the IPA in a synchronised > way. > > The recent changes to get the pipeline handler to pass a ControlList > with exposure and gain values means this is no longer required. Disable > the use of the embedded data stream when a sensor does not support it. Nice :-) > This change also removes the mappedEmbeddedBuffers_ map as it is no > longer used. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 112 +++++++++++------- > 1 file changed, 70 insertions(+), 42 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index b43d86166c63..d969c77993eb 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -177,12 +177,6 @@ public: > /* Stores the ids of the buffers mapped in the IPA. */ > std::unordered_set<unsigned int> ipaBuffers_; > > - /* > - * Map of (internal) mmaped embedded data buffers, to avoid having to > - * map/unmap on every frame. > - */ > - std::map<unsigned int, MappedFrameBuffer> mappedEmbeddedBuffers_; > - > /* DMAHEAP allocation helper. */ > RPi::DmaHeap dmaHeap_; > FileDescriptor lsTable_; > @@ -636,14 +630,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > > if (isRaw(cfg.pixelFormat)) { > cfg.setStream(&data->unicam_[Unicam::Image]); > - /* > - * We must set both Unicam streams as external, even > - * though the application may only request RAW frames. > - * This is because we match timestamps on both streams > - * to synchronise buffers. > - */ > data->unicam_[Unicam::Image].setExternal(true); > - data->unicam_[Unicam::Embedded].setExternal(true); > continue; > } > > @@ -715,17 +702,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > return ret; > } > > - /* Unicam embedded data output format. */ > - format = {}; > - format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA); > - LOG(RPI, Debug) << "Setting embedded data format."; > - ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format); > - if (ret) { > - LOG(RPI, Error) << "Failed to set format on Unicam embedded: " > - << format.toString(); > - return ret; > - } > - > /* Figure out the smallest selection the ISP will allow. */ > Rectangle testCrop(0, 0, 1, 1); > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop); > @@ -742,6 +718,41 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > if (ret) > LOG(RPI, Error) << "Failed to configure the IPA: " << ret; > > + /* > + * The IPA will set data->sensorMetadata_ to true if embedded data is > + * supported on this sensor. If so, open the Unicam embedded data > + * node and configure the output format. > + */ > + if (data->sensorMetadata_) { > + format = {}; > + format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA); > + LOG(RPI, Debug) << "Setting embedded data format."; > + data->unicam_[Unicam::Embedded].dev()->open(); The device is opened here, but never closed. Should that be fixed ? What are the drawbacks in opening the device in match() as done today, even if not used ? > + ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format); > + if (ret) { > + LOG(RPI, Error) << "Failed to set format on Unicam embedded: " > + << format.toString(); > + return ret; > + } > + > + /* > + * If a RAW/Bayer stream has been requested by the application, > + * we must set both Unicam streams as external, even though the > + * application may only request RAW frames. This is because we > + * match timestamps on both streams to synchronise buffers. > + */ > + if (rawStream) > + data->unicam_[Unicam::Embedded].setExternal(true); > + } else { > + /* > + * No embedded data present, so we do not want to iterate over > + * the embedded data stream when starting and stopping. > + */ > + data->streams_.erase(std::remove(data->streams_.begin(), data->streams_.end(), > + &data->unicam_[Unicam::Embedded]), > + data->streams_.end()); Hmmmm... Given that only one sensor, and thus one camera, is supported by this pipeline handler, this should work, but conceptually it's not very nice. Isn't this a decision that should be made at match() time, not configure() time ? It would be right to do this here if the code was modelled to support multiple sensors, but in that case we would need to close() the embedded data video node in appropriate locations, and also add the embedded stream back to data->streams_ when metadata is present. One option to move this to match() time would be for the IPA to return if the sensor supports metadata from init() instead of configure(). That would require passing the sensor name to the IPA in init() too. It's the mix n' match that bothers me I think. I don't mind if we decide to model the pipeline handler and IPA with the assumption that there can only be a single camera, with a hardcoded pipeline known from the very beginning, or in a more dynamic way that could allow runtime switching between sensors, but it would be nice if the architecture of the pipeline handler and IPA was consistent relatively to the model we pick. Does this make sense ? > + } > + > /* > * Update the ScalerCropMaximum to the correct value for this camera mode. > * For us, it's the same as the "analogue crop". > @@ -949,10 +960,16 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > for (auto &stream : data->isp_) > data->streams_.push_back(&stream); > > - /* Open all Unicam and ISP streams. */ > + /* > + * Open all Unicam and ISP streams. The exception is the embedded data > + * stream, which only gets opened if the IPA reports that the sensor > + * supports embedded data. This happens in RPiCameraData::configureIPA(). > + */ > for (auto const stream : data->streams_) { > - if (stream->dev()->open()) > - return false; > + if (stream != &data->unicam_[Unicam::Embedded]) { > + if (stream->dev()->open()) > + return false; > + } > } > > /* Wire up all the buffer connections. */ > @@ -1109,19 +1126,13 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > return ret; > } > > - if (!data->sensorMetadata_) { > - for (auto const &it : data->unicam_[Unicam::Embedded].getBuffers()) { > - MappedFrameBuffer fb(it.second, PROT_READ | PROT_WRITE); > - data->mappedEmbeddedBuffers_.emplace(it.first, std::move(fb)); > - } > - } > - > /* > * Pass the stats and embedded data buffers to the IPA. No other > * buffers need to be passed. > */ > mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), ipa::RPi::MaskStats); > - mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), ipa::RPi::MaskEmbeddedData); > + if (data->sensorMetadata_) > + mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), ipa::RPi::MaskEmbeddedData); Maybe a bit of line wrap ? :-) > > return 0; > } > @@ -1154,7 +1165,6 @@ void PipelineHandlerRPi::freeBuffers(Camera *camera) > std::vector<unsigned int> ipaBuffers(data->ipaBuffers_.begin(), data->ipaBuffers_.end()); > data->ipa_->unmapBuffers(ipaBuffers); > data->ipaBuffers_.clear(); > - data->mappedEmbeddedBuffers_.clear(); > > for (auto const stream : data->streams_) > stream->releaseBuffers(); > @@ -1652,7 +1662,7 @@ void RPiCameraData::tryRunPipeline() > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > if (state_ != State::Idle || requestQueue_.empty() || > - bayerQueue_.empty() || embeddedQueue_.empty()) > + bayerQueue_.empty() || (embeddedQueue_.empty() && sensorMetadata_)) > return; > > if (!findMatchingBuffers(bayerFrame, embeddedBuffer)) > @@ -1675,17 +1685,24 @@ void RPiCameraData::tryRunPipeline() > state_ = State::Busy; > > unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer); > - unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > LOG(RPI, Debug) << "Signalling signalIspPrepare:" > - << " Bayer buffer id: " << bayerId > - << " Embedded buffer id: " << embeddedId; > + << " Bayer buffer id: " << bayerId; > > ipa::RPi::ISPConfig ispPrepare; > - ispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData | embeddedId; > ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId; > - ispPrepare.embeddedBufferPresent = true; > ispPrepare.controls = std::move(bayerFrame.controls); > + > + if (embeddedBuffer) { > + unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > + > + ispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData | embeddedId; > + ispPrepare.embeddedBufferPresent = true; > + > + LOG(RPI, Debug) << "Signalling signalIspPrepare:" > + << " Bayer buffer id: " << embeddedId; > + } > + > ipa_->signalIspPrepare(ispPrepare); > } > > @@ -1727,6 +1744,17 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em > > LOG(RPI, Debug) << "Could not find matching embedded buffer"; > > + if (!sensorMetadata_) { > + /* > + * If there is no sensor metadata, simply return the > + * first bayer frame in the queue. > + */ > + LOG(RPI, Debug) << "Returning bayer frame without a match"; > + bayerQueue_.pop(); > + embeddedBuffer = nullptr; > + return true; > + } > + > if (!embeddedQueue_.empty()) { > /* > * Not found a matching embedded buffer for the bayer buffer in
Hi Laurent, Thank you for your review feedback. On Sun, 21 Feb 2021 at 23:08, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > s/enabled/enable/ in the subject ? > > On Thu, Feb 18, 2021 at 05:01:25PM +0000, Naushir Patuck wrote: > > The pipeline handler would enable and use the Unicam embedded data stream > > even if the sensor did not support it. This was to allow a means to > > pass exposure and gain values for the frame to the IPA in a synchronised > > way. > > > > The recent changes to get the pipeline handler to pass a ControlList > > with exposure and gain values means this is no longer required. Disable > > the use of the embedded data stream when a sensor does not support it. > > Nice :-) > > > This change also removes the mappedEmbeddedBuffers_ map as it is no > > longer used. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 112 +++++++++++------- > > 1 file changed, 70 insertions(+), 42 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index b43d86166c63..d969c77993eb 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -177,12 +177,6 @@ public: > > /* Stores the ids of the buffers mapped in the IPA. */ > > std::unordered_set<unsigned int> ipaBuffers_; > > > > - /* > > - * Map of (internal) mmaped embedded data buffers, to avoid having > to > > - * map/unmap on every frame. > > - */ > > - std::map<unsigned int, MappedFrameBuffer> mappedEmbeddedBuffers_; > > - > > /* DMAHEAP allocation helper. */ > > RPi::DmaHeap dmaHeap_; > > FileDescriptor lsTable_; > > @@ -636,14 +630,7 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > > > if (isRaw(cfg.pixelFormat)) { > > cfg.setStream(&data->unicam_[Unicam::Image]); > > - /* > > - * We must set both Unicam streams as external, > even > > - * though the application may only request RAW > frames. > > - * This is because we match timestamps on both > streams > > - * to synchronise buffers. > > - */ > > data->unicam_[Unicam::Image].setExternal(true); > > - data->unicam_[Unicam::Embedded].setExternal(true); > > continue; > > } > > > > @@ -715,17 +702,6 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > return ret; > > } > > > > - /* Unicam embedded data output format. */ > > - format = {}; > > - format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA); > > - LOG(RPI, Debug) << "Setting embedded data format."; > > - ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format); > > - if (ret) { > > - LOG(RPI, Error) << "Failed to set format on Unicam > embedded: " > > - << format.toString(); > > - return ret; > > - } > > - > > /* Figure out the smallest selection the ISP will allow. */ > > Rectangle testCrop(0, 0, 1, 1); > > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, > &testCrop); > > @@ -742,6 +718,41 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > if (ret) > > LOG(RPI, Error) << "Failed to configure the IPA: " << ret; > > > > + /* > > + * The IPA will set data->sensorMetadata_ to true if embedded data > is > > + * supported on this sensor. If so, open the Unicam embedded data > > + * node and configure the output format. > > + */ > > + if (data->sensorMetadata_) { > > + format = {}; > > + format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA); > > + LOG(RPI, Debug) << "Setting embedded data format."; > > + data->unicam_[Unicam::Embedded].dev()->open(); > > The device is opened here, but never closed. Should that be fixed ? What > are the drawbacks in opening the device in match() as done today, even > if not used ? > We rely on the V4L2VideoDevice destructor to close down all the device nodes. I take it this is not the way to do it? :-) If not, where would you advise to put this call? Since we call open() in match(), it does not seem right to close the node in stop(). I did have a quick scan of rkisp1 and ipu3 pipelines, and they do not seem to call close() on devices either, so no hints there. The reason why we don't open the embedded data node in match() like we used to is because of the kernel driver. In order to synchronize buffers for embedded data and image data nodes in the driver, we must open the embedded data node if it is used, i.e. we cannot keep it open and not queue it buffers to "disable" the stream. Of course, this can change in the driver, but it does add mode complexity to the buffer sync logic. Perhaps this should be noted as a \todo to fix in the future. > > > + ret = > data->unicam_[Unicam::Embedded].dev()->setFormat(&format); > > + if (ret) { > > + LOG(RPI, Error) << "Failed to set format on Unicam > embedded: " > > + << format.toString(); > > + return ret; > > + } > > + > > + /* > > + * If a RAW/Bayer stream has been requested by the > application, > > + * we must set both Unicam streams as external, even > though the > > + * application may only request RAW frames. This is > because we > > + * match timestamps on both streams to synchronise buffers. > > + */ > > + if (rawStream) > > + data->unicam_[Unicam::Embedded].setExternal(true); > > + } else { > > + /* > > + * No embedded data present, so we do not want to iterate > over > > + * the embedded data stream when starting and stopping. > > + */ > > + data->streams_.erase(std::remove(data->streams_.begin(), > data->streams_.end(), > > + > &data->unicam_[Unicam::Embedded]), > > + data->streams_.end()); > > Hmmmm... Given that only one sensor, and thus one camera, is supported > by this pipeline handler, this should work, but conceptually it's not > very nice. Isn't this a decision that should be made at match() time, > not configure() time ? It would be right to do this here if the code was > modelled to support multiple sensors, but in that case we would need to > close() the embedded data video node in appropriate locations, and also > add the embedded stream back to data->streams_ when metadata is present. > Fully agree. I did want this to be handled in match(), but as you said, we do not have the information about sensor metadata support there. > > One option to move this to match() time would be for the IPA to return > if the sensor supports metadata from init() instead of configure(). That > would require passing the sensor name to the IPA in init() too. > > It's the mix n' match that bothers me I think. I don't mind if we decide > to model the pipeline handler and IPA with the assumption that there can > only be a single camera, with a hardcoded pipeline known from the very > beginning, or in a more dynamic way that could allow runtime switching > between sensors, but it would be nice if the architecture of the > pipeline handler and IPA was consistent relatively to the model we pick. > Does this make sense ? > Yes, I do agree with this as well. I am happy for ipa->init() to pass back the required parameters so match() can be used to open the embedded data node if required. I presume we have all the tools needed to do this with the IPA interfaces change to use mojom definitions? If so I can update the signature of ipa->init() to pass in the sensor name and return out the "SensorConfig" parameters. Regards, Naush > > > + } > > + > > /* > > * Update the ScalerCropMaximum to the correct value for this > camera mode. > > * For us, it's the same as the "analogue crop". > > @@ -949,10 +960,16 @@ bool PipelineHandlerRPi::match(DeviceEnumerator > *enumerator) > > for (auto &stream : data->isp_) > > data->streams_.push_back(&stream); > > > > - /* Open all Unicam and ISP streams. */ > > + /* > > + * Open all Unicam and ISP streams. The exception is the embedded > data > > + * stream, which only gets opened if the IPA reports that the > sensor > > + * supports embedded data. This happens in > RPiCameraData::configureIPA(). > > + */ > > for (auto const stream : data->streams_) { > > - if (stream->dev()->open()) > > - return false; > > + if (stream != &data->unicam_[Unicam::Embedded]) { > > + if (stream->dev()->open()) > > + return false; > > + } > > } > > > > /* Wire up all the buffer connections. */ > > @@ -1109,19 +1126,13 @@ int PipelineHandlerRPi::prepareBuffers(Camera > *camera) > > return ret; > > } > > > > - if (!data->sensorMetadata_) { > > - for (auto const &it : > data->unicam_[Unicam::Embedded].getBuffers()) { > > - MappedFrameBuffer fb(it.second, PROT_READ | > PROT_WRITE); > > - data->mappedEmbeddedBuffers_.emplace(it.first, > std::move(fb)); > > - } > > - } > > - > > /* > > * Pass the stats and embedded data buffers to the IPA. No other > > * buffers need to be passed. > > */ > > mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), > ipa::RPi::MaskStats); > > - mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), > ipa::RPi::MaskEmbeddedData); > > + if (data->sensorMetadata_) > > + mapBuffers(camera, > data->unicam_[Unicam::Embedded].getBuffers(), ipa::RPi::MaskEmbeddedData); > > Maybe a bit of line wrap ? :-) > > > > > return 0; > > } > > @@ -1154,7 +1165,6 @@ void PipelineHandlerRPi::freeBuffers(Camera > *camera) > > std::vector<unsigned int> ipaBuffers(data->ipaBuffers_.begin(), > data->ipaBuffers_.end()); > > data->ipa_->unmapBuffers(ipaBuffers); > > data->ipaBuffers_.clear(); > > - data->mappedEmbeddedBuffers_.clear(); > > > > for (auto const stream : data->streams_) > > stream->releaseBuffers(); > > @@ -1652,7 +1662,7 @@ void RPiCameraData::tryRunPipeline() > > > > /* If any of our request or buffer queues are empty, we cannot > proceed. */ > > if (state_ != State::Idle || requestQueue_.empty() || > > - bayerQueue_.empty() || embeddedQueue_.empty()) > > + bayerQueue_.empty() || (embeddedQueue_.empty() && > sensorMetadata_)) > > return; > > > > if (!findMatchingBuffers(bayerFrame, embeddedBuffer)) > > @@ -1675,17 +1685,24 @@ void RPiCameraData::tryRunPipeline() > > state_ = State::Busy; > > > > unsigned int bayerId = > unicam_[Unicam::Image].getBufferId(bayerFrame.buffer); > > - unsigned int embeddedId = > unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > > > LOG(RPI, Debug) << "Signalling signalIspPrepare:" > > - << " Bayer buffer id: " << bayerId > > - << " Embedded buffer id: " << embeddedId; > > + << " Bayer buffer id: " << bayerId; > > > > ipa::RPi::ISPConfig ispPrepare; > > - ispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData | > embeddedId; > > ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId; > > - ispPrepare.embeddedBufferPresent = true; > > ispPrepare.controls = std::move(bayerFrame.controls); > > + > > + if (embeddedBuffer) { > > + unsigned int embeddedId = > unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > + > > + ispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData | > embeddedId; > > + ispPrepare.embeddedBufferPresent = true; > > + > > + LOG(RPI, Debug) << "Signalling signalIspPrepare:" > > + << " Bayer buffer id: " << embeddedId; > > + } > > + > > ipa_->signalIspPrepare(ispPrepare); > > } > > > > @@ -1727,6 +1744,17 @@ bool > RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em > > > > LOG(RPI, Debug) << "Could not find matching > embedded buffer"; > > > > + if (!sensorMetadata_) { > > + /* > > + * If there is no sensor metadata, simply > return the > > + * first bayer frame in the queue. > > + */ > > + LOG(RPI, Debug) << "Returning bayer frame > without a match"; > > + bayerQueue_.pop(); > > + embeddedBuffer = nullptr; > > + return true; > > + } > > + > > if (!embeddedQueue_.empty()) { > > /* > > * Not found a matching embedded buffer > for the bayer buffer in > > -- > Regards, > > Laurent Pinchart >
Hi Laurent, On Mon, 22 Feb 2021 at 13:40, Naushir Patuck <naush@raspberrypi.com> wrote: <snip> One option to move this to match() time would be for the IPA to return >> if the sensor supports metadata from init() instead of configure(). That >> would require passing the sensor name to the IPA in init() too. >> > >> It's the mix n' match that bothers me I think. I don't mind if we decide >> to model the pipeline handler and IPA with the assumption that there can >> only be a single camera, with a hardcoded pipeline known from the very >> beginning, or in a more dynamic way that could allow runtime switching >> between sensors, but it would be nice if the architecture of the >> pipeline handler and IPA was consistent relatively to the model we pick. >> Does this make sense ? >> > > Yes, I do agree with this as well. I am happy for ipa->init() to pass > back the required > parameters so match() can be used to open the embedded data node if > required. > I presume we have all the tools needed to do this with the IPA interfaces > change to > use mojom definitions? If so I can update the signature of ipa->init() to > pass in the > sensor name and return out the "SensorConfig" parameters. > I had a very brief go at prototyping ipa->init() returning the SensorConfig parameters, but unfortunately I ran into a few issues. It seems like the mojom interface generating script (mojom_libcamera_generator.py) requires the init() method to return out only one integer parameters: # Validate parameters to init() ValidateSingleLength(f_init.parameters, 'input parameter to init()') ValidateSingleLength(f_init.response_parameters, 'output parameter from init()') if f_init.parameters[0].kind.mojom_name != 'IPASettings': raise Exception('init() must have single IPASettings input parameter') if f_init.response_parameters[0].kind.spec != 'i32': raise Exception('init() must have single int32 output parameter') Simply commenting out these lines does not help me either, as I think the IPA proxy template(?) requires a specific signature as well. Any advice on how to get around this?
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index b43d86166c63..d969c77993eb 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -177,12 +177,6 @@ public: /* Stores the ids of the buffers mapped in the IPA. */ std::unordered_set<unsigned int> ipaBuffers_; - /* - * Map of (internal) mmaped embedded data buffers, to avoid having to - * map/unmap on every frame. - */ - std::map<unsigned int, MappedFrameBuffer> mappedEmbeddedBuffers_; - /* DMAHEAP allocation helper. */ RPi::DmaHeap dmaHeap_; FileDescriptor lsTable_; @@ -636,14 +630,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) if (isRaw(cfg.pixelFormat)) { cfg.setStream(&data->unicam_[Unicam::Image]); - /* - * We must set both Unicam streams as external, even - * though the application may only request RAW frames. - * This is because we match timestamps on both streams - * to synchronise buffers. - */ data->unicam_[Unicam::Image].setExternal(true); - data->unicam_[Unicam::Embedded].setExternal(true); continue; } @@ -715,17 +702,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) return ret; } - /* Unicam embedded data output format. */ - format = {}; - format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA); - LOG(RPI, Debug) << "Setting embedded data format."; - ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format); - if (ret) { - LOG(RPI, Error) << "Failed to set format on Unicam embedded: " - << format.toString(); - return ret; - } - /* Figure out the smallest selection the ISP will allow. */ Rectangle testCrop(0, 0, 1, 1); data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop); @@ -742,6 +718,41 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) if (ret) LOG(RPI, Error) << "Failed to configure the IPA: " << ret; + /* + * The IPA will set data->sensorMetadata_ to true if embedded data is + * supported on this sensor. If so, open the Unicam embedded data + * node and configure the output format. + */ + if (data->sensorMetadata_) { + format = {}; + format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA); + LOG(RPI, Debug) << "Setting embedded data format."; + data->unicam_[Unicam::Embedded].dev()->open(); + ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format); + if (ret) { + LOG(RPI, Error) << "Failed to set format on Unicam embedded: " + << format.toString(); + return ret; + } + + /* + * If a RAW/Bayer stream has been requested by the application, + * we must set both Unicam streams as external, even though the + * application may only request RAW frames. This is because we + * match timestamps on both streams to synchronise buffers. + */ + if (rawStream) + data->unicam_[Unicam::Embedded].setExternal(true); + } else { + /* + * No embedded data present, so we do not want to iterate over + * the embedded data stream when starting and stopping. + */ + data->streams_.erase(std::remove(data->streams_.begin(), data->streams_.end(), + &data->unicam_[Unicam::Embedded]), + data->streams_.end()); + } + /* * Update the ScalerCropMaximum to the correct value for this camera mode. * For us, it's the same as the "analogue crop". @@ -949,10 +960,16 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) for (auto &stream : data->isp_) data->streams_.push_back(&stream); - /* Open all Unicam and ISP streams. */ + /* + * Open all Unicam and ISP streams. The exception is the embedded data + * stream, which only gets opened if the IPA reports that the sensor + * supports embedded data. This happens in RPiCameraData::configureIPA(). + */ for (auto const stream : data->streams_) { - if (stream->dev()->open()) - return false; + if (stream != &data->unicam_[Unicam::Embedded]) { + if (stream->dev()->open()) + return false; + } } /* Wire up all the buffer connections. */ @@ -1109,19 +1126,13 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) return ret; } - if (!data->sensorMetadata_) { - for (auto const &it : data->unicam_[Unicam::Embedded].getBuffers()) { - MappedFrameBuffer fb(it.second, PROT_READ | PROT_WRITE); - data->mappedEmbeddedBuffers_.emplace(it.first, std::move(fb)); - } - } - /* * Pass the stats and embedded data buffers to the IPA. No other * buffers need to be passed. */ mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), ipa::RPi::MaskStats); - mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), ipa::RPi::MaskEmbeddedData); + if (data->sensorMetadata_) + mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), ipa::RPi::MaskEmbeddedData); return 0; } @@ -1154,7 +1165,6 @@ void PipelineHandlerRPi::freeBuffers(Camera *camera) std::vector<unsigned int> ipaBuffers(data->ipaBuffers_.begin(), data->ipaBuffers_.end()); data->ipa_->unmapBuffers(ipaBuffers); data->ipaBuffers_.clear(); - data->mappedEmbeddedBuffers_.clear(); for (auto const stream : data->streams_) stream->releaseBuffers(); @@ -1652,7 +1662,7 @@ void RPiCameraData::tryRunPipeline() /* If any of our request or buffer queues are empty, we cannot proceed. */ if (state_ != State::Idle || requestQueue_.empty() || - bayerQueue_.empty() || embeddedQueue_.empty()) + bayerQueue_.empty() || (embeddedQueue_.empty() && sensorMetadata_)) return; if (!findMatchingBuffers(bayerFrame, embeddedBuffer)) @@ -1675,17 +1685,24 @@ void RPiCameraData::tryRunPipeline() state_ = State::Busy; unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer); - unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); LOG(RPI, Debug) << "Signalling signalIspPrepare:" - << " Bayer buffer id: " << bayerId - << " Embedded buffer id: " << embeddedId; + << " Bayer buffer id: " << bayerId; ipa::RPi::ISPConfig ispPrepare; - ispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData | embeddedId; ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId; - ispPrepare.embeddedBufferPresent = true; ispPrepare.controls = std::move(bayerFrame.controls); + + if (embeddedBuffer) { + unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); + + ispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData | embeddedId; + ispPrepare.embeddedBufferPresent = true; + + LOG(RPI, Debug) << "Signalling signalIspPrepare:" + << " Bayer buffer id: " << embeddedId; + } + ipa_->signalIspPrepare(ispPrepare); } @@ -1727,6 +1744,17 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em LOG(RPI, Debug) << "Could not find matching embedded buffer"; + if (!sensorMetadata_) { + /* + * If there is no sensor metadata, simply return the + * first bayer frame in the queue. + */ + LOG(RPI, Debug) << "Returning bayer frame without a match"; + bayerQueue_.pop(); + embeddedBuffer = nullptr; + return true; + } + if (!embeddedQueue_.empty()) { /* * Not found a matching embedded buffer for the bayer buffer in
The pipeline handler would enable and use the Unicam embedded data stream even if the sensor did not support it. This was to allow a means to pass exposure and gain values for the frame to the IPA in a synchronised way. The recent changes to get the pipeline handler to pass a ControlList with exposure and gain values means this is no longer required. Disable the use of the embedded data stream when a sensor does not support it. This change also removes the mappedEmbeddedBuffers_ map as it is no longer used. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 112 +++++++++++------- 1 file changed, 70 insertions(+), 42 deletions(-)