Message ID | 20211027092803.3671096-10-naush@raspberrypi.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Quoting Naushir Patuck (2021-10-27 10:28:03) > 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 before 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> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Will the user be able to flip at runtime? I suspect they can't currently anyway so it doesn't matter, but anyway, I can't see anything wrong with this: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 56 ++++++------------- > 1 file changed, 16 insertions(+), 40 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 52521857b61c..eaa2676b891e 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -657,16 +657,28 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > } > } > > + /* > + * 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); > + } > + > /* First calculate the best sensor mode we can use based on the user request. */ > V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize); > 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. > - */ > V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing); > ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > if (ret) > @@ -676,10 +688,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > << " - Selected sensor format: " << sensorFormat.toString() > << " - Selected unicam format: " << 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; > @@ -902,23 +910,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 = mbusCodeToBayerFormat(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); > > @@ -1332,10 +1323,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; > @@ -1386,17 +1373,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); > > -- > 2.25.1 >
Hi Kieran, Thank you for your feedback, On Wed, 27 Oct 2021 at 13:33, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2021-10-27 10:28:03) > > 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 before 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> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Will the user be able to flip at runtime? I suspect they can't currently > anyway so it doesn't matter, but anyway, I can't see anything wrong with > this: > No runtime changes of flips are not possible. That would be too complicated to deal with :-) Regards, Naush > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 56 ++++++------------- > > 1 file changed, 16 insertions(+), 40 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 52521857b61c..eaa2676b891e 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -657,16 +657,28 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > } > > } > > > > + /* > > + * 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); > > + } > > + > > /* First calculate the best sensor mode we can use based on the > user request. */ > > V4L2SubdeviceFormat sensorFormat = > findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize); > > 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. > > - */ > > V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, > packing); > > ret = > data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > > if (ret) > > @@ -676,10 +688,6 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > << " - Selected sensor format: " << > sensorFormat.toString() > > << " - Selected unicam format: " << > 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; > > @@ -902,23 +910,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 = > mbusCodeToBayerFormat(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); > > > > @@ -1332,10 +1323,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; > > @@ -1386,17 +1373,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); > > > > -- > > 2.25.1 > > >
Hi Naush, Thank you for the patch. On Wed, Oct 27, 2021 at 10:28:03AM +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 before 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> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 56 ++++++------------- > 1 file changed, 16 insertions(+), 40 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 52521857b61c..eaa2676b891e 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -657,16 +657,28 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > } > } > > + /* > + * 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); > + } > + > /* First calculate the best sensor mode we can use based on the user request. */ > V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize); > 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. > - */ > V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing); > ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > if (ret) > @@ -676,10 +688,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > << " - Selected sensor format: " << sensorFormat.toString() > << " - Selected unicam format: " << 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; > @@ -902,23 +910,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 = mbusCodeToBayerFormat(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); > > @@ -1332,10 +1323,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; > @@ -1386,17 +1373,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); >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 52521857b61c..eaa2676b891e 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -657,16 +657,28 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) } } + /* + * 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); + } + /* First calculate the best sensor mode we can use based on the user request. */ V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize); 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. - */ V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing); ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); if (ret) @@ -676,10 +688,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) << " - Selected sensor format: " << sensorFormat.toString() << " - Selected unicam format: " << 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; @@ -902,23 +910,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 = mbusCodeToBayerFormat(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); @@ -1332,10 +1323,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; @@ -1386,17 +1373,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);