| Message ID | 20211022143907.3089419-7-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:07PM +0100, Naushir Patuck wrote: > Sensor flips might change the Bayer order of the requested format. The existing > code would set a sensor format along with the appropriate Unicam and ISP input > formats, but reset the latter two on start() once the flips had been requested. > > We can now set the sensor flips just after we set the sensor mode in > configure(), thereby not needing the second pair of format sets in start(). > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Review-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 60 +++++++------------ > 1 file changed, 21 insertions(+), 39 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index c5e9607c7d95..ad6f1231bbf6 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -627,16 +627,34 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > > /* First calculate the best sensor mode we can use based on the user request. */ > V4L2SubdeviceFormat sensorFormat = findBestMode(data->sensorFormats_, rawStream ? sensorSize : maxSize); > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, unpacked); > > ret = data->sensor_->setFormat(&sensorFormat); > if (ret) > return ret; > > /* > - * Unicam image output format. The ISP input format gets set at start, > - * just in case we have swapped bayer orders due to flips. > + * Configure the H/V flip controls based on the combination of > + * the sensor and user transform. > */ > + if (data->supportsFlips_) { > + const RPiCameraConfiguration *rpiConfig = > + static_cast<const RPiCameraConfiguration *>(config); > + ControlList controls; > + > + controls.set(V4L2_CID_HFLIP, > + static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); > + controls.set(V4L2_CID_VFLIP, > + static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); > + data->setSensorControls(controls); > + > + /* > + * IPA configure may have changed the sensor flips - hence the bayer > + * order. So update the sensor format now. > + */ > + data->sensor_->device()->getFormat(0, &sensorFormat); > + } If you moved this whole block before the sensor_->setFormat() call, could you drop getFormat() ? > + > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, unpacked); > ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > if (ret) > return ret; > @@ -645,10 +663,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > << " - Selected sensor mode: " << sensorFormat.toString() > << " - Selected unicam mode: " << unicamFormat.toString(); > > - /* > - * This format may be reset on start() if the bayer order has changed > - * because of flips in the sensor. > - */ > ret = data->isp_[Isp::Input].dev()->setFormat(&unicamFormat); > if (ret) > return ret; > @@ -871,23 +885,6 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > return ret; > } > > - /* > - * IPA configure may have changed the sensor flips - hence the bayer > - * order. Get the sensor format and set the ISP input now. > - */ > - V4L2SubdeviceFormat sensorFormat; > - data->sensor_->device()->getFormat(0, &sensorFormat); > - > - V4L2DeviceFormat ispFormat; > - ispFormat.fourcc = BayerFormat::fromMbusCode(sensorFormat.mbus_code).toV4L2PixelFormat(); > - ispFormat.size = sensorFormat.size; > - > - ret = data->isp_[Isp::Input].dev()->setFormat(&ispFormat); > - if (ret) { > - stop(camera); > - return ret; > - } > - > /* Enable SOF event generation. */ > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); > > @@ -1301,10 +1298,6 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) > > int RPiCameraData::configureIPA(const CameraConfiguration *config) > { > - /* We know config must be an RPiCameraConfiguration. */ > - const RPiCameraConfiguration *rpiConfig = > - static_cast<const RPiCameraConfiguration *>(config); > - > std::map<unsigned int, IPAStream> streamConfig; > std::map<unsigned int, ControlInfoMap> entityControls; > ipa::RPi::IPAConfig ipaConfig; > @@ -1355,17 +1348,6 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) > return -EPIPE; > } > > - /* > - * Configure the H/V flip controls based on the combination of > - * the sensor and user transform. > - */ > - if (supportsFlips_) { > - controls.set(V4L2_CID_HFLIP, > - static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); > - controls.set(V4L2_CID_VFLIP, > - static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); > - } > - > if (!controls.empty()) > setSensorControls(controls); >
Hi Laurent, Thank you for your feedback. On Mon, 25 Oct 2021 at 18:09, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Fri, Oct 22, 2021 at 03:39:07PM +0100, Naushir Patuck wrote: > > Sensor flips might change the Bayer order of the requested format. The > existing > > code would set a sensor format along with the appropriate Unicam and ISP > input > > formats, but reset the latter two on start() once the flips had been > requested. > > > > We can now set the sensor flips just after we set the sensor mode in > > configure(), thereby not needing the second pair of format sets in > start(). > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Review-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 60 +++++++------------ > > 1 file changed, 21 insertions(+), 39 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index c5e9607c7d95..ad6f1231bbf6 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -627,16 +627,34 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > > > /* First calculate the best sensor mode we can use based on the > user request. */ > > V4L2SubdeviceFormat sensorFormat = > findBestMode(data->sensorFormats_, rawStream ? sensorSize : maxSize); > > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, > unpacked); > > > > ret = data->sensor_->setFormat(&sensorFormat); > > if (ret) > > return ret; > > > > /* > > - * Unicam image output format. The ISP input format gets set at > start, > > - * just in case we have swapped bayer orders due to flips. > > + * Configure the H/V flip controls based on the combination of > > + * the sensor and user transform. > > */ > > + if (data->supportsFlips_) { > > + const RPiCameraConfiguration *rpiConfig = > > + static_cast<const RPiCameraConfiguration > *>(config); > > + ControlList controls; > > + > > + controls.set(V4L2_CID_HFLIP, > > + > static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); > > + controls.set(V4L2_CID_VFLIP, > > + > static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); > > + data->setSensorControls(controls); > > + > > + /* > > + * IPA configure may have changed the sensor flips - hence > the bayer > > + * order. So update the sensor format now. > > + */ > > + data->sensor_->device()->getFormat(0, &sensorFormat); > > + } > > If you moved this whole block before the sensor_->setFormat() call, > could you drop getFormat() ? > Yes I could! Will fix this and all other suggested changes in a v3 shortly. Thanks, Naush > > > + > > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, > unpacked); > > ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > > if (ret) > > return ret; > > @@ -645,10 +663,6 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > << " - Selected sensor mode: " << > sensorFormat.toString() > > << " - Selected unicam mode: " << > unicamFormat.toString(); > > > > - /* > > - * This format may be reset on start() if the bayer order has > changed > > - * because of flips in the sensor. > > - */ > > ret = data->isp_[Isp::Input].dev()->setFormat(&unicamFormat); > > if (ret) > > return ret; > > @@ -871,23 +885,6 @@ int PipelineHandlerRPi::start(Camera *camera, const > ControlList *controls) > > return ret; > > } > > > > - /* > > - * IPA configure may have changed the sensor flips - hence the > bayer > > - * order. Get the sensor format and set the ISP input now. > > - */ > > - V4L2SubdeviceFormat sensorFormat; > > - data->sensor_->device()->getFormat(0, &sensorFormat); > > - > > - V4L2DeviceFormat ispFormat; > > - ispFormat.fourcc = > BayerFormat::fromMbusCode(sensorFormat.mbus_code).toV4L2PixelFormat(); > > - ispFormat.size = sensorFormat.size; > > - > > - ret = data->isp_[Isp::Input].dev()->setFormat(&ispFormat); > > - if (ret) { > > - stop(camera); > > - return ret; > > - } > > - > > /* Enable SOF event generation. */ > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); > > > > @@ -1301,10 +1298,6 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig > *sensorConfig) > > > > int RPiCameraData::configureIPA(const CameraConfiguration *config) > > { > > - /* We know config must be an RPiCameraConfiguration. */ > > - const RPiCameraConfiguration *rpiConfig = > > - static_cast<const RPiCameraConfiguration *>(config); > > - > > std::map<unsigned int, IPAStream> streamConfig; > > std::map<unsigned int, ControlInfoMap> entityControls; > > ipa::RPi::IPAConfig ipaConfig; > > @@ -1355,17 +1348,6 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config) > > return -EPIPE; > > } > > > > - /* > > - * Configure the H/V flip controls based on the combination of > > - * the sensor and user transform. > > - */ > > - if (supportsFlips_) { > > - controls.set(V4L2_CID_HFLIP, > > - > static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); > > - controls.set(V4L2_CID_VFLIP, > > - > static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); > > - } > > - > > if (!controls.empty()) > > setSensorControls(controls); > > > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index c5e9607c7d95..ad6f1231bbf6 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -627,16 +627,34 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) /* First calculate the best sensor mode we can use based on the user request. */ V4L2SubdeviceFormat sensorFormat = findBestMode(data->sensorFormats_, rawStream ? sensorSize : maxSize); - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, unpacked); ret = data->sensor_->setFormat(&sensorFormat); if (ret) return ret; /* - * Unicam image output format. The ISP input format gets set at start, - * just in case we have swapped bayer orders due to flips. + * Configure the H/V flip controls based on the combination of + * the sensor and user transform. */ + if (data->supportsFlips_) { + const RPiCameraConfiguration *rpiConfig = + static_cast<const RPiCameraConfiguration *>(config); + ControlList controls; + + controls.set(V4L2_CID_HFLIP, + static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); + controls.set(V4L2_CID_VFLIP, + static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); + data->setSensorControls(controls); + + /* + * IPA configure may have changed the sensor flips - hence the bayer + * order. So update the sensor format now. + */ + data->sensor_->device()->getFormat(0, &sensorFormat); + } + + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, unpacked); ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); if (ret) return ret; @@ -645,10 +663,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) << " - Selected sensor mode: " << sensorFormat.toString() << " - Selected unicam mode: " << unicamFormat.toString(); - /* - * This format may be reset on start() if the bayer order has changed - * because of flips in the sensor. - */ ret = data->isp_[Isp::Input].dev()->setFormat(&unicamFormat); if (ret) return ret; @@ -871,23 +885,6 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) return ret; } - /* - * IPA configure may have changed the sensor flips - hence the bayer - * order. Get the sensor format and set the ISP input now. - */ - V4L2SubdeviceFormat sensorFormat; - data->sensor_->device()->getFormat(0, &sensorFormat); - - V4L2DeviceFormat ispFormat; - ispFormat.fourcc = BayerFormat::fromMbusCode(sensorFormat.mbus_code).toV4L2PixelFormat(); - ispFormat.size = sensorFormat.size; - - ret = data->isp_[Isp::Input].dev()->setFormat(&ispFormat); - if (ret) { - stop(camera); - return ret; - } - /* Enable SOF event generation. */ data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); @@ -1301,10 +1298,6 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) int RPiCameraData::configureIPA(const CameraConfiguration *config) { - /* We know config must be an RPiCameraConfiguration. */ - const RPiCameraConfiguration *rpiConfig = - static_cast<const RPiCameraConfiguration *>(config); - std::map<unsigned int, IPAStream> streamConfig; std::map<unsigned int, ControlInfoMap> entityControls; ipa::RPi::IPAConfig ipaConfig; @@ -1355,17 +1348,6 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) return -EPIPE; } - /* - * Configure the H/V flip controls based on the combination of - * the sensor and user transform. - */ - if (supportsFlips_) { - controls.set(V4L2_CID_HFLIP, - static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); - controls.set(V4L2_CID_VFLIP, - static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); - } - if (!controls.empty()) setSensorControls(controls);
Sensor flips might change the Bayer order of the requested format. The existing code would set a sensor format along with the appropriate Unicam and ISP input formats, but reset the latter two on start() once the flips had been requested. We can now set the sensor flips just after we set the sensor mode in configure(), thereby not needing the second pair of format sets in start(). Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Review-by: David Plowman <david.plowman@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 60 +++++++------------ 1 file changed, 21 insertions(+), 39 deletions(-)