<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>