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