<div dir="ltr"><div dir="ltr">Hi all,<div><br></div><div><br></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 22 Jan 2021 at 12:20, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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 Fabian, Jacopo, Naush, David ;-)<br>
<br>
On 22/01/2021 08:52, Jacopo Mondi wrote:<br>
> Hi Fabian,<br>
> + Naush for RPi question<br>
<br>
Fixing the +CC for the question below - but also adding David as he<br>
wrote the original Transforms.<br>
<br>
--<br>
Kieran<br>
<br>
<br>
> <br>
> On Fri, Jan 22, 2021 at 09:10:42AM +0100, Fabian Wüthrich wrote:<br>
>> Hi Laurent,<br>
>><br>
>> On 22.01.21 00:37, Laurent Pinchart wrote:<br>
>>> Hi Jacopo and Fabian,<br>
>>><br>
>>> On Mon, Jan 18, 2021 at 11:06:31AM +0100, Jacopo Mondi wrote:<br>
>>>> On Sat, Jan 16, 2021 at 04:02:58PM +0100, Fabian Wüthrich wrote:<br>
>>>>> Use the same transformation logic as in the raspberry pipeline to<br>
>>>>> implement rotations in the ipu3 pipeline.<br>
>>><br>
>>> This should eventually be shared between different pipeline handlers,<br>
>>> but for now it's fine.<br>
>>><br>
>><br>
>> Agreed.<br>
>><br>
>>>>> Tested on a Surface Book 2 with an experimental driver for OV5693.<br>
>>>>><br>
>>>>> Signed-off-by: Fabian Wüthrich <<a href="mailto:me@fabwu.ch" target="_blank">me@fabwu.ch</a>><br>
>>>>> ---<br>
>>>>> Changes in v2:<br>
>>>>> - Cache rotationTransform in CameraData<br>
>>>>> - Use separate controls for sensor<br>
>>>>><br>
>>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 81 +++++++++++++++++++++++++++-<br>
>>>>> 1 file changed, 79 insertions(+), 2 deletions(-)<br>
>>>>><br>
>>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
>>>>> index 73304ea7..8db5f2f1 100644<br>
>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
>>>>> @@ -14,6 +14,7 @@<br>
>>>>> #include <libcamera/camera.h><br>
>>>>> #include <libcamera/control_ids.h><br>
>>>>> #include <libcamera/formats.h><br>
>>>>> +#include <libcamera/property_ids.h><br>
>>>>> #include <libcamera/request.h><br>
>>>>> #include <libcamera/stream.h><br>
>>>>><br>
>>>>> @@ -62,6 +63,15 @@ public:<br>
>>>>> Stream outStream_;<br>
>>>>> Stream vfStream_;<br>
>>>>> Stream rawStream_;<br>
>>>>> +<br>
>>>>> + /* Save transformation given by the sensor rotation */<br>
>>>>> + Transform rotationTransform_;<br>
>>>>> +<br>
>>>>> + /*<br>
>>>>> + * Manage horizontal and vertical flips supported (or not) by the<br>
>>>>> + * sensor.<br>
>>>>> + */<br>
>>>>> + bool supportsFlips_;<br>
>>>>> };<br>
>>>>><br>
>>>>> class IPU3CameraConfiguration : public CameraConfiguration<br>
>>>>> @@ -74,6 +84,9 @@ public:<br>
>>>>> const StreamConfiguration &cio2Format() const { return cio2Configuration_; }<br>
>>>>> const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }<br>
>>>>><br>
>>>>> + /* Cache the combinedTransform_ that will be applied to the sensor */<br>
>>>>> + Transform combinedTransform_;<br>
>>>>> +<br>
>>>>> private:<br>
>>>>> /*<br>
>>>>> * The IPU3CameraData instance is guaranteed to be valid as long as the<br>
>>>>> @@ -143,11 +156,49 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()<br>
>>>>> if (config_.empty())<br>
>>>>> return Invalid;<br>
>>>>><br>
>>>>> - if (transform != Transform::Identity) {<br>
>>>>> - transform = Transform::Identity;<br>
>>>>> + Transform combined = transform * data_->rotationTransform_;<br>
>>>>> +<br>
>>>>> + /*<br>
>>>>> + * We combine the platform and user transform, but must "adjust away"<br>
>>>>> + * any combined result that includes a transposition, as we can't do those.<br>
>>>>> + * In this case, flipping only the transpose bit is helpful to<br>
>>>>> + * applications - they either get the transform they requested, or have<br>
>>>>> + * to do a simple transpose themselves (they don't have to worry about<br>
>>>>> + * the other possible cases).<br>
>>>>> + */<br>
>>>>> + if (!!(combined & Transform::Transpose)) {<br>
>>>>> + /*<br>
>>>>> + * Flipping the transpose bit in "transform" flips it in the<br>
>>>>> + * combined result too (as it's the last thing that happens),<br>
>>>>> + * which is of course clearing it.<br>
>>>>> + */<br>
>>>>> + transform ^= Transform::Transpose;<br>
>>>>> + combined &= ~Transform::Transpose;<br>
>>>>> status = Adjusted;<br>
>>>>> }<br>
>>>>><br>
>>>>> + /*<br>
>>>>> + * We also check if the sensor doesn't do h/vflips at all, in which<br>
>>>>> + * case we clear them, and the application will have to do everything.<br>
>>>>> + */<br>
>>>>> + if (!data_->supportsFlips_ && !!combined) {<br>
>>>>> + /*<br>
>>>>> + * If the sensor can do no transforms, then combined must be<br>
>>>>> + * changed to the identity. The only user transform that gives<br>
>>>>> + * rise to this is the inverse of the rotation. (Recall that<br>
>>>>> + * combined = transform * rotationTransform.)<br>
>>>>> + */<br>
>>>>> + transform = -data_->rotationTransform_;<br>
>>>>> + combined = Transform::Identity;<br>
>>>>> + status = Adjusted;<br>
>>>>> + }<br>
>>>>> +<br>
>>>>> + /*<br>
>>>>> + * Store the final combined transform that configure() will need to<br>
>>>>> + * apply to the sensor to save us working it out again.<br>
>>>>> + */<br>
>>>>> + combinedTransform_ = combined;<br>
>>>>> +<br>
>>>>> /* Cap the number of entries to the available streams. */<br>
>>>>> if (config_.size() > IPU3_MAX_STREAMS) {<br>
>>>>> config_.resize(IPU3_MAX_STREAMS);<br>
>>>>> @@ -540,6 +591,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)<br>
>>>>> return ret;<br>
>>>>> }<br>
>>>>><br>
>>>>> + /*<br>
>>>>> + * Configure the H/V flip controls based on the combination of<br>
>>>>> + * the sensor and user transform.<br>
>>>>> + */<br>
>>>>> + if (data->supportsFlips_) {<br>
>>>>> + ControlList sensor_ctrls(cio2->sensor()->controls());<br>
>>>>> + sensor_ctrls.set(V4L2_CID_HFLIP,<br>
>>>>> + static_cast<int32_t>(!!(config->combinedTransform_ & Transform::HFlip)));<br>
>>>>> + sensor_ctrls.set(V4L2_CID_VFLIP,<br>
>>>>> + static_cast<int32_t>(!!(config->combinedTransform_ & Transform::VFlip)));<br>
>>>>> + cio2->sensor()->setControls(&sensor_ctrls);<br>
>>>>> + }<br>
>>>>> +<br>
>>>>> return 0;<br>
>>>>> }<br>
>>>>><br>
>>>>> @@ -775,9 +839,22 @@ int PipelineHandlerIPU3::registerCameras()<br>
>>>>> /* Initialize the camera properties. */<br>
>>>>> data->properties_ = cio2->sensor()->properties();<br>
>>>>><br>
>>>>> + /* Convert the sensor rotation to a transformation */<br>
>>>>> + int32_t rotation = data->properties_.get(properties::Rotation);<br>
>>>><br>
>>>> There's no guarantee the CameraSensor class register the Rotation<br>
>>>> property. It was defaulted to 0 if the information was not available<br>
>>>> from the firmware interface.<br>
>>>><br>
>>>> It is now not anymore (a very recent change, sorry)<br>
>>>> <a href="https://git.libcamera.org/libcamera/libcamera.git/commit/?id=c35123e6f91b6269cf6f5ae9216aeccebc545c75" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/commit/?id=c35123e6f91b6269cf6f5ae9216aeccebc545c75</a><br>
>>>><br>
>>>> Not sure what would be best, default to 0 in each pipeline handler<br>
>>>> that need that information ?<br>
>>><br>
>>> That sounds good to me for now.<br>
>>><br>
>><br>
>> Ok, thanks for checking. Should I provide a patch for the raspberry pipeline as well?<br>
> <br>
> I would not mind, but feel free to stick to IPU3 if that's quicker.<br>
> <br>
> Cc Naush for RPi. I'm thinking that, being RPi a bit more 'strict' as<br>
> platform and with a little more control over sensor drivers/fw, maybe<br>
> they want to refuse setups where no rotation is specified.<br></blockquote><div><br></div><div>I think we should be ok with defaulting to 0 if the rotation property is unavailable. Worst that would happen is that we would produce an inverted picture - and then the user knows to add rotation in the driver :-)</div><div>I'll let David have the final say though.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </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>
>>>>> + bool success;<br>
>>>>> + data->rotationTransform_ = transformFromRotation(rotation, &success);<br>
>>>>> + if (!success)<br>
>>>>> + LOG(IPU3, Warning) << "Invalid rotation of " << rotation << " degrees - ignoring";<br>
>>>>> +<br>
>>>>> /* Initialze the camera controls. */<br>
>>>>> data->controlInfo_ = IPU3Controls;<br>
>>>>><br>
>>>>> + ControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP });<br>
>>>>> + if (!ctrls.empty()) {<br>
>>>>> + /* We assume it will support vflips too... */<br>
>>>>> + data->supportsFlips_ = true;<br>
>>>>> + }<br>
>>>>> +<br>
>>>>> /**<br>
>>>>> * \todo Dynamically assign ImgU and output devices to each<br>
>>>>> * stream and camera; as of now, limit support to two cameras<br>
>>><br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>