Replying to myself to add a personal comment. 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 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. -- Regards, Laurent Pinchart
