Hi Laurent, On Thu, Jan 26, 2023 at 02:01:43PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Thu, Jan 26, 2023 at 01:46:17PM +0200, Sakari Ailus wrote: > > On Thu, Jan 26, 2023 at 01:31:39PM +0200, Laurent Pinchart wrote: > > > On Thu, Jan 26, 2023 at 01:20:11PM +0200, Sakari Ailus wrote: > > > > On Thu, Jan 26, 2023 at 12:58:08PM +0200, Laurent Pinchart wrote: > > > > > On Thu, Jan 26, 2023 at 11:16:26AM +0100, Jacopo Mondi wrote: > > > > > > On Thu, Jan 26, 2023 at 11:48:21AM +0200, Sakari Ailus wrote: > > > > > > > On Thu, Jan 26, 2023 at 01:18:36AM +0200, Laurent Pinchart wrote: > > > > > > > > On Thu, Jan 26, 2023 at 01:12:06AM +0200, Laurent Pinchart wrote: > > > > > > > > > Hello, > > > > > > > > > > > > > > > > > > Jacopo, Sakari and I ended up having a long discussion today about the > > > > > > > > > interactions between sensor rotation (as described in the device tree) > > > > > > > > > and the V4L2 flip controls. The conversation started from the imx258 > > > > > > > > > series that Jacopo recently posted ([1]) and ended up as an in-depth > > > > > > > > > analysis of the problem. > > > > > > > > > > > > > > > > > > The notes we have taken are copied below. Feedback would be appreciated, > > > > > > > > > I will then translate that into patches for the kernel documentation. > > > > > > > > > > > > > > > > > > [1] https://lore.kernel.org/linux-media/20230117100603.51631-1-jacopo.mondi@xxxxxxxxxxxxxxxx/ > > > > > > > > > > > > > > > > > > ## Problem description > > > > > > > > > > > > > > > > > > V4L2 has five different elements that related to flipping and rotation: > > > > > > > > > > > > > > > > > > - Device tree "rotation" property > > > > > > > > > - V4L2_CID_CAMERA_SENSOR_ROTATION control > > > > > > > > > - V4L2_CID_ROTATE control > > > > > > > > > - V4L2_CID_HFLIP and V4L2_CID_VFLIP controls > > > > > > > > > - Bayer pattern exposed through media bus codes > > > > > > > > > > > > > > > > > > While all those components are (more or less) well-defined in the API, their > > > > > > > > > interactions have never been defined. This has led to different drivers > > > > > > > > > implementing different behaviours. > > > > > > > > > > > > > > > > > > ### Full-featured drivers > > > > > > > > > > > > > > > > > > List of drivers that parse the DT rotation property and expose > > > > > > > > > V4L2_CID_CAMERA_SENSOR_ROTATION, V4L2_CID_HFLIP and V4L2_CID_VFLIP: > > > > > > > > > > > > > > > > > > $ git grep -l FLIP $(git grep -l v4l2_ctrl_new_fwnode_properties -- drivers/media/i2c/) > > > > > > > > > - drivers/media/i2c/imx219.c > > > > > > > > > - drivers/media/i2c/ov08x40.c > > > > > > > > > - drivers/media/i2c/ov13b10.c > > > > > > > > > - drivers/media/i2c/ov5640.c > > > > > > > > > - drivers/media/i2c/ov5675.c (to be upstreamed) > > > > > > > > > - drivers/media/i2c/ov5693.c > > > > > > > > > - drivers/media/i2c/ov8865.c > > > > > > > > > - drivers/media/i2c/ov9282.c > > > > > > > > > > > > > > > > > > All those drivers by ov5640 program the sensor with the HFLIP and VFLIP values > > > > > > > > > as-is, without taking the rotation property into account. ov5640 inverts the > > > > > > > > > flipping controls transparently when the rotation is 180, but does still expose > > > > > > > > > the rotation value to userspace unmodified (commit > > > > > > > > > 1066fc1c2afdbe5977eae37314f0c21462e82b9a, merged in v6.0). > > > > > > > > > > > > > > > > > > ### Flip-enabled drivers > > > > > > > > > > > > > > > > > > List of drivers that expose the V4L2_CID_HFLIP and V4L2_CID_VFLIP but not > > > > > > > > > V4L2_CID_CAMERA_SENSOR_ROTATION: > > > > > > > > > > > > > > > > > > $ git grep -vl v4l2_ctrl_new_fwnode_properties $(git grep -l V4L2_CID_HFLIP -- drivers/media/i2c/) > > > > > > > > > - drivers/media/i2c/ccs/ccs-core.c > > > > > > > > > - drivers/media/i2c/hi847.c > > > > > > > > > - drivers/media/i2c/imx208.c > > > > > > > > > - drivers/media/i2c/imx219.c > > > > > > > > > - drivers/media/i2c/imx319.c > > > > > > > > > - drivers/media/i2c/imx355.c > > > > > > > > > - drivers/media/i2c/mt9m032.c > > > > > > > > > - drivers/media/i2c/mt9m111.c > > > > > > > > > - drivers/media/i2c/mt9p031.c > > > > > > > > > - drivers/media/i2c/mt9v011.c > > > > > > > > > - drivers/media/i2c/ov08d10.c > > > > > > > > > - drivers/media/i2c/ov08x40.c > > > > > > > > > - drivers/media/i2c/ov13b10.c > > > > > > > > > - drivers/media/i2c/ov2640.c > > > > > > > > > - drivers/media/i2c/ov2680.c > > > > > > > > > - drivers/media/i2c/ov5640.c > > > > > > > > > - drivers/media/i2c/ov5645.c > > > > > > > > > - drivers/media/i2c/ov5648.c > > > > > > > > > - drivers/media/i2c/ov5675.c > > > > > > > > > - drivers/media/i2c/ov5693.c > > > > > > > > > - drivers/media/i2c/ov6650.c > > > > > > > > > - drivers/media/i2c/ov7251.c > > > > > > > > > - drivers/media/i2c/ov7670.c > > > > > > > > > - drivers/media/i2c/ov772x.c > > > > > > > > > - drivers/media/i2c/ov7740.c > > > > > > > > > - drivers/media/i2c/ov8856.c > > > > > > > > > - drivers/media/i2c/ov8865.c > > > > > > > > > - drivers/media/i2c/ov9282.c > > > > > > > > > - drivers/media/i2c/ov9640.c > > > > > > > > > - drivers/media/i2c/ov9650.c > > > > > > > > > - drivers/media/i2c/rj54n1cb0c.c > > > > > > > > > - drivers/media/i2c/s5k5baf.c > > > > > > > > > - drivers/media/i2c/s5k6aa.c > > > > > > > > > - drivers/media/i2c/st-vgxy61.c > > > > > > > > > - drivers/media/i2c/vs6624.c > > > > > > > > > > > > > > > > > > Among those, the ccs driver parses the DT rotation property manually and > > > > > > > > > compensates for it transparently by inverting the flip values. The ov772x and > > > > > > > > > s5k6aa use a similar mechanism, but based on platform data instead of DT. > > > > > > > > > > > > > > > > > > ### Rotation-aware drivers > > > > > > > > > > > > > > > > > > List of drivers that parse the DT rotation property manually: > > > > > > > > > > > > > > > > > > $ git grep -l '"rotation"' -- drivers/media/i2c/ > > > > > > > > > - drivers/media/i2c/ccs/ccs-core.c > > > > > > > > > - drivers/media/i2c/imx258.c > > > > > > > > > - drivers/media/i2c/ov02a10.c > > > > > > > > > > > > > > > > > > All those drivers parse the DT rotation property and compensates for it > > > > > > > > > transparently. The ccs driver inverts the HFLIP and VFLIP controls exposed to > > > > > > > > > userspace, while the imx258 and ov02a10 flip the image internally but do not > > > > > > > > > expose the HFLIP and VFLIP controls. > > > > > > > > > > > > > > > > > > ## API standardization > > > > > > > > > > > > > > > > > > There is a consensus that a standardized API is required. There is also a > > > > > > > > > consensus that the V4L2_CID_ROTATE control must *not* be used by any sensor > > > > > > > > > driver. No sensor driver expose that control at the moment, so this shouldn't be > > > > > > > > > a problem. > > > > > > > > > > > > > > > > > > ### API for new drivers > > > > > > > > > > > > > > > > > > - Expose the rotation property through V4L2_CID_CAMERA_SENSOR_ROTATION as-is. > > > > > > > > > - Expose the V4L2_CID_HFLIP and V4L2_CID_VFLIP controls as-is. > > > > > > > > > - A sensor driver that enables horizontal or vertical flipping *must* expose the > > > > > > > > > HFLIP and VFLIP controls. It *should* expose them writable, but *may* expose > > > > > > > > > them read-only if not enough information is available to implement them as > > > > > > > > > writable in the driver. > > > > > > > > > > > > > > > > > > ### Backward-compatibility > > > > > > > > > > > > > > > > > > For drivers: > > > > > > > > > > > > > > > > > > - We don't care about existing drivers that use platform data (ov772x and > > > > > > > > > s5k6aa). The s5k6aa driver requires platform data, so it could be dropped as > > > > > > > > > nobody is supplying platform data in mainline. > > > > > > > > > - The full-featured drivers comply with the API for new drivers except for > > > > > > > > > ov5640. Those are thus fine. > > > > > > > > > - The ov5640 gained V4L2_CID_CAMERA_SENSOR_ROTATION support in v6.0, we should > > > > > > > > > fix it, even if it changes the userspace-visible behaviour. > > > > > > > > > - Dropping the internal flip has a higher risk of breaking applications. > > > > > > > > > - Overriding the V4L2_CID_CAMERA_SENSOR_ROTATION value and setting it > > > > > > > > > to 0 when it is 180 is less risky. > > > > > > > > > - ccs should expose the V4L2_CID_CAMERA_SENSOR_ROTATION control, and modify it > > > > > > > > > internally to account the transparent 180° compensation. > > > > > > > > > - For imx258 and ov02a10, two options are possible: > > > > > > > > > - Expose the V4L2_CID_CAMERA_SENSOR_ROTATION control as-is, and expose the > > > > > > > > > HFLIP and VFLIP controls read-only and hardcoded to enabled (for imx258) or > > > > > > > > > set based on the rotation (for ov02a10). The controls could later be made > > > > > > > > > writable. This only risk of userspace breakage would be with applications > > > > > > > > > that consider the V4L2_CID_CAMERA_SENSOR_ROTATION control but not the flip > > > > > > > > > controls. This is considered to be low-risk. > > > > > > > > > - Do as ccs (overriding the V4L2_CID_CAMERA_SENSOR_ROTATION value). > > > > > > > > > This is the option preferred by Sakari as it would unify the > > > > > > > > > behaviour of the ccs, imx258 and ov02a10 drivers. > > > > > > > > > > > > > > > > There is possibly an important user-visible difference between those two > > > > > > > > options. For rolling-shutter sensors, the motion of objects in the scene > > > > > > > > will have a different skew effect depending on the sensor rotation. It > > > > > > > > is thus important for userspace to know the real rotation. For this > > > > > > > > > > > > > > How will it be different? The only user-visible difference, as far as I can > > > > > > > tell, is the order of the Bayer pattern. > > > > > > > > > > > > I expressed a similar concern as the one Laurent has here summarized. > > > > > > > > > > > > I'm thinking about industrial/machine vision applications where knowing the > > > > > > real mounting rotation and being in control of the row/col read > > > > > > direction is possibily important with rolling shutters and fast moving > > > > > > objects? This is mostly speculation, but it might be a valid use case > > > > > > for very specialized applications.. > > > > > > > > > > > > If we compensate V4L2_CID_CAMERA_SENSOR_ROTATION and invert flips to > > > > > > hide the implicit mirroring that takes place in the driver, > > > > > > application that expects full control might get confused. > > > > > > > > > > That's my concern too. Imagine an object moving in the top-down > > > > > direction in front of the sensor. With rolling-shutter sensors, the > > > > > object will appear stretched vertically if the sensor is mounted > > > > > upside-up, and squashed if the sensor is mounted upside-down. This may > > > > > be important information for applications. > > > > > > > > That's not how it works: with the pixel matrix reading direction changed, > > > > it is read in the same (upright) order than when mounted upright. It > > > > couldn't be different: the sensor does not have a large buffer where to > > > > store the data. It's simply sent over the CSI-2 bus in the order it is > > > > read. > > > > > > Now that you mention it, it's quite obvious indeed. I'm not sure how I > > > got this wrong. > > > > > > > > > > The users of these drivers have relied on getting upright images without > > > > > > > further device configuration. The existing user space can be expected to > > > > > > > break if this is changed. > > > > > > > > > > > > Users that do not manipulate flips doesn't need to be changed, and as > > > > > > those drivers do not expose flips I don't think there's any issue > > > > > > here ? IOW current users will continue to work as they do afaict > > > > > > > > > > > > However they will find H/VFLIP enabled by default and (once V/FLIP > > > > > > controls are registered and made writable) they will have to "disable" > > > > > > the control to mirror/flip the image if they want to do so. > > > > > > > > > > > > I don't think that's an issue but maybe I'm overlooking it. > > > > > > > > > > For the imx258 and ov02a10, existing users that have the ability to take > > > > > flip controls or the sensor rotation controls into account, but not > > > > > both, may be affected. Users that take neither controls into account, or > > > > > all of them, should be fine. I thus don't expect regressions. For ccs, > > > > > we may need to check more carefully. > > > > > > > > If the flip control defaults are changed, then there could be fewer > > > > regressions. But it's hard to tell: the control values are have a > > > > different meaning after the change. > > > > > > For ccs I agree. Are you concerned about imx258 and ov02a10 ? While we > > > could handle those two the same way as ccs, handling them the same way > > > as new drivers but with different defaults for the h/v flip controls has > > > the advantage of making the implementation simpler, and aligning it with > > > all the other drivers except ccs. That would be my preference, but if > > > you strongly prefer otherwise, I'm OK with that as well. > > > > After thinking about this more, I think in the long run it'd be great if we > > had all the interfaces aligned, including CCS. > > I can't disagree :-) > > > The fallout is likely be > > very limited, if there is any, even if we change how this works for CCS. > > That's my opinion too. > > > Likely the best way to avoid it would be to default the flipping controls > > into values that cause the image to be upright with the default > > configuration, but that, I suppose, would again be a divergence from what > > the other drivers are doing. Still for applications that already get the > > CAMERA_SENSOR_ROTATION property, it would be a small step to set the > > flipping controls as well. Again this not exactly as it has been documented > > to work, nevertheless still closer to it. > > I'm fine with that. > > > I don't have a wrong opinion about this either way actually. All I want is > > that we all are aware of the same facts when making the decision. > > > > In fact there are two things that I'm interested in: not breaking existing > > applications if possible and making this work reasonably well for > > libcamera. > > We can certainly take care of the latter. As for the former, I'll do my > best :-) > > Does this mean that you're fine with Jacopo's IMX258 series ? I replied to the thread. Beyond these patches, it'd be great if documentation would be written on how this will work correctly on all sensors. The control documentation would be probably a good place. > > > > > > > > > reason, I think it would be better to never override the > > > > > > > > V4L2_CID_CAMERA_SENSOR_ROTATION value exposed to userspace, thus going > > > > > > > > for the first of the above two options. The ccs driver should ideally do > > > > > > > > the same. > > > > > > > > > > > > > > > > > For userspace: > > > > > > > > > > > > > > > > > > - If the V4L2_CID_CAMERA_SENSOR_ROTATION control is not exposed, userspace > > > > > > > > > *must* assume that the rotation is 0. > > > > > > > > > - If the HFLIP and VFLIP controls are not exposed, userspace *must* assume that > > > > > > > > > no flipping occurs. > > > > > > > > > - The captured video is upright if rotation == 0 and both flipping controls are > > > > > > > > > disabled or rotation == 180 and both flipping controls are enabled. > > > > > > > > > - Userspace *must* support read-only HFLIP and VFLIP controls. -- Kind regards, Sakari Ailus
